-
Notifications
You must be signed in to change notification settings - Fork 0
Integrate harness.py functionality for comprehensive codebase analysis and context management #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
1a13468
cbf94bc
34d1739
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,6 +19,9 @@ jobs: | |||||||||||||||||||||||||||||||||||
| require: write | ||||||||||||||||||||||||||||||||||||
| username: ${{ github.triggering_actor }} | ||||||||||||||||||||||||||||||||||||
| error-if-missing: true | ||||||||||||||||||||||||||||||||||||
| # Allow the codegen-sh bot to bypass permission check | ||||||||||||||||||||||||||||||||||||
| allow-bot: true | ||||||||||||||||||||||||||||||||||||
| bot-list: 'codegen-sh[bot]' | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
19
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Concern: The addition of the bot access bypass ('allow-bot' and 'bot-list') parameters should be carefully reviewed. Ensure that 'codegen-sh[bot]' has the minimum required permissions and that the bot's access is properly audited.
Suggested change
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| unit-tests: | ||||||||||||||||||||||||||||||||||||
| needs: access-check | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,164 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be beneficial to add unit tests for the |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CodebaseAnalysisHarness - Integration of the harness.py functionality from swebench. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This module provides comprehensive codebase analysis capabilities by integrating | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| the core functionality from the swebench harness.py module. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Dict, List, Optional, Set, Union | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+8
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Remove unused imports and update type annotations The import json
-import subprocess
from pathlib import Path
-from typing import Dict, List, Optional, Set, Union
+from typing import Optional, UnionFor modern type annotations, consider using Python 3.10+ syntax: -from typing import Dict, List, Optional, Set, Union
+from typing import Optional, Union
# Then use built-in types like dict, list instead of Dict, List📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.8.2)9-9: Remove unused import: (F401) 11-11: (UP035) 11-11: (UP035) 11-11: (UP035) 11-11: Remove unused import: (F401) 🤖 Prompt for AI Agents (early access) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from loguru import logger | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from codegen import Codebase | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from codegen.configs.models.codebase import CodebaseConfig | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class CodebaseAnalysisHarness: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A harness for comprehensive codebase analysis, integrating functionality | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from the swebench harness.py module. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __init__( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| codebase: Codebase, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metadata: Optional[Dict] = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tags: Optional[List[str]] = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Initialize the CodebaseAnalysisHarness with a codebase. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| codebase: The Codebase object to analyze | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metadata: Optional metadata to associate with the analysis | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tags: Optional tags to categorize the analysis | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.codebase = codebase | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.metadata = metadata or {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.tags = tags or [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.analysis_results = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+35
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The class lacks proper input validation for repository names and potential rate limiting handling. Consider adding validation for repository names and implement retry logic for network operations.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def from_repo( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cls, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| repo_full_name: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| commit: Optional[str] = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| language: str = "python", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| disable_file_parse: bool = False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> "CodebaseAnalysisHarness": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Create a CodebaseAnalysisHarness from a repository. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| repo_full_name: The full name of the repository (e.g., "owner/repo") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| commit: Optional commit hash to checkout | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| language: The primary language of the codebase | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| disable_file_parse: Whether to disable file parsing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A new CodebaseAnalysisHarness instance | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config = CodebaseConfig( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| disable_file_parse=disable_file_parse, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| codebase = Codebase.from_repo( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| repo_full_name=repo_full_name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| commit=commit, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| language=language, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check failure on line 70 in codegen-on-oss/codegen_on_oss/analysis/harness_integration.py
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config=config, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return cls(codebase=codebase) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def analyze_codebase(self) -> Dict: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Perform comprehensive analysis of the codebase. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A dictionary containing analysis results | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info(f"Analyzing codebase: {self.codebase.repo_name}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check failure on line 82 in codegen-on-oss/codegen_on_oss/analysis/harness_integration.py
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Collect basic codebase statistics | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stats = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "repo_name": self.codebase.repo_name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check failure on line 86 in codegen-on-oss/codegen_on_oss/analysis/harness_integration.py
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "language": self.codebase.language, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "file_count": len(self.codebase.files), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "metadata": self.metadata, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "tags": self.tags, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Get file structure | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| file_structure = self._get_file_structure() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stats["file_structure"] = file_structure | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Store the results | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.analysis_results = stats | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return stats | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _get_file_structure(self) -> Dict: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Get the file structure of the codebase. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A dictionary representing the file structure | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| structure = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for file_path in self.codebase.files: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parts = file_path.split("/") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current = structure | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i, part in enumerate(parts): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if i == len(parts) - 1: # This is a file | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current.setdefault("files", []).append(part) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: # This is a directory | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+97
to
+115
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The class lacks proper input validation and error handling. Consider adding parameter validation and more specific error types. The _get_file_structure method could be optimized to handle large codebases better.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current.setdefault("dirs", {}).setdefault(part, {}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current = current["dirs"][part] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return structure | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def diff_versus_commit(self, commit: str) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Take a diff of current contents versus the specified commit. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| commit: The commit hash to diff against | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| The diff output as a string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return self.codebase.get_diff(base=commit) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def files_in_patch(self, patch: str) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Extract the list of modified files from a unified diff patch string. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| patch: The unified diff patch string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A list of modified file paths | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| files = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for line in patch.split("\n"): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if line.startswith("--- a/") or line.startswith("+++ b/"): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fname = line.split("/", 1)[1] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if fname not in files: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| files.append(fname) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return files | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def save_analysis_results(self, output_path: Union[str, Path]) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Save the analysis results to a JSON file. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| output_path: The path to save the results to | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| output_path = Path(output_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| output_path.parent.mkdir(parents=True, exist_ok=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with open(output_path, "w") as f: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| json.dump(self.analysis_results, f, indent=2) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info(f"Analysis results saved to {output_path}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -124,5 +124,46 @@ def run( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parser.parse(repo_url, commit_hash) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @cli.command() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (bug_risk): Potential logger duplication in the serve command. Each invocation of the serve command adds a new logger sink, causing duplicate log output when run repeatedly (e.g., in tests). Move logger setup out of the command or check for existing sinks before adding. Suggested implementation: # Global logger configuration
from loguru import logger
if not logger._core.handlers:
logger.add("codegen_on_oss.log", rotation="1 MB")@cli.command()
@click.option(
"--host",
type=str,
default="0.0.0.0",
help="Host to bind the server to",
)
@click.option(
"--port",
type=int,
default=8000,
help="Port to bind the server to",
)
def serve(host, port):
# Removed duplicate logger sink addition, as global configuration is already set up.
...Make sure that any other logger.add() calls related to the serve command are removed or similarly guarded elsewhere in the file. Adjust the file path and log sink configuration according to your project's logging conventions.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CLI command |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @click.option( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--host", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type=str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default="0.0.0.0", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| help="Host to bind the server to", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @click.option( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--port", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type=int, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default=8000, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| help="Port to bind the server to", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @click.option( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--debug", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| is_flag=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| help="Debug mode", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def serve( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| host: str = "0.0.0.0", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| port: int = 8000, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| debug: bool = False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This comment was marked as resolved.
Sorry, something went wrong. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Start the Code Context Retrieval Server. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This server provides endpoints for codebase analysis, context management, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and agent execution. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing debug parameter impact in docstring
Tell me moreWhat is the issue?The serve function's docstring doesn't explain the important impact of the debug parameter on logging levels. Why this mattersUsers won't understand how the debug flag affects server behavior and logging, which could impact troubleshooting. Suggested change ∙ Feature PreviewProvide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.add( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sys.stdout, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| format="{time: HH:mm:ss} {level} {message}", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| level="DEBUG" if debug else "INFO", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from codegen_on_oss.context_server import start_server | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Late Import of Critical Server Component
Tell me moreWhat is the issue?The import statement for start_server is placed inside the serve function instead of at the module level with other imports. Why this mattersLate imports can mask import errors until runtime and cause the server to fail unexpectedly when the serve command is invoked. Suggested change ∙ Feature PreviewMove the import statement to the module level with other imports at the top of the file: import sys
from pathlib import Path
import click
from loguru import logger
from codegen_on_oss.context_server import start_server
# ... rest of the code ...Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info(f"Starting Code Context Retrieval Server on {host}:{port}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CLI implementation should include better error handling and progress feedback. Consider adding a progress bar for long-running operations and more detailed error messages. Also, add command validation and help text.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incomplete Server Configuration Logging
Tell me moreWhat is the issue?Server startup log message doesn't include key configuration information about debug mode Why this mattersMissing debug mode status in logs makes it harder to verify the server's running configuration during deployment and debugging. Suggested change ∙ Feature Previewlogger.info(f"Starting Code Context Retrieval Server on {host}:{port} (debug={debug})")Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| start_server(host=host, port=port) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if __name__ == "__main__": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cli() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| """Context server module for code context retrieval.""" | ||
|
|
||
| from codegen_on_oss.context_server.server import ( | ||
| app, | ||
| start_server, | ||
| ) | ||
|
|
||
| __all__ = ["app", "start_server"] | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security concern: Adding bot bypass should be carefully reviewed. Consider adding more granular controls for bot permissions and documenting the security implications of allowing bot access. Also, consider using GitHub environments to manage access control more securely.