Jens-G commented on PR #3585: URL: https://github.com/apache/thrift/pull/3585#issuecomment-4636336366
### Code review The code change itself looks correct: the guard is threaded through `thrift_protocol_skip_impl`, both `transport` and `configuration` are null-checked before reading `recursionLimit_`, the `-1` return propagates via `THRIFT_SKIP_RESULT_OR_RETURN`, all five recursive sites pass `recursion_depth + 1`, and the public `thrift_protocol_skip` signature is preserved. The gap is real and still present on `master` — `recursionLimit_` has existed since THRIFT-5237 but was never consulted in `skip`. One item to address before merge: no test accompanies the change. AGENTS.md says "Provide tests for every submitted change", and the analogous per-language limit changes each added one. A test under `lib/c_glib/test/` that nests past the configured limit and asserts `THRIFT_PROTOCOL_ERROR_DEPTH_LIMIT` would cover it. https://github.com/apache/thrift/blob/6934f8e3ad8b5de855efffd110673c75573855f5/lib/c_glib/src/thrift/c_glib/protocol/thrift_protocol.c#L436-L454 🤖 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]
