feat: close AgentScope D2+D4 thinking/transport gaps - #134
Conversation
|
@github-copilot please review this PR for regressions, compatibility risks, and missing tests in the D2/D4 transport + thinking changes. |
|
@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: 39a69ad55a
ℹ️ 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".
| """Extract reasoning token count from provider-specific usage payloads.""" | ||
| candidates: list[Any] = [ | ||
| usage.get("reasoning_tokens"), | ||
| usage.get("output_tokens_details", {}).get("reasoning") |
There was a problem hiding this comment.
The new reasoning-token extraction path misses a common OpenAI usage shape: this code reads output_tokens_details.reasoning but not output_tokens_details.reasoning_tokens. When providers only populate output_tokens_details.reasoning_tokens, _extract_usage returns usage without reasoning_tokens, so downstream token accounting/telemetry underreports reasoning cost even though this change set is intended to normalize that field.
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: 4bb3f817b7
ℹ️ 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 is executed through the transport loop (BaseAgent._execute_polled_message), the base class always calls _send_transport_result after execute() returns, so this new explicit message send causes two terminal envelopes for one request (message here plus base result). In interactive channel flows this duplicates user-visible final output and can render the second response as a raw dict payload, which is a regression in transport behavior.
Useful? React with 👍 / 👎.
Codex review on PR #134 reported duplicate terminal transport output when ReactAgent runs under BaseAgent's polled-message loop.\n\nRoot cause: ReactAgent emitted a terminal MESSAGE event for final text while BaseAgent always emits a terminal RESULT envelope after execute() returns, causing two terminal payloads for one request in transport-driven flows.\n\nThis change tracks transport-loop execution context in BaseAgent and gates ReactAgent terminal MESSAGE emission in that path. A regression test verifies transport-loop execution now emits THINKING/TOOL_CALL/TOOL_RESULT followed by a single terminal RESULT envelope.
|
已处理本轮新增 review 并补测试：\n- 修复 transport loop 下 ReactAgent 终态 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1d31d20cf
ℹ️ 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._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.
_execute_polled_message flips a single agent-wide _transport_loop_execution flag before awaiting execute, and _is_transport_loop_execution treats any non-None transport as loop-driven while that flag is set. If a polled request is in flight and another execute(..., transport=...) runs concurrently on the same agent, the second execution is misclassified as loop execution, so ReactAgent suppresses its terminal message envelope and the caller can miss completion on that transport stream.
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: b1d31d20cf
ℹ️ 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._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.
This stores transport-loop state in a shared instance flag, so concurrent executions on the same agent can clobber each other: if two _execute_polled_message calls overlap, the first completion sets the flag to False while the second task is still running, causing _is_transport_loop_execution() to misclassify that in-flight execution. In practice this can emit an extra terminal message event (alongside BaseAgent’s terminal result) or suppress a direct-execute terminal message depending on overlap timing, which makes transport behavior nondeterministic under parallel load.
Useful? React with 👍 / 👎.
Address the PR #134 transport-loop concurrency regression where a single\ninstance-level flag marked all concurrent execute() calls as loop-driven.\n\nKey changes:\n- Replace BaseAgent's global boolean with a\n task-local ContextVar so loop state does not leak across concurrent tasks.\n- Set/reset the context token inside to scope the\n loop marker to that coroutine only.\n- Keep behavior unchanged for callers while\n making it concurrency-safe.\n- Add a regression test that runs a polled loop execution and a concurrent\n direct execute call to verify only the loop task is marked loop-driven.\n\nRationale:\nReactAgent suppresses terminal MESSAGE envelopes when execution is detected as\ntransport-loop initiated. With shared mutable state this suppression could\nincorrectly affect unrelated concurrent direct transport calls.
|
Processed the new actionable review comment from this patrol window and confirmed the fix is 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: 042001632b
ℹ️ 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_terminal_transport_message( | ||
| transport=transport, | ||
| output=final_text, | ||
| ) |
There was a problem hiding this comment.
ReactAgent.execute only emits the terminal message envelope in the if not response.tool_calls branch, but other successful exits (the repeated-tool loop guard and the max-round fallback) return RunResult without any final transport event. In direct execute(..., transport=...) usage this leaves transport consumers with only intermediate tool_call/tool_result events and no completion signal, which can cause clients to wait indefinitely for a terminal message.
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 returns immediately for any dictionary resp, so the later resp['phase'] handling is unreachable. As a result, status events shaped like {"resp": {"phase": ...}} are rendered as the generic approval update string instead of the actual phase, dropping useful status detail in stdio output.
Useful? React with 👍 / 👎.
Address newly reported review findings on PR #134.\n\nKey changes:\n- Emit terminal MESSAGE envelopes for ReactAgent direct execute() exits when the loop guard triggers or max tool rounds are reached, matching the existing terminal signaling behavior in the normal no-tool-calls path.\n- Adjust status rendering order so structured status payloads with resp.phase render their phase text before approval fallback handling.\n\nRegression coverage:\n- Added ReactAgent transport tests to verify terminal MESSAGE emission for repeated-tool guard exits and max-round exits.\n- Added transport adapter test to verify STATUS events with resp.phase no longer degrade to generic approval text.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_react_agent_gateway_injection.py tests/unit/test_transport_adapters.py tests/unit/test_base_agent_transport_contract.py
|
Processed the new actionable review comments from this patrol window and pushed fixes to the PR branch. Pushed:
What this addresses:
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". |
Implement the agentscope-d2-d4-thinking-transport OpenSpec slice end-to-end, including transport taxonomy extensions, model reasoning preservation, ReAct intermediate event emission, and evidence sync. Key changes: - Extend transport event taxonomy with canonical categories (message/tool_call/tool_result/thinking/error/status) while keeping legacy aliases consumable. - Add canonicalize_transport_event_type and switch stdio adapter routing to canonical classification without breaking legacy payload handling. - Preserve provider reasoning in ModelResponse via optional thinking_content and normalize reasoning_tokens in OpenAI/OpenRouter adapter usage metadata. - Emit canonical thinking/tool_call/tool_result/message events from ReactAgent execute loop and emit structured error payload on tool-call exceptions. - Add/expand unit tests for transport type canonicalization, adapter reasoning extraction, and ordered ReactAgent intermediate transport events. - Sync OpenSpec tasks (13/13 complete) and feature evidence documentation for verification traceability. Verification: - openspec validate --changes "agentscope-d2-d4-thinking-transport" (pass) - pytest targeted suite (transport/adapters/react): pass - pytest impacted regression subset: pass - pytest -q full suite: 528 passed, 12 skipped, 1 warning
OpenAI-compatible providers may return reasoning token usage under token_usage.output_tokens_details.reasoning_tokens.\n\nThe adapter only read reasoning_tokens at the top level and output_tokens_details.reasoning, which caused reasoning usage to be dropped for providers that follow the newer field name.\n\nThis change adds output_tokens_details.reasoning_tokens as a first-class candidate in the fallback chain and adds a unit test that locks this behavior to prevent regressions.
Codex review on PR #134 reported duplicate terminal transport output when ReactAgent runs under BaseAgent's polled-message loop.\n\nRoot cause: ReactAgent emitted a terminal MESSAGE event for final text while BaseAgent always emits a terminal RESULT envelope after execute() returns, causing two terminal payloads for one request in transport-driven flows.\n\nThis change tracks transport-loop execution context in BaseAgent and gates ReactAgent terminal MESSAGE emission in that path. A regression test verifies transport-loop execution now emits THINKING/TOOL_CALL/TOOL_RESULT followed by a single terminal RESULT envelope.
Address the PR #134 transport-loop concurrency regression where a single\ninstance-level flag marked all concurrent execute() calls as loop-driven.\n\nKey changes:\n- Replace BaseAgent's global boolean with a\n task-local ContextVar so loop state does not leak across concurrent tasks.\n- Set/reset the context token inside to scope the\n loop marker to that coroutine only.\n- Keep behavior unchanged for callers while\n making it concurrency-safe.\n- Add a regression test that runs a polled loop execution and a concurrent\n direct execute call to verify only the loop task is marked loop-driven.\n\nRationale:\nReactAgent suppresses terminal MESSAGE envelopes when execution is detected as\ntransport-loop initiated. With shared mutable state this suppression could\nincorrectly affect unrelated concurrent direct transport calls.
Address newly reported review findings on PR #134.\n\nKey changes:\n- Emit terminal MESSAGE envelopes for ReactAgent direct execute() exits when the loop guard triggers or max tool rounds are reached, matching the existing terminal signaling behavior in the normal no-tool-calls path.\n- Adjust status rendering order so structured status payloads with resp.phase render their phase text before approval fallback handling.\n\nRegression coverage:\n- Added ReactAgent transport tests to verify terminal MESSAGE emission for repeated-tool guard exits and max-round exits.\n- Added transport adapter test to verify STATUS events with resp.phase no longer degrade to generic approval text.\n\nVerification:\n- /Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_react_agent_gateway_injection.py tests/unit/test_transport_adapters.py tests/unit/test_base_agent_transport_contract.py
768e90a to
da5b7f6
Compare
1 participant
Summary
agentscope-d2-d4-thinking-transportend-to-endmessage/tool_call/tool_result/thinking/error/statusand legacy alias canonicalizationthinking_contentand normalizereasoning_tokensin OpenAI/OpenRouter adaptersthinking -> tool_call -> tool_result -> message)Verification
openspec validate --changes "agentscope-d2-d4-thinking-transport"pytest -q tests/unit/test_transport_types.py tests/unit/test_transport_adapters.py tests/unit/test_openrouter_adapter.py tests/unit/test_openai_model_adapter.py tests/unit/test_react_agent_gateway_injection.pypytest -q tests/unit/test_transport_channel.py tests/unit/test_base_agent_transport_contract.py tests/unit/test_agent_event_transport_hook.py tests/unit/test_dare_agent_hook_transport_boundary.py tests/unit/test_example_10_agentscope_compat.pypytest -q(528 passed, 12 skipped, 1 warning)Notes
edge.openspec.devunreachable), but validate/status/apply commands complete successfully and evidence is recorded indocs/features/agentscope-d2-d4-thinking-transport.md.