feat(agentscope): implement D5 safe compression and auto-trigger - #136
Conversation
|
@copilot review |
Synchronize D5 documentation state after PR creation so TODO claims and feature evidence stay consistent with repository workflow governance. Key changes:\n- update D5 claim notes in both overall and domain TODO ledgers from 'pending PR' to 'PR #136 in review'\n- move feature aggregation status to in_review and mark PR submission complete\n- add explicit PR link and merge-gate placeholder in evidence section Rationale:\nThe claim-ledger policy requires explicit ownership and current execution state to avoid parallel conflict. These updates keep docs as the canonical source of truth while PR #136 is under 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: 44cd138c6a
ℹ️ 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".
| await self._emit_transport_success( | ||
| transport=transport, | ||
| event_type=TransportEventType.MESSAGE.value, | ||
| target="prompt", | ||
| resp={"output": final_text}, |
There was a problem hiding this comment.
When ReactAgent.execute is invoked from the transport loop (BaseAgent._execute_polled_message), the base class already emits a terminal result envelope after execute returns. Emitting a message envelope here adds a second terminal event for the same turn, which leads clients (notably stdio) to render duplicate/conflicting final output for one request. This should be gated to avoid double terminal emission in transport-loop execution.
Useful? React with 👍 / 👎.
| usage.get("output_tokens_details", {}).get("reasoning") | ||
| if isinstance(usage.get("output_tokens_details"), dict) |
There was a problem hiding this comment.
_extract_reasoning_tokens checks output_tokens_details["reasoning"] but not output_tokens_details["reasoning_tokens"]; OpenAI-compatible usage payloads frequently use the latter key. In those responses, reasoning token usage is silently dropped from normalized usage, so downstream token accounting/observability loses reasoning-cost data even though the provider returned it.
Useful? React with 👍 / 👎.
|
已根据 review feedback 完成修复并补充回归测试，辛苦再看一轮。@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 633ac24cec
ℹ️ 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".
| except (TypeError, ValueError): | ||
| target_tokens = None | ||
|
|
||
| if (max_messages is None or max_messages < 0) and (target_tokens is None or target_tokens <= 0): |
There was a problem hiding this comment.
With the new token-aware path, max_messages < 0 no longer returns early when target_tokens is set, so sentinel values like -1 now flow into the count-trim step and trigger messages[-max_messages:], which drops history even when token usage is already under target. This silently rewrites context for configurations that use negative max_messages to mean “no message cap,” and can change model behavior unexpectedly.
Useful? React with 👍 / 👎.
| if not self._is_transport_loop_execution(transport=transport): | ||
| await self._emit_transport_success( |
There was a problem hiding this comment.
Terminal transport emission is currently only wired into the not response.tool_calls branch, so when execution exits via the repeated-tool guard or max-round fallback, execute(..., transport=...) returns without sending any final MESSAGE/RESULT event. In those cases stream consumers receive intermediate tool events but no completion signal, which makes transport-driven UIs hang waiting for a terminal event.
Useful? React with 👍 / 👎.
…mit terminal events on non-converged exits Address new Codex review findings on PR #136.\n\n1) compression max_messages sentinel:\n accepted negative max_messages as an unbounded sentinel, but token-aware compression path later reused the negative value in list slicing (), dropping history unexpectedly. The fix normalizes negative max_messages to current message count before count-based trim.\n\n2) ReactAgent non-converged terminal transport signaling:\nDirect emitted terminal MESSAGE only on normal no-tool completion, not on repeated-tool guard or max-round fallback. Stream consumers could receive intermediate events without a completion signal. The fix introduces a shared terminal MESSAGE helper and uses it for all terminal exits while still suppressing duplicates under transport-loop execution.\n\nTests added to prove both regressions and lock behavior.
|
已处理本轮新增 review 并补测试：\n- |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0742e28a02
ℹ️ 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".
| if isinstance(tool_id, str) and tool_id.strip() and tool_id.strip() in tool_result_ids: | ||
| filtered_calls.append(call) |
There was a problem hiding this comment.
The new tool-pair safety pass only keeps assistant tool calls when tool_call.id is a non-empty string present in tool-result names, so providers that omit IDs (but still emit valid tool calls/results) get both sides stripped during compression. This is reachable from ReactAgent auto-compression (tool_pair_safe=True), and it can erase prior tool observations mid-session, causing degraded reasoning or repeated tool loops for ID-less adapters.
Useful? React with 👍 / 👎.
| if isinstance(resp, dict): | ||
| request_id = resp.get("request_id") | ||
| decision = resp.get("decision") | ||
| if isinstance(resp.get("request"), dict): | ||
| request_id = resp["request"].get("request_id") |
There was a problem hiding this comment.
_render_status_output treats any dict resp as an approval payload and returns "approval update" when no request ID is found, which makes the later resp["phase"] handling unreachable. As a result, canonical status events that carry structured non-approval resp data (for example {"phase": ...}) are displayed incorrectly in stdio output.
Useful? React with 👍 / 👎.
|
Processed new actionable review items in this patrol run.\n\nImplemented locally on branch |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0742e28a02
ℹ️ 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".
| tool_id = call.get("id") | ||
| if isinstance(tool_id, str) and tool_id.strip() and tool_id.strip() in tool_result_ids: | ||
| filtered_calls.append(call) |
There was a problem hiding this comment.
_enforce_tool_pair_safety currently retains assistant tool calls only when call["id"] is a non-empty string that matches a tool result id. When a provider omits tool_call.id (a supported pattern, since tool results are stored with tool_call_id or name), this drops valid assistant calls and then removes matching tool messages in the second pass because no call id is retained, so compression can silently erase critical tool context before the next model turn.
Useful? React with 👍 / 👎.
| return "approval update" | ||
|
|
||
| if isinstance(resp, dict) and "phase" in resp: |
There was a problem hiding this comment.
The status renderer returns "approval update" for any dict resp without a request_id, which makes the subsequent resp.phase handling unreachable. As a result, structured non-approval status payloads like {"resp": {"phase": "running"}} are rendered as a generic approval message instead of the actual phase, so stdio users lose meaningful status updates.
Useful? React with 👍 / 👎.
…ering Address unresolved PR #136 review findings that affect compression safety and\ntransport status output.\n\nKey changes:\n- Keep assistant tool calls that do not provide an id during tool-pair safety\n filtering, and avoid dropping tool messages when id-less calls exist.\n- Update status rendering so non-approval structured payloads in resp (e.g.\n {"phase": ...}) are rendered as phase/event text instead of forced\n "approval update".\n- Add regression tests for id-less tool call/tool result preservation under\n tool_pair_safe mode.\n- Add regression test for stdio status rendering from resp.phase payloads.\n\nRationale:\nOpenAI-compatible and other adapters may omit tool call ids while still emitting\nvalid tool call/result context. Compression should not erase that context.\nLikewise, status envelopes should display canonical progress fields instead of\nbeing coerced to approval-only text.
|
Processed the new actionable review comments from this patrol window and confirmed the fixes are now on the PR branch. Pushed:
What this addresses:
Targeted verification:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 761faeab07
ℹ️ 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".
| if has_idless_tool_calls: | ||
| final_messages.append(message) | ||
| continue |
There was a problem hiding this comment.
Do not short-circuit tool-result pruning when any id-less tool call exists. This branch keeps all tool messages whenever one assistant call lacks an id, so mixed histories (an id-less tool call plus stale id-based tool results from older turns) still retain orphan tool messages even with tool_pair_safe=True. That breaks the function’s own “never leaves orphan pairs” guarantee and can reintroduce unrelated tool outputs into later model prompts.
Useful? React with 👍 / 👎.
Address the new PR #136 review finding on tool-pair safety behavior in mixed id/id-less histories.\n\nKey changes:\n- Updated _enforce_tool_pair_safety to retain tool results by explicit call-id matches and by id-less call tool names, instead of preserving all tool results whenever any id-less call exists.\n- This keeps valid id-less tool context while removing stale orphan tool results from unrelated turns.\n\nRegression coverage:\n- Added a compression regression test for mixed assistant tool_calls (id-based + id-less) verifying matched results are preserved and orphan tool results are pruned.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_context_compression.py tests/unit/test_openai_model_adapter.py tests/unit/test_transport_adapters.py
|
Processed the new actionable review comment from this patrol window and pushed the fix to the PR branch. Pushed:
What this addresses:
Targeted verification:
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ 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". |
Synchronize D5 documentation state after PR creation so TODO claims and feature evidence stay consistent with repository workflow governance. Key changes:\n- update D5 claim notes in both overall and domain TODO ledgers from 'pending PR' to 'PR #136 in review'\n- move feature aggregation status to in_review and mark PR submission complete\n- add explicit PR link and merge-gate placeholder in evidence section Rationale:\nThe claim-ledger policy requires explicit ownership and current execution state to avoid parallel conflict. These updates keep docs as the canonical source of truth while PR #136 is under review.
…mit terminal events on non-converged exits Address new Codex review findings on PR #136.\n\n1) compression max_messages sentinel:\n accepted negative max_messages as an unbounded sentinel, but token-aware compression path later reused the negative value in list slicing (), dropping history unexpectedly. The fix normalizes negative max_messages to current message count before count-based trim.\n\n2) ReactAgent non-converged terminal transport signaling:\nDirect emitted terminal MESSAGE only on normal no-tool completion, not on repeated-tool guard or max-round fallback. Stream consumers could receive intermediate events without a completion signal. The fix introduces a shared terminal MESSAGE helper and uses it for all terminal exits while still suppressing duplicates under transport-loop execution.\n\nTests added to prove both regressions and lock behavior.
df50ead to
31627eb
Compare
…ering Address unresolved PR #136 review findings that affect compression safety and\ntransport status output.\n\nKey changes:\n- Keep assistant tool calls that do not provide an id during tool-pair safety\n filtering, and avoid dropping tool messages when id-less calls exist.\n- Update status rendering so non-approval structured payloads in resp (e.g.\n {"phase": ...}) are rendered as phase/event text instead of forced\n "approval update".\n- Add regression tests for id-less tool call/tool result preservation under\n tool_pair_safe mode.\n- Add regression test for stdio status rendering from resp.phase payloads.\n\nRationale:\nOpenAI-compatible and other adapters may omit tool call ids while still emitting\nvalid tool call/result context. Compression should not erase that context.\nLikewise, status envelopes should display canonical progress fields instead of\nbeing coerced to approval-only text.
Address the new PR #136 review finding on tool-pair safety behavior in mixed id/id-less histories.\n\nKey changes:\n- Updated _enforce_tool_pair_safety to retain tool results by explicit call-id matches and by id-less call tool names, instead of preserving all tool results whenever any id-less call exists.\n- This keeps valid id-less tool context while removing stale orphan tool results from unrelated turns.\n\nRegression coverage:\n- Added a compression regression test for mixed assistant tool_calls (id-based + id-less) verifying matched results are preserved and orphan tool results are pruned.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_context_compression.py tests/unit/test_openai_model_adapter.py tests/unit/test_transport_adapters.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31627eb284
ℹ️ 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".
| messages[0] = CtxMessage( | ||
| role=head.role, | ||
| content=head.content, | ||
| name=head.name, | ||
| metadata=metadata, |
There was a problem hiding this comment.
When compression rewrites context, _annotate_strategy rebuilds the head message without carrying over id/mark, so SmartContext sentinel messages (for example core/task_complete) lose their persistent identity and can be re-added as new TEMPORARY entries when STM is rewritten. This is reachable on any compression that removes at least one message (e.g., dedup), and it breaks downstream logic that relies on stable message IDs/marks for ordering and retention.
Useful? React with 👍 / 👎.
| self._transport_loop_execution = True | ||
| try: | ||
| result = await self.execute(task, transport=channel) | ||
| finally: | ||
| self._transport_loop_execution = False |
There was a problem hiding this comment.
_transport_loop_execution is stored as a single agent-wide boolean, so concurrent executions can interfere: while _execute_polled_message is running, any separate direct execute(..., transport=...) call will also observe the flag and be treated as transport-loop execution. In ReactAgent this suppresses terminal MESSAGE emission for that direct call, leaving its transport consumer without a completion event. The marker needs task-local scoping to avoid cross-request leakage.
Useful? React with 👍 / 👎.
Implement the AgentScope D5 gap slice for context compression safety and budget convergence, with docs-first/OpenSpec artifacts and evidence synced. Key changes:\n- route Context.compress through dare_framework.compression.core.compress_context for unified behavior\n- add token-aware trimming (target_tokens), tool pair safety enforcement, and compression strategy metadata annotation\n- add pre-model auto-compression trigger in ReactAgent with configurable ratios and preserved default backward-compatible behavior\n- add D5 regression tests for tool pair safety, token-budget trimming, and auto-compress trigger timing\n- add OpenSpec change artifacts and feature evidence doc; update project/domain claim ledgers to owner zts212653 with D5 execution evidence Rationale:\nAgentScope compatibility requires compression that does not orphan tool call/result pairs and can converge by token budget before model invocation. This commit closes that gap while keeping legacy execution unchanged when auto-compress is disabled.
Synchronize D5 documentation state after PR creation so TODO claims and feature evidence stay consistent with repository workflow governance. Key changes:\n- update D5 claim notes in both overall and domain TODO ledgers from 'pending PR' to 'PR #136 in review'\n- move feature aggregation status to in_review and mark PR submission complete\n- add explicit PR link and merge-gate placeholder in evidence section Rationale:\nThe claim-ledger policy requires explicit ownership and current execution state to avoid parallel conflict. These updates keep docs as the canonical source of truth while PR #136 is under review.
…mit terminal events on non-converged exits Address new Codex review findings on PR #136.\n\n1) compression max_messages sentinel:\n accepted negative max_messages as an unbounded sentinel, but token-aware compression path later reused the negative value in list slicing (), dropping history unexpectedly. The fix normalizes negative max_messages to current message count before count-based trim.\n\n2) ReactAgent non-converged terminal transport signaling:\nDirect emitted terminal MESSAGE only on normal no-tool completion, not on repeated-tool guard or max-round fallback. Stream consumers could receive intermediate events without a completion signal. The fix introduces a shared terminal MESSAGE helper and uses it for all terminal exits while still suppressing duplicates under transport-loop execution.\n\nTests added to prove both regressions and lock behavior.
…ering Address unresolved PR #136 review findings that affect compression safety and\ntransport status output.\n\nKey changes:\n- Keep assistant tool calls that do not provide an id during tool-pair safety\n filtering, and avoid dropping tool messages when id-less calls exist.\n- Update status rendering so non-approval structured payloads in resp (e.g.\n {"phase": ...}) are rendered as phase/event text instead of forced\n "approval update".\n- Add regression tests for id-less tool call/tool result preservation under\n tool_pair_safe mode.\n- Add regression test for stdio status rendering from resp.phase payloads.\n\nRationale:\nOpenAI-compatible and other adapters may omit tool call ids while still emitting\nvalid tool call/result context. Compression should not erase that context.\nLikewise, status envelopes should display canonical progress fields instead of\nbeing coerced to approval-only text.
Address the new PR #136 review finding on tool-pair safety behavior in mixed id/id-less histories.\n\nKey changes:\n- Updated _enforce_tool_pair_safety to retain tool results by explicit call-id matches and by id-less call tool names, instead of preserving all tool results whenever any id-less call exists.\n- This keeps valid id-less tool context while removing stale orphan tool results from unrelated turns.\n\nRegression coverage:\n- Added a compression regression test for mixed assistant tool_calls (id-based + id-less) verifying matched results are preserved and orphan tool results are pruned.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_context_compression.py tests/unit/test_openai_model_adapter.py tests/unit/test_transport_adapters.py
During the #136 rebase onto latest main, conflict resolution in test_react_agent_gateway_injection.py dropped the @pytest.mark.asyncio decorator on test_react_agent_auto_compress_triggers_before_model_call.\n\nThis commit restores the decorator so pytest executes the async test correctly under the existing asyncio plugin configuration.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_context_compression.py tests/unit/test_openai_model_adapter.py tests/unit/test_transport_adapters.py tests/unit/test_react_agent_gateway_injection.py\n- Result: 33 passed.
31627eb to
2cfbcbb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cfbcbbf3c
ℹ️ 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".
| from dare_framework.compression.core import compress_context | ||
|
|
||
| compress_context(self, **options) |
There was a problem hiding this comment.
Context.compress now bypasses short_term_memory.compress and always runs compress_context on the assembled STM API, which drops memory-backend-specific guarantees (notably InMemorySmartSTM.compress preserving IMMUTABLE/PERSISTENT messages). In SmartContext flows, any compression with a low max_messages can now evict persistent state like core/task_complete, changing agent behavior and corrupting long-running task memory that was previously protected.
Useful? React with 👍 / 👎.
| messages = self._build_model_messages(assembled) | ||
| if self._maybe_auto_compress(messages): |
There was a problem hiding this comment.
The new auto-compression hook is only invoked in _execute_basic at this call site, but execute() routes all SmartContext instances to _execute_with_smart_context, where model calls still happen without _maybe_auto_compress. As a result, ReactAgent(auto_compress=True) is silently ineffective for SmartContext agents, so long smart-context runs continue without the intended pre-model compression and can still hit token-budget pressure.
Useful? React with 👍 / 👎.
…auto compress Address new PR #136 review feedback around compression identity semantics, STM backend behavior, and SmartContext parity.\n\nKey changes:\n- Preserve message identity on compression annotation by retaining message id/mark when rewriting the head message metadata.\n- Keep IMMUTABLE/PERSISTENT messages during max_messages and token trimming; when only protected messages remain, stop trimming safely.\n- Fix the keep_temporary=0 edge case in max_messages truncation (avoid accidental temporary retention from list[-0:]).\n- Update Context.compress to preserve backend STM compress semantics for max_messages-only calls and to pre-apply backend count-based compression before advanced strategy compression.\n- Add auto-compress trigger in ReactAgent SmartContext execution path and rebuild ordered model messages post-compression while retaining injected reflection prompts.\n\nTests:\n- Added regression coverage for identity preservation, protected mark retention, Context.compress backend semantics, and SmartContext auto-compress behavior.\n- Verified via targeted pytest run for compression/context/react-agent suites.
|
Implemented the new review fixes on What was fixed:
Tests added/updated:
Targeted verification:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11647cae80
ℹ️ 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".
| CtxMessage( | ||
| role=message.role, | ||
| content=message.content, | ||
| name=message.name, | ||
| metadata=metadata, |
There was a problem hiding this comment.
When tool_pair_safe filters an assistant message’s tool_calls, this branch rebuilds the message without carrying over id and mark, so on STM rewrite the message gets a new id/default TEMPORARY mark. In SmartContext flows this silently breaks message identity (e.g., discard-by-id references) and can downgrade protected messages so later compression may remove them unexpectedly; copy id/mark through this reconstruction the same way _annotate_strategy does.
Useful? React with 👍 / 👎.
Address remaining PR #136 review concern in tool_pair_safe reconstruction path.\n\nProblem:\n- _enforce_tool_pair_safety rebuilt assistant messages after filtering tool_calls but dropped id/mark.\n- In SmartContext and identity-sensitive flows this could silently break message identity, downgrade protected marks, and impact later retention/ordering behavior.\n\nChanges:\n- Preserve assistant message id and mark when rebuilding filtered assistant messages in dare_framework/compression/core.py.\n- Add regression test to ensure id/mark survive tool_call filtering while unmatched tool_call IDs are still removed.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q -p no:cacheprovider tests/unit/test_context_compression.py tests/unit/test_react_agent_gateway_injection.py\n- Result: 21 passed, 1 warning.
|
Processed the actionable compression review item on PR #136 and pushed Fix applied:
Regression coverage:
Targeted verification:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b918558b1b
ℹ️ 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".
| if max_messages is not None: | ||
| compress_impl(max_messages=max_messages) |
There was a problem hiding this comment.
Context.compress currently calls the STM backend compress(max_messages=...) before compress_context(...) when advanced options are present, which means strategies that need the full pre-trim history (for example strategy="summary_preview") never get a chance to execute on long contexts. In practice, Context.compress(strategy="summary_preview", max_messages=N) degrades to plain tail truncation and drops the summary metadata/behavior the feature advertises, because the message count is already <= N by the time compress_context runs.
Useful? React with 👍 / 👎.
Address new PR #136 review item about advanced compression ordering.\n\nProblem:\n- Context.compress applied backend STM max_messages compression before advanced strategy compression when advanced options were present.\n- Strategies that depend on full history (e.g. summary_preview) would receive already-trimmed context and degrade to plain truncation behavior.\n\nChanges:\n- Keep basic path unchanged: max_messages-only calls still delegate directly to backend compress.\n- For advanced path: run compress_context first, then apply backend compress(max_messages=...) as a retention guardrail.\n- This preserves backend semantics while allowing advanced strategies to operate on full pre-trim history.\n\nTests:\n- Updated advanced-path unit test to assert strategy sees the full STM size before backend trimming, while backend semantics still apply after strategy execution.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q -p no:cacheprovider tests/unit/test_context_implementation.py tests/unit/test_context_compression.py tests/unit/test_react_agent_gateway_injection.py\n- Result: 42 passed, 1 warning.
|
Processed the new actionable review comment ( What was fixed:
Test updates:
Targeted verification:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f54cb85ca8
ℹ️ 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".
| output_text=final_message, | ||
| ) | ||
|
|
||
| async def _emit_terminal_transport_message( |
There was a problem hiding this comment.
This class now defines _emit_terminal_transport_message twice; the later definition silently overrides this earlier one, so any future fix applied to the first method will have no runtime effect. In maintenance scenarios (e.g., changing terminal event payloads), this can lead to confusing dead-code edits and behavior drift that is hard to diagnose.
Useful? React with 👍 / 👎.
Address new PR #136 review comment about duplicate method definition.\n\nChanges:\n- Remove the earlier duplicate definition in ReactAgent.\n- Keep the later implementation as the single canonical runtime path.\n\nRationale:\n- Avoid dead-code shadowing where edits to the first definition have no runtime effect.\n- Reduce maintenance risk and prevent behavior drift confusion.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q -p no:cacheprovider tests/unit/test_react_agent_gateway_injection.py tests/unit/test_context_implementation.py\n- Result: 33 passed, 1 warning.
|
Processed the new actionable review comment ( What was fixed:
Why:
Targeted verification:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfcaf4573c
ℹ️ 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".
| if ratio <= 0: | ||
| return default | ||
| if ratio > 1: | ||
| return 1.0 | ||
| return ratio |
There was a problem hiding this comment.
The new _clamp_ratio helper lets NaN pass through because both comparisons (<= 0 and > 1) are false for NaN, so self._compress_trigger_ratio/self._compress_target_ratio can become non-finite; when auto-compress runs, _maybe_auto_compress calls int(max_tokens * ratio) and raises ValueError: cannot convert float NaN to integer, which aborts execution before the model call whenever auto_compress=True and a token budget is set.
Useful? React with 👍 / 👎.
Address new PR #136 review comment about non-finite compression ratios.\n\nChanges:\n- In _clamp_ratio, reject non-finite values with math.isfinite and fall back to default ratio.\n- Prevent int(max_tokens * ratio) from raising when ratio is NaN/Inf in auto-compress path.\n- Add regression test covering NaN ratio inputs and default-ratio fallback behavior during auto compression.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q -p no:cacheprovider tests/unit/test_react_agent_gateway_injection.py tests/unit/test_context_implementation.py\n- Result: 34 passed, 1 warning.
|
Processed the new actionable review comment ( What was fixed:
Regression coverage:
Targeted verification:
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
Root cause: governance-evidence-truth runs in PR merge context, and latest main introduced docs/features/agentscope-d5-safe-compression.md with status in_review but legacy evidence structure. That caused strict acceptance-pack enforcement to fail unrelated PR checks after rebasing to current main. Updates:\n- Set status from in_review to done.\n- Update updated date to 2026-03-03.\n- Mark progress items as completed after PR #136 merged.\n- Replace pending merge-gate placeholder with concrete merge PR and merge commit links.\n\nRationale:\n- The D5 work is already merged to main (merge commit 3b80d73), so keeping in_review is stale state and incorrectly subjects this historical doc to strict in_review gate checks.\n\nVerification:\n- ./scripts/ci/check_governance_evidence_truth.sh (passed)\n- python3 -m unittest tests.unit.test_governance_evidence_truth_gate -v (32/32 passed).
1 participant
Summary
tool_pair_safe, token-aware trimming, metadata annotation)ReactAgentwith backward-compatible default behaviorzts212653and sync D2/D4/D5 execution evidenceValidation
/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -qopenspec validate --changes "agentscope-d5-safe-compression"openspec instructions apply --change "agentscope-d5-safe-compression" --jsonNotes
dare.logremains untracked and is intentionally excluded from this PR.edge.openspec.dev) after successful command completion; validation results are still successful.