Jens-G commented on PR #3606: URL: https://github.com/apache/thrift/pull/3606#issuecomment-4813598915
### Code review Found 3 issues: 1. Commit message does not follow the required format ([AGENTS.md §2](https://github.com/apache/thrift/blob/deb36fa409849de45973b04ffc3ce49d277ca90a/AGENTS.md#L34-L37)): the headline is `"Add Tongsuo NTLS support to C++ TSSLSocket"` — missing the required `THRIFT-6073:` prefix — and the body omits the required `Client: cpp` line. 2. All 8 new `load*` methods call `requireNtlsSupport()` before the null-pointer argument check, reversing the established contract of every existing `load*` method in the file (`loadCertificate`, `loadPrivateKey`, etc. all validate arguments first). On non-NTLS builds, callers that pass null arguments receive `TSSLException("NTLS is not available")` instead of `TTransportException(BAD_ARGS)`, making errors undiagnosable. https://github.com/apache/thrift/blob/0d5e69aae60c5dbf3c6c746b42705642c8362711/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L1110-L1115 3. Tongsuo is introduced as a new optional build dependency but the PR description contains no mention of license verification against the ASF Category A/X list, as required by [AGENTS.md §1](https://github.com/apache/thrift/blob/deb36fa409849de45973b04ffc3ce49d277ca90a/AGENTS.md#L13-L19): *"Before introducing any dependency … verify its license is compatible with Apache 2.0 … If in doubt … add it and flag it in the PR description for committer review."* 🤖 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]
