fix(gerrit): use action parameter for dispatch and clean up temp repos

[gvago]

Summary

• Action parameter ignored: The {action} path parameter in POST /api/v1/gerrit/{action} was captured but never used for command dispatch. Instead, item.msg was always passed as the command. Now the action enum value determines the command (/review, /describe, etc.), with item.msg only appended for the ask action. Also made msg optional with a default empty string since non-ask actions don't require it.
• HTTPException returned instead of raised: return HTTPException(...) silently returns a 200 with the exception object serialized as JSON. Changed to raise HTTPException(...) so the 400 status code is actually sent.
• Temp repo never cleaned up: prepare_repo() creates a temp directory via mkdtemp() but remove_initial_comment() had shutil.rmtree commented out. Added a cleanup() method to GerritProvider that safely removes the temp directory, called from the server's finally block after each request. A __del__ method serves as a safety net. remove_initial_comment() now delegates to cleanup().

Test plan

• Verify POST /api/v1/gerrit/review with empty msg dispatches /review command (not / )
• Verify POST /api/v1/gerrit/ask with msg: "what does this do?" dispatches /ask what does this do?
• Verify POST /api/v1/gerrit/ask with empty msg returns 400
• Verify temp directories under /tmp/ are cleaned up after request completes
• Verify temp directories are cleaned up even when handle_request raises an exception

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Fix Gerrit action parameter dispatch and add temp repo cleanup

🐞 Bug fix

Walkthroughs

Description

• Use action parameter to determine command dispatch instead of item.msg
• Fix HTTPException to be raised instead of returned for proper error handling
• Add cleanup() method to GerritProvider to remove temporary repositories
• Call cleanup() in finally block after each request to prevent temp directory leaks

Diagram

flowchart LR
  A["Gerrit Request<br/>with action param"] -->|"action determines<br/>command"| B["PRAgent<br/>handle_request"]
  B -->|"try block"| C["Process Request"]
  C -->|"finally block"| D["GerritProvider<br/>cleanup"]
  D -->|"removes"| E["Temp Directory"]
  F["HTTPException<br/>raise not return"] -->|"proper 400<br/>status code"| G["Error Response"]

Loading

File Changes

1. pr_agent/git_providers/gerrit_provider.py 🐞 Bug fix +20/-3

Add cleanup method for temp repository removal

• Added cleanup() method to safely remove temporary repository directory with error handling
• Added __del__() destructor as safety net for cleanup if not explicitly called
• Modified remove_initial_comment() to delegate to cleanup() instead of commented-out code

pr_agent/git_providers/gerrit_provider.py

---

2. pr_agent/servers/gerrit_server.py 🐞 Bug fix +28/-6

Fix action dispatch and add cleanup in finally block

• Made msg field optional with empty string default in Item model
• Changed command dispatch to use action enum value instead of item.msg content
• Fixed return HTTPException to raise HTTPException for proper 400 status code
• Added try-finally block to call cleanup() on GerritProvider after each request
• Added import for GerritProvider to access cleanup functionality

pr_agent/servers/gerrit_server.py

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (3)   📘 Rule violations (1)   📎 Requirement gaps (0)

🐞\ ≡ Correctness (2) ◔ Observability (1) ⭐ New (1)

📘\ ☼ Reliability (1) ⭐ New (1)

1. Repo deleted mid-execution 🐞 ≡ ⭐

Description

GerritProvider.remove_initial_comment() now calls cleanup(), which deletes the cloned temp repo even
though PR-Agent tools call remove_initial_comment() before later steps that still need repo_path
(e.g., publishing code suggestions). This can cause subsequent file operations in
publish_code_suggestions() to fail because the repo directory has already been removed.

Code

pr_agent/git_providers/gerrit_provider.py[R409-410]

    def remove_initial_comment(self):
-        # remove repo, cloned in previous steps
-        # shutil.rmtree(self.repo_path)
-        pass
+        self.cleanup()

Evidence

The code-suggestions workflow removes the initial (temporary) comment before it publishes inline
code suggestions, but publishing code suggestions for Gerrit requires reading/writing files under
GerritProvider.repo_path. With remove_initial_comment() now deleting repo_path,
publish_code_suggestions() can run after the repo has been deleted and fail when it tries to patch
files and compute diffs.

pr_agent/git_providers/gerrit_provider.py[409-410]
pr_agent/tools/pr_code_suggestions.py[126-130]
pr_agent/tools/pr_code_suggestions.py[539-576]
pr_agent/git_providers/gerrit_provider.py[343-358]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`GerritProvider.remove_initial_comment()` is part of the GitProvider interface and is called mid-flow by multiple tools to remove a temporary “preparing…” comment. In this PR it now deletes the cloned temp repo, which can break later steps in the same request that still need `repo_path` (e.g., `publish_code_suggestions`).

### Issue Context
For Gerrit, temp repo lifecycle should be cleaned up at the *end* of the request (your new `finally` block in `gerrit_server.py` is the right place). Deleting the repo inside `remove_initial_comment()` changes semantics and can invalidate the provider during the ongoing run.

### Fix Focus Areas
- pr_agent/git_providers/gerrit_provider.py[409-410]

### Suggested change
Make `remove_initial_comment()` a no-op again (or only handle comment deletion semantics), and keep repo cleanup exclusively in `GerritProvider.cleanup()` invoked from the server `finally` (plus optional safety-net `__del__`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. cleanup() broad exception handling ☑ 📘 ☼

Description

GerritProvider.cleanup() catches Exception (and __del__ silently suppresses all exceptions),
which can mask unexpected failures and make debugging temp-repo leaks harder. Catch specific
filesystem-related exceptions and preserve context (e.g., log with exc_info=True).

Code

pr_agent/git_providers/gerrit_provider.py[R392-407]

+        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 Exception 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

Evidence

PR Compliance ID 19 requires targeted exception handling (avoid broad except Exception) and
preserving context; the added cleanup()/__del__ code introduces broad catches and suppression.
These patterns are also likely to be flagged by repository linting expectations (PR Compliance ID
21).

pr_agent/git_providers/gerrit_provider.py[392-407]
Best Practice: Learned patterns
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GerritProvider.cleanup()` and `__del__` use broad `except Exception` handling, including silent suppression, which can hide real errors and make diagnosing temp-repo cleanup issues difficult.
## Issue Context
The compliance checklist requires targeted exception handling and preserving error context. Cleanup paths should typically catch specific filesystem exceptions (e.g., `OSError`, `PermissionError`, `FileNotFoundError`) and log tracebacks when failures occur.
## Fix Focus Areas
- pr_agent/git_providers/gerrit_provider.py[392-407]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Cleanup misses temp repos ☑ 🐞 ☼

Description

The server finally cleanup only cleans providers stored in context["git_provider"], but provider
caching is broken and some tools (e.g. /ask) instantiate providers outside context, leaving temp
clone directories untracked and potentially uncleaned.

Code

pr_agent/servers/gerrit_server.py[R58-67]

+    finally:
+        # Clean up the cloned temp repo created by GerritProvider.
+        # The provider is cached in the starlette context during
+        # get_git_provider_with_context().
+        try:
+            git_providers = context.get("git_provider", {})
+            for provider in git_providers.values():
+                if isinstance(provider, GerritProvider):
+                    provider.cleanup()
+        except Exception:

Evidence

get_git_provider_with_context() checks for context["git_provider"]["pr_url"] but stores
{pr_url: git_provider}, so repeated calls create new providers and overwrite the dict instead of
reusing a single provider. This happens within a single request because apply_repo_settings()
calls get_git_provider_with_context(), and tools like PRReviewer call it again. Additionally,
PRQuestions (used by the ask action) creates a provider via get_git_provider()(pr_url), so
that provider is never placed into context["git_provider"] and will not be reached by the new
server cleanup loop.

pr_agent/git_providers/init.py[40-65]
pr_agent/git_providers/utils.py[14-17]
pr_agent/tools/pr_reviewer.py[47-55]
pr_agent/tools/pr_questions.py[19-23]
pr_agent/servers/gerrit_server.py[58-67]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Temp repo cleanup is incomplete because:
1) `get_git_provider_with_context()` never hits its cache (key mismatch) and overwrites `context["git_provider"]`, allowing multiple GerritProvider instances (and temp clones) per request.
2) `PRQuestions` (used by `/ask`) instantiates a provider via `get_git_provider()(pr_url)` which bypasses context storage, so the new server `finally` cleanup cannot find it.
## Issue Context
The Gerrit server cleanup assumes providers are cached in `starlette_context` and reachable via `context.get("git_provider")`.
## Fix Focus Areas
- pr_agent/git_providers/__init__.py[40-65]
- pr_agent/servers/gerrit_server.py[36-71]
- pr_agent/tools/pr_questions.py[19-23]
## Implementation notes
- Make `context["git_provider"]` consistently a dict mapping `pr_url -> provider`.
- In `get_git_provider_with_context(pr_url)`, reuse the provider via `context.get("git_provider", {}).get(pr_url)` and avoid overwriting the entire dict (use `setdefault` / assignment into the dict).
- Update `PRQuestions` to use `get_git_provider_with_context(pr_url)` (like other tools) so GerritProvider instances are tracked and can be cleaned.
- (Optional hardening) In `gerrit_server.handle_gerrit_request`, initialize `context["git_provider"] = {}` at request start (similar to other webhook servers) to ensure the structure is always a dict.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. No pytest coverage for endpoint 📘 ☼ ⭐

Description

The Gerrit API request dispatch behavior was changed (command construction, msg optionality, and
raising HTTPException), but no pytest coverage was added/updated for these new behaviors. This
increases regression risk for the POST /api/v1/gerrit/{action} handler.

Code

pr_agent/servers/gerrit_server.py[R30-80]

class Item(BaseModel):
    refspec: str
    project: str
-    msg: str
+    msg: str = ""


@router.post("/api/v1/gerrit/{action}")
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__"
+            )

Evidence

The checklist requires adding/updating pytest tests when behavior changes. The diff shows
significant behavioral changes to the request handler (action-based command dispatch, msg
defaulting, and error handling), but no corresponding test changes are present in the PR diff.

AGENTS.md
pr_agent/servers/gerrit_server.py[30-80]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Behavior of `handle_gerrit_request` changed (dispatch uses `{action}`, `msg` is optional except for `ask`, and `HTTPException` is raised). Add/update pytest tests to cover these behaviors.

## Issue Context
This endpoint is a key integration surface; tests should validate command construction and error handling for `ask` vs non-`ask` actions.

## Fix Focus Areas
- pr_agent/servers/gerrit_server.py[30-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. handle_gerrit_request broad cleanup except ☑ 📘 ☼

Description

The request cleanup block uses a broad except Exception that can hide unexpected failures
retrieving providers or running cleanup(), potentially leaving temp directories behind without
actionable error details. Narrow the exception handling and log with traceback to preserve context.

Code

pr_agent/servers/gerrit_server.py[R58-71]

+    finally:
+        # Clean up the cloned temp repo created by GerritProvider.
+        # The provider is cached in the starlette context during
+        # get_git_provider_with_context().
+        try:
+            git_providers = context.get("git_provider", {})
+            for provider in git_providers.values():
+                if isinstance(provider, GerritProvider):
+                    provider.cleanup()
+        except Exception:
+            get_logger().debug(
+                "Could not retrieve GerritProvider for cleanup; "
+                "temp directory may persist"
+            )

Evidence

PR Compliance ID 19 disallows overly broad exception handling; the added except Exception in the
cleanup finally block is a broad catch that suppresses underlying errors. This also relates to
robust handling of edge cases around cleanup logic (PR Compliance ID 3).

Rule 3: Robust Error Handling
pr_agent/servers/gerrit_server.py[58-71]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The temp-repo cleanup logic in `handle_gerrit_request` catches `Exception` broadly and only logs a generic debug message, which can mask real issues and hinder troubleshooting.
## Issue Context
The compliance checklist requires catching expected exception types and preserving context. If failure should not break the request, it should still provide actionable logs (e.g., `exc_info=True`) and catch only expected exceptions.
## Fix Focus Areas
- pr_agent/servers/gerrit_server.py[58-71]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Whitespace ask bypass 🐞 ≡

Description

handle_gerrit_request() only checks not item.msg, so a whitespace-only msg passes validation and
becomes /ask  (empty question) instead of returning 400.

Code

pr_agent/servers/gerrit_server.py[R43-51]

  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}"

Evidence

The endpoint validates item.msg before stripping but builds the command using item.msg.strip(),
so "   " produces an empty question string. The PRQuestions tool accepts empty args and turns them
into an empty question.

pr_agent/servers/gerrit_server.py[36-52]
pr_agent/tools/pr_questions.py[46-51]
pr_agent/agent/pr_agent.py[59-66]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`POST /api/v1/gerrit/ask` accepts whitespace-only `msg`, which becomes an empty question after `.strip()` and is processed instead of returning HTTP 400.
## Issue Context
The handler validates `item.msg` before stripping, but constructs the command using `item.msg.strip()`.
## Fix Focus Areas
- pr_agent/servers/gerrit_server.py[41-51]
### Suggested change
Change the validation to reject whitespace-only messages, e.g.:
- `if not item.msg or not item.msg.strip(): raise HTTPException(...)`
- build the command using the already-stripped value to avoid double-handling.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

7. Cleanup log always success 🐞 ◔

Description

GerritProvider.cleanup() calls shutil.rmtree(..., ignore_errors=True) and then logs "Cleaned up"
unconditionally, so failures can be silently ignored while emitting a misleading success log.

Code

pr_agent/git_providers/gerrit_provider.py[R390-400]

+    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 Exception as e:
+                get_logger().warning(
+                    "Failed to clean up temp repo at %s: %s",
+                    self.repo_path, e
+                )

Evidence

With ignore_errors=True, shutil.rmtree suppresses deletion errors, so the subsequent info log
can claim success even when the directory was not removed, and the except block may never run for
common deletion failures.

pr_agent/git_providers/gerrit_provider.py[390-400]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`cleanup()` can log success even if the temp directory was not actually deleted.
## Issue Context
`shutil.rmtree(..., ignore_errors=True)` suppresses errors, but the code logs "Cleaned up" immediately after.
## Fix Focus Areas
- pr_agent/git_providers/gerrit_provider.py[390-400]
## Suggested change
- Either remove `ignore_errors=True` and rely on the `try/except` to log failures, or
- keep `ignore_errors=True` but verify deletion (e.g., check `Path.exists()` after) and only log success if it’s actually gone; otherwise log a warning.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

Previous review results

Review updated until commit 16e6a5e

Results up to commit N/A

🐞 Bugs (2)  
📘 Rule violations (0)  
📎 Requirement gaps (0)

1. cleanup() broad exception handling ☑ 📘 ☼

Description

GerritProvider.cleanup() catches Exception (and __del__ silently suppresses all exceptions),
which can mask unexpected failures and make debugging temp-repo leaks harder. Catch specific
filesystem-related exceptions and preserve context (e.g., log with exc_info=True).

Code

pr_agent/git_providers/gerrit_provider.py[R392-407]

+        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 Exception 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

Evidence

PR Compliance ID 19 requires targeted exception handling (avoid broad except Exception) and
preserving context; the added cleanup()/__del__ code introduces broad catches and suppression.
These patterns are also likely to be flagged by repository linting expectations (PR Compliance ID
21).

pr_agent/git_providers/gerrit_provider.py[392-407]
Best Practice: Learned patterns
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GerritProvider.cleanup()` and `__del__` use broad `except Exception` handling, including silent suppression, which can hide real errors and make diagnosing temp-repo cleanup issues difficult.
## Issue Context
The compliance checklist requires targeted exception handling and preserving error context. Cleanup paths should typically catch specific filesystem exceptions (e.g., `OSError`, `PermissionError`, `FileNotFoundError`) and log tracebacks when failures occur.
## Fix Focus Areas
- pr_agent/git_providers/gerrit_provider.py[392-407]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Cleanup misses temp repos ☑ 🐞 ☼

Description

The server finally cleanup only cleans providers stored in context["git_provider"], but provider
caching is broken and some tools (e.g. /ask) instantiate providers outside context, leaving temp
clone directories untracked and potentially uncleaned.

Code

pr_agent/servers/gerrit_server.py[R58-67]

+    finally:
+        # Clean up the cloned temp repo created by GerritProvider.
+        # The provider is cached in the starlette context during
+        # get_git_provider_with_context().
+        try:
+            git_providers = context.get("git_provider", {})
+            for provider in git_providers.values():
+                if isinstance(provider, GerritProvider):
+                    provider.cleanup()
+        except Exception:

Evidence

get_git_provider_with_context() checks for context["git_provider"]["pr_url"] but stores
{pr_url: git_provider}, so repeated calls create new providers and overwrite the dict instead of
reusing a single provider. This happens within a single request because apply_repo_settings()
calls get_git_provider_with_context(), and tools like PRReviewer call it again. Additionally,
PRQuestions (used by the ask action) creates a provider via get_git_provider()(pr_url), so
that provider is never placed into context["git_provider"] and will not be reached by the new
server cleanup loop.

pr_agent/git_providers/init.py[40-65]
pr_agent/git_providers/utils.py[14-17]
pr_agent/tools/pr_reviewer.py[47-55]
pr_agent/tools/pr_questions.py[19-23]
pr_agent/servers/gerrit_server.py[58-67]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Temp repo cleanup is incomplete because:
1) `get_git_provider_with_context()` never hits its cache (key mismatch) and overwrites `context["git_provider"]`, allowing multiple GerritProvider instances (and temp clones) per request.
2) `PRQuestions` (used by `/ask`) instantiates a provider via `get_git_provider()(pr_url)` which bypasses context storage, so the new server `finally` cleanup cannot find it.
## Issue Context
The Gerrit server cleanup assumes providers are cached in `starlette_context` and reachable via `context.get("git_provider")`.
## Fix Focus Areas
- pr_agent/git_providers/__init__.py[40-65]
- pr_agent/servers/gerrit_server.py[36-71]
- pr_agent/tools/pr_questions.py[19-23]
## Implementation notes
- Make `context["git_provider"]` consistently a dict mapping `pr_url -> provider`.
- In `get_git_provider_with_context(pr_url)`, reuse the provider via `context.get("git_provider", {}).get(pr_url)` and avoid overwriting the entire dict (use `setdefault` / assignment into the dict).
- Update `PRQuestions` to use `get_git_provider_with_context(pr_url)` (like other tools) so GerritProvider instances are tracked and can be cleaned.
- (Optional hardening) In `gerrit_server.handle_gerrit_request`, initialize `context["git_provider"] = {}` at request start (similar to other webhook servers) to ensure the structure is always a dict.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. handle_gerrit_request broad cleanup except ☑ 📘 ☼

Description

The request cleanup block uses a broad except Exception that can hide unexpected failures
retrieving providers or running cleanup(), potentially leaving temp directories behind without
actionable error details. Narrow the exception handling and log with traceback to preserve context.

Code

pr_agent/servers/gerrit_server.py[R58-71]

+    finally:
+        # Clean up the cloned temp repo created by GerritProvider.
+        # The provider is cached in the starlette context during
+        # get_git_provider_with_context().
+        try:
+            git_providers = context.get("git_provider", {})
+            for provider in git_providers.values():
+                if isinstance(provider, GerritProvider):
+                    provider.cleanup()
+        except Exception:
+            get_logger().debug(
+                "Could not retrieve GerritProvider for cleanup; "
+                "temp directory may persist"
+            )

Evidence

PR Compliance ID 19 disallows overly broad exception handling; the added except Exception in the
cleanup finally block is a broad catch that suppresses underlying errors. This also relates to
robust handling of edge cases around cleanup logic (PR Compliance ID 3).

Rule 3: Robust Error Handling
pr_agent/servers/gerrit_server.py[58-71]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The temp-repo cleanup logic in `handle_gerrit_request` catches `Exception` broadly and only logs a generic debug message, which can mask real issues and hinder troubleshooting.
## Issue Context
The compliance checklist requires catching expected exception types and preserving context. If failure should not break the request, it should still provide actionable logs (e.g., `exc_info=True`) and catch only expected exceptions.
## Fix Focus Areas
- pr_agent/servers/gerrit_server.py[58-71]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Whitespace ask bypass 🐞 ≡

Description

handle_gerrit_request() only checks not item.msg, so a whitespace-only msg passes validation and
becomes /ask  (empty question) instead of returning 400.

Code

pr_agent/servers/gerrit_server.py[R43-51]

   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}"

Evidence

The endpoint validates item.msg before stripping but builds the command using item.msg.strip(),
so "   " produces an empty question string. The PRQuestions tool accepts empty args and turns them
into an empty question.

pr_agent/servers/gerrit_server.py[36-52]
pr_agent/tools/pr_questions.py[46-51]
pr_agent/agent/pr_agent.py[59-66]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`POST /api/v1/gerrit/ask` accepts whitespace-only `msg`, which becomes an empty question after `.strip()` and is processed instead of returning HTTP 400.
## Issue Context
The handler validates `item.msg` before stripping, but constructs the command using `item.msg.strip()`.
## Fix Focus Areas
- pr_agent/servers/gerrit_server.py[41-51]
### Suggested change
Change the validation to reject whitespace-only messages, e.g.:
- `if not item.msg or not item.msg.strip(): raise HTTPException(...)`
- build the command using the already-stripped value to avoid double-handling.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Cleanup log always success 🐞 ◔

Description

GerritProvider.cleanup() calls shutil.rmtree(..., ignore_errors=True) and then logs "Cleaned up"
unconditionally, so failures can be silently ignored while emitting a misleading success log.

Code

pr_agent/git_providers/gerrit_provider.py[R390-400]

+    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 Exception as e:
+                get_logger().warning(
+                    "Failed to clean up temp repo at %s: %s",
+                    self.repo_path, e
+                )

Evidence

With ignore_errors=True, shutil.rmtree suppresses deletion errors, so the subsequent info log
can claim success even when the directory was not removed, and the except block may never run for
common deletion failures.

pr_agent/git_providers/gerrit_provider.py[390-400]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`cleanup()` can log success even if the temp directory was not actually deleted.
## Issue Context
`shutil.rmtree(..., ignore_errors=True)` suppresses errors, but the code logs "Cleaned up" immediately after.
## Fix Focus Areas
- pr_agent/git_providers/gerrit_provider.py[390-400]
## Suggested change
- Either remove `ignore_errors=True` and rely on the `try/except` to log failures, or
- keep `ignore_errors=True` but verify deletion (e.g., check `Path.exists()` after) and only log success if it’s actually gone; otherwise log a warning.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 16e6a5e

[qodo-free-for-open-source-projects]

1. Repo deleted mid-execution 🐞 Bug ≡ Correctness

GerritProvider.remove_initial_comment() now calls cleanup(), which deletes the cloned temp repo even
though PR-Agent tools call remove_initial_comment() before later steps that still need repo_path
(e.g., publishing code suggestions). This can cause subsequent file operations in
publish_code_suggestions() to fail because the repo directory has already been removed.

Agent Prompt

### Issue description
`GerritProvider.remove_initial_comment()` is part of the GitProvider interface and is called mid-flow by multiple tools to remove a temporary “preparing…” comment. In this PR it now deletes the cloned temp repo, which can break later steps in the same request that still need `repo_path` (e.g., `publish_code_suggestions`).

### Issue Context
For Gerrit, temp repo lifecycle should be cleaned up at the *end* of the request (your new `finally` block in `gerrit_server.py` is the right place). Deleting the repo inside `remove_initial_comment()` changes semantics and can invalidate the provider during the ongoing run.

### Fix Focus Areas
- pr_agent/git_providers/gerrit_provider.py[409-410]

### Suggested change
Make `remove_initial_comment()` a no-op again (or only handle comment deletion semantics), and keep repo cleanup exclusively in `GerritProvider.cleanup()` invoked from the server `finally` (plus optional safety-net `__del__`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[gvago]

Closing to reopen from fork (lost push access to org branch).