This PR replaces silent assert failures with structured DiffError raises in apply_patch, adds a missing path-traversal guard to the edit tool, and fixes a race condition and unhandled telemetry crash in event_service

[heyitspratik]

• A human has tested these changes.

---

Why

Six safety issues found across three files:

1. apply_patch/core.py uses assert for all internal validation. Python silently disables assert when run with -O (optimized mode), which is common in Docker production images. A malformed patch silently corrupts state instead of returning an error.

2. The assert inside process_patch() is especially critical — AssertionError is not caught by the except DiffError block in definition.py, so a bad patch header crashes the entire tool instead of returning is_error=True to the agent.

3. apply_patch/core.py Parser.parse() had a TODO comment acknowledging that move_to path validation was missing. An absolute or ..-traversal move path was accepted without any check.

4. gemini/edit/impl.py did not resolve workspace_root before comparing paths. An LLM-supplied path like ../../etc/passwd could escape the sandbox. ApplyPatchExecutor already had this protection via _resolve_path — EditExecutor was missing the equivalent.

5. event_service.py _emit_event_from_thread reads self._main_loop and self._conversation twice with no lock in between. A concurrent close() call can set them to None between the check and the use, causing an AttributeError.

6. event_service.py log_callback had no error handling. A telemetry failure could propagate and crash the entire agent run.

Summary

• Replace all 9 assert statements in apply_patch/core.py with explicit raise DiffError(...) so validation always runs in production.
• Add move_to path validation in Parser.parse() to reject absolute paths and .. components.
• Resolve workspace_root in EditExecutor.__init__ and guard resolved paths against traversal outside the workspace boundary.
• Capture _main_loop and _conversation into local variables before the if-check in _emit_event_from_thread to eliminate the race condition.
• Wrap log_callback body in try/except so telemetry failures are logged and swallowed instead of crashing the agent run.

[enyst]

While this makes perfect sense, I think, when we talked about this before, we settled on not constraining edit tools. The reason is that it may give a false sense of security: since the agent has terminal tool, it can do this anyway. The current SOTA LLMs don’t really blink for an error like this, they’ll figure out an alternative way.

I think of its environment as “agent’s box”, specially when run in docker/container/VM. Even locally, I know my laptop is “agent’s box” 😊 . Due to the power of its tools, it can do technically about anything in there

I do think the user may want better restrictions, however we have other means to enforce them: confirmations, security analyzers, sandboxing, hooks. WDYT?

[enyst]

Thank you for the contribution!

Please let’s see what the agent thinks, too, because, you know… OpenHands is also one of our reviewers. 😅

@OpenHands /codereview this PR. Make sure you read inline threads too. Post your feedback as a review with gh api, with event.

[openhands-ai]

I'm on it! enyst can track my progress at all-hands.dev

[enyst]

This review was created by an AI agent (OpenHands) on behalf of @enyst.

🔴 Taste Rating: Needs improvement

I read the PR diff and the existing inline thread. The direction is good — replacing optimized-away asserts with structured DiffErrors is exactly the right kind of hardening — but two of the claimed fixes are incomplete.

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/event_service.py, Line 486] Race condition still exists: _main_loop is captured into main_loop, but the scheduling call still dereferences self._main_loop. A concurrent close() can still set self._main_loop = None after the check and before this line. Use the captured local (main_loop.run_in_executor(...)) or the race is not actually fixed.
• [openhands-tools/openhands/tools/gemini/edit/impl.py, Line 47] Security/correctness: string-prefix containment is not a path boundary check. With a workspace like /tmp/workspace, ../workspace-escape/file.txt resolves outside the workspace but still passes str(resolved).startswith(str(workspace_root)). Use resolved_path.is_relative_to(self.workspace_root) (or equivalent boundary-aware logic). Since this PR is hardening workspace containment, please also consider fixing the same pre-existing pattern in ApplyPatchExecutor._resolve_path rather than copying it.

[TESTING GAPS]

• [tests] These are behavior/security changes to tool execution and event emission, but the PR changes no tests. Please add focused regression tests for: malformed apply_patch headers returning structured errors; invalid *** Move to: paths; edit path traversal with a sibling-prefix case like ../workspace-escape/file.txt; and the event-service _main_loop race fix. These should exercise real code paths, not mocks of the unit under test.

[POLICY NOTE]

• This PR changes tool execution behavior (apply_patch, Gemini edit) and agent-server event emission. Per this repo's /codereview guidance, I’m using a COMMENT review rather than APPROVE unless eval evidence is provided or a human maintainer decides the eval-risk policy is satisfied.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This touches security-sensitive workspace path validation, tool execution semantics, and event-service concurrency. The intended safety improvements are valuable, but the current implementation leaves a real containment bypass and does not fully eliminate the race it claims to fix. Recommendation: Do not auto-merge; request human maintainer/security review after the boundary check and race fix are corrected and regression tests are added.

VERDICT:
❌ Needs rework: The core idea is worth merging, but the race fix and path-boundary check need to be made correct first.

KEY INSIGHT:
Hardening code must avoid approximate checks: local captures only help if you use them, and path containment needs filesystem path semantics, not string prefixes.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.