Skip to content

Handle tuple arguments in pytest full suite handler #635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Handle tuple arguments in pytest full suite handler #635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/core/services/tool_call_handlers/pytest_full_suite_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

import logging
import re
from collections.abc import Sequence
from dataclasses import dataclass
from pathlib import Path
from typing import Any
Expand Down Expand Up @@ -81,7 +82,11 @@ def _extract_command(arguments: Any) -> str | None:

return None

if isinstance(arguments, list):
if (
isinstance(arguments, Sequence)
and not isinstance(arguments, str | bytes)
and arguments
):
return " ".join(str(item) for item in arguments)
Comment on lines +85 to 90
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Implementation correctly handles generic sequences.

The change from checking only list to checking any Sequence (excluding str and bytes) is well-implemented. The exclusion of str and bytes is necessary because they're also sequences in Python but shouldn't be split into command components.

However, verify the type contract: ToolCallContext.tool_arguments is typed as dict[str, Any] in the interface (see src/core/interfaces/tool_call_reactor_interface.py lines 17-42), but this code path handles sequences at the root level. This suggests either:

  1. The implementation is being defensive against runtime type contract violations from different LLM providers
  2. The type annotation in ToolCallContext may need updating to dict[str, Any] | Sequence[Any]

Run this script to check if tool_arguments is passed as a non-dict elsewhere in the codebase:

🏁 Script executed:

#!/bin/bash
# Search for ToolCallContext instantiations where tool_arguments might not be a dict

# Look for ToolCallContext construction with tool_arguments parameter
rg -nP --type=py -C3 'ToolCallContext\s*\(' | rg -P 'tool_arguments\s*='

Length of output: 249

🏁 Script executed:

#!/bin/bash
# Display ToolCallContext definition
sed -n '1,200p' src/core/interfaces/tool_call_reactor_interface.py

Length of output: 5079

🏁 Script executed:

#!/bin/bash
# Show context around Sequence handling in pytest_full_suite_handler.py
sed -n '1,200p' src/core/services/tool_call_handlers/pytest_full_suite_handler.py

Length of output: 6419

🏁 Script executed:

#!/bin/bash
# Locate the _extract_pytest_command method
rg -n 'def _extract_pytest_command' -C5 src/core/services/tool_call_handlers/pytest_full_suite_handler.py

Length of output: 426

Update ToolCallContext.tool_arguments annotation to accept both mappings and sequences
Change its type in src/core/interfaces/tool_call_reactor_interface.py from

tool_arguments: dict[str, Any]

to

tool_arguments: dict[str, Any] | Sequence[Any]

so it matches the handler’s support for root-level sequences.

🤖 Prompt for AI Agents 
In src/core/services/tool_call_handlers/pytest_full_suite_handler.py around
lines 85 to 90, the handler accepts root-level sequences for tool arguments but
the ToolCallContext.tool_arguments type is still declared as dict[str, Any];
update the annotation in src/core/interfaces/tool_call_reactor_interface.py to
accept either a mapping or a sequence by changing tool_arguments: dict[str, Any]
to tool_arguments: dict[str, Any] | Sequence[Any], and add the necessary import
for Sequence from typing (or collections.abc) to ensure the type resolves.


return None
Expand Down
18 changes: 18 additions & 0 deletions tests/unit/core/services/tool_call_handlers/test_pytest_full_suite_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,24 @@ async def test_handler_detects_list_based_command() -> None:
assert result.should_swallow is True


@pytest.mark.asyncio
async def test_handler_detects_tuple_arguments_root_level() -> None:
handler = PytestFullSuiteHandler(enabled=True)
context = ToolCallContext(
session_id="session-tuple",
backend_name="backend",
model_name="model",
full_response={},
tool_name="bash",
tool_arguments=("pytest", "-q"),
)

assert await handler.can_handle(context) is True
result = await handler.handle(context)

assert result.should_swallow is True

Comment on lines +146 to +162
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Type contract violation: tool_arguments should be dict[str, Any].

The test passes a tuple ("pytest", "-q") directly as tool_arguments, but ToolCallContext.tool_arguments is typed as dict[str, Any] (see src/core/interfaces/tool_call_reactor_interface.py line 38). This creates a type safety violation.

Consider one of these solutions:

  1. If tuples are intentionally supported: Update ToolCallContext.tool_arguments type annotation to dict[str, Any] | Sequence[Any] to reflect the actual supported types
  2. If this is just defensive programming: Keep the test but add a comment explaining that it tests edge cases from providers that violate the type contract
  3. If tuples shouldn't be supported at root level: Wrap the tuple in a dict like existing tests do: tool_arguments={"command": ("pytest", "-q")}

To verify the intended behavior, please clarify:

What is the expected type contract for ToolCallContext.tool_arguments in Python tool call handling frameworks? Should it strictly be a dictionary or support sequences at the root level?
🤖 Prompt for AI Agents 
In tests/unit/core/services/tool_call_handlers/test_pytest_full_suite_handler.py
around lines 146-162 the test passes a tuple as tool_arguments which violates
the declared ToolCallContext.tool_arguments type (dict[str, Any]); fix the test
by wrapping the tuple inside a dictionary (use a key like "command") so
tool_arguments is a dict, and add a short comment explaining this mirrors the
expected contract; alternatively, if the framework should accept sequences at
the root level, update the ToolCallContext type annotation to include
Sequence[Any] and adjust any callers/tests accordingly—pick one approach and
make the corresponding change and comment.


@pytest.mark.asyncio
async def test_handler_enabled_flag_controls_behavior() -> None:
handler = PytestFullSuiteHandler(enabled=False)
Expand Down
Loading