Conversation
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些整体性的反馈：
- 数字端口的解析逻辑现在分散在
parse_port_from_endpoint和新的Transceiver::parse_tcp_port之间；建议把它们合并到一个单独的辅助函数中，以避免验证逻辑出现细微差异以及未来产生偏移。 - 在 Python 测试中，
reserve_tcp_port()会在AgentClient绑定之前先预留并立即释放一个端口；如果这个测试在高负载下变得不稳定，你可能需要改为使用端口 0，并从agent.identifier中读取实际端口，而不是事先预留。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- 数字端口的解析逻辑现在分散在 `parse_port_from_endpoint` 和新的 `Transceiver::parse_tcp_port` 之间；建议把它们合并到一个单独的辅助函数中，以避免验证逻辑出现细微差异以及未来产生偏移。
- 在 Python 测试中，`reserve_tcp_port()` 会在 `AgentClient` 绑定之前先预留并立即释放一个端口；如果这个测试在高负载下变得不稳定，你可能需要改为使用端口 0，并从 `agent.identifier` 中读取实际端口，而不是事先预留。
## Individual Comments
### Comment 1
<location path="source/AgentCommon/Transceiver.cpp" line_range="131" />
<code_context>
static uint16_t parse_port_from_endpoint(const std::string& endpoint)
</code_context>
<issue_to_address>
**suggestion:** Port parsing and range checking logic is duplicated between `parse_port_from_endpoint` and `parse_tcp_port`; consider factoring out a shared helper.
Both `parse_port_from_endpoint` and `Transceiver::parse_tcp_port` implement nearly identical digit/`strtoul`/range checks. A shared helper (e.g. `static std::optional<uint16_t> parse_port(const char* str, size_t len)`) would remove duplication and keep future changes to port validation consistent.
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The numeric-port parsing logic is now spread between
parse_port_from_endpointand the newTransceiver::parse_tcp_port; consider consolidating these into a single helper to avoid subtle differences in validation and future drift. - In the Python test
reserve_tcp_port()reserves and immediately releases a port beforeAgentClientbinds to it; if this test becomes flaky under load, you may want to switch to using port 0 and reading back the actual port fromagent.identifierinstead of pre-reserving.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The numeric-port parsing logic is now spread between `parse_port_from_endpoint` and the new `Transceiver::parse_tcp_port`; consider consolidating these into a single helper to avoid subtle differences in validation and future drift.
- In the Python test `reserve_tcp_port()` reserves and immediately releases a port before `AgentClient` binds to it; if this test becomes flaky under load, you may want to switch to using port 0 and reading back the actual port from `agent.identifier` instead of pre-reserving.
## Individual Comments
### Comment 1
<location path="source/AgentCommon/Transceiver.cpp" line_range="131" />
<code_context>
static uint16_t parse_port_from_endpoint(const std::string& endpoint)
</code_context>
<issue_to_address>
**suggestion:** Port parsing and range checking logic is duplicated between `parse_port_from_endpoint` and `parse_tcp_port`; consider factoring out a shared helper.
Both `parse_port_from_endpoint` and `Transceiver::parse_tcp_port` implement nearly identical digit/`strtoul`/range checks. A shared helper (e.g. `static std::optional<uint16_t> parse_port(const char* str, size_t len)`) would remove duplication and keep future changes to port validation consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #1149 where AgentClient did not automatically recognize numeric string identifiers as TCP port numbers. The fix moves the parse_tcp_port function from AgentServer to the shared Transceiver base class and adds numeric-identifier auto-detection logic to AgentClient's constructor and create_socket method.
Changes:
- Moved
parse_tcp_portfromAgentServer::start_up(file-local static) toTransceiver(class static method), making it reusable by bothAgentClientandAgentServer. - Added numeric-identifier TCP auto-detection in
AgentClient's constructor andcreate_socket, so passing e.g."12345"automatically uses TCP mode. - Updated both Chinese and English documentation, Python binding docstrings, and test to cover the new behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| source/AgentCommon/Transceiver.cpp | Added Transceiver::parse_tcp_port as a shared static method with necessary includes |
| source/include/MaaAgent/Transceiver.h | Declared parse_tcp_port as a protected static method |
| source/MaaAgentClient/Client/AgentClient.cpp | Added numeric-identifier TCP detection in constructor and create_socket |
| source/MaaAgentServer/Server/AgentServer.cpp | Removed local parse_tcp_port and unused includes (now uses inherited method) |
| source/binding/Python/maa/agent_client.py | Updated docstrings to document numeric-string TCP behavior |
| docs/en_us/2.2-IntegratedInterfaceOverview.md | Updated MaaAgentClientCreateV2 docs, moved Windows fallback note |
| test/agent/agent_tcp_main_test.py | Refactored test into reusable functions, added numeric-identifier test scenario |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #1149
Summary by Sourcery
将数值类型的 AgentClient 标识符视为 TCP 端口，并扩展测试和文档以涵盖新的 TCP 选择行为。
Bug Fixes（错误修复）:
Enhancements（增强）:
Transceiver::parse_tcp_port辅助函数，以在客户端逻辑中共享。Documentation（文档）:
Tests（测试）:
Original summary in English
Summary by Sourcery
Treat numeric AgentClient identifiers as TCP ports and extend tests and documentation to cover the new TCP selection behavior.
Bug Fixes:
Enhancements:
Documentation:
Tests: