跨shi代的shi诗级的伟大的更新#8590
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
step,_simple_print_message_rolenow always logsself.run_context.messages, so the[BefCompact]and[AftCompact]logs will be identical; consider passing/using the provider-processed_provider_messagesfor the post-compaction log so the debug output actually reflects the transformation. - The tool call filtering in
stepbuildsvalid_indicesfromzip(llm_resp.tools_call_name, llm_resp.tools_call_ids)but then indexes intollm_resp.tools_call_argsusing those indices; iftools_call_argsis shorter thantools_call_name/tools_call_ids, this will raiseIndexError, so it would be safer to derive indices from zipping all three lists or clamp to the minimum length. - The new
_should_fix_modalities_for_provideronly treats a non-emptylistas a configured modalities value, whereas previously any truthy value (including non-lists) triggered sanitization; if there are providers with non-listmodalitiesconfigs, this silently changes behavior, so consider either validating/normalizingmodalitiesto a list or preserving the old truthiness semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `step`, `_simple_print_message_role` now always logs `self.run_context.messages`, so the `[BefCompact]` and `[AftCompact]` logs will be identical; consider passing/using the provider-processed `_provider_messages` for the post-compaction log so the debug output actually reflects the transformation.
- The tool call filtering in `step` builds `valid_indices` from `zip(llm_resp.tools_call_name, llm_resp.tools_call_ids)` but then indexes into `llm_resp.tools_call_args` using those indices; if `tools_call_args` is shorter than `tools_call_name`/`tools_call_ids`, this will raise `IndexError`, so it would be safer to derive indices from zipping all three lists or clamp to the minimum length.
- The new `_should_fix_modalities_for_provider` only treats a non-empty `list` as a configured modalities value, whereas previously any truthy value (including non-lists) triggered sanitization; if there are providers with non-list `modalities` configs, this silently changes behavior, so consider either validating/normalizing `modalities` to a list or preserving the old truthiness semantics.
## Individual Comments
### Comment 1
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="596-598" />
<code_context>
log_context_sanitize_stats(stats)
return sanitized_contexts
+ def _should_fix_modalities_for_provider(self) -> bool:
+ modalities = self.provider.provider_config.get("modalities", None)
+ return (
+ isinstance(modalities, list) and modalities
+ ) # Empty list is treated as unconfigured
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The new modalities check only supports non-empty `list` values and will skip sanitization for other sequence types.
This narrows the definition of “configured” from any truthy value to only a non-empty `list`. If a provider config uses another sequence type (e.g. a tuple), sanitization will now silently stop applying. If that’s intended, consider normalizing `modalities` to a list earlier; otherwise, widen the check to accept other iterable sequence types (or coerce to `list`) so the previous behavior is preserved while still treating `None`/`[]` as unconfigured.
Suggested implementation:
```python
def _should_fix_modalities_for_provider(self) -> bool:
modalities = self.provider.provider_config.get("modalities", None)
# Treat None and empty sequences (e.g. [], (), etc.) as "unconfigured".
# For non-sequence types, preserve the previous "truthy means configured" behavior.
if modalities is None:
return False
try:
from collections.abc import Sequence # local import to avoid module-level changes if not already present
except ImportError: # very defensive; Sequence should be available in supported Python versions
return bool(modalities)
if isinstance(modalities, Sequence) and not isinstance(modalities, (str, bytes)):
return len(modalities) > 0
return bool(modalities)
```
If `Sequence` is already imported at the module level (e.g. `from collections.abc import Sequence`), you should remove the local import inside `_should_fix_modalities_for_provider` and rely on the existing import instead to keep imports centralized and avoid redundant imports.
</issue_to_address>
### Comment 2
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="905-914" />
<code_context>
parts = None
+
+ # 过滤掉无效的 tool calls,确保 assistant 消息不包含无 id/name 的条目
+ if llm_resp.tools_call_name and llm_resp.tools_call_ids:
+ valid_indices = [
+ i for i, (name, tid) in enumerate(
+ zip(llm_resp.tools_call_name, llm_resp.tools_call_ids)
+ )
+ if name and tid
+ ]
+ if len(valid_indices) < len(llm_resp.tools_call_name):
+ llm_resp.tools_call_name = [llm_resp.tools_call_name[i] for i in valid_indices]
+ llm_resp.tools_call_args = [llm_resp.tools_call_args[i] for i in valid_indices]
+ llm_resp.tools_call_ids = [llm_resp.tools_call_ids[i] for i in valid_indices]
+
+ # 如果过滤后没有有效的 tool calls,跳过构建 tool_calls_result
</code_context>
<issue_to_address>
**issue (bug_risk):** Filtering tool calls by index assumes `tools_call_args` is at least as long as `tools_call_name`/`tools_call_ids`, which may raise `IndexError` on malformed provider output.
`valid_indices` comes from `zip(tools_call_name, tools_call_ids)`, but is then applied to `tools_call_args` without verifying that `tools_call_args` is at least as long. If a provider returns fewer args than names/ids, indexing `tools_call_args[i]` will raise `IndexError`. To avoid this, build the filtered lists in a single pass over `zip(tools_call_name, tools_call_args, tools_call_ids)` and append only fully valid triples, or use `zip_longest` with explicit handling of missing values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request refactors the tool loop agent runner to prevent mutating canonical messages during compaction, updates the LLM compressor parameters, and adds filtering to skip invalid tool calls with missing names or IDs. The review feedback highlights several critical issues: a parameter renaming mismatch that will cause an AttributeError in ContextManager, potential IndexError exceptions when accessing tool call names and arguments without bounds checks, and a type annotation mismatch where a method returns a list instead of a boolean.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

修复了 #8546 也修复了 #8552
Summary by Sourcery
Fix tool-loop agent handling of request-time context compaction and invalid tool calls.
Bug Fixes:
Enhancements:
上面的是一个Bot编辑的
实际上是
之后将要做的: