Skip to content

feat: wasm add field endpoint/req_type/biz_code/bic_sc#11614

Merged
lzf575 merged 1 commit into
v6.6from
v66-wasm-add-field
Apr 14, 2026
Merged

feat: wasm add field endpoint/req_type/biz_code/bic_sc#11614
lzf575 merged 1 commit into
v6.6from
v66-wasm-add-field

Conversation

@lzf575
Copy link
Copy Markdown
Contributor

@lzf575 lzf575 commented Apr 13, 2026

This PR is for:

@lzf575 lzf575 enabled auto-merge (rebase) April 13, 2026 15:18
@lzf575
Copy link
Copy Markdown
Contributor Author

lzf575 commented Apr 13, 2026

@claude reveiw the pr

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 13, 2026

Claude finished @lzf575's task in 6m 44s —— View job


Todo

  • Set up review checklist in the PR comment
  • Read CLAUDE.md and inspect the PR diff
  • Analyze the modified code for bugs, regressions, and maintainability issues
  • Update this comment with the final review

Review

I found two correctness issues in agent/src/plugin/mod.rs.

  1. New response fields are never preserved during merge
    CustomInfoResp now contains req_type and endpoint, but merge_log() still only merges exception and result for the response side. If a request/response pair is assembled from multiple partial logs, these new response fields can be dropped. See agent/src/plugin/mod.rs:591-agent/src/plugin/mod.rs:593.
    Fix this →

  2. is_async / is_reversed are parsed but never merged
    The new protobuf fields are assigned in from_protobuf() at agent/src/plugin/mod.rs:459-agent/src/plugin/mod.rs:460, and downstream protocols already consume them as optional overrides, but CustomInfo::merge_log() never carries them forward when multiple CustomInfo fragments are merged. That means the final merged record can silently lose these flags. See agent/src/plugin/mod.rs:564-agent/src/plugin/mod.rs:615.
    Fix this →

I did not make code changes since the request was to review the PR.

@lzf575 lzf575 force-pushed the v66-wasm-add-field branch from e6f38a8 to 9fd8de3 Compare April 13, 2026 15:46
@lzf575
Copy link
Copy Markdown
Contributor Author

lzf575 commented Apr 13, 2026

@claude review the pr

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 13, 2026

Claude finished @lzf575's task in 4m 46s —— View job


Todo

  • Set up the PR review checklist in the GitHub comment
  • Read repository instructions and inspect the PR diff
  • Analyze the modified code for correctness issues
  • Post the final review to the GitHub comment

Review

I found three correctness issues in agent/src/plugin/mod.rs.

  1. New fields are written with self, which does not exist in From<CustomInfo>
    In impl From<CustomInfo> for L7ProtocolSendLog, the new assignments use self.biz_code and self.biz_scenario, but this function takes w: CustomInfo and has no self in scope. That makes the change fail to compile. See agent/src/plugin/mod.rs:707-agent/src/plugin/mod.rs:708.
    Fix this →

  2. Response-side req_type / endpoint are still dropped during merge
    This PR makes From<CustomInfo> fall back to resp.req_type and resp.endpoint when the request side is empty, but CustomInfo::merge_log() still never merges those response fields. If request/response data arrives in separate fragments, the final merged record can still lose the response-provided values before conversion. See agent/src/plugin/mod.rs:591-agent/src/plugin/mod.rs:592 and the new fallback usage at agent/src/plugin/mod.rs:667-agent/src/plugin/mod.rs:678.
    Fix this →

  3. is_async / is_reversed are still parsed but not preserved across merges
    The protobuf parser fills these fields in from_protobuf() (agent/src/plugin/mod.rs:459-agent/src/plugin/mod.rs:460), and downstream behavior depends on them (agent/src/plugin/mod.rs:627-agent/src/plugin/mod.rs:655), but merge_log() still never carries them over. Any multi-fragment merge can therefore silently clear the async/reversed overrides. See agent/src/plugin/mod.rs:564-agent/src/plugin/mod.rs:615.
    Fix this →

I did not make code changes because the request was to review the PR.

@lzf575 lzf575 force-pushed the v66-wasm-add-field branch 2 times, most recently from 6f42672 to 2367227 Compare April 13, 2026 16:17
@lzf575
Copy link
Copy Markdown
Contributor Author

lzf575 commented Apr 13, 2026

@claude review the pr

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 13, 2026

Claude finished @lzf575's task in 8m 54s —— View job


a. Event type/context: GENERAL_COMMENT on PR #11614, triggered by @claude review the pr. Need to review the current PR branch against origin/v6.6. b. Request type: code review only, not implementation. c. Key info: PR branch v66-wasm-add-field -> v6.6, one changed file (`agent/src/plugin/mod.rs`), prior Claude review comments may now be stale, so I inspected the current diff and surrounding code directly. d. Main tasks/challenges: verify whether prior issues were fixed and identify any remaining correctness/regression problems in the updated branch. e. High-level plan completed: checked for `CLAUDE.md` (none present), inspected `git diff origin/v6.6...HEAD`, read the modified Rust code around parsing, merge logic, and `From` conversion, and prepared review feedback. f. I did not implement changes because the trigger comment asked only for a PR review.

Todo

  • Set up the review checklist in the GitHub comment
  • Read repository instructions and inspect the PR diff
  • Analyze the modified code for correctness issues
  • Post the final review to the GitHub comment

Review

I found two correctness issues in agent/src/plugin/mod.rs.

  1. The new version assignment still uses a non-existent field and will not compile
    CustomInfo does not have a top-level version field; the version lives under req.version (agent/src/plugin/mod.rs:53-agent/src/plugin/mod.rs:59, agent/src/plugin/mod.rs:82-agent/src/plugin/mod.rs:124). But the new conversion code writes version: w.version.unwrap_or_default() in impl From<CustomInfo> for L7ProtocolSendLog (agent/src/plugin/mod.rs:709). That field access is invalid, so this change still breaks compilation. It should read from the request-side version that is already parsed and merged.
    Fix this →

  2. is_async / is_reversed are still parsed but dropped during merge
    from_protobuf() fills is_async and is_reversed (agent/src/plugin/mod.rs:459-agent/src/plugin/mod.rs:460), and downstream behavior depends on them via needs_session_aggregation() and is_reversed() (agent/src/plugin/mod.rs:629-agent/src/plugin/mod.rs:656). But CustomInfo::merge_log() still never copies either field when multiple fragments are merged (agent/src/plugin/mod.rs:564-agent/src/plugin/mod.rs:617). So if one fragment carries these overrides and another does not, the final merged record can silently lose them.
    Fix this →

I did not make code changes because the request was to review the PR.

@lzf575 lzf575 force-pushed the v66-wasm-add-field branch from 2367227 to a8028e9 Compare April 13, 2026 23:56
@lzf575 lzf575 changed the title feat: wasm add field endpoint/req_type/is_async/is_reversed feat: wasm add field endpoint/req_type/biz_code/bic_sc Apr 13, 2026
@lzf575
Copy link
Copy Markdown
Contributor Author

lzf575 commented Apr 13, 2026

@claude review the pr

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 13, 2026

Claude finished @lzf575's task in 4m 26s —— View job


a. Event type/context: GENERAL_COMMENT on PR #11614, triggered by `@claude review the pr`. b. Request type: code review only, not implementation. c. Key info: review current PR branch `v66-wasm-add-field` against `origin/v6.6`; only changed file is `agent/src/plugin/mod.rs`; no `CLAUDE.md` file was present in the repo checkout. d. Main tasks/challenges: inspect the updated Rust code directly, verify whether earlier issues were fixed, and identify any remaining correctness problems in the new field plumbing. e. High-level plan completed: checked for repo instructions, read `agent/src/plugin/mod.rs`, inspected the merge/conversion logic around the new fields, and prepared review feedback. I did not run tests because this was a review-only request. f. Limitation: `git diff origin/v6.6...HEAD` required extra approval in this environment, so I validated the current file contents directly instead.

Todo

  • Update the PR comment with the review checklist
  • Read repository instructions and the PR diff
  • Analyze the modified Rust code for review issues
  • Post the final review in the PR comment

Review

I found two correctness issues in agent/src/plugin/mod.rs.

  1. The new version assignment still does not compile
    CustomInfoRequest.version is a plain String (agent/src/plugin/mod.rs:53-agent/src/plugin/mod.rs:59), and from_protobuf() already normalizes the protobuf field into that string with unwrap_or_default() (agent/src/plugin/mod.rs:465-agent/src/plugin/mod.rs:470). But impl From<CustomInfo> for L7ProtocolSendLog now does version: w.req.version.unwrap_or_default() (agent/src/plugin/mod.rs:709). String has no unwrap_or_default() method, so this conversion still fails to compile. The assignment should use the existing string value directly.
    Fix this →

  2. is_async / is_reversed are still parsed but dropped during merge
    from_protobuf() fills is_async and is_reversed (agent/src/plugin/mod.rs:459-agent/src/plugin/mod.rs:460), and the protocol behavior consumes them via needs_session_aggregation() and is_reversed() (agent/src/plugin/mod.rs:629-agent/src/plugin/mod.rs:656). But CustomInfo::merge_log() still never carries either field over when request/response fragments are merged (agent/src/plugin/mod.rs:564-agent/src/plugin/mod.rs:617). If one fragment provides these overrides and the other does not, the merged record can silently lose them.
    Fix this →

I did not make code changes because the request was to review the PR.

@lzf575 lzf575 force-pushed the v66-wasm-add-field branch from a8028e9 to db2e185 Compare April 14, 2026 01:10
@lzf575 lzf575 force-pushed the v66-wasm-add-field branch from db2e185 to 4358880 Compare April 14, 2026 01:41
@lzf575
Copy link
Copy Markdown
Contributor Author

lzf575 commented Apr 14, 2026

@claude review the pr ,用中文

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 14, 2026

Claude finished @lzf575's task in 5m 35s —— View job


a. 事件类型/上下文:这是 PR #11614 上的 GENERAL_COMMENT 事件,触发语句是“@claude review the pr ,用中文”。 b. 请求类型:仅代码评审,不是实现修改。 c. 关键信息:PR 分支为 `v66-wasm-add-field -> v6.6`;本次变更集中在 `agent/src/plugin/mod.rs`;评审基线是 `origin/v6.6`;仓库中未找到 `CLAUDE.md`。 d. 主要任务/挑战:确认此前指出的问题是否已修复,并继续检查新增字段在 protobuf 解析、分片合并、导出发送日志与缓存路径上的一致性。 e. 高层计划:已检查 PR diff 和相关源码上下文,重点审查 `from_protobuf()`、`merge_log()`、`From for L7ProtocolSendLog`、`get_endpoint()`/`LogCache` 路径;本次为 review-only,不做代码修改。 f. 限制:我没有运行测试,这次结论基于当前 diff 与源码静态审查。

Todo

  • 更新评审评论清单
  • 读取仓库说明和 PR 变更
  • 分析代码并确认是否存在问题
  • 用中文发布最终评审结果

评审结果

我看完当前版本后,仍然发现 2 个正确性问题,都在 agent/src/plugin/mod.rs

  1. is_async / is_reversed 仍然会在 merge 时丢失
    这两个字段在 from_protobuf() 里已经被解析出来(agent/src/plugin/mod.rs:459-agent/src/plugin/mod.rs:460),后续逻辑也依赖它们(agent/src/plugin/mod.rs:629-agent/src/plugin/mod.rs:656)。但 CustomInfo::merge_log() 里仍然没有把它们从 w 合并到 selfagent/src/plugin/mod.rs:564-agent/src/plugin/mod.rs:617)。如果请求/响应日志是分片上报的,只要其中一个分片携带了这两个覆盖值,最终合并结果就可能把它们静默丢掉,导致 session aggregation / reversed 判断错误。
    Fix this →

  2. endpoint 的 fallback 只在发送日志里生效,缓存/标签路径仍然会丢值
    这次 PR 在 From<CustomInfo> for L7ProtocolSendLog 里新增了 response-side fallback:当 w.req.endpoint 为空时,会回退到 w.resp.endpointagent/src/plugin/mod.rs:676-agent/src/plugin/mod.rs:680)。但 get_endpoint() 仍然只返回 self.req.endpoint.clone()agent/src/plugin/mod.rs:647-agent/src/plugin/mod.rs:649),而 LogCache::from(&CustomInfo) 又直接用这个 getter 写入 endpointagent/src/plugin/mod.rs:721-agent/src/plugin/mod.rs:728)。这会导致同一条日志在导出 L7ProtocolSendLog 时有 endpoint,但在缓存/标签路径上仍然是空值,行为不一致。只要插件只在响应侧填了 endpoint,这条路径就会继续丢字段。
    Fix this →

我没有改代码,因为这次请求是 review PR。

@lzf575 lzf575 merged commit 90000fb into v6.6 Apr 14, 2026
8 checks passed
@lzf575 lzf575 deleted the v66-wasm-add-field branch April 14, 2026 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants