Jens-G commented on PR #3595: URL: https://github.com/apache/thrift/pull/3595#issuecomment-4698131382
### Code review Found 2 issues: 1. The fast path calls `impl()->writeI32(len)` to write the string length, which is correct for `BinaryProtocol` but wrong for `CompactProtocol`. `CompactProtocol::writeString` encodes the length with `writeVarint(len)` (plain unsigned varint), while `writeI32` encodes with `writeVarint(toZigZag(val))` (zigzag). The fast path will produce a malformed Compact frame for any unicode string field, and a decoder will read the wrong number of bytes. https://github.com/apache/thrift/blob/0691570336b46b4eb5f83bc24c57012de9c68f0e/lib/py/src/ext/protocol.tcc#L469-L473 2. AI tool use is acknowledged in the PR body but neither commit contains the required `Co-Authored-By:` or `Generated-by:` label ([AGENTS.md ยง4](https://github.com/apache/thrift/blob/35c1a53dd6de205a4a7c23fc356f1fba490116c4/AGENTS.md#L57-L71) says "**Always** label AI-assisted commits and PRs โฆ Apply this label even when AI only generated a portion of the change"). https://github.com/apache/thrift/blob/35c1a53dd6de205a4a7c23fc356f1fba490116c4/AGENTS.md#L57-L71 ๐ค Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with ๐. Otherwise, react with ๐.</sub> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
