Codegen bot/add team roles documentation 1757612602 #159
Conversation
- Create comprehensive team-roles.mdx documentation explaining ADMIN, MANAGER, and MEMBER roles - Include permission matrix showing what each role can and cannot do - Add best practices and security considerations for role management - Update docs.json navigation to include team-roles and agent-permissions pages Co-authored-by: Jay Hack <[email protected]>
- Fixed ruff formatting issues in multiple files - Standardized quote usage and formatting - Fixed ambiguous unicode character in tui.py
- Replace ambiguous ℹ (INFORMATION SOURCE) with 'i' in org and repo TUI files - Resolves RUF001 linting errors
- Replace ambiguous ℹ (INFORMATION SOURCE) with 'i' in console print statements - Resolves remaining RUF001 linting errors
- Add timezone import and use UTC timezone for datetime.now() calls - Resolves DTZ005 linting errors
- Add type ignore comments for intentional test errors in Agent() constructor calls - Fix callable check for create_claude_session function - Replace LocalGitRepo method calls with direct git CLI operations in agent_detail.py - Add try/except blocks around SDK imports with fallback behavior in language.py and pr_review.py This addresses multiple type checking diagnostics including: - Missing argument errors in tests - Call non-callable errors - Unresolved attribute errors - Unresolved import errors for SDK modules
- Renamed tests/cli/mcp to tests/cli/mcp_integration to avoid namespace conflict with installed mcp package - Renamed src/codegen/cli/mcp to src/codegen/cli/mcp_server to avoid namespace conflict - Updated import paths in src/codegen/cli/commands/mcp/main.py - Added missing MCP dependencies: fastmcp>=2.9.0 and mcp-python>=0.1.4 - Fixed import resolution issues that were causing test failures - All MCP integration tests now pass successfully The issue was that local directories named 'mcp' were shadowing the installed mcp package, causing ImportError: No module named 'mcp.types' when pytest tried to import the package. This fix resolves the namespace conflicts while maintaining all functionality.
Reviewer's GuideThis PR refactors import structures and code formatting across multiple CLI and TUI modules, standardizes type hint syntax and environment file update logic, enhances fallback handling and telemetry timestamp consistency, adjusts documentation formatting, and introduces a new team-roles documentation page along with an MCP server command. Class diagram for updated type hints in ClaudeLogWatcher and ManagerclassDiagram
class ClaudeLogWatcher {
-session_id: str
-org_id: int | None
-poll_interval: float
-on_log_entry: Callable[[dict[str, Any]], None] | None
+__init__(session_id: str, org_id: int | None = None, poll_interval: float = 1.0, on_log_entry: Callable[[dict[str, Any]], None] | None = None)
+_read_new_lines(start_line: int, end_line: int) list[dict[str, Any]]
+_process_log_entry(log_entry: dict[str, Any])
+_send_log_entry(log_entry: dict[str, Any])
+get_stats() dict[str, Any]
}
class ClaudeLogWatcherManager {
-watchers: dict[str, ClaudeLogWatcher]
+start_watcher(session_id: str, org_id: int | None = None, poll_interval: float = 1.0, on_log_entry: Callable[[dict[str, Any]], None] | None = None) bool
+get_active_sessions() list[str]
+get_watcher_stats(session_id: str) dict[str, Any] | None
+get_all_stats() dict[str, dict[str, Any]]
}
ClaudeLogWatcherManager "1" *-- "*" ClaudeLogWatcher
Class diagram for new MCP command integrationclassDiagram
class mcp {
+mcp(transport: str = "stdio", host: str = "localhost", port: int = 8000)
}
class run_server {
+run_server(transport: str, host: str, port: int)
}
mcp --> run_server
Class diagram for updated RepoConfig.from_envs fallback logicclassDiagram
class RepoConfig {
+from_envs() RepoConfig
}
class RepositoryConfig {
+path: str
+language: str
+name: str
+full_name: str
}
RepoConfig ..> RepositoryConfig
Class diagram for updated repo.py utility functions with new type hintsclassDiagram
class repo_utils {
+get_repo_env_status() dict[str, str]
+get_repo_display_info() list[dict[str, str]]
+fetch_repositories_for_org(org_id: int) list[dict[str, Any]]
+get_mock_repositories() list[dict[str, Any]]
+ensure_repositories_cached(org_id: int | None = None) list[dict[str, Any]]
}
Class diagram for AgentDetailTUI and JSONViewerTUI with updated type hintsclassDiagram
class AgentDetailTUI {
+__init__(agent_run: dict[str, Any], org_id: int | None = None)
-agent_run: dict[str, Any]
-agent_data: dict[str, Any] | None
}
class JSONViewerTUI {
+__init__(data: dict[str, Any])
-data: dict[str, Any]
}
AgentDetailTUI --> JSONViewerTUI
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughDocumentation updates, new MCP CLI command and server bootstrap, type-hint standardization, improved Claude log watcher with thread-based loop and stats, TUI enhancements (agent details, repo/org persistence), telemetry timestamp changes to UTC, minor import/format cleanups, tests adjusted for typing, and small API tweaks in git repo operator and schemas. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as codegen (Typer)
participant CMD as mcp.main.mcp
participant SVR as run_server
U->>CLI: codegen mcp --transport ... --host ... --port ...
CLI->>CMD: Invoke command with options
CMD->>SVR: run_server(transport, host, port)
alt transport==stdio
SVR-->>U: Start MCP server over stdio
else transport==http
SVR-->>U: Start MCP server on http://host:port
end
sequenceDiagram
autonumber
participant C as Caller
participant W as ClaudeLogWatcher
participant T as Watch Thread
participant F as File
participant API as send_claude_session_log
C->>W: start()
W->>T: spawn daemon _watch_loop()
loop poll interval
T->>F: read new lines
alt read ok
T->>T: validate + format
opt on_log_entry callback
T-->>C: on_log_entry(log_entry)
end
T->>API: send entry
API-->>T: success/failure
else read error
T->>T: warn + backoff
end
end
C->>W: stop()
W-->>C: stats (counts, success_rate)
sequenceDiagram
autonumber
participant U as User
participant UI as AgentDetailTUI
participant G as git_cli
U->>UI: Pull PR branch
UI->>UI: Pre-check remotes
alt no "codegen-pr" remote
UI->>G: add remote codegen-pr
end
UI->>G: fetch codegen-pr
alt branch not local
UI->>G: create from remote
else exists
UI->>G: checkout existing
end
UI-->>U: Status message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
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. ✨ 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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Zeeeepa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the Codegen CLI by introducing detailed documentation for team member roles, clarifying permissions and management. It also integrates a new mcp command, backed by a new dependency and a refactored internal structure, to manage the MCP server. Alongside these functional additions, the PR includes substantial code quality improvements, such as modernizing Python type hints, enhancing error logging with more comprehensive stack traces, and refining the user experience in the Text User Interface (TUI) for various commands.
Highlights
- Team Roles Documentation: Introduced comprehensive documentation for team member roles (ADMIN, MANAGER, MEMBER), detailing their hierarchical permissions, management processes, and best practices within the Codegen organization.
- New MCP CLI Command: Added a new
mcpcommand to the Codegen CLI, along with a newmcp-pythondependency, to facilitate starting the Codegen MCP server. Related files were refactored into a dedicatedmcp_serverdirectory. - Code Modernization and Robustness: Updated Python type hints from
Dict[str, Any]todict[str, Any]across multiple files for modern syntax. Implemented fallback mechanisms for SDK imports in various utilities to improve robustness when SDK modules are not available. - Logging and Telemetry Enhancements: Improved error logging by switching from
logger.errortologger.exceptionin several TUI components, providing more detailed stack traces. Session logging now uses UTC timestamps for consistency. - TUI and Git Operation Improvements: Refined the Text User Interface (TUI) for organization and repository selection, and agent run details. Git operations within the TUI were updated to use
git_clidirectly for better control when pulling branches.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/codegen/cli/commands/mcp/main.py:7` </location>
<code_context>
+from codegen.cli.mcp_server.runner import run_server
+
+
+def mcp(
+ transport: str = typer.Option("stdio", help="Transport type (stdio, http)"),
+ host: str = typer.Option("localhost", help="Host to bind to (for http transport)"),
+ port: int = typer.Option(8000, help="Port to bind to (for http transport)"),
+):
+ """Start the Codegen MCP server."""
+ run_server(transport=transport, host=host, port=port)
</code_context>
<issue_to_address>
Introduced new MCP command entrypoint.
Please verify that error handling and user feedback in this command match the standards set by other CLI commands.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def mcp( | ||
| transport: str = typer.Option("stdio", help="Transport type (stdio, http)"), | ||
| host: str = typer.Option("localhost", help="Host to bind to (for http transport)"), | ||
| port: int = typer.Option(8000, help="Port to bind to (for http transport)"), | ||
| ): | ||
| """Start the Codegen MCP server.""" | ||
| run_server(transport=transport, host=host, port=port) |
There was a problem hiding this comment.
suggestion: Introduced new MCP command entrypoint.
Please verify that error handling and user feedback in this command match the standards set by other CLI commands.
|
|
||
| except Exception as e: | ||
| console.print(f"[red]Error:[/red] Failed to clear repository configuration: {e}") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
| raise typer.Exit(1) | |
| raise typer.Exit(1) from e |
|
|
||
| def _run_repo_selector_tui() -> None: | ||
| """Launch the repository selector TUI.""" | ||
| try: |
There was a problem hiding this comment.
issue (code-quality): Explicitly raise from a previous error [×2] (raise-from-previous-error)
|
|
||
| def _update_info_with_detailed_data(self) -> None: | ||
| """Update the info table with detailed data from the API.""" | ||
| if not self.agent_data: |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Remove unnecessary calls to
enumeratewhen the index is not used (remove-unused-enumerate)
| for var_name, value in env_status.items(): | ||
| info.append({ | ||
| "label": f"{var_name}", | ||
| "value": value, | ||
| "status": "active" if value != "Not set" else "inactive" | ||
| }) | ||
|
|
||
| info.append({"label": f"{var_name}", "value": value, "status": "active" if value != "Not set" else "inactive"}) | ||
|
|
There was a problem hiding this comment.
suggestion (code-quality): Replace a for append loop with list extend (for-append-to-extend)
| for var_name, value in env_status.items(): | |
| info.append({ | |
| "label": f"{var_name}", | |
| "value": value, | |
| "status": "active" if value != "Not set" else "inactive" | |
| }) | |
| info.append({"label": f"{var_name}", "value": value, "status": "active" if value != "Not set" else "inactive"}) | |
| info.extend( | |
| { | |
| "label": f"{var_name}", | |
| "value": value, | |
| "status": "active" if value != "Not set" else "inactive", | |
| } | |
| for var_name, value in env_status.items() | |
| ) |
| def ensure_repositories_cached(org_id: int | None = None) -> list[dict[str, Any]]: | ||
| """Ensure repositories are cached for the given organization. | ||
There was a problem hiding this comment.
issue (code-quality): Use named expression to simplify assignment and conditional [×2] (use-named-expression)
| return _telemetry_config | ||
|
|
||
| _telemetry_config_checked = True | ||
|
|
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Remove redundant exceptions from an except clause (
remove-redundant-exception) - Merge except handlers with identical code blocks (
merge-except-handler) - Replace length-one exception tuple with exception (
simplify-single-exception-tuple)
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new mcp command, adds documentation for team roles, and implements a wide range of refactorings and improvements across the CLI and TUI. Key enhancements include unifying environment file handling, adopting modern Python type hints, making imports more robust with fallbacks, standardizing on UTC timestamps for telemetry, and improving exception logging. The changes are generally positive, improving code quality and robustness. My review focuses on correcting some documentation formatting, fixing a potentially broken link, and suggesting improvements to exception handling and user interface consistency.
| 1. **Import the logger**: `from codegen.shared.logging.get_logger import get_logger` | ||
| 2. **Add `extra={}` to your log calls**: `logger.info("message", extra={"key": "value"})` | ||
| 3. **Enable telemetry**: `codegen config telemetry enable` | ||
| 1. **Add `extra={}` to your log calls**: `logger.info("message", extra={"key": "value"})` | ||
| 1. **Enable telemetry**: `codegen config telemetry enable` |
There was a problem hiding this comment.
The numbered list for the "3 Step Process" is incorrect. All items are numbered as 1.. For a correctly rendered ordered list, they should be numbered sequentially (1., 2., 3.).
| 1. **Import the logger**: `from codegen.shared.logging.get_logger import get_logger` | |
| 2. **Add `extra={}` to your log calls**: `logger.info("message", extra={"key": "value"})` | |
| 3. **Enable telemetry**: `codegen config telemetry enable` | |
| 1. **Add `extra={}` to your log calls**: `logger.info("message", extra={"key": "value"})` | |
| 1. **Enable telemetry**: `codegen config telemetry enable` | |
| 1. **Import the logger**: `from codegen.shared.logging.get_logger import get_logger` | |
| 2. **Add `extra={}` to your log calls**: `logger.info("message", extra={"key": "value"})` | |
| 3. **Enable telemetry**: `codegen config telemetry enable` |
| # At start of function | ||
| logger.info("Operation started", extra={ | ||
| "operation": "command.subcommand", | ||
| "user_input": relevant_input | ||
| }) | ||
|
|
||
| # At end of function | ||
| logger.info("Operation completed", extra={ | ||
| "operation": "command.subcommand", | ||
| "success": True | ||
| }) | ||
| logger.info("Operation started", extra={"operation": "command.subcommand", "user_input": relevant_input}) | ||
|
|
||
| # At end of function | ||
| logger.info("Operation completed", extra={"operation": "command.subcommand", "success": True}) |
There was a problem hiding this comment.
The logger calls in this example have been condensed to a single line. For calls with multiple parameters in the extra dictionary, the previous multi-line format was more readable and is used in other examples within this same file (e.g., lines 25-32). For consistency and improved readability, consider reverting to the multi-line format for these examples.
| # At start of function | |
| logger.info("Operation started", extra={ | |
| "operation": "command.subcommand", | |
| "user_input": relevant_input | |
| }) | |
| # At end of function | |
| logger.info("Operation completed", extra={ | |
| "operation": "command.subcommand", | |
| "success": True | |
| }) | |
| logger.info("Operation started", extra={"operation": "command.subcommand", "user_input": relevant_input}) | |
| # At end of function | |
| logger.info("Operation completed", extra={"operation": "command.subcommand", "success": True}) | |
| # At start of function | |
| logger.info("Operation started", extra={ | |
| "operation": "command.subcommand", | |
| "user_input": relevant_input | |
| }) | |
| # At end of function | |
| logger.info("Operation completed", extra={ | |
| "operation": "command.subcommand", | |
| "success": True | |
| }) |
| 1. **Run the command**: Execute your enhanced CLI command | ||
| 2. **Check telemetry status**: `codegen config telemetry status` | ||
| 3. **Look for logs in Grafana Cloud**: Search for your operation names | ||
| 4. **Test with telemetry disabled**: `codegen config telemetry disable` - should still work normally | ||
| 1. **Check telemetry status**: `codegen config telemetry status` | ||
| 1. **Look for logs in Grafana Cloud**: Search for your operation names | ||
| 1. **Test with telemetry disabled**: `codegen config telemetry disable` - should still work normally |
There was a problem hiding this comment.
The numbered list under the "Verification" section is incorrect. All items are numbered as 1., which will not render as a sequential list.
| 1. **Run the command**: Execute your enhanced CLI command | |
| 2. **Check telemetry status**: `codegen config telemetry status` | |
| 3. **Look for logs in Grafana Cloud**: Search for your operation names | |
| 4. **Test with telemetry disabled**: `codegen config telemetry disable` - should still work normally | |
| 1. **Check telemetry status**: `codegen config telemetry status` | |
| 1. **Look for logs in Grafana Cloud**: Search for your operation names | |
| 1. **Test with telemetry disabled**: `codegen config telemetry disable` - should still work normally | |
| 1. **Run the command**: Execute your enhanced CLI command | |
| 2. **Check telemetry status**: `codegen config telemetry status` | |
| 3. **Look for logs in Grafana Cloud**: Search for your operation names | |
| 4. **Test with telemetry disabled**: `codegen config telemetry disable` - should still work normally |
| ## Related Documentation | ||
|
|
||
| - [Agent Permissions](/settings/agent-permissions) - Configure what actions agents can perform | ||
| - [Organization Rules](/settings/organization-rules) - Set organization-wide rules |
There was a problem hiding this comment.
The link to [Organization Rules](/settings/organization-rules) appears to be broken. The docs/docs.json file does not list an organization-rules page under settings, but it does list repo-rules. Please verify the correct link target.
- [Repository Rules](/settings/repo-rules) - Configure repository-specific behavior
| else: | ||
| console.print(f"[green]✓[/green] Set repository ID to: [cyan]{repo_id}[/cyan]") | ||
| console.print("[yellow]ℹ[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | ||
| console.print("[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") |
There was a problem hiding this comment.
The informational icon ℹ has been replaced with a lowercase i. While this change is consistent across the pull request, the standard Unicode info icon (ℹ) is more universally recognized and provides better visual clarity for users. Consider reverting to the standard icon to improve the user experience.
| console.print("[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | |
| console.print("[yellow]ℹ[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") |
| try: | ||
| current_repo.add_remote(remote_name, repo_clone_url) | ||
| git_cli.create_remote(remote_name, repo_clone_url) | ||
| except Exception: | ||
| # Remote might already exist | ||
| pass |
There was a problem hiding this comment.
Catching a broad Exception can hide underlying issues and make debugging more difficult. It's better to catch a more specific exception or check for the condition preemptively. In this case, you can check if the remote already exists before attempting to create it.
if remote_name not in [remote.name for remote in git_cli.remotes]:
git_cli.create_remote(remote_name, repo_clone_url)![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Vague ADMIN role assignment criteria ▹ view | |
 |
Silent ImportError handling ▹ view | |
 |
Duplicate Integration Management Permission ▹ view | |
 |
Ambiguous Organization Settings Access ▹ view | |
 |
Interface Segregation Violation in Server Configuration ▹ view | |
|
Unclear module docstring ▹ view | |
|
Missing parameter impact documentation ▹ view | |
 |
Unrestricted Host Binding ▹ view | |
|
Missing Transport Type Validation ▹ view | |
|
Missing Port Range Validation ▹ view |
Files scanned
Due to the exceptionally large size of this PR, I've limited my review to the following files:
| File Path | Reviewed |
|---|---|
| src/codegen/cli/commands/mcp/init.py | ✅ |
| src/codegen/cli/commands/claude/init.py | ✅ |
| src/codegen/cli/commands/org/init.py | ✅ |
| src/codegen/cli/commands/repo/init.py | ✅ |
| src/codegen/cli/commands/mcp/main.py | ✅ |
| src/codegen/cli/commands/claude/quiet_console.py | ✅ |
| src/codegen/cli/utils/inplace_print.py | ✅ |
| src/codegen/cli/mcp_server/tools/executor.py | ✅ |
| src/codegen/cli/mcp_server/api_client.py | ✅ |
| src/codegen/git/schemas/repo_config.py | ✅ |
| src/codegen/cli/telemetry/consent.py | ✅ |
| src/codegen/cli/commands/claude/claude_log_utils.py | ✅ |
| src/codegen/shared/logging/get_logger.py | ✅ |
| src/codegen/cli/utils/org.py | ✅ |
| src/codegen/cli/cli.py | ✅ |
| src/codegen/cli/commands/org/main.py | ✅ |
| src/codegen/git/utils/pr_review.py | ✅ |
| src/codegen/cli/telemetry/debug_exporter.py | ✅ |
| src/codegen/cli/telemetry/exception_logger.py | ✅ |
| docs/settings/team-roles.mdx | ✅ |
| src/codegen/cli/commands/claude/claude_session_api.py | ✅ |
| src/codegen/cli/commands/repo/main.py | ✅ |
| src/codegen/git/utils/language.py | ✅ |
| src/codegen/cli/telemetry/otel_setup.py | ✅ |
| src/codegen/cli/utils/repo.py | ✅ |
| src/codegen/cli/commands/claude/claude_log_watcher.py | ✅ |
| src/codegen/cli/commands/claude/main.py | ✅ |
| src/codegen/cli/tui/agent_detail.py | ✅ |
| src/codegen/cli/commands/org/tui.py | ✅ |
| src/codegen/cli/commands/repo/tui.py | ✅ |
| src/codegen/git/repo_operator/repo_operator.py | ✅ |
| src/codegen/cli/tui/app.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| - **MANAGER**: Team leads, project managers, or senior developers who need to manage integrations | ||
| - **ADMIN**: Organization owners, DevOps engineers, or senior leadership who need full control |
There was a problem hiding this comment.
Vague ADMIN role assignment criteria 
Tell me more
What is the issue?
The Role Assignment Guidelines lack specifics about when to grant ADMIN access, which could lead to overprivileged accounts.
Why this matters
Overly broad ADMIN role assignments increase security risks and complicate access management.
Suggested change ∙ Feature Preview
- MANAGER: Team leads, project managers, or senior developers who need to manage integrations
- ADMIN: Organization owners or designated security leads - limit to 2-3 trusted individuals per organization
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| try: | ||
| from codegen.sdk.core.file import SourceFile | ||
| except ImportError: | ||
| # If SDK is not available, return empty list | ||
| return [] |
There was a problem hiding this comment.
Silent ImportError handling 
Tell me more
What is the issue?
The ImportError catch block silently returns an empty list without logging the error or providing context about the missing SDK dependency.
Why this matters
Silent failure masks the root cause of missing dependencies, making it harder to diagnose why the modified_symbols property is returning empty results.
Suggested change ∙ Feature Preview
try:
from codegen.sdk.core.file import SourceFile
except ImportError as e:
logging.warning(f"SDK dependency not available, modified_symbols will return empty list: {e}")
return []Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| - **Manage integrations** (GitHub, Linear, Slack, etc.) | ||
| - **View and manage organization integrations** |
There was a problem hiding this comment.
Duplicate Integration Management Permission 
Tell me more
What is the issue?
Redundant permission listed for Manager role. 'Manage integrations' and 'View and manage organization integrations' describe the same functionality.
Why this matters
Listing the same permission twice in different words can cause confusion about whether these are distinct capabilities, potentially leading to misunderstandings about the role's actual permissions.
Suggested change ∙ Feature Preview
Remove one of the redundant entries. Keep only:
- **Manage integrations** (GitHub, Linear, Slack, etc.)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| | Delete repositories | ❌ | ❌ | ✅ | | ||
| | Access billing | ❌ | ❌ | ✅ | | ||
| | Change user roles | ❌ | ❌ | ✅ | | ||
| | Organization settings | Limited | Limited | Full | |
There was a problem hiding this comment.
Ambiguous Organization Settings Access 
Tell me more
What is the issue?
The permission matrix shows 'Limited' access for both Member and Manager roles without specifying what 'Limited' means.
Why this matters
Without clear definition of 'Limited' access, users won't understand what organization settings they can actually access, potentially leading to confusion or access-related issues.
Suggested change ∙ Feature Preview
Either define what 'Limited' means or be specific about the exact permissions in the matrix:
| Organization settings | View Only | View & Edit Basic | Full |Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| def mcp( | ||
| transport: str = typer.Option("stdio", help="Transport type (stdio, http)"), | ||
| host: str = typer.Option("localhost", help="Host to bind to (for http transport)"), | ||
| port: int = typer.Option(8000, help="Port to bind to (for http transport)"), | ||
| ): |
There was a problem hiding this comment.
Interface Segregation Violation in Server Configuration 
Tell me more
What is the issue?
The MCP command function combines transport options for different server types (stdio and http) into a single interface, violating the Interface Segregation Principle.
Why this matters
This design forces clients using stdio transport to deal with irrelevant http-specific parameters (host, port), making the interface less maintainable and more prone to misuse.
Suggested change ∙ Feature Preview
Split the command into separate interfaces for different transport types:
def mcp_stdio():
run_server(transport="stdio")
def mcp_http(
host: str = typer.Option("localhost", help="Host to bind to"),
port: int = typer.Option(8000, help="Port to bind to")
):
run_server(transport="http", host=host, port=port)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| @@ -0,0 +1,13 @@ | |||
| """MCP command implementation.""" | |||
There was a problem hiding this comment.
Unclear module docstring
Tell me more
What is the issue?
The module docstring is too vague and doesn't explain what MCP means or its purpose.
Why this matters
Without understanding what MCP means and does, developers will struggle to maintain or use this module effectively.
Suggested change ∙ Feature Preview
"""Module containing the MCP (Master Control Program) server command implementation.
This module provides the CLI interface for starting and configuring the Codegen MCP server,
which handles code generation coordination and communication."""
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| host: str = typer.Option("localhost", help="Host to bind to (for http transport)"), | ||
| port: int = typer.Option(8000, help="Port to bind to (for http transport)"), | ||
| ): | ||
| """Start the Codegen MCP server.""" |
There was a problem hiding this comment.
Missing parameter impact documentation
Tell me more
What is the issue?
The function docstring doesn't describe the configuration options or their impact.
Why this matters
Users won't understand how different transport options affect server behavior without reading the code.
Suggested change ∙ Feature Preview
"""Start the Codegen MCP server with the specified transport configuration.
The server can run using either stdio for direct process communication
or HTTP for network-based communication."""
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
|
|
||
| def mcp( | ||
| transport: str = typer.Option("stdio", help="Transport type (stdio, http)"), | ||
| host: str = typer.Option("localhost", help="Host to bind to (for http transport)"), |
There was a problem hiding this comment.
Unrestricted Host Binding 
Tell me more
What is the issue?
Default host binding to localhost without input validation could allow unintended network exposure if changed to '0.0.0.0' or other insecure values.
Why this matters
Without validation, binding to unsafe host values could expose the server to unauthorized network access from external sources.
Suggested change ∙ Feature Preview
Add input validation to restrict host values to safe options:
def validate_host(value: str) -> str:
allowed_hosts = ['localhost', '127.0.0.1']
if value not in allowed_hosts:
raise typer.BadParameter(f'Host must be one of: {allowed_hosts}')
return value
host: str = typer.Option(
"localhost",
help="Host to bind to (for http transport)",
callback=validate_host
)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
|
|
||
|
|
||
| def mcp( | ||
| transport: str = typer.Option("stdio", help="Transport type (stdio, http)"), |
There was a problem hiding this comment.
Missing Transport Type Validation
Tell me more
What is the issue?
The transport parameter lacks input validation to ensure only valid transport types ('stdio' or 'http') are accepted.
Why this matters
Invalid transport types could cause unexpected behavior or crashes when passed to run_server().
Suggested change ∙ Feature Preview
Add validation for transport parameter using a callback function in typer:
def validate_transport(value: str) -> str:
if value not in ["stdio", "http"]:
raise typer.BadParameter("Transport must be either 'stdio' or 'http'")
return value
def mcp(
transport: str = typer.Option(
"stdio",
help="Transport type (stdio, http)",
callback=validate_transport
),
# ... rest of the parameters
):Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| def mcp( | ||
| transport: str = typer.Option("stdio", help="Transport type (stdio, http)"), | ||
| host: str = typer.Option("localhost", help="Host to bind to (for http transport)"), | ||
| port: int = typer.Option(8000, help="Port to bind to (for http transport)"), |
There was a problem hiding this comment.
Missing Port Range Validation
Tell me more
What is the issue?
The port parameter lacks validation for a valid port range (typically 1-65535).
Why this matters
Invalid port numbers could cause network binding failures or security issues.
Suggested change ∙ Feature Preview
Add validation for port parameter using a callback function:
def validate_port(value: int) -> int:
if not (1 <= value <= 65535):
raise typer.BadParameter("Port must be between 1 and 65535")
return value
def mcp(
# ... other parameters
port: int = typer.Option(
8000,
help="Port to bind to (for http transport)",
callback=validate_port
),
):Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/codegen/git/repo_operator/repo_operator.py (1)
569-584: Skip unreadable files in iter_files and use logging — don’t yield Noneget_file returns str | None but iter_files is annotated Generator[tuple[str, str]] and yields content unfiltered; callers can receive None and crash.
- In src/codegen/git/repo_operator/repo_operator.py, inside iter_files before yielding:
content = self.get_file(filepath) if content is None: logger.warning(f"Skipping unreadable/undecodable file: {filepath}") continue yield rel_filepath, content
- Replace the print in get_file with structured logging:
- print(f"Warning: Unable to decode file {file_path}. Skipping.") + logger.warning(f"Unable to decode file {file_path}. Skipping.")Locations: get_file (≈ lines 569–584), iter_files (≈ lines 635–671).
src/codegen/cli/tui/app.py (1)
170-188: Add timeouts to API calls in recent-runs loader (prevent TUI hangs).These
requests.getcalls run in the UI’s background loop; without timeouts the TUI can stall on slow networks.- user_response = requests.get(f"{API_ENDPOINT.rstrip('/')}/v1/users/me", headers=headers) + user_response = requests.get(f"{API_ENDPOINT.rstrip('/')}/v1/users/me", headers=headers, timeout=10) ... - response = requests.get(url, headers=headers, params=params) + response = requests.get(url, headers=headers, params=params, timeout=10)Also applies to: 185-188
src/codegen/cli/commands/claude/main.py (1)
76-79: Add a timeout to the background run API callThis external POST can hang indefinitely without a timeout.
Apply:
- response = requests.post(url, headers=headers, json=payload) + response = requests.post(url, headers=headers, json=payload, timeout=30)
🧹 Nitpick comments (31)
tests/unit/codegen/agents/test_usage_demo.py (1)
42-42: Align ignore code with mypy or make it tool-agnosticUse
call-argfor mypy or plaintype: ignoreif you want cross-checker compatibility.- Agent() # type: ignore[missing-argument] # No token provided + Agent() # type: ignore[call-arg] # No token providedtests/unit/codegen/test_top_level_imports.py (2)
37-42: Replace constant getattr calls with direct attribute access (Ruff B009)Direct access is clearer and satisfies the linter.
- assert hasattr(Agent, "run") - assert hasattr(Agent, "get_status") - assert callable(getattr(Agent, "run")) - assert callable(getattr(Agent, "get_status")) + assert hasattr(Agent, "run") + assert hasattr(Agent, "get_status") + assert callable(Agent.run) + assert callable(Agent.get_status)
46-47: Trim redundant hasattr before attribute assertionYou already assert the value on the next line; the hasattr is redundant.
- assert hasattr(agent, "token") assert agent.token is Nonepyproject.toml (2)
28-28: Constrain mcp-python to a minor range to avoid unexpected breaks.
Pre-1.0 packages can introduce breaking changes on minor bumps. Suggest adding an upper bound.Apply:
- "mcp-python>=0.1.4", + "mcp-python>=0.1.4,<0.2.0",
28-28: Optionally make MCP deps extra-installable.
If the MCP CLI is optional for most users, consider moving mcp-python (and any MCP-only deps) to an extra (e.g., [project.optional-dependencies].mcp) to reduce base footprint.Happy to draft the extra + guards in CLI wiring if you’d like.
src/codegen/cli/commands/mcp/__init__.py (1)
1-1: Add explicit export for consistency with org/repo modules.
Keeps public surface predictable for importers.Apply:
"""MCP command module.""" +from .main import mcp + +__all__ = ["mcp"]src/codegen/git/utils/pr_review.py (1)
132-136: Good: graceful fallback when SDK is missing; add a one-time debug log.
Helps diagnose why modified_symbols is empty without spamming stdout.Apply:
- except ImportError: - # If SDK is not available, return empty list - return [] + except ImportError: + # If SDK is not available, return empty list + import logging + logging.getLogger(__name__).debug("codegen.sdk not available; modified_symbols=[]") + return []src/codegen/cli/mcp_server/api_client.py (1)
36-38: Normalize base URL regardless of source.
Trim trailing slashes for both env and default to avoid accidental double slashes.Apply:
- base_url = os.getenv("CODEGEN_API_BASE_URL", API_ENDPOINT.rstrip("/")) + base_url = (os.getenv("CODEGEN_API_BASE_URL") or API_ENDPOINT).rstrip("/")src/codegen/cli/commands/claude/config/claude_session_active_hook.py (1)
22-22: Fix incorrect type annotation for fallback symbolAnnotate as an optional callable; current
: Nonemakes the symbol non-callable for type checkers.-except ImportError: - update_claude_session_status: None = None +except ImportError: + update_claude_session_status: Callable[[str, str, int | None], bool] | None = NoneAdd import near the top:
from collections.abc import Callablesrc/codegen/cli/telemetry/otel_setup.py (1)
296-302: Minor: simplify shutdown guard and ensure reset in finallyThe local alias is fine; the assert is redundant. Consider a guard plus finally to always clear the global.
- try: - # Type checker workaround: assert that provider is not None after the check - provider = _logger_provider - assert provider is not None - provider.shutdown() - except Exception: - pass # Ignore shutdown errors - _logger_provider = None + try: + provider = _logger_provider + if provider is not None: + provider.shutdown() + except Exception: + pass # Ignore shutdown errors + finally: + _logger_provider = Nonesrc/codegen/cli/commands/claude/config/claude_session_stop_hook.py (1)
22-22: Type the ImportError fallback as Optional[Callable], notNoneAnnotating the symbol with
Nonedefeats static checking. Use an Optional[Callable] matching the real signature.Apply within this hunk:
-except ImportError: - update_claude_session_status: None = None +except ImportError: + update_claude_session_status: Optional[Callable[[str, str, Optional[int]], bool]] = NoneAdd this import near the top:
from typing import Callable, Optionalsrc/codegen/cli/commands/mcp/main.py (1)
8-11: Constraintransportto allowed values; clarify http fallbackValidate at the CLI boundary to prevent typos, and make the http fallback explicit in help.
-from codegen.cli.mcp_server.runner import run_server +from codegen.cli.mcp_server.runner import run_server +from typing import Literal @@ -def mcp( - transport: str = typer.Option("stdio", help="Transport type (stdio, http)"), - host: str = typer.Option("localhost", help="Host to bind to (for http transport)"), - port: int = typer.Option(8000, help="Port to bind to (for http transport)"), -): +def mcp( + transport: Literal["stdio", "http"] = typer.Option("stdio", help="Transport type: 'stdio' (default) or 'http' (not yet implemented; falls back to stdio)."), + host: str = typer.Option("localhost", help="Host (http transport only)"), + port: int = typer.Option(8000, help="Port (http transport only)"), +):Optionally echo a one-line warning when
transport=="http"before callingrun_server(...).src/codegen/cli/commands/org/tui.py (2)
152-153: Avoid blanketexcept Exceptionin file updateCatching all exceptions hides real errors (e.g., permission issues). Prefer narrowing to
OSErrorand log the error.
273-305: DRY: duplicate .env update logic across two classesBoth classes implement nearly identical
_update_env_file. Extract a shared helper (or reuse the one inorg/main.py) to reduce divergence.docs/settings/team-roles.mdx (1)
33-35: Minor duplication in Manager capabilities“Manage integrations” appears twice (lines 33–34). Consider removing one.
src/codegen/cli/tui/app.py (2)
604-607: Use error level with explicit traceback for non-zero Typer exits (reduce noise).
logger.exceptionimplies an unhandled crash; here it's a controlled failure path. Preferlogger.error(..., exc_info=True)to keep the traceback without elevating severity.- logger.exception( + logger.error( "Local pull failed via typer exit", extra={"operation": "tui.pull_branch", "agent_id": agent_id, "org_id": self.org_id, "duration_ms": duration_ms, "exit_code": e.exit_code, "success": False}, - ) + , exc_info=True)
611-614: Treat invalid user input as warning, not exception.
ValueErroronagent_idis user error; logging a full traceback is unnecessary and noisy.- logger.exception( + logger.warning( "Invalid agent ID for pull", extra={"operation": "tui.pull_branch", "agent_id": agent_id, "org_id": getattr(self, "org_id", None), "duration_ms": duration_ms, "error_type": "invalid_agent_id"}, - ) + )src/codegen/cli/commands/org/main.py (1)
129-129: Chain the original exception when exiting to aid diagnostics.Adopt Ruff B904 guidance so the root cause isn’t lost in tooling/logs.
- raise typer.Exit(1) + raise typer.Exit(1) from esrc/codegen/git/utils/language.py (5)
49-63: Solid SDK import fallback; normalize extensions to lowercase.Prevents case-sensitivity mismatches on files like “.PY” or “.TSX”.
EXTENSIONS = { ProgrammingLanguage.PYTHON: PyFile.get_extensions(), ProgrammingLanguage.TYPESCRIPT: TSFile.get_extensions(), } + # Normalize to lowercase for case-insensitive suffix checks + EXTENSIONS = {lang: {ext.lower() for ext in exts} for lang, exts in EXTENSIONS.items()}And mirror the same normalization in the fallback block:
EXTENSIONS = { ProgrammingLanguage.PYTHON: [".py", ".pyx", ".pyi"], ProgrammingLanguage.TYPESCRIPT: [".ts", ".tsx", ".js", ".jsx"], } + EXTENSIONS = {lang: {ext.lower() for ext in exts} for lang, exts in EXTENSIONS.items()}
86-89: Make the suffix comparison case-insensitive.- if file_path.suffix in exts: + if file_path.suffix.lower() in exts: language_counts[language] += 1
120-135: Mirror lowercase-extension normalization in git strategy.EXTENSIONS = { ProgrammingLanguage.PYTHON: PyFile.get_extensions(), ProgrammingLanguage.TYPESCRIPT: TSFile.get_extensions(), } + EXTENSIONS = {lang: {ext.lower() for ext in exts} for lang, exts in EXTENSIONS.items()}And in the fallback:
EXTENSIONS = { ProgrammingLanguage.PYTHON: [".py", ".pyx", ".pyi"], ProgrammingLanguage.TYPESCRIPT: [".ts", ".tsx", ".js", ".jsx"], } + EXTENSIONS = {lang: {ext.lower() for ext in exts} for lang, exts in EXTENSIONS.items()}
163-165: Case-insensitive suffix match here as well.- if file_path.suffix in exts: + if file_path.suffix.lower() in exts: language_counts[language] += 1
90-97: Docstring mismatch: function returns OTHER, not None.Update the docstring to reflect actual behavior.
src/codegen/cli/utils/repo.py (1)
172-174: Narrow broad exception to network errors (keep mock fallback).Prevents masking programming errors while preserving UX fallback.
- except Exception: + except requests.RequestException: # If API fails, return mock data return get_mock_repositories()src/codegen/cli/tui/agent_detail.py (3)
42-43: Remove unused variable.
summaryis assigned but not used.- summary = self.agent_run.get("summary", "No summary available")
233-238: Avoid broad try/except when creating remote; check first.Don’t swallow real errors; create remote only if missing.
- try: - git_cli.create_remote(remote_name, repo_clone_url) - except Exception: - # Remote might already exist - pass + if remote_name not in [r.name for r in git_cli.remotes]: + git_cli.create_remote(remote_name, repo_clone_url)
241-247: Consider narrowing checkout exception handling.Catching all Exceptions may hide unrelated errors; prefer GitPython’s
GitCommandErrorif available.src/codegen/cli/commands/claude/config/claude_session_hook.py (1)
23-24: Type the ImportError fallbacks as Optional[Callable]Annotating with
None-only types hinders static checking; prefer Optional[Callable].Apply:
- create_claude_session: None = None - resolve_org_id: None = None + create_claude_session: Optional[Callable[[str, Optional[int]], Optional[str]]] = None + resolve_org_id: Optional[Callable[[Optional[int]], Optional[int]]] = NoneAlso add at the top (near other imports):
from typing import Callable, Optionalsrc/codegen/cli/commands/claude/claude_log_watcher.py (3)
39-57: Use an Event to control thread shutdownReplacing the boolean with
threading.Eventavoids races and spurious wake-ups.Example patch:
- self.is_running = True - self.watcher_thread = threading.Thread(target=self._watch_loop, daemon=True) + self._stop = threading.Event() + self.watcher_thread = threading.Thread(target=self._watch_loop, daemon=True) self.watcher_thread.start()And in
stop():- self.is_running = False + if hasattr(self, "_stop"): + self._stop.set()And in
_watch_loop():- while self.is_running: + while not getattr(self, "_stop", None) or not self._stop.is_set(): try: - self._check_for_new_entries() - time.sleep(self.poll_interval) + self._check_for_new_entries() + # Wait with timeout so stop() is responsive + if self._stop.wait(self.poll_interval): + break
119-133: Avoid O(n) re-reads for large logsReading all lines on every poll scales poorly; consider tracking a byte offset and using incremental reads.
If staying line-based, cache
lines[start_line:end_line]without reading twice (you callread_existing_log_linesthenreadlines()).
171-175: Gate verbose per-entry prints behind a debug flagFrequent prints can flood terminals; make this conditional on an env var or a verbosity flag.
Example:
- console.print(f"📤 Sent log entry: {log_entry.get('type', 'unknown')}", style="dim") + if os.environ.get("CODEGEN_VERBOSE_LOGS") == "1": + console.print(f"📤 Sent log entry: {log_entry.get('type', 'unknown')}", style="dim")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
QUICK_START_LOGGING.md(6 hunks)docs/docs.json(1 hunks)docs/settings/team-roles.mdx(1 hunks)pyproject.toml(1 hunks)src/codegen/cli/cli.py(2 hunks)src/codegen/cli/commands/claude/__init__.py(0 hunks)src/codegen/cli/commands/claude/claude_log_utils.py(4 hunks)src/codegen/cli/commands/claude/claude_log_watcher.py(9 hunks)src/codegen/cli/commands/claude/claude_session_api.py(1 hunks)src/codegen/cli/commands/claude/config/claude_session_active_hook.py(1 hunks)src/codegen/cli/commands/claude/config/claude_session_hook.py(2 hunks)src/codegen/cli/commands/claude/config/claude_session_stop_hook.py(1 hunks)src/codegen/cli/commands/claude/main.py(1 hunks)src/codegen/cli/commands/claude/quiet_console.py(1 hunks)src/codegen/cli/commands/config/telemetry.py(0 hunks)src/codegen/cli/commands/mcp/__init__.py(1 hunks)src/codegen/cli/commands/mcp/main.py(1 hunks)src/codegen/cli/commands/org/__init__.py(1 hunks)src/codegen/cli/commands/org/main.py(3 hunks)src/codegen/cli/commands/org/tui.py(11 hunks)src/codegen/cli/commands/repo/__init__.py(1 hunks)src/codegen/cli/commands/repo/main.py(7 hunks)src/codegen/cli/commands/repo/tui.py(8 hunks)src/codegen/cli/mcp_server/api_client.py(1 hunks)src/codegen/cli/mcp_server/tools/executor.py(0 hunks)src/codegen/cli/telemetry/consent.py(0 hunks)src/codegen/cli/telemetry/debug_exporter.py(4 hunks)src/codegen/cli/telemetry/exception_logger.py(1 hunks)src/codegen/cli/telemetry/otel_setup.py(1 hunks)src/codegen/cli/tui/agent_detail.py(12 hunks)src/codegen/cli/tui/app.py(4 hunks)src/codegen/cli/tui/codegen_theme.tcss(2 hunks)src/codegen/cli/utils/inplace_print.py(1 hunks)src/codegen/cli/utils/org.py(1 hunks)src/codegen/cli/utils/repo.py(6 hunks)src/codegen/git/repo_operator/repo_operator.py(4 hunks)src/codegen/git/schemas/repo_config.py(1 hunks)src/codegen/git/utils/language.py(2 hunks)src/codegen/git/utils/pr_review.py(1 hunks)src/codegen/shared/logging/get_logger.py(1 hunks)tests/unit/codegen/agents/test_agent.py(1 hunks)tests/unit/codegen/agents/test_usage_demo.py(1 hunks)tests/unit/codegen/test_top_level_imports.py(1 hunks)
💤 Files with no reviewable changes (4)
- src/codegen/cli/commands/config/telemetry.py
- src/codegen/cli/telemetry/consent.py
- src/codegen/cli/mcp_server/tools/executor.py
- src/codegen/cli/commands/claude/init.py
🧰 Additional context used
🧬 Code graph analysis (21)
tests/unit/codegen/agents/test_agent.py (1)
src/codegen/agents/agent.py (1)
Agent(46-100)
src/codegen/cli/commands/claude/config/claude_session_active_hook.py (1)
src/codegen/cli/commands/claude/claude_session_api.py (1)
update_claude_session_status(88-140)
src/codegen/shared/logging/get_logger.py (1)
src/codegen/configs/models/telemetry.py (1)
TelemetryConfig(6-29)
tests/unit/codegen/agents/test_usage_demo.py (1)
src/codegen/agents/agent.py (1)
Agent(46-100)
src/codegen/cli/commands/mcp/main.py (1)
src/codegen/cli/mcp_server/runner.py (1)
run_server(10-43)
src/codegen/git/utils/language.py (1)
src/codegen/shared/enums/programming_language.py (1)
ProgrammingLanguage(4-8)
src/codegen/cli/commands/repo/main.py (1)
src/codegen/cli/utils/repo.py (6)
clear_repo_env_variables(76-81)ensure_repositories_cached(195-225)get_current_repo_id(45-47)get_repo_env_status(50-55)set_repo_env_variable(58-73)update_env_file_with_repo(84-117)
src/codegen/cli/telemetry/exception_logger.py (2)
src/codegen/cli/telemetry/otel_setup.py (2)
get_otel_logging_handler(262-276)get_session_uuid(279-286)src/codegen/shared/logging/get_logger.py (1)
get_logger(103-106)
src/codegen/cli/commands/claude/config/claude_session_hook.py (2)
src/codegen/cli/commands/claude/claude_session_api.py (1)
create_claude_session(27-85)src/codegen/cli/utils/org.py (1)
resolve_org_id(22-116)
src/codegen/cli/commands/claude/config/claude_session_stop_hook.py (1)
src/codegen/cli/commands/claude/claude_session_api.py (1)
update_claude_session_status(88-140)
src/codegen/cli/telemetry/otel_setup.py (1)
src/codegen/cli/telemetry/debug_exporter.py (2)
shutdown(104-119)shutdown(164-166)
src/codegen/cli/commands/org/main.py (2)
src/codegen/cli/commands/org/tui.py (2)
_update_env_file(117-153)_update_env_file(273-309)src/codegen/cli/commands/repo/tui.py (2)
_update_env_file(107-141)_update_env_file(250-284)
src/codegen/git/schemas/repo_config.py (2)
src/codegen/configs/models/repository.py (1)
base_dir(28-29)src/codegen/shared/enums/programming_language.py (1)
ProgrammingLanguage(4-8)
src/codegen/cli/utils/org.py (2)
src/codegen/cli/auth/token_manager.py (2)
get_cached_organizations(177-189)get_cached_organizations(324-331)src/codegen/cli/commands/org/main.py (1)
org(14-42)
src/codegen/cli/utils/repo.py (2)
src/codegen/cli/auth/token_manager.py (3)
get_current_token(263-301)cache_repositories(386-403)get_cached_repositories(373-383)src/codegen/cli/utils/org.py (1)
resolve_org_id(22-116)
src/codegen/cli/commands/org/tui.py (3)
src/codegen/cli/utils/org.py (1)
resolve_org_id(22-116)src/codegen/cli/commands/org/main.py (2)
org(14-42)_update_env_file(89-119)src/codegen/cli/commands/repo/tui.py (4)
_update_env_file(107-141)_update_env_file(250-284)_close_screen(143-148)action_quit(150-152)
tests/unit/codegen/test_top_level_imports.py (1)
src/codegen/agents/agent.py (1)
Agent(46-100)
src/codegen/cli/cli.py (1)
src/codegen/cli/commands/mcp/main.py (1)
mcp(7-13)
src/codegen/cli/tui/agent_detail.py (3)
src/codegen/cli/utils/org.py (1)
resolve_org_id(22-116)src/codegen/git/repo_operator/local_git_repo.py (1)
LocalGitRepo(15-84)src/codegen/git/repo_operator/repo_operator.py (1)
create_remote(345-350)
src/codegen/cli/commands/claude/claude_log_watcher.py (2)
src/codegen/cli/commands/claude/claude_log_utils.py (5)
format_log_for_api(115-126)get_claude_session_log_path(24-39)parse_jsonl_line(42-58)read_existing_log_lines(75-91)validate_log_entry(94-112)src/codegen/cli/commands/claude/claude_session_api.py (1)
send_claude_session_log(143-193)
src/codegen/cli/commands/repo/tui.py (3)
src/codegen/cli/utils/repo.py (2)
ensure_repositories_cached(195-225)get_current_repo_id(45-47)src/codegen/cli/commands/org/tui.py (2)
_update_env_file(117-153)_update_env_file(273-309)src/codegen/cli/commands/org/main.py (1)
_update_env_file(89-119)
🪛 Ruff (0.12.2)
src/codegen/cli/commands/repo/main.py
148-148: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
149-149: Do not catch blind exception: Exception
(BLE001)
151-151: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
src/codegen/cli/commands/org/main.py
129-129: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
src/codegen/cli/utils/repo.py
113-113: Consider moving this statement to an else block
(TRY300)
115-115: Do not catch blind exception: Exception
(BLE001)
163-163: Probable use of requests call without timeout
(S113)
172-172: Do not catch blind exception: Exception
(BLE001)
src/codegen/cli/commands/org/tui.py
150-150: Consider moving this statement to an else block
(TRY300)
306-306: Consider moving this statement to an else block
(TRY300)
308-308: Do not catch blind exception: Exception
(BLE001)
tests/unit/codegen/test_top_level_imports.py
40-40: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
41-41: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
src/codegen/cli/tui/agent_detail.py
42-42: Local variable summary is assigned to but never used
Remove assignment to unused variable summary
(F841)
128-128: Probable use of requests call without timeout
(S113)
236-238: try-except-pass detected, consider logging the exception
(S110)
236-236: Do not catch blind exception: Exception
(BLE001)
244-244: Do not catch blind exception: Exception
(BLE001)
308-308: Do not catch blind exception: Exception
(BLE001)
src/codegen/cli/commands/repo/tui.py
138-138: Consider moving this statement to an else block
(TRY300)
140-140: Do not catch blind exception: Exception
(BLE001)
281-281: Consider moving this statement to an else block
(TRY300)
283-283: 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Sourcery review
🔇 Additional comments (38)
tests/unit/codegen/agents/test_agent.py (1)
76-76: Cannot verify — ripgrep skipped files; re-run these checks'missing-argument' is not a mypy code. Use 'call-arg' (mypy) or remove the tool-specific ignore.
- Agent() # type: ignore[missing-argument] # Missing required token parameter + Agent() # type: ignore[call-arg] # Missing required token parameterRun these and paste results:
# find type: ignore[...] occurrences (avoid glob filters that skip files) rg -n "type:\s*ignore\[[^\]]+\]" || true # check for pyright config fd -a pyrightconfig.json || true # check for mypy config sections rg -n "^\s*\[tool\.mypy\]" pyproject.toml setup.cfg mypy.ini 2>/dev/null || truetests/unit/codegen/test_top_level_imports.py (2)
10-20: Top-level alias identity check looks goodVerifies the public facade matches the submodule export. No issues.
26-31: all visibility checks are appropriateGood guardrails to keep the package surface stable.
src/codegen/cli/commands/repo/__init__.py (1)
5-5: LGTM: consistent export and trailing newline.
Matches org/repo pattern and keeps API unchanged.pyproject.toml (1)
28-28: No change required — [email protected] is current and compatible with fastmcp.
PyPI shows mcp-python 0.1.4 (uploaded Jan 8, 2025); there are no newer releases or breaking changes to compare, and fastmcp is a separate package that can coexist.src/codegen/cli/commands/org/__init__.py (1)
5-5: LGTM: consistent export and trailing newline.
No behavioral changes; consistent with repo/init.py.src/codegen/cli/commands/claude/quiet_console.py (1)
11-11: LGTM: harmless whitespace tidy-up.
No runtime changes; module behavior intact.src/codegen/cli/utils/inplace_print.py (1)
2-2: LGTM: switch to collections.abc.Iterable for 3.12+.
Correct modern typing; no behavior change.src/codegen/shared/logging/get_logger.py (1)
67-67: LGTM: import reordering is safeKeeps telemetry config import within the guarded block; no behavior change.
src/codegen/cli/utils/org.py (1)
36-50: LGTM: cache validation path reads cleanlyWhitespace tidy-up is fine; the “not in cache” message with available orgs improves UX.
src/codegen/cli/telemetry/exception_logger.py (1)
12-14: LGTM: import orderReordering otel setup imports ahead of logger helps avoid cycles; behavior unchanged.
src/codegen/cli/tui/codegen_theme.tcss (1)
3-7: LGTM: formatting onlyComment header cleanup and EOF newline; no semantic changes.
Also applies to: 185-185
src/codegen/cli/telemetry/debug_exporter.py (1)
10-10: LGTM: timestamps moved to UTCUsing
datetime.now(timezone.utc)for session id and markers improves consistency across environments.Also applies to: 36-36, 45-46, 113-113
docs/docs.json (1)
62-67: Ensure new nav entries resolve (avoid 404s)Both pages are present and listed in navigation: docs/settings/team-roles.mdx and docs/settings/agent-permissions.mdx (asset: docs/images/agent-permissions.png).
src/codegen/cli/cli.py (2)
15-15: LGTM: command wiringImporting and exposing the new
mcpsubcommand is correct.
69-69: LGTM: user-facing helpHelp text is clear and consistent with other commands.
src/codegen/cli/commands/org/tui.py (3)
33-35: LGTM: clear no-org warningGood UX: actionable message when cache is empty.
55-56: LGTM: helpful persistence hintThe post-table hint about updating CODEGEN_ORG_ID is concise and useful.
130-134: LGTM: newline normalizationEnsuring every line ends with
\navoids accidental concatenation on writeback.src/codegen/git/repo_operator/repo_operator.py (1)
760-772: Log a warning when PR not found in create_pr_commentWhen get_pull_safe(pr_number) returns None, log a concise warning to aid debugging; docstring already marks the return Optional. Repo search found no internal callers; absence of matches isn't proof—ensure external consumers handle None.
Location: src/codegen/git/repo_operator/repo_operator.py:760-772
pr = self.remote_git_repo.get_pull_safe(pr_number) if pr: comment = self.remote_git_repo.create_issue_comment(pr, body) return comment - return None + logger.warning(f"PR not found; cannot create comment (pr_number={pr_number})") + return Nonesrc/codegen/cli/tui/app.py (3)
888-889: Good switch to logger.exception for unexpected browser errors.Keeps the traceback for diagnostics while preserving user feedback.
985-986: Good: preserve traceback when failing to open PR.The exception context will help diagnose platform/browser issues.
998-999: Good: preserve traceback when failing to open trace.Consistent with the PR’s logging intent.
src/codegen/cli/commands/org/main.py (1)
101-105: EOF newline normalization looks good.Ensures robust round-tripping of .env content.
src/codegen/cli/commands/claude/claude_log_utils.py (4)
42-59: Typing update LGTM.
Optional[dict[str, Any]]for parsed JSON lines is appropriate; error handling is conservative.
88-89: File open change LGTM.Dropping explicit mode while adding encoding is fine.
94-113: Validation signature and logic LGTM.Straightforward checks and future-proof for stricter schemas.
115-126: Pass-through formatter LGTM.Keeps surface compatible while allowing future transforms.
src/codegen/cli/utils/repo.py (3)
50-55: Typing modernization LGTM.Return-type switch to built-in generics is consistent with PEP 585.
91-95: Safe read of .env LGTM.Keeps behavior consistent with other modules.
195-226: Caching flow LGTM.Reasonable fallback to mock data and cache population.
src/codegen/cli/tui/agent_detail.py (2)
32-37: Constructor typing and state init LGTM.Aligns with built-in generics and optional org resolution.
266-268: Open-in-browser flow LGTM.Good user feedback via notifications.
src/codegen/cli/commands/claude/claude_session_api.py (1)
13-14: Import move is fineReordering the
consoleimport belowresolve_org_idis harmless here. No functional impact.src/codegen/cli/commands/claude/main.py (1)
298-301: Good switch to logger.exceptionUsing
logger.exception(...)preserves the stack trace for FileNotFoundError paths.src/codegen/cli/commands/claude/config/claude_session_hook.py (1)
68-70: Guarded API call is correctThe
create_claude_sessioncheck prevents calling a missing symbol when the import fails.src/codegen/cli/commands/repo/tui.py (2)
11-12: Scoped imports look goodUsing only
ensure_repositories_cachedandget_current_repo_idtightens dependencies.
32-34: Clear “no repositories” messageConcise single-line container is fine.
| - [Agent Permissions](/settings/agent-permissions) - Configure what actions agents can perform | ||
| - [Organization Rules](/settings/organization-rules) - Set organization-wide rules | ||
| - [Repository Rules](/settings/repo-rules) - Configure repository-specific behavior |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Broken/undeclared link: Organization Rules
/settings/organization-rules isn’t in docs.json navigation. Either add the page to docs/docs.json or update the link to an existing page.
Run:
🏁 Script executed:
#!/bin/bash
fd -a 'organization-rules.*' docs || true
jq -r '.navigation.tabs[0].groups[]?|select(.group=="Settings")|.pages' docs/docs.jsonLength of output: 174
Add missing navigation entry: Organization Rules
docs/settings/team-roles.mdx (lines 109–111) links to /settings/organization-rules; docs/settings/organization-rules.mdx exists but "settings/organization-rules" is not listed in docs/docs.json (navigation.tabs[0].groups[] where group == "Settings"). Add "settings/organization-rules" to that pages array or update the link to an existing page.
🤖 Prompt for AI Agents
In docs/settings/team-roles.mdx around lines 109 to 111 the page links to
/settings/organization-rules but the route "settings/organization-rules" is
missing from the documentation navigation; open docs/docs.json, locate
navigation.tabs[0].groups[] item where group == "Settings" and add
"settings/organization-rules" to that group's pages array (or alternatively
change the link in team-roles.mdx to an existing listed page); ensure the path
string matches the existing routing format used elsewhere in docs.json.
| # Before API call | ||
| logger.info("Making API request", extra={ | ||
| "operation": "api.request", | ||
| "endpoint": "agent/run", | ||
| "org_id": org_id | ||
| }) | ||
| logger.info("Making API request", extra={"operation": "api.request", "endpoint": "agent/run", "org_id": org_id}) | ||
|
|
||
| # After successful API call | ||
| logger.info("API request successful", extra={ | ||
| "operation": "api.request", | ||
| "endpoint": "agent/run", | ||
| "response_id": response.get("id"), | ||
| "status_code": response.status_code | ||
| }) | ||
| logger.info("API request successful", extra={"operation": "api.request", "endpoint": "agent/run", "response_id": response.get("id"), "status_code": response.status_code}) | ||
| ``` |
There was a problem hiding this comment.
Fix incorrect Response usage in example
requests.Response doesn’t support .get(...). Use response.json().get(...) or the parsed dict.
-logger.info("API request successful", extra={"operation": "api.request", "endpoint": "agent/run", "response_id": response.get("id"), "status_code": response.status_code})
+logger.info(
+ "API request successful",
+ extra={
+ "operation": "api.request",
+ "endpoint": "agent/run",
+ "response_id": response.json().get("id"),
+ "status_code": response.status_code,
+ },
+)Optionally align this with the agent example that assigns agent_run_data = response.json().
📝 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.
| # Before API call | |
| logger.info("Making API request", extra={ | |
| "operation": "api.request", | |
| "endpoint": "agent/run", | |
| "org_id": org_id | |
| }) | |
| logger.info("Making API request", extra={"operation": "api.request", "endpoint": "agent/run", "org_id": org_id}) | |
| # After successful API call | |
| logger.info("API request successful", extra={ | |
| "operation": "api.request", | |
| "endpoint": "agent/run", | |
| "response_id": response.get("id"), | |
| "status_code": response.status_code | |
| }) | |
| logger.info("API request successful", extra={"operation": "api.request", "endpoint": "agent/run", "response_id": response.get("id"), "status_code": response.status_code}) | |
| ``` | |
| # Before API call | |
| logger.info("Making API request", extra={"operation": "api.request", "endpoint": "agent/run", "org_id": org_id}) | |
| # After successful API call | |
| logger.info( | |
| "API request successful", | |
| extra={ | |
| "operation": "api.request", | |
| "endpoint": "agent/run", | |
| "response_id": response.json().get("id"), | |
| "status_code": response.status_code, | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In QUICK_START_LOGGING.md around lines 79 to 84, the example incorrectly calls
response.get(...) on a requests.Response object; parse the JSON first (e.g.,
agent_run_data = response.json()) or call response.json().get(...) and then use
that parsed dict for response_id and any other fields so the logging example
uses a dict (response_id = agent_run_data.get("id")) instead of
response.get(...).
| self.notify("i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") | ||
|
|
There was a problem hiding this comment.
Fix f-string bug: literal {org_id} shown to users
self.notify("...{org_id}...") should be an f-string; currently prints the braces verbatim.
- self.notify("i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence")
+ self.notify(f"i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence")📝 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.
| self.notify("i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") | |
| self.notify(f"i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") |
🤖 Prompt for AI Agents
In src/codegen/cli/commands/org/tui.py around lines 112 to 113, the notify call
uses a plain string with "{org_id}" so the literal braces are printed; change it
to an f-string that interpolates the org_id variable (e.g., prefix the string
with f and include {org_id} in the message) so the actual organization ID is
shown to users.
| self.notify("i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") | ||
|
|
There was a problem hiding this comment.
Fix f-string bug (duplicate occurrence)
Same issue as above in OrgSelectorApp.
- self.notify("i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence")
+ self.notify(f"i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence")📝 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.
| self.notify("i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") | |
| self.notify(f"i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") |
🤖 Prompt for AI Agents
In src/codegen/cli/commands/org/tui.py around lines 268-269, the notification
string incorrectly uses a literal "{org_id}" (or a duplicated formatting)
instead of interpolating the org_id; change the message to use a single f-string
that injects the org_id (e.g. prefix the string with f and include {org_id}
once) so the notification shows the actual org id and remove any
duplicate/incorrect formatting.
| console.print("[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | ||
|
|
There was a problem hiding this comment.
Fix missing interpolation in user message
{repo_id} won’t interpolate without an f-string.
Apply:
- console.print("[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")
+ console.print(f"[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.print("[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | |
| console.print(f"[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") |
🤖 Prompt for AI Agents
In src/codegen/cli/commands/repo/main.py around lines 117-118 the console.print
message uses a literal "{repo_id}" so the repository id won't be interpolated;
update the call to use an f-string (or .format) to inject the repo_id variable
(e.g. console.print(f"...{repo_id}...")) so the actual ID appears in the
message.
| self.notify(f"✓ Set repository: {repo_name} (ID: {repo_id})") | ||
| self.notify("ℹ Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | ||
| self.notify("i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | ||
|
|
There was a problem hiding this comment.
Fix missing interpolation in TUI notification
{repo_id} is not formatted.
Apply:
- self.notify("i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")
+ self.notify(f"i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")📝 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.
| self.notify(f"✓ Set repository: {repo_name} (ID: {repo_id})") | |
| self.notify("ℹ Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | |
| self.notify("i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | |
| self.notify(f"✓ Set repository: {repo_name} (ID: {repo_id})") | |
| self.notify(f"i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") |
🤖 Prompt for AI Agents
In src/codegen/cli/commands/repo/tui.py around lines 101 to 103, the
notification message that instructs adding CODEGEN_REPO_ID to the shell contains
a literal "{repo_id}" instead of inserting the repo_id value; change the call to
use string interpolation (e.g., an f-string or .format) so the actual repo_id is
included in the message, ensuring the notify call outputs the correct value for
persistence instructions.
| self.notify("i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | ||
|
|
There was a problem hiding this comment.
Fix missing interpolation in App notification
Same formatting issue in App variant.
Apply:
- self.notify("i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")
+ self.notify(f"i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")📝 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.
| self.notify("i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | |
| self.notify(f"i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") |
🤖 Prompt for AI Agents
In src/codegen/cli/commands/repo/tui.py around lines 245-246, the notify call
prints a literal "{repo_id}" instead of the variable's value; update the string
to interpolate the repo_id (e.g., use an f-string or .format) so the actual
repository id is inserted into "Add 'export CODEGEN_REPO_ID=...' to your shell
for persistence" before calling self.notify.
| response = requests.get(url, headers=headers) | ||
| response.raise_for_status() | ||
| self.agent_data = response.json() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout to detail fetch.
Prevents TUI from hanging on network issues.
- response = requests.get(url, headers=headers)
+ response = requests.get(url, headers=headers, timeout=10)📝 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.
| response = requests.get(url, headers=headers) | |
| response.raise_for_status() | |
| self.agent_data = response.json() | |
| response = requests.get(url, headers=headers, timeout=10) | |
| response.raise_for_status() | |
| self.agent_data = response.json() |
🧰 Tools
🪛 Ruff (0.12.2)
128-128: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
In src/codegen/cli/tui/agent_detail.py around lines 128 to 131, the requests.get
call that fetches agent details has no timeout and can hang on network issues;
add a timeout argument (e.g. timeout=5 or configurable) to requests.get and
handle potential timeout/RequestException by catching it, logging or showing a
user-friendly error in the TUI, and ensuring the code still calls
response.raise_for_status()/response.json() only when a response was
successfully returned.
| response = requests.get(url, headers=headers) | ||
|
|
||
| if response.status_code == 200: | ||
| data = response.json() | ||
| return data.get("items", []) | ||
| else: | ||
| # API endpoint doesn't exist yet, return mock data for demo | ||
| return get_mock_repositories() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout to repository API call.
Avoids CLI stalls on slow networks.
- response = requests.get(url, headers=headers)
+ response = requests.get(url, headers=headers, timeout=10)🧰 Tools
🪛 Ruff (0.12.2)
163-163: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
In src/codegen/cli/utils/repo.py around lines 163-170, the requests.get call
needs a timeout to prevent CLI stalls; change the code to call requests.get(url,
headers=headers, timeout=10) (or another sensible timeout), wrap the request in
a try/except catching requests.exceptions.RequestException, and on exception
return get_mock_repositories() (and optionally log a warning); keep the existing
behavior for non-200 responses.
| path = default_repo_config.path or os.getcwd() | ||
| language = default_repo_config.language or "python" | ||
| return RepoConfig( | ||
| name=default_repo_config.name, | ||
| full_name=default_repo_config.full_name, | ||
| base_dir=os.path.dirname(default_repo_config.path), | ||
| language=ProgrammingLanguage(default_repo_config.language.upper()), | ||
| base_dir=os.path.dirname(path), | ||
| language=ProgrammingLanguage(language.upper()), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden enum construction to avoid ValueError on unknown language
ProgrammingLanguage(language.upper()) will raise if an unexpected value is present. Guard and default to OTHER.
- path = default_repo_config.path or os.getcwd()
- language = default_repo_config.language or "python"
+ path = default_repo_config.path or os.getcwd()
+ language_str = (default_repo_config.language or "python").upper()
+ try:
+ language_enum = ProgrammingLanguage(language_str)
+ except ValueError:
+ logger.warning("Unknown language '%s'; defaulting to OTHER", language_str)
+ language_enum = ProgrammingLanguage.OTHER
return RepoConfig(
name=default_repo_config.name,
full_name=default_repo_config.full_name,
- base_dir=os.path.dirname(path),
- language=ProgrammingLanguage(language.upper()),
+ base_dir=os.path.dirname(path),
+ language=language_enum,
)📝 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.
| path = default_repo_config.path or os.getcwd() | |
| language = default_repo_config.language or "python" | |
| return RepoConfig( | |
| name=default_repo_config.name, | |
| full_name=default_repo_config.full_name, | |
| base_dir=os.path.dirname(default_repo_config.path), | |
| language=ProgrammingLanguage(default_repo_config.language.upper()), | |
| base_dir=os.path.dirname(path), | |
| language=ProgrammingLanguage(language.upper()), | |
| ) | |
| path = default_repo_config.path or os.getcwd() | |
| language_str = (default_repo_config.language or "python").upper() | |
| try: | |
| language_enum = ProgrammingLanguage(language_str) | |
| except ValueError: | |
| logger.warning("Unknown language '%s'; defaulting to OTHER", language_str) | |
| language_enum = ProgrammingLanguage.OTHER | |
| return RepoConfig( | |
| name=default_repo_config.name, | |
| full_name=default_repo_config.full_name, | |
| base_dir=os.path.dirname(path), | |
| language=language_enum, | |
| ) |
🤖 Prompt for AI Agents
In src/codegen/git/schemas/repo_config.py around lines 34 to 41, constructing
the enum with ProgrammingLanguage(language.upper()) can raise ValueError for
unknown values; change this to a safe mapping that falls back to
ProgrammingLanguage.OTHER (e.g., check if language.upper() is a valid member
name via ProgrammingLanguage.__members__ or use a try/except around the enum
construction and set to ProgrammingLanguage.OTHER on failure) so unknown/invalid
language strings do not crash the code.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 56 files
Prompt for AI agents (all 11 issues)
Understand the root cause of the following 11 issues and fix them.
<file name="src/codegen/cli/commands/repo/tui.py">
<violation number="1" location="src/codegen/cli/commands/repo/tui.py:102">
User message does not interpolate repo_id; string lacks f prefix, so the placeholder is shown literally.</violation>
</file>
<file name="src/codegen/cli/commands/claude/config/claude_session_active_hook.py">
<violation number="1" location="src/codegen/cli/commands/claude/config/claude_session_active_hook.py:22">
Type annotation constrains update_claude_session_status to None, conflicting with callable import path and likely breaking static type checking; remove or correct the annotation.</violation>
</file>
<file name="src/codegen/cli/commands/org/tui.py">
<violation number="1" location="src/codegen/cli/commands/org/tui.py:112">
The message shows {org_id} literally because the string is not formatted; use an f-string to interpolate the org_id.</violation>
</file>
<file name="src/codegen/git/utils/pr_review.py">
<violation number="1" location="src/codegen/git/utils/pr_review.py:134">
Catching ImportError is too broad for this intent and can mask missing/renamed symbol errors; catch ModuleNotFoundError to only handle a missing SDK package.</violation>
</file>
<file name="src/codegen/cli/utils/repo.py">
<violation number="1" location="src/codegen/cli/utils/repo.py:135">
Empty string environment variables are treated as active; mark empty values as inactive for accurate status.</violation>
</file>
<file name="src/codegen/cli/commands/claude/claude_log_utils.py">
<violation number="1" location="src/codegen/cli/commands/claude/claude_log_utils.py:42">
Function can return non-dict JSON from json.loads, but annotation specifies Optional[dict[str, Any]]; adjust the annotation or enforce dict-only returns.</violation>
</file>
<file name="src/codegen/cli/commands/claude/config/claude_session_hook.py">
<violation number="1" location="src/codegen/cli/commands/claude/config/claude_session_hook.py:23">
Variable is annotated as type None but used as a callable, which can confuse or break static type checking; remove the incorrect annotation.</violation>
<violation number="2" location="src/codegen/cli/commands/claude/config/claude_session_hook.py:24">
Variable is annotated as type None but intended to refer to a callable; remove the incorrect annotation to avoid type-checking confusion.</violation>
</file>
<file name="src/codegen/cli/commands/claude/config/claude_session_stop_hook.py">
<violation number="1" location="src/codegen/cli/commands/claude/config/claude_session_stop_hook.py:22">
Incorrect type annotation narrows update_claude_session_status to None instead of Optional[Callable], conflicting with its callable usage and likely breaking type checking.</violation>
</file>
<file name="QUICK_START_LOGGING.md">
<violation number="1" location="QUICK_START_LOGGING.md:83">
response.get(...) conflicts with response.status_code; use response.json().get("id") to access the JSON id from a requests.Response</violation>
</file>
<file name="src/codegen/cli/commands/repo/main.py">
<violation number="1" location="src/codegen/cli/commands/repo/main.py:117">
The message shows {repo_id} literally; without an f-string the export command won't include the actual repository ID.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| self.notify(f"✓ Set repository: {repo_name} (ID: {repo_id})") | ||
| self.notify("ℹ Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | ||
| self.notify("i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") |
There was a problem hiding this comment.
User message does not interpolate repo_id; string lacks f prefix, so the placeholder is shown literally.
Prompt for AI agents
Address the following comment on src/codegen/cli/commands/repo/tui.py at line 102:
<comment>User message does not interpolate repo_id; string lacks f prefix, so the placeholder is shown literally.</comment>
<file context>
@@ -103,59 +90,59 @@ def _set_repository(self, repo_id: int, repo_name: str) -> None:
self.notify(f"✓ Set repository: {repo_name} (ID: {repo_id})")
- self.notify("ℹ Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")
-
+ self.notify("i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")
+
# Wait a moment for user to see the notifications, then exit
</file context>
| self.notify("i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | |
| self.notify(f"i Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") |
| from codegen.cli.commands.claude.claude_session_api import update_claude_session_status | ||
| except ImportError: | ||
| update_claude_session_status = None | ||
| update_claude_session_status: None = None |
There was a problem hiding this comment.
Type annotation constrains update_claude_session_status to None, conflicting with callable import path and likely breaking static type checking; remove or correct the annotation.
Prompt for AI agents
Address the following comment on src/codegen/cli/commands/claude/config/claude_session_active_hook.py at line 22:
<comment>Type annotation constrains update_claude_session_status to None, conflicting with callable import path and likely breaking static type checking; remove or correct the annotation.</comment>
<file context>
@@ -19,7 +19,7 @@
from codegen.cli.commands.claude.claude_session_api import update_claude_session_status
except ImportError:
- update_claude_session_status = None
+ update_claude_session_status: None = None
</file context>
| update_claude_session_status: None = None | |
| update_claude_session_status = None |
| self.notify(f"✓ Set organization: {org_name} (ID: {org_id})") | ||
| self.notify("ℹ Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") | ||
| self.notify("i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") |
There was a problem hiding this comment.
The message shows {org_id} literally because the string is not formatted; use an f-string to interpolate the org_id.
Prompt for AI agents
Address the following comment on src/codegen/cli/commands/org/tui.py at line 112:
<comment>The message shows {org_id} literally because the string is not formatted; use an f-string to interpolate the org_id.</comment>
<file context>
@@ -106,24 +100,24 @@ def _set_organization(self, org_id: int, org_name: str) -> None:
self.notify(f"✓ Set organization: {org_name} (ID: {org_id})")
- self.notify("ℹ Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence")
-
+ self.notify("i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence")
+
# Wait a moment for user to see the notifications, then close
</file context>
| self.notify("i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") | |
| self.notify(f"i Add 'export CODEGEN_ORG_ID={org_id}' to your shell for persistence") |
| from codegen.sdk.core.file import SourceFile | ||
| try: | ||
| from codegen.sdk.core.file import SourceFile | ||
| except ImportError: |
There was a problem hiding this comment.
Catching ImportError is too broad for this intent and can mask missing/renamed symbol errors; catch ModuleNotFoundError to only handle a missing SDK package.
Prompt for AI agents
Address the following comment on src/codegen/git/utils/pr_review.py at line 134:
<comment>Catching ImportError is too broad for this intent and can mask missing/renamed symbol errors; catch ModuleNotFoundError to only handle a missing SDK package.</comment>
<file context>
@@ -129,7 +129,11 @@ def is_modified(self, editable: "Editable") -> bool:
- from codegen.sdk.core.file import SourceFile
+ try:
+ from codegen.sdk.core.file import SourceFile
+ except ImportError:
+ # If SDK is not available, return empty list
+ return []
</file context>
| except ImportError: | |
| except ModuleNotFoundError: |
| "status": "active" if value != "Not set" else "inactive" | ||
| }) | ||
|
|
||
| info.append({"label": f"{var_name}", "value": value, "status": "active" if value != "Not set" else "inactive"}) |
There was a problem hiding this comment.
Empty string environment variables are treated as active; mark empty values as inactive for accurate status.
Prompt for AI agents
Address the following comment on src/codegen/cli/utils/repo.py at line 135:
<comment>Empty string environment variables are treated as active; mark empty values as inactive for accurate status.</comment>
<file context>
@@ -87,107 +87,96 @@ def update_env_file_with_repo(repo_id: int, env_file_path: str = ".env") -> bool
- "status": "active" if value != "Not set" else "inactive"
- })
-
+ info.append({"label": f"{var_name}", "value": value, "status": "active" if value != "Not set" else "inactive"})
+
return info
</file context>
| info.append({"label": f"{var_name}", "value": value, "status": "active" if value != "Not set" else "inactive"}) | |
| info.append({"label": f"{var_name}", "value": value, "status": "active" if value not in ("", "Not set") else "inactive"}) |
| except ImportError: | ||
| create_claude_session = None | ||
| create_claude_session: None = None | ||
| resolve_org_id: None = None |
There was a problem hiding this comment.
Variable is annotated as type None but intended to refer to a callable; remove the incorrect annotation to avoid type-checking confusion.
Prompt for AI agents
Address the following comment on src/codegen/cli/commands/claude/config/claude_session_hook.py at line 24:
<comment>Variable is annotated as type None but intended to refer to a callable; remove the incorrect annotation to avoid type-checking confusion.</comment>
<file context>
@@ -20,7 +20,8 @@
except ImportError:
- create_claude_session = None
+ create_claude_session: None = None
+ resolve_org_id: None = None
</file context>
| resolve_org_id: None = None | |
| resolve_org_id = None |
| from codegen.cli.utils.org import resolve_org_id | ||
| except ImportError: | ||
| create_claude_session = None | ||
| create_claude_session: None = None |
There was a problem hiding this comment.
Variable is annotated as type None but used as a callable, which can confuse or break static type checking; remove the incorrect annotation.
Prompt for AI agents
Address the following comment on src/codegen/cli/commands/claude/config/claude_session_hook.py at line 23:
<comment>Variable is annotated as type None but used as a callable, which can confuse or break static type checking; remove the incorrect annotation.</comment>
<file context>
@@ -20,7 +20,8 @@
from codegen.cli.utils.org import resolve_org_id
except ImportError:
- create_claude_session = None
+ create_claude_session: None = None
+ resolve_org_id: None = None
</file context>
| create_claude_session: None = None | |
| create_claude_session = None |
| from codegen.cli.commands.claude.claude_session_api import update_claude_session_status | ||
| except ImportError: | ||
| update_claude_session_status = None | ||
| update_claude_session_status: None = None |
There was a problem hiding this comment.
Incorrect type annotation narrows update_claude_session_status to None instead of Optional[Callable], conflicting with its callable usage and likely breaking type checking.
Prompt for AI agents
Address the following comment on src/codegen/cli/commands/claude/config/claude_session_stop_hook.py at line 22:
<comment>Incorrect type annotation narrows update_claude_session_status to None instead of Optional[Callable], conflicting with its callable usage and likely breaking type checking.</comment>
<file context>
@@ -19,7 +19,7 @@
from codegen.cli.commands.claude.claude_session_api import update_claude_session_status
except ImportError:
- update_claude_session_status = None
+ update_claude_session_status: None = None
</file context>
| update_claude_session_status: None = None | |
| update_claude_session_status = None |
| "response_id": response.get("id"), | ||
| "status_code": response.status_code | ||
| }) | ||
| logger.info("API request successful", extra={"operation": "api.request", "endpoint": "agent/run", "response_id": response.get("id"), "status_code": response.status_code}) |
There was a problem hiding this comment.
response.get(...) conflicts with response.status_code; use response.json().get("id") to access the JSON id from a requests.Response
Prompt for AI agents
Address the following comment on QUICK_START_LOGGING.md at line 83:
<comment>response.get(...) conflicts with response.status_code; use response.json().get("id") to access the JSON id from a requests.Response</comment>
<file context>
@@ -48,69 +51,56 @@ codegen config telemetry status
- "response_id": response.get("id"),
- "status_code": response.status_code
-})
+logger.info("API request successful", extra={"operation": "api.request", "endpoint": "agent/run", "response_id": response.get("id"), "status_code": response.status_code})
```
</file context>
| logger.info("API request successful", extra={"operation": "api.request", "endpoint": "agent/run", "response_id": response.get("id"), "status_code": response.status_code}) | |
| logger.info("API request successful", extra={"operation": "api.request", "endpoint": "agent/run", "response_id": response.json().get("id"), "status_code": response.status_code}) |
| else: | ||
| console.print(f"[green]✓[/green] Set repository ID to: [cyan]{repo_id}[/cyan]") | ||
| console.print("[yellow]ℹ[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | ||
| console.print("[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") |
There was a problem hiding this comment.
The message shows {repo_id} literally; without an f-string the export command won't include the actual repository ID.
Prompt for AI agents
Address the following comment on src/codegen/cli/commands/repo/main.py at line 117:
<comment>The message shows {repo_id} literally; without an f-string the export command won't include the actual repository ID.</comment>
<file context>
@@ -116,13 +108,13 @@ def _set_default_repository(repo_id: int) -> None:
else:
console.print(f"[green]✓[/green] Set repository ID to: [cyan]{repo_id}[/cyan]")
- console.print("[yellow]ℹ[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")
+ console.print("[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence")
except Exception as e:
</file context>
| console.print("[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") | |
| console.print(f"[yellow]i[/yellow] Could not update .env file. Add 'export CODEGEN_REPO_ID={repo_id}' to your shell for persistence") |
User description
Motivation
Content
Testing
Please check the following before marking your PR as ready for review
PR Type
Enhancement, Bug fix
Description
Add MCP server command and integration
Fix pre-commit formatting and linting issues
Resolve type checking and import errors
Add team roles documentation
Diagram Walkthrough
File Walkthrough
3 files
Add MCP command to CLICreate MCP command moduleImplement MCP server command17 files
Fix type annotations and importsFix imports and type annotationsReorder imports for consistencyFix imports and remove trailing newlinesRemove unused importsFix missing newline at EOFFix string formatting and newlinesFix missing newline at EOFFix import orderingRemove duplicate importsRemove unused importsFix import orderingRemove unused import and fix formattingFix type annotations and formattingFix import orderingFix formatting and importsFix missing newline at EOF14 files
Fix type annotation for None assignmentFix type annotations and conditional checksFix type annotation for None assignmentChange logger.error to logger.exceptionReplace unicode character and formatting fixesFix imports and unicode characterFix formatting and unicode characterFix timezone usage for datetimeFix type checking assertionFix git operations and importsChange logger.error to logger.exceptionFix import for collections.abcFix return types and string handlingFix None handling in config2 files
Add import error handlingAdd import error handling2 files
Add type ignore for test errorsAdd type ignore for test errors3 files
Fix formatting and code examplesAdd team roles navigationAdd comprehensive team roles documentation1 files
Add MCP dependency14 files
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores