fix: improve prompts, error messages, and workspace concurrency

- Externalize coordinator, notification, and planning prompts to separate files - Add workspace mutex for concurrent file operations - Improve error messages across file_tools, issue_tools, and pr_tools - Add logging to tool modules for better debugging - Update tests to match new error message strings
2026-07-19 17:49:33 +02:00
parent ee01487ce3
commit dfdd8c0931
11 changed files with 300 additions and 107 deletions
@@ -1,8 +1,11 @@
+import logging
 import os
 from pathlib import Path
 from typing import Any
 from gitea.client import GiteaClient

+logger: logging.Logger = logging.getLogger("gitea-file-tools")
+

 class FileTools:
    """Tools for Gitea file/content operations."""
@@ -67,18 +70,28 @@ class FileTools:
        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:
+                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
+            except Exception as exc:
+                logger.debug(
+                    f"Local read failed for {owner}/{repo}/{path}: {exc}",
+                    exc_info=True,
+                )

        try:
            content = self._client.files.get_file_content(owner, repo, path)
            raw: str = "\n".join(content) if isinstance(content, list) else content
            return self._paginate_lines(raw, offset, limit)
        except Exception as e:
-            return f"Error getting file content: {str(e)}"
+            logger.error(
+                f"Failed to get file content for {owner}/{repo}/{path}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not retrieve file '{path}' from {owner}/{repo}. "
+                f"Verify the file path and branch are correct. Details: {e}"
+            )

    def get_file_content_with_ref(
        self,
@@ -104,21 +117,34 @@ class FileTools:
            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,
+                        capture_output=True,
+                        text=True,
+                        timeout=15,
                    )
                    if result.returncode == 0:
                        return self._paginate_lines(result.stdout, offset, limit)
-                except Exception:
-                    pass
+                except Exception as exc:
+                    logger.debug(
+                        f"Local git show failed for {owner}/{repo}/{path}@{ref}: {exc}",
+                        exc_info=True,
+                    )

        try:
            content = self._client.files.get_file_content(owner, repo, path, ref)
            raw: str = "\n".join(content) if isinstance(content, list) else content
            return self._paginate_lines(raw, offset, limit)
        except Exception as e:
-            return f"Error getting file content: {str(e)}"
+            logger.error(
+                f"Failed to get file content for {owner}/{repo}/{path}@{ref}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not retrieve file '{path}' at ref '{ref}' from {owner}/{repo}. "
+                f"Verify the file path and ref are correct. Details: {e}"
+            )

    def commit_file(
        self, owner: str, repo: str, path: str, message: str, content: str, branch: str
@@ -127,7 +153,14 @@ class FileTools:
            self._client.files.update_file(owner, repo, path, message, content, branch)
            return f"File '{path}' committed successfully to {owner}/{repo}."
        except Exception as e:
-            return f"Error committing file: {str(e)}"
+            logger.error(
+                f"Failed to commit file '{path}' to {owner}/{repo}@{branch}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not commit file '{path}' to {owner}/{repo} on branch '{branch}'. "
+                f"Check for conflicts or permission issues. Details: {e}"
+            )

    def update_file(
        self, owner: str, repo: str, path: str, message: str, content: str, branch: str
@@ -136,4 +169,11 @@ class FileTools:
            self._client.files.update_file(owner, repo, path, message, content, branch)
            return f"File '{path}' updated in {owner}/{repo}."
        except Exception as e:
-            return f"Error updating file: {str(e)}"
+            logger.error(
+                f"Failed to update file '{path}' in {owner}/{repo}@{branch}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not update file '{path}' in {owner}/{repo} on branch '{branch}'. "
+                f"Check for conflicts or permission issues. Details: {e}"
+            )
@@ -27,7 +27,14 @@ class IssueTools:
            self._client.issues.close_issue(owner, repo, issue_number)
            return f"Issue #{issue_number} closed successfully."
        except Exception as e:
-            return f"Error closing issue: {str(e)}"
+            logger.error(
+                f"Failed to close issue #{issue_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not close issue #{issue_number} in {owner}/{repo}. "
+                f"Check permissions or if the issue is already closed. Details: {e}"
+            )

    def get_issue_comments(
        self,
@@ -58,7 +65,14 @@ class IssueTools:
                )
            return result
        except Exception as e:
-            return f"Error getting issue comments: {str(e)}"
+            logger.error(
+                f"Failed to get comments for issue #{issue_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not retrieve comments for issue #{issue_number} in {owner}/{repo}. "
+                f"Verify the issue exists and you have access. Details: {e}"
+            )

    def list_assigned_issues(self) -> list[dict[str, Any]]:
        try:
@@ -86,11 +100,18 @@ class IssueTools:
        try:
            issues = self._client.issues.list_repo_issues(owner, repo, state)
            if not issues:
-                return f"No issues in {owner}/{repo}."
+                return f"No {state} issues in {owner}/{repo}."
            summary = [f"#{issue.number}: {issue.title}" for issue in issues]
            return "\n".join(summary)
        except Exception as e:
-            return f"Error listing issues: {str(e)}"
+            logger.error(
+                f"Failed to list {state} issues in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not list issues for {owner}/{repo}. "
+                f"Verify the repository exists and you have access. Details: {e}"
+            )

    def create_issue(
        self,
@@ -107,7 +128,14 @@ class IssueTools:
            )
            return f"Issue #{issue.number} created successfully in {owner}/{repo}."
        except Exception as e:
-            return f"Error creating issue: {str(e)}"
+            logger.error(
+                f"Failed to create issue '{title}' in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not create issue '{title}' in {owner}/{repo}. "
+                f"Check repository permissions and label/assignee names. Details: {e}"
+            )

    def add_label_to_issue(
        self, owner: str, repo: str, issue_number: int, label: str
@@ -116,7 +144,14 @@ class IssueTools:
            self._client.issues.add_label(owner, repo, issue_number, label)
            return f"Label '{label}' added to issue #{issue_number}."
        except Exception as e:
-            return f"Error adding label to issue #{issue_number}: {e}"
+            logger.error(
+                f"Failed to add label '{label}' to issue #{issue_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not add label '{label}' to issue #{issue_number} in {owner}/{repo}. "
+                f"Verify the label exists in the repository. Details: {e}"
+            )

    def add_comment_to_issue(
        self, owner: str, repo: str, issue_number: int, body: str
@@ -125,4 +160,11 @@ class IssueTools:
            self._client.issues.add_comment(owner, repo, issue_number, body)
            return f"Comment added to issue #{issue_number}."
        except Exception as e:
-            return f"Error adding comment to issue #{issue_number}: {e}"
+            logger.error(
+                f"Failed to add comment to issue #{issue_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not add comment to issue #{issue_number} in {owner}/{repo}. "
+                f"Verify the issue exists and you have write access. Details: {e}"
+            )
@@ -40,11 +40,15 @@ class PRTools:
    def __init__(self, client: GiteaClient) -> None:
        self._client = client

-    def get_pull_request(self, owner: str, repo: str, pull_number: int) -> PullRequestModel:
+    def get_pull_request(
+        self, owner: str, repo: str, pull_number: int
+    ) -> PullRequestModel:
        try:
            return self._client.prs.get_pull_request(owner, repo, pull_number)
        except Exception as e:
-            logger.error(f"Error getting pull request #{pull_number}: {e}", exc_info=True)
+            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:
@@ -52,7 +56,14 @@ class PRTools:
            self._client.prs.close_pull_request(owner, repo, pull_number)
            return f"Pull request #{pull_number} closed successfully."
        except Exception as e:
-            return f"Error closing pull request: {str(e)}"
+            logger.error(
+                f"Failed to close PR #{pull_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not close PR #{pull_number} in {owner}/{repo}. "
+                f"Check permissions or if the PR is already closed. Details: {e}"
+            )

    def get_pull_request_comments(
        self,
@@ -83,7 +94,14 @@ class PRTools:
                )
            return result
        except Exception as e:
-            return f"Error getting PR comments: {str(e)}"
+            logger.error(
+                f"Failed to get comments for PR #{pull_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not retrieve comments for PR #{pull_number} in {owner}/{repo}. "
+                f"Verify the PR exists and you have access. Details: {e}"
+            )

    def list_assigned_pull_requests(self) -> list[dict[str, Any]]:
        try:
@@ -111,11 +129,18 @@ class PRTools:
        try:
            prs = self._client.prs.list_repo_pull_requests(owner, repo, state)
            if not prs:
-                return f"No PRs in {owner}/{repo}."
+                return f"No {state} PRs in {owner}/{repo}."
            summary = [f"#{pr.number}: {pr.title}" for pr in prs]
            return "\n".join(summary)
        except Exception as e:
-            return f"Error listing PRs: {str(e)}"
+            logger.error(
+                f"Failed to list {state} PRs in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not list PRs for {owner}/{repo}. "
+                f"Verify the repository exists and you have access. Details: {e}"
+            )

    def create_pull_request(
        self,
@@ -156,7 +181,14 @@ class PRTools:
            self._client.prs.add_label_pr(owner, repo, pr_number, label)
            return f"Label '{label}' added to PR #{pr_number}."
        except Exception as e:
-            return f"Error adding label to PR #{pr_number}: {e}"
+            logger.error(
+                f"Failed to add label '{label}' to PR #{pr_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not add label '{label}' to PR #{pr_number} in {owner}/{repo}. "
+                f"Verify the label exists in the repository. Details: {e}"
+            )

    def get_pull_request_diff(
        self,
@@ -177,7 +209,14 @@ class PRTools:
            diff: str = self._client.prs.get_pull_request_diff(owner, repo, pull_number)
            return _truncate_diff(diff, max_chars, char_offset)
        except Exception as e:
-            return f"Error getting PR diff: {str(e)}"
+            logger.error(
+                f"Failed to get diff for PR #{pull_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not retrieve diff for PR #{pull_number} in {owner}/{repo}. "
+                f"The PR may have no changes or the API is unavailable. Details: {e}"
+            )

    def get_pull_request_patch(
        self,
@@ -200,7 +239,14 @@ class PRTools:
            )
            return _truncate_diff(patch, max_chars, char_offset)
        except Exception as e:
-            return f"Error getting PR patch: {str(e)}"
+            logger.error(
+                f"Failed to get patch for PR #{pull_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not retrieve patch for PR #{pull_number} in {owner}/{repo}. "
+                f"The PR may have no changes or the API is unavailable. Details: {e}"
+            )

    def approve_pull_request(
        self, owner: str, repo: str, pull_number: int, comment: str
@@ -209,7 +255,14 @@ class PRTools:
            self._client.prs.approve_pr(owner, repo, pull_number, comment)
            return f"Approved PR #{pull_number}."
        except Exception as e:
-            return f"Error approving PR: {str(e)}"
+            logger.error(
+                f"Failed to approve PR #{pull_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not approve PR #{pull_number} in {owner}/{repo}. "
+                f"Check that you have review permissions. Details: {e}"
+            )

    def request_changes(
        self, owner: str, repo: str, pull_number: int, comment: str
@@ -218,4 +271,11 @@ class PRTools:
            self._client.prs.request_changes_pr(owner, repo, pull_number, comment)
            return f"Requested changes on PR #{pull_number}."
        except Exception as e:
-            return f"Error requesting changes: {str(e)}"
+            logger.error(
+                f"Failed to request changes on PR #{pull_number} in {owner}/{repo}: {e}",
+                exc_info=True,
+            )
+            return (
+                f"Error: Could not request changes on PR #{pull_number} in {owner}/{repo}. "
+                f"Check that you have review permissions. Details: {e}"
+            )
@@ -3,6 +3,7 @@ import logging
 import subprocess
 import base64
 import shutil
+import threading
 from pathlib import Path
 from urllib.parse import urlparse
 from .config import GITEA_REPOS_ROOT, GITEA_URL, GITEA_TOKEN
@@ -12,12 +13,27 @@ logger: logging.Logger = logging.getLogger("gitea-workspace")


 class WorkspaceManager:
-    """Manages local workspace for Gitea repositories."""
+    """Manages local workspace for Gitea repositories.
+
+    Uses per-repo threading locks to prevent concurrent git operations
+    on the same repository from causing race conditions.
+    """
+
+    _repo_locks: dict[str, threading.Lock] = {}
+    _locks_lock: threading.Lock = threading.Lock()

    def __init__(self) -> None:
        self.root_dir: Path = Path(GITEA_REPOS_ROOT).expanduser().resolve()
        self.root_dir.mkdir(parents=True, exist_ok=True)

+    @classmethod
+    def get_repo_lock(cls, repo_full_name: str) -> threading.Lock:
+        """Get or create a thread-safe lock for a specific repository."""
+        with cls._locks_lock:
+            if repo_full_name not in cls._repo_locks:
+                cls._repo_locks[repo_full_name] = threading.Lock()
+            return cls._repo_locks[repo_full_name]
+
    def _configure_repo_user(self, repo_path: Path) -> None:
        try:
            client = GiteaClient()
@@ -70,6 +86,12 @@ class WorkspaceManager:
        return f"{parsed.scheme}://{parsed.netloc}{path}/{repo_full_name}.git"

    def sanitize_repo(self, repo_full_name: str, repo_path: Path) -> None:
+        lock = self.get_repo_lock(repo_full_name)
+        with lock:
+            logger.debug(f"Acquired workspace lock for {repo_full_name} (sanitize)")
+            self._sanitize_repo_inner(repo_full_name, repo_path)
+
+    def _sanitize_repo_inner(self, repo_full_name: str, repo_path: Path) -> None:
        try:
            auth_url = self._get_authenticated_url(repo_full_name)
            subprocess.run(
@@ -146,6 +168,14 @@ class WorkspaceManager:
            ) from e

    def clone_repo(self, repo_full_name: str, clone_url: str | None = None) -> Path:
+        lock = self.get_repo_lock(repo_full_name)
+        with lock:
+            logger.debug(f"Acquired workspace lock for {repo_full_name} (clone)")
+            return self._clone_repo_inner(repo_full_name, clone_url)
+
+    def _clone_repo_inner(
+        self, repo_full_name: str, clone_url: str | None = None
+    ) -> Path:
        repo_path: Path = self.get_repo_path(repo_full_name)
        if repo_path.exists():
            if not (repo_path / ".git").exists():