Skip to content

Conversation

@matdev83
Copy link
Owner

@matdev83 matdev83 commented Oct 12, 2025

Summary

  • ensure the tool call reactor middleware still swallows calls when handlers return an empty replacement response
  • add a regression unit test covering the empty replacement scenario

Testing

  • python -m pytest -o addopts='' tests/unit/core/services/test_tool_call_reactor_middleware.py
  • python -m pytest -o addopts='' (fails: missing pytest_asyncio, respx, pytest_httpx optional dependencies)

https://chatgpt.com/codex/tasks/task_e_68ec25355ce08333b87d0889ca85b9a5

Summary by CodeRabbit

  • Bug Fixes

    • Empty replacement responses from tool-driven actions are now respected rather than ignored, preventing unintended fallback content and ensuring outputs can be intentionally blank when appropriate.
    • Preserves related metadata flags to accurately reflect when a tool call was swallowed.
  • Tests

    • Added unit test to validate behavior when a swallowed tool call provides an empty replacement, ensuring the response content is empty and metadata is correctly set.

@coderabbitai
Copy link

coderabbitai bot commented Oct 12, 2025

Walkthrough

Updated middleware to treat empty replacement responses as valid by switching from a truthiness check to an explicit None check. Added a unit test to validate that an empty string replacement produces empty content while marking the tool call as swallowed.

Changes

Cohort / File(s) Summary
Middleware logic update
src/core/services/tool_call_reactor_middleware.py
Changed condition from truthy check to is not None for replacement_response, allowing empty strings to be used as valid replacements.
Unit tests
tests/unit/core/services/test_tool_call_reactor_middleware.py
Added test test_process_with_empty_replacement asserting empty replacement content is respected and tool_call_swallowed metadata is set; note duplication of the test within the file in the diff.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Client
  participant M as ToolCallReactorMiddleware
  participant T as Tool Call Handler

  C->>M: process(request)
  M->>T: evaluate tool call
  T-->>M: swallow_result (replacement_response may be "", None, or text)

  alt replacement_response is not None
    note right of M: Use replacement even if ""
    M-->>C: ProcessedResponse(content = replacement_response, metadata.tool_call_swallowed = true)
  else replacement_response is None
    M-->>C: ProcessedResponse(content = original, metadata.tool_call_swallowed = false)
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nibbled the code, a delicate bite,
Turned “falsy” to fine in the moonlit night.
An empty whisper now counts as a say,
Middleware dances the swallow-away.
Tests hop beside, two footprints in snow—
A bunny approves: on with the flow! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly identifies the main change—improving the middleware’s handling of empty steering responses—and matches the adjustments made to the tool call reactor and related tests in this PR. It is concise, specific, and directly reflects the intent without extraneous detail.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/fix-a-bug-in-reactor-tool-97b52g

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd216cf and 00a31b8.

📒 Files selected for processing (2)
  • src/core/services/tool_call_reactor_middleware.py (1 hunks)
  • tests/unit/core/services/test_tool_call_reactor_middleware.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting

Files:

  • src/core/services/tool_call_reactor_middleware.py
  • tests/unit/core/services/test_tool_call_reactor_middleware.py
src/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions

Files:

  • src/core/services/tool_call_reactor_middleware.py
🧬 Code graph analysis (1)
tests/unit/core/services/test_tool_call_reactor_middleware.py (4)
tests/unit/test_tool_call_loop_middleware.py (1)
  • tool_call_response (43-72)
src/core/interfaces/response_processor_interface.py (1)
  • ProcessedResponse (8-30)
src/core/interfaces/tool_call_reactor_interface.py (1)
  • ToolCallReactionResult (47-57)
src/core/services/tool_call_reactor_middleware.py (1)
  • process (57-220)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (python)
🔇 Additional comments (2)
src/core/services/tool_call_reactor_middleware.py (1)

198-198: LGTM! Critical fix for empty replacement handling.

Switching from truthiness check to explicit is not None correctly allows empty strings as valid replacement content. This aligns with the type signature str | None for replacement_response and enables handlers to swallow tool calls silently by returning an empty string.

tests/unit/core/services/test_tool_call_reactor_middleware.py (1)

309-352: LGTM! Solid regression test.

This test properly validates the empty replacement scenario, ensuring that handlers can swallow tool calls with empty content while still marking them as swallowed in metadata. The assertions are comprehensive and align with the middleware fix.

Note: The AI summary mentions test duplication, but I only see this test defined once in the provided code.


Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant