fix(openai-agents): populate tool_call field to enforce safety checks#354
fix(openai-agents): populate tool_call field to enforce safety checks#354DebasmitaBose0 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe OpenAI Agents adapter's ChangesFix Safety Bypass: Populate ToolCallData/ToolResultData in OpenAI Agents Adapter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧪 PR Test Results
Python 3.12 · commit ff5060a |
|
@DebasmitaBose0 This addresses a real safety gap and the direction of the fix looks correct. Before merge, please:
Once conflicts are resolved and the enforcement path is verified, I'll take another look. |
|
This PR has been waiting for several days and is now holding up work in the same area of the codebase. Please either:
At the moment, this security-related change is sitting in a stale state while other contributors continue modifying the same files, which is creating unnecessary conflict resolution work for everyone else. If I don't hear back or see progress soon, I'll have to assume the PR is inactive and move forward accordingly. Please update the branch and ping me once it's ready for review. |
|
@sreerevanth Sorry for the inconvenience caused. I am currently fixing this conflict, plz give me some time.... |
|
@sreerevanth kindly check now |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/adapters/openai_agents.py`:
- Around line 85-90: The tool-call path in OpenAI agent handling is still
emitting events via `_emit()` before safety is guaranteed, so the caller may
continue to execution before `SafetyEngine.check_event()` finishes. Update the
tool-call flow around `ToolCallData` creation and `_emit()` so safety is checked
on an awaited preflight path, or return/raise a block decision before any tool
execution can proceed. Ensure the fix is applied in the event emission path used
by the tool-call handler, not just inside the subscriber.
- Around line 77-81: The string-input handling in openai_agents.py only promotes
command-like tools to raw_command for a narrow set of names, so tools like cmd
or execute_command can leave executable text in arguments["input"] and bypass
the safety scan. Update the tool-name check in the string branch of the input
handling logic to recognize additional command-oriented names in the same place
where raw_command is assigned, so all command-style string inputs are routed
through raw_command instead of input.
In `@tests/test_openai_agents_adapter.py`:
- Around line 114-128: The test in safety_handler currently waits on a fixed
asyncio.sleep, which makes the regression timing-dependent. Update the test
around AgentWatchOpenAIAgentsAdapter.on_tool_call and bus.subscribe_fn to use a
deterministic signal from the async handler instead of sleeping, then await that
signal with a timeout so the assertion only proceeds once the event has been
processed.
- Line 4: The async test setup is redundant because asyncio_mode = "auto"
already handles it, so remove the unnecessary pytest dependency from the test
module. Update the async test in test_openai_agents_adapter by dropping both the
`@pytest.mark.asyncio` decorator and the import pytest statement, leaving the test
to run under the existing auto asyncio configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ebee1c7c-524d-4528-9204-855ab16779a7
📒 Files selected for processing (2)
agentwatch/adapters/openai_agents.pytests/test_openai_agents_adapter.py
| elif isinstance(input, str): | ||
| arguments = {"input": input} | ||
| # If it's a command-like tool name, promote the string input to raw_command | ||
| if any(kw in tool_name.lower() for kw in ("bash", "shell", "terminal", "run")): | ||
| raw_command = input |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Broaden string command promotion to avoid residual safety bypasses.
For string inputs, only tool names containing bash, shell, terminal, or run populate raw_command. Common command tools like cmd or execute_command would leave dangerous string content in arguments["input"], which the safety path does not scan as command text.
🛡️ Suggested fix
elif isinstance(input, str):
arguments = {"input": input}
# If it's a command-like tool name, promote the string input to raw_command
- if any(kw in tool_name.lower() for kw in ("bash", "shell", "terminal", "run")):
- raw_command = input
+ tool_name_lower = tool_name.lower()
+ command_like_names = (
+ "bash",
+ "shell",
+ "terminal",
+ "run",
+ "cmd",
+ "command",
+ "exec",
+ "powershell",
+ "pwsh",
+ )
+ if input.strip() and any(kw in tool_name_lower for kw in command_like_names):
+ raw_command = input.strip()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif isinstance(input, str): | |
| arguments = {"input": input} | |
| # If it's a command-like tool name, promote the string input to raw_command | |
| if any(kw in tool_name.lower() for kw in ("bash", "shell", "terminal", "run")): | |
| raw_command = input | |
| elif isinstance(input, str): | |
| arguments = {"input": input} | |
| # If it's a command-like tool name, promote the string input to raw_command | |
| tool_name_lower = tool_name.lower() | |
| command_like_names = ( | |
| "bash", | |
| "shell", | |
| "terminal", | |
| "run", | |
| "cmd", | |
| "command", | |
| "exec", | |
| "powershell", | |
| "pwsh", | |
| ) | |
| if input.strip() and any(kw in tool_name_lower for kw in command_like_names): | |
| raw_command = input.strip() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agentwatch/adapters/openai_agents.py` around lines 77 - 81, The string-input
handling in openai_agents.py only promotes command-like tools to raw_command for
a narrow set of names, so tools like cmd or execute_command can leave executable
text in arguments["input"] and bypass the safety scan. Update the tool-name
check in the string branch of the input handling logic to recognize additional
command-oriented names in the same place where raw_command is assigned, so all
command-style string inputs are routed through raw_command instead of input.
| event.tool_call = ToolCallData( | ||
| tool_name=tool_name, | ||
| arguments=arguments, | ||
| raw_command=raw_command, | ||
| ) | ||
| self._emit(event) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Ensure safety is enforced before the tool can execute.
_emit() uses publish_sync; inside an already-running event loop this schedules safety subscribers asynchronously and returns immediately. That means SafetyEngine.check_event() can block the event after the caller has already continued to tool execution. Please route tool calls through an awaited preflight path or return/raise a block decision before execution proceeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agentwatch/adapters/openai_agents.py` around lines 85 - 90, The tool-call
path in OpenAI agent handling is still emitting events via `_emit()` before
safety is guaranteed, so the caller may continue to execution before
`SafetyEngine.check_event()` finishes. Update the tool-call flow around
`ToolCallData` creation and `_emit()` so safety is checked on an awaited
preflight path, or return/raise a block decision before any tool execution can
proceed. Ensure the fix is applied in the event emission path used by the
tool-call handler, not just inside the subscriber.
| from agentwatch.adapters.openai_agents import AgentWatchOpenAIAgentsAdapter | ||
| from agentwatch.core.event_bus import EventBus | ||
| from agentwatch.core.schema import EventType, ExecutionStatus | ||
| import pytest |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify pytest async mode and remaining redundant asyncio markers.
rg -n "asyncio_mode\s*=" -g 'pyproject.toml' -g 'pytest.ini' -g 'setup.cfg' -g 'tox.ini' .
rg -n "`@pytest`\.mark\.asyncio|import pytest" tests/test_openai_agents_adapter.pyRepository: sreerevanth/AgentWatch
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file around the pytest import and async test.
sed -n '1,180p' tests/test_openai_agents_adapter.pyRepository: sreerevanth/AgentWatch
Length of output: 4323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether pytest is used anywhere else in this file.
rg -n "\bpytest\b" tests/test_openai_agents_adapter.pyRepository: sreerevanth/AgentWatch
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure to locate the async test and any fixtures.
ast-grep outline tests/test_openai_agents_adapter.py --view expandedRepository: sreerevanth/AgentWatch
Length of output: 601
Remove the redundant pytest async marker/import. asyncio_mode = "auto" already covers this async test, so both @pytest.mark.asyncio and import pytest can be dropped.
🧰 Tools
🪛 GitHub Actions: PR Tests / 0_Test & lint.txt
[error] 1-4: ruff check: I001 Import block is un-sorted or un-formatted. Help: Organize imports.
🪛 GitHub Actions: PR Tests / Test & lint
[error] 1-4: ruff check (I001): Import block is un-sorted or un-formatted. Help: Organize imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_openai_agents_adapter.py` at line 4, The async test setup is
redundant because asyncio_mode = "auto" already handles it, so remove the
unnecessary pytest dependency from the test module. Update the async test in
test_openai_agents_adapter by dropping both the `@pytest.mark.asyncio` decorator
and the import pytest statement, leaving the test to run under the existing auto
asyncio configuration.
Source: Coding guidelines
| captured = [] | ||
| async def safety_handler(event): | ||
| checked = await safety.check_event(event) | ||
| captured.append(checked) | ||
|
|
||
| bus.subscribe_fn(safety_handler, handler_id="test.safety") | ||
|
|
||
| adapter = AgentWatchOpenAIAgentsAdapter(event_bus=bus) | ||
|
|
||
| # Trigger a highly dangerous command | ||
| adapter.on_tool_call("bash", input={"command": "rm -rf /"}) | ||
|
|
||
| # Wait a brief moment for async subscription | ||
| import asyncio | ||
| await asyncio.sleep(0.05) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace the fixed sleep with a deterministic signal.
await asyncio.sleep(0.05) makes this regression timing-dependent; a slow audit or busy CI loop can fail even when the handler eventually blocks. Signal from the handler and wait with a timeout instead.
✅ Suggested fix
+ import asyncio
+
# Intercept tool calls through safety engine
captured = []
+ done = asyncio.Event()
+
async def safety_handler(event):
checked = await safety.check_event(event)
captured.append(checked)
+ done.set()
bus.subscribe_fn(safety_handler, handler_id="test.safety")
@@
- # Wait a brief moment for async subscription
- import asyncio
- await asyncio.sleep(0.05)
+ await asyncio.wait_for(done.wait(), timeout=1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| captured = [] | |
| async def safety_handler(event): | |
| checked = await safety.check_event(event) | |
| captured.append(checked) | |
| bus.subscribe_fn(safety_handler, handler_id="test.safety") | |
| adapter = AgentWatchOpenAIAgentsAdapter(event_bus=bus) | |
| # Trigger a highly dangerous command | |
| adapter.on_tool_call("bash", input={"command": "rm -rf /"}) | |
| # Wait a brief moment for async subscription | |
| import asyncio | |
| await asyncio.sleep(0.05) | |
| import asyncio | |
| # Intercept tool calls through safety engine | |
| captured = [] | |
| done = asyncio.Event() | |
| async def safety_handler(event): | |
| checked = await safety.check_event(event) | |
| captured.append(checked) | |
| done.set() | |
| bus.subscribe_fn(safety_handler, handler_id="test.safety") | |
| adapter = AgentWatchOpenAIAgentsAdapter(event_bus=bus) | |
| # Trigger a highly dangerous command | |
| adapter.on_tool_call("bash", input={"command": "rm -rf /"}) | |
| await asyncio.wait_for(done.wait(), timeout=1) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_openai_agents_adapter.py` around lines 114 - 128, The test in
safety_handler currently waits on a fixed asyncio.sleep, which makes the
regression timing-dependent. Update the test around
AgentWatchOpenAIAgentsAdapter.on_tool_call and bus.subscribe_fn to use a
deterministic signal from the async handler instead of sleeping, then await that
signal with a timeout so the assertion only proceeds once the event has been
processed.
ci is red still and adress coderabbit errors too |
2 participants
Closes #346
Summary
This PR fixes a security vulnerability where tool calls made via the OpenAI Agents framework silently bypassed pre-execution safety validations, resolving #346.
Problem
The
AgentWatchOpenAIAgentsAdapterclass emittedTOOL_CALLevents but failed to populate the top-leveltool_call(ToolCallData) field on theAgentEvent.Because
SafetyEngine.check_eventearly-returns without performing policy evaluation ifevent.tool_callisNone: