diff --git a/pr_agent/git_providers/gerrit_provider.py b/pr_agent/git_providers/gerrit_provider.py index ced150c915..0ff80b5007 100644 --- a/pr_agent/git_providers/gerrit_provider.py +++ b/pr_agent/git_providers/gerrit_provider.py @@ -387,9 +387,30 @@ def publish_labels(self, labels): # but required by the interface pass + def cleanup(self): + """Remove the temporary cloned repository from disk.""" + if self.repo_path and pathlib.Path(self.repo_path).exists(): + try: + shutil.rmtree(self.repo_path, ignore_errors=True) + get_logger().info("Cleaned up temp repo at %s", self.repo_path) + except (OSError, PermissionError) as e: + get_logger().warning( + "Failed to clean up temp repo at %s: %s", + self.repo_path, e + ) + + def __del__(self): + """Safety net: clean up temp repo if cleanup() was not called.""" + try: + self.cleanup() + except Exception: + pass + def remove_initial_comment(self): - # remove repo, cloned in previous steps - # shutil.rmtree(self.repo_path) + # Do NOT call cleanup() here — this method is invoked during the + # request lifecycle while the cloned repo is still needed by + # subsequent commands. Actual cleanup happens in the server's + # finally block and in __del__ as a safety net. pass def remove_comment(self, comment): diff --git a/pr_agent/servers/gerrit_server.py b/pr_agent/servers/gerrit_server.py index 1783f6b994..cdbfe4d908 100644 --- a/pr_agent/servers/gerrit_server.py +++ b/pr_agent/servers/gerrit_server.py @@ -11,6 +11,7 @@ from pr_agent.agent.pr_agent import PRAgent from pr_agent.config_loader import get_settings, global_settings +from pr_agent.git_providers.gerrit_provider import GerritProvider from pr_agent.log import get_logger, setup_logger setup_logger() @@ -29,7 +30,7 @@ class Action(str, Enum): class Item(BaseModel): refspec: str project: str - msg: str + msg: str = "" @router.post("/api/v1/gerrit/{action}") @@ -37,16 +38,46 @@ async def handle_gerrit_request(action: Action, item: Item): get_logger().debug("Received a Gerrit request") context["settings"] = copy.deepcopy(global_settings) + # For the "ask" action, the question must come from item.msg. + # For all other actions, use the action path parameter as the command. if action == Action.ask: if not item.msg: - return HTTPException( + raise HTTPException( status_code=400, detail="msg is required for ask command" ) - await PRAgent().handle_request( - f"{item.project}:{item.refspec}", - f"/{item.msg.strip()}" - ) + command = f"/{action.value} {item.msg.strip()}" + else: + command = f"/{action.value}" + + try: + await PRAgent().handle_request( + f"{item.project}:{item.refspec}", + command + ) + finally: + # Clean up the cloned temp repo created by GerritProvider. + # The provider is cached in the starlette context during + # get_git_provider_with_context(). + # + # We guard against two failure modes: + # 1. The starlette context is inaccessible (e.g. middleware not + # active) — caught by the outer try/except. + # 2. The provider was never stored in the context (e.g. an error + # occurred before get_git_provider_with_context ran) — the + # dict will simply be empty, and the GerritProvider.__del__ + # safety net handles cleanup on garbage collection. + try: + git_providers = context.get("git_provider", {}) + if isinstance(git_providers, dict): + for provider in git_providers.values(): + if isinstance(provider, GerritProvider): + provider.cleanup() + except (LookupError, RuntimeError): + get_logger().debug( + "Could not retrieve GerritProvider for cleanup; " + "temp directory will be cleaned up by __del__" + ) async def get_body(request):