fix: avoid premature context compression from persisted usage - #9865
fix: avoid premature context compression from persisted usage#9865wcqqq1214 wants to merge 3 commits into
Conversation
|
@codex review |
![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: 283a4906e9
ℹ️ 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".
| self.request_context_manager.process( | ||
| self.run_context.messages, | ||
| trusted_token_usage=token_usage, | ||
| ) |
There was a problem hiding this comment.
On a later tool-loop step where the provider-reported context exceeds the 82% threshold because of tool schemas, multimodal accounting, or tokenizer differences while the message-only estimate remains below it, this skips compression and can send an over-limit request. _iter_llm_responses() sends func_tool separately, EstimateTokenCounter examines only messages, and conversation.token_usage is refreshed after every completed response, so during a loop this value is a recent provider-derived baseline rather than merely unrelated persisted data. Retain a sane provider-informed guard for subsequent steps instead of dropping the usage entirely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in da0cdb8. Persisted Conversation.token_usage remains excluded. The runner now keeps an in-memory snapshot of the last provider prompt only while the provider, tool set, and message prefix still match, then estimates newly appended messages. A regression test covers high provider prompt usage with a low messages-only estimate.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da0cdb8d93
ℹ️ 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".
| and snapshot.func_tool is current_func_tool | ||
| and len(self.run_context.messages) >= len(snapshot.message_ids) | ||
| and all( | ||
| id(message) == message_id |
There was a problem hiding this comment.
When a tool or on_tool_start/on_tool_end hook mutates an existing Message.content in place between loop steps, every id(message) still matches, so the old provider usage is reused even though the request prefix has changed. An expanded message can therefore be omitted from token accounting and produce an over-limit request; similarly, the ToolSet identity check misses in-place schema changes. Store and compare an immutable request fingerprint or otherwise invalidate the snapshot around mutable tool and hook execution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in b053a87. The snapshot now stores immutable SHA-256 fingerprints for each request-prefix message and the exposed tool schema. Any in-place mutation falls back to estimating the current request. Added regression coverage for on_tool_end mutations of an existing message and FunctionTool.parameters.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
dosubot
Bot
added
size:L
![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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="693-735" />
<code_context>
+ def _context_usage_fingerprints(
</code_context>
<issue_to_address>
**issue (broader_impact):** The snapshot fingerprint omits `req.extra_user_content_parts`, even though `_iter_llm_responses` sends those parts to the provider on every request. If a hook or caller mutates the extra content between tool-loop requests while messages, tools, and provider remain unchanged, the runner reuses the old full prompt usage and underestimates the current context.
**Triggers:** When `extra_user_content_parts` changes in place between two tool-loop requests.
**Suggested fix:** Include the provider-visible extra content parts in the snapshot fingerprint, or invalidate the snapshot whenever they change.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: astrbot/core/agent/runners/tool_loop_agent_runner.py:735
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def _context_usage_fingerprints( | ||
| self, | ||
| messages: list[Message], | ||
| func_tool: ToolSet | None, | ||
| ) -> tuple[tuple[str, ...], str | None]: | ||
| """Build immutable fingerprints for a context-usage snapshot. | ||
|
|
||
| Args: | ||
| messages: Messages that form the provider request context. | ||
| func_tool: Tools exposed to the provider for that request. | ||
|
|
||
| Returns: | ||
| Immutable fingerprints for the messages and tool schema. | ||
| """ | ||
| json_dump_kwargs = { | ||
| "default": str, | ||
| "ensure_ascii": False, | ||
| "separators": (",", ":"), | ||
| "sort_keys": True, | ||
| } | ||
| message_fingerprints = tuple( | ||
| hashlib.sha256( | ||
| json.dumps(message.model_dump(), **json_dump_kwargs).encode() | ||
| ).hexdigest() | ||
| for message in messages | ||
| ) | ||
| tool_schema_fingerprint = None | ||
| if func_tool is not None: | ||
| tool_schema_fingerprint = hashlib.sha256( | ||
| json.dumps( | ||
| [ | ||
| { | ||
| "active": getattr(tool, "active", True), | ||
| "description": tool.description, | ||
| "name": tool.name, | ||
| "parameters": tool.parameters, | ||
| } | ||
| for tool in func_tool.tools | ||
| ], | ||
| **json_dump_kwargs, | ||
| ).encode() | ||
| ).hexdigest() | ||
| return message_fingerprints, tool_schema_fingerprint |
There was a problem hiding this comment.
issue (broader_impact): The snapshot fingerprint omits req.extra_user_content_parts, even though _iter_llm_responses sends those parts to the provider on every request. If a hook or caller mutates the extra content between tool-loop requests while messages, tools, and provider remain unchanged, the runner reuses the old full prompt usage and underestimates the current context.
Triggers: When extra_user_content_parts changes in place between two tool-loop requests.
Suggested fix: Include the provider-visible extra content parts in the snapshot fingerprint, or invalidate the snapshot whenever they change.
1 participant
Fixes #9864.
Modifications / 改动点
Stop using persisted
conversation.token_usageas the token count for a new request context.Retain a runner-local provider prompt snapshot for the next tool-loop request only while the provider remains the same and the message-prefix and exposed-tool-schema fingerprints still match. The snapshot keeps provider-only overhead such as tool schemas while newly appended messages are estimated.
Fall back to full message estimation after a change to the snapshotted prefix or tool schema, or when no matching runtime snapshot exists.
Add regressions for stale persisted usage, a high provider prompt usage with a low messages-only estimate, and in-place mutations of an existing message or
FunctionTool.parameters.This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Verification steps:
uv run pytest tests/test_tool_loop_agent_runner.py tests/agent/test_context_manager.py tests/agent/test_token_counter.py -q— passed: 107 tests.make pr-test-neo— passed: Ruff format and lint checks, 13 tests, and the startup smoke test.Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Correct context compression accounting so requests use current message estimates and only reuse validated provider prompt usage from the active tool loop.
Bug Fixes:
Enhancements:
Tests: