Trim code comments across the repo by Frauschi · Pull Request #49 · wolfSSL/wolfCert

Frauschi · 2026-10-07T08:22:45Z

Problem

Comments made up a large share of the source, and most of them argued rather than documented. Before this PR, non-license comment lines made up 44% of src/internal.h, 56-62% of wolfcert/est.h, scep.h and types.h, and about 4,560 lines across the C sources in total.

Justified knobs: each tunable #define in internal.h carried six to nine lines of sizing math, stack-versus-heap placement and "override with -D" advice.
Theory of operation: file headers in log.h, memory.h, status.h, check_config.h, options.h.in, pkcs7_util.c, key_algs.c and net_posix.c explained how the module works, and est.h carried an ASN.1 walkthrough of CsrAttrs.
Argument in the tests: nearly every test function opened with a paragraph on why the behaviour matters and what a pass does or does not prove.
Cross-file and defensive notes: comments pointed at where a matching check lives in another file, explained #error guards whose message already says the same, or defended a branch against a hypothetical edit.
Banners: /* ---- section ---- */ blocks in the C sources, and # ---- ... ---- dividers in CMake, autoconf, the CI scripts and the workflows.
Stale claims:
- est_server.c described itself as plaintext HTTP only, but it serves TLS.
- store.h called the POSIX store safe to pass to a wolfcert_free() that does not exist.
- est.h suggested handing csrattrs bytes to meta.csr_attributes_der, which wolfcert_csr_build never emits.
- pending_add claimed a duplicate check it does not do.
- Two blocks in http.c sat above the wrong declaration.

Fix

Comments are cut to what a reader would otherwise get wrong: RFC section references, wolfSSL workarounds (the pid_t include, EncodeSignedData not being retryable after BUFFER_E, the reversed wc_PKCS7_AddCertificate order), and traps a later edit would reintroduce. Most survivors are one or two lines. A few run longer where they carry a real constraint.

Trim the comments in the public headers: each function and field keeps its contract: ownership, what NULL or 0 means, units, and the return codes and PENDING outcomes a caller has to handle. The status tables in est.h and the fingerprint, GetCertInitial and GetCert rules in scep.h stay at three to nine lines, since they are the API reference. The stale store.h and est.h claims are dropped, and status.h describes the thread-local fallback as src/internal.c implements it.
Trim the comments in the library, CLIs and examples: the knob paragraphs, banners, defensive rationale and narration go. The examples keep their step labels and the notes an integrator copying them needs, and the user_settings headers keep one label per group.
Trim the comments in the tests: each test keeps at most one line naming what it checks, with the RFC section where there is one, or no comment when the function name already says it. Comments on fixture values, byte offsets and timing constraints stay.
Trim the comments in the build system, scripts and CI: each block becomes a short label. A few constraints stay, such as the libest FIPS_mode stub for OpenSSL 3 and step-ca's RSA chain swap for SCEP. The # headers of assert-configure-fails.sh, check-config-resolution.sh and compile-freestanding.sh stay, because --help prints them.

Across 113 files, non-license comment lines in the C sources go from about 4,560 to 1,950:

Area	Before	After
`src/`	1,630	604
`wolfcert/*.h`	821	343
Tests	1,802	804
CLIs, examples, Zephyr, CI helpers	306	199

Notes for reviewers

Only comments change. A script stripped the comments from every changed file at the base and at the tip and compared what was left. All 111 C, header, shell, CMake and YAML files match. The two cmake/*.cmake.in templates it does not parse change only # lines.
Each commit is self-contained, so the four can be reviewed one at a time.
Some removed explanations are worth keeping somewhere other than the source. The commit messages carry the ones that matter.

Frauschi · 2026-10-07T08:23:05Z

@wolfSSL-Fenrir-bot review

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #49

Scan targets checked: wolfcert-src, wolfcert-bugs
Coverage: 3 of 50 in-scope changed file(s) opened by the reviewer; not opened: cli/wolfcert_client.c, cli/wolfcert_server.c, src/est/csr_attrs.c, src/est/est_client.c, src/est/est_server.c, src/scep/scep_client.c, src/scep/scep_msg.c, tests/integration/test_est_async_roundtrip.c, tests/integration/test_est_chunked_robustness.c, tests/integration/test_est_csr_attrs_apply_roundtrip.c and 37 more

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

Copilot

Copilot review overview

🟡 Changes recommended

Two condensed comments now inaccurately describe error-state lifetime and server shutdown test coverage.

Review effort: Balanced
Findings: 2

Open (2)

Correct overview description of case 5 · New
Avoid incorrect per-thread error message lifetime guarantee · New

What changed in this PR

Trims verbose comments across public APIs, implementation, tests, examples, Zephyr integration, and CI while retaining essential contracts and constraints.

Changes:

Condenses public and internal API documentation.
Simplifies test and interoperability commentary.
Replaces build and CI banners with concise labels.

File	Description
`zephyr/wolfssl_user_settings.h`	Condenses Zephyr wolfSSL configuration notes.
`zephyr/tests/wolfcert_unit_smoke/testcase.yaml`	Shortens build-only test explanation.
`zephyr/tests/wolfcert_est/testcase.yaml`	Condenses fixture requirement.
`zephyr/tests/wolfcert_est/src/main.c`	Trims Zephyr EST test comments.
`zephyr/tests/wolfcert_est/CMakeLists.txt`	Removes redundant embedding note.
`zephyr/tests/common/wolfcert.conf`	Condenses shared configuration header.
`zephyr/samples/wolfcert_est_client/wolfcert_sample_settings.h`	Removes redundant stack note.
`zephyr/samples/wolfcert_est_client/src/main.c`	Shortens EST authentication rationale.
`zephyr/CMakeLists.txt`	Condenses generated-header guidance.
`wolfcert/wolfcert.h`	Shortens initialization contract.
`wolfcert/store.h`	Condenses storage API documentation.
`wolfcert/status.h`	Revises error-state documentation.
`wolfcert/options.h.in`	Trims generated-header comments.
`wolfcert/memory.h`	Condenses allocator documentation.
`wolfcert/log.h`	Shortens logging API comments.
`wolfcert/keygen.h`	Condenses key-generation contracts.
`wolfcert/http.h`	Trims HTTP and session API documentation.
`wolfcert/errors.h`	Condenses pending and nonblocking descriptions.
`wolfcert/client.h`	Shortens CA-chain encoding contract.
`wolfcert/check_config.h`	Removes redundant configuration rationale.
`wolfcert/api.h`	Condenses test-symbol visibility note.
`tests/unit/test_server_ca_store.c`	Trims CA-store test explanations.
`tests/unit/test_parse_negative.c`	Condenses parser test commentary.
`tests/unit/test_net.c`	Trims transport and SIGPIPE notes.
`tests/unit/test_keygen.c`	Shortens key round-trip notes.
`tests/unit/test_csr.c`	Condenses CSR test commentary.
`tests/test_static_mem.h`	Shortens static-memory setup guidance.
`tests/interop/scep_micromdm.sh`	Trims micromdm interoperability notes.
`tests/interop/openssl_pkcs7_xcheck.sh`	Condenses OpenSSL cross-check explanations.
`tests/interop/lib/common.sh`	Removes redundant exit-code annotation.
`tests/interop/est_stepca.sh`	Trims step-ca setup commentary.
`tests/interop/est_libest.sh`	Condenses libest interoperability guidance.
`tests/interop/est_globalsign.sh`	Shortens GlobalSign interoperability notes.
`tests/integration/tls_test_util.h`	Condenses TLS helper documentation.
`tests/integration/test_tls_http.c`	Shortens TLS HTTP test overview.
`tests/integration/test_server_stop_idle.c`	Condenses server shutdown test descriptions.
`tests/integration/test_scep_poll_roundtrip.c`	Trims SCEP polling commentary.
`tests/integration/test_scep_get_cert.c`	Condenses GetCert test explanations.
`tests/integration/test_est_tls_roundtrip.c`	Shortens EST TLS test notes.
`tests/integration/test_est_roundtrip.c`	Trims EST round-trip commentary.
`tests/integration/test_est_pha_roundtrip.c`	Condenses post-handshake-auth notes.
`tests/integration/test_est_mtls_roundtrip.c`	Shortens mutual-TLS test overview.
`tests/integration/test_est_mldsa_roundtrip.c`	Condenses ML-DSA test documentation.
`tests/integration/test_est_csr_attrs_roundtrip.c`	Trims CSR-attributes test notes.
`tests/integration/test_est_csr_attrs_enforce.c`	Condenses enforcement commentary.
`tests/integration/test_est_csr_attrs_apply_roundtrip.c`	Shortens auto-apply test notes.
`tests/integration/test_est_async_roundtrip.c`	Condenses asynchronous EST commentary.
`tests/integration/est_client_cases.h`	Trims shared EST case documentation.
`tests/integration/cli_proto_scoping.sh`	Shortens CLI validation explanations.
`tests/CMakeLists.txt`	Condenses test registration comments.
`src/wolfcert.c`	Removes initialization implementation narrative.
`src/store.c`	Removes section banners and trims platform note.
`src/pkcs7_util.c`	Condenses PKCS#7 implementation commentary.
`src/net_posix.c`	Trims POSIX transport explanations.
`src/keygen.c`	Condenses key-generation implementation notes.
`src/key_algs.h`	Shortens algorithm dispatch contracts.
`src/key_algs.c`	Removes banners and trims algorithm notes.
`src/internal.c`	Removes banners and redundant implementation comments.
`src/est/est_client.c`	Condenses EST validation and session notes.
`src/csr.c`	Trims DN, SAN, and signature commentary.
`src/client.c`	Condenses high-level client orchestration notes.
`scripts/gen_test_ca.sh`	Shortens placeholder description.
`scripts/ci/wolfcert-only-user_settings.h`	Condenses configuration fixture guidance.
`scripts/ci/sigpipe-launcher.sh`	Shortens SIGPIPE launcher rationale.
`scripts/ci/freestanding-user_settings.h`	Condenses freestanding configuration labels.
`scripts/ci/config-probe.c`	Shortens configuration probe overview.
`scripts/ci/compile-freestanding.sh`	Condenses Bash compatibility note.
`scripts/ci/check-config-resolution.sh`	Shortens case-table documentation.
`scripts/ci/check-buildsystem-parity.sh`	Condenses parity-check descriptions.
`scripts/ci/build-wolfssl.sh`	Trims wolfSSL build-script commentary.
`scripts/ci/assert-configure-fails.sh`	Removes banners and shortens VPATH note.
`Makefile.am`	Condenses build and test comments.
`examples/user_settings.h.example`	Streamlines template configuration guidance.
`examples/enroll_scep.c`	Shortens SCEP constraints and step labels.
`examples/enroll_est.c`	Condenses key-selection step.
`examples/enroll_cryptocb.c`	Trims CryptoCb integration explanation.
`examples/certs/gen-certs.sh`	Simplifies certificate-generation labels.
`CMakeLists.txt`	Condenses configuration, source, and install notes.
`cmake/wolfCertTargets.cmake.in`	Shortens generated-target description.
`cmake/Config.cmake.in`	Condenses package configuration notes.
`cli/wolfcert_server.c`	Shortens CSR-attributes validation note.
`.github/workflows/sanitizers.yml`	Condenses sanitizer workflow commentary.
`.github/workflows/pr.yml`	Trims PR matrix and execution notes.
`.github/workflows/nightly.yml`	Condenses nightly workflow commentary.
`.github/workflows/lint.yml`	Shortens lint workflow description.
`.github/workflows/interop.yml`	Trims interoperability workflow notes.
`.github/actions/zephyr-workspace/action.yml`	Condenses workspace-cache explanation.
`.github/actions/build-wolfssl/action.yml`	Shortens cache-key and restore rationale.
`.github/actions/build-wolfcert-cli/action.yml`	Condenses cache and runtime-path notes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The headers are the API reference, so each function and field keeps its contract: ownership, what NULL or 0 means, units, and the return codes and PENDING outcomes a caller has to handle. What goes is the prose around it: the CsrAttrs ASN.1 walkthrough, theory-of-operation file headers in log.h, memory.h, status.h, check_config.h and options.h.in, explanations above #error lines whose message already says the same, and section banners. Two wrong claims are dropped: store.h called the POSIX store safe to pass to a wolfcert_free() that does not exist, and est.h suggested handing csrattrs bytes to meta.csr_attributes_der although wolfcert_csr_build never emits that field. Five more are corrected: status.h now states the thread-local fallback as src/internal.c implements it, challenge_password cites RFC 8894 section 2.4 rather than 2.9, subject_dn no longer rules out '=' in a value, an empty /csrattrs reply covers 404 and an empty body as well as 204, and ca_id lists GetNextCACert. No code changes.

Most of the comment volume in src/ was argument rather than documentation: six to nine line paragraphs over each tunable in internal.h covering sizing math and stack versus heap placement, defensive rationale for branches, notes on where a matching check lives in another file, section banners, and narration of what the next call does. Those go. RFC section references, wolfSSL workarounds (the pid_t include, EncodeSignedData not being retryable after BUFFER_E, the reversed AddCertificate order) and traps a later edit would reintroduce stay, shortened. Stale comments removed on the way: est_server.c described itself as plaintext HTTP only though it serves TLS, pending_add claimed a duplicate check it does not do, and two blocks in http.c sat above the wrong declaration. The examples keep their step labels and the notes an integrator copying them needs; the user_settings headers keep one label per group. No code changes.

Nearly every test carried a paragraph arguing why the behaviour matters and what a pass does or does not prove. Each is now a line or two naming what is checked, with the RFC section where there is one, or nothing when the function name already says it. File headers listing every case are cut to a short summary. Comments explaining a fixture value, a byte offset or a timing constraint stay, such as the hand-built high-bit serial in test_scep_msg.c and the request timeout STALL_WAIT_MS has to exceed. No code changes.

CMakeLists.txt, configure.ac, Makefile.am, the CMake package templates, the CI scripts, probes and user_settings headers, the interop and certificate scripts, the workflows and the Zephyr module and test configs explained their own variable ladders, option choices and standard CMake, autoconf and shell behaviour. Most blocks are now a short label, keeping only constraints a maintainer would get wrong, such as the libest FIPS_mode stub for OpenSSL 3 and step-ca's RSA chain swap for SCEP. Section banners become plain labels. The '#' headers of assert-configure-fails.sh, check-config-resolution.sh and compile-freestanding.sh stay, since --help prints them. No code changes.

Frauschi self-assigned this Oct 7, 2026

Copilot AI balanced review requested due to automatic review settings October 7, 2026 08:22

Copilot started reviewing on behalf of Frauschi October 7, 2026 08:23 View session

wolfSSL-Fenrir-bot reviewed Oct 7, 2026

View reviewed changes

Copilot AI reviewed Oct 7, 2026

View reviewed changes

Comment thread tests/integration/test_server_stop_idle.c Outdated

Comment thread wolfcert/status.h Outdated

Frauschi added 4 commits October 7, 2026 17:40

Frauschi force-pushed the comments branch from cdc9cd1 to d88cc8d Compare October 7, 2026 15:44

Frauschi assigned yosuke-wolfssl and unassigned Frauschi Oct 7, 2026

Frauschi requested a review from yosuke-wolfssl October 7, 2026 15:45

Navigation Menu

Repository navigation

Trim code comments across the repo - #49

Trim code comments across the repo#49
Frauschi wants to merge 4 commits into
wolfSSL:mainfrom
Frauschi:comments

Frauschi commented Oct 7, 2026

Uh oh!

Frauschi commented Oct 7, 2026

Uh oh!

wolfSSL-Fenrir-bot left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Frauschi commented Oct 7, 2026

Problem

Fix

Notes for reviewers

Uh oh!

Frauschi commented Oct 7, 2026

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #49

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants