fix: sanitize incomplete tool history before provider fallback - #9707
fix: sanitize incomplete tool history before provider fallback#9707SunmiJJW wants to merge 5 commits into
Conversation
dosubot
Bot
added
size:M
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new validation logic in
flush_pending_if_validis fairly dense; consider extracting it into a helper function (e.g.,_is_valid_tool_block(pending_assistant, pending_tools)) to improve readability and future maintenance. - The warning in
ContextManager.processabout removed invalid tool history will fire on every call where sanitization happens; consider adding contextual information (e.g., conversation ID or a capped rate) to avoid noisy logs in high-throughput scenarios.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new validation logic in `flush_pending_if_valid` is fairly dense; consider extracting it into a helper function (e.g., `_is_valid_tool_block(pending_assistant, pending_tools)`) to improve readability and future maintenance.
- The warning in `ContextManager.process` about removed invalid tool history will fire on every call where sanitization happens; consider adding contextual information (e.g., conversation ID or a capped rate) to avoid noisy logs in high-throughput scenarios.
## Individual Comments
### Comment 1
<location path="astrbot/core/agent/context/truncator.py" line_range="68" />
<code_context>
def flush_pending_if_valid() -> None:
nonlocal pending_assistant, pending_tools
- if pending_assistant is not None and pending_tools:
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the ID extraction and validation logic in `flush_pending_if_valid` into small helper functions and named boolean checks to make the function easier to read and test while keeping the same behavior.
You can keep the stricter validation while making `flush_pending_if_valid` much easier to read and test by splitting out the ID extraction and the compound condition into small helpers and named checks.
For example:
```python
def _extract_tool_call_ids(assistant: Message) -> list[str]:
ids: list[str] = []
for tool_call in assistant.tool_calls or []:
if isinstance(tool_call, dict):
tool_call_id = tool_call.get("id")
else:
tool_call_id = tool_call.id
if not isinstance(tool_call_id, str) or not tool_call_id:
return [] # invalid -> signal failure
ids.append(tool_call_id)
return ids
def _extract_result_ids(pending_tools: list[Message]) -> list[str]:
return [tool.tool_call_id for tool in pending_tools]
```
Then `flush_pending_if_valid` can focus on validation, with named boolean checks instead of one dense condition:
```python
def flush_pending_if_valid() -> None:
nonlocal pending_assistant, pending_tools
if pending_assistant is not None:
expected_ids = _extract_tool_call_ids(pending_assistant)
result_ids = _extract_result_ids(pending_tools)
has_valid_expected_ids = bool(expected_ids) and len(expected_ids) == len(set(expected_ids))
has_valid_result_ids = (
len(result_ids) == len(expected_ids)
and all(isinstance(tool_id, str) and tool_id for tool_id in result_ids)
and len(result_ids) == len(set(result_ids))
)
ids_match = set(result_ids) == set(expected_ids)
if has_valid_expected_ids and has_valid_result_ids and ids_match:
fixed_messages.append(pending_assistant)
fixed_messages.extend(pending_tools)
pending_assistant = None
pending_tools = []
```
This keeps all existing behavior (including the dict/object handling and the “abort on any invalid ID” semantics), but makes:
- ID extraction reusable and testable in isolation.
- The validation rules explicit via `has_valid_expected_ids`, `has_valid_result_ids`, and `ids_match`, instead of one long compound condition.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8beb863b90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
AstrBot/astrbot/core/agent/context/manager.py
Lines 75 to 77 in 3dbd608
Recompute token count after sanitizing history
When fix_messages() removes an incomplete tool block, the supplied trusted_token_usage still describes the unsanitized history, and EstimateTokenCounter.count_tokens() returns that value without examining result. If a conversation's previous usage exceeds the compression threshold, even a now-small sanitized context is unnecessarily compressed; with the default turn compressor and the common surviving sequence [old user, current user], this can discard the current request and retain only the old user message. Ignore the trusted value and recount whenever sanitization changes the message list.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 643eae73e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the review note about stale trusted token usage in e077836. ContextManager.process() now re-estimates tokens whenever tool-history sanitization changes the message list, while unchanged histories still use provider-reported usage. The regression proves the concrete 83-token stale value is replaced by the sanitized context estimate and that compression is not spuriously invoked. This is included in the current 138/138 focused pass. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Root cause
ContextTruncator.fix_messages()previously retained an assistant tool-call block when it had any following tool result. A block containing multiple tool calls could therefore be kept with only a subset of its results. In addition,ContextManager.process()only reached this repair through size-limited truncation/compression paths, so configurations without those limits could pass malformed history unchanged to both primary and fallback providers.Two compatibility edges also matter: local tools may yield more than one result for one invocation, and Gemini may omit native call IDs for parallel invocations of the same function. The generic history must preserve those results while remaining valid if a later request switches to an OpenAI-compatible fallback.
Behavior
Complete multi-tool blocks are preserved, including when matching tool results arrive in a different order. Incomplete, duplicate, or otherwise invalid generic tool-call/result blocks are removed as a unit. Multi-yield output is coalesced per invocation, and Gemini fallback IDs are made unique before entering generic history while their original function names are restored for Gemini function responses. The repair does not synthesize tool results.
When sanitization changes the message list, token usage is recounted from the sanitized context. Unchanged history still benefits from provider-reported trusted usage.
Tests
python -m pytest -q tests/agent/test_truncator.py tests/agent/test_context_manager.py tests/test_gemini_source.py tests/test_tool_loop_agent_runner.py(138 passed)python -m ruff format --check .python -m ruff check .python -m compileall -q astrbot/core/agent/context astrbot/core/agent/runners/tool_loop_agent_runner.py astrbot/core/provider/sources/gemini_source.pygit diff --check