ci: add static analysis, sanitizers, and formatting enforcement #24

Merged

TapTap merged 15 commits from ci/tier1-review-automation into main 2026-07-19 18:10:07 +02:00

Conversation 1 Commits 15 Files Changed 52

+1397 -1137

TapTap commented 2026-07-19 15:38:00 +02:00

Owner

Copy Link

Copy Source

Summary

Add Tier 1 review automation to catch issues before human review. This reduces the review burden by automating the mechanical checks that were previously caught in multi-round post-review fix cycles.

Changes

CI Pipeline (.gitea/workflows/ci.yaml)

Expanded from 1 job to 3 parallel jobs:

• lint: clang-format format check + cppcheck static analysis
• build-and-test: builds with -DSTRICT_WARNINGS=ON (-Wextra -Wpedantic -Werror), runs unit + integration tests
• sanitizers: ASan + TSan matrix builds, runs unit tests

Build System (CMakeLists.txt)

• Added SANITIZER option (address/thread/none) for sanitizer builds
• Added STRICT_WARNINGS option for -Werror in CI
• Removed commented-out hardcoded ASan lines

Code Quality

• Added .clang-format matching existing code style
• Updated Dockerfile with cppcheck and clang-format
• Fixed 3 compiler warnings (compression.c, file.c, protocol.c) to pass -Werror

Verification

• Build with -Werror: clean (0 warnings)
• Unit tests: all 7 pass
• ASan build: passes, found 4 pre-existing memory leaks in scanner tests (noted for follow-up)

Impact

Every PR now automatically gates on format, static analysis, strict warnings, and memory/thread safety — reducing human review rounds from 3-4 to 0-1 for mechanical issues.

## Summary Add Tier 1 review automation to catch issues before human review. This reduces the review burden by automating the mechanical checks that were previously caught in multi-round post-review fix cycles. ## Changes ### CI Pipeline (.gitea/workflows/ci.yaml) Expanded from 1 job to 3 parallel jobs: - **lint**: clang-format format check + cppcheck static analysis - **build-and-test**: builds with -DSTRICT_WARNINGS=ON (-Wextra -Wpedantic -Werror), runs unit + integration tests - **sanitizers**: ASan + TSan matrix builds, runs unit tests ### Build System (CMakeLists.txt) - Added SANITIZER option (address/thread/none) for sanitizer builds - Added STRICT_WARNINGS option for -Werror in CI - Removed commented-out hardcoded ASan lines ### Code Quality - Added .clang-format matching existing code style - Updated Dockerfile with cppcheck and clang-format - Fixed 3 compiler warnings (compression.c, file.c, protocol.c) to pass -Werror ## Verification - Build with -Werror: clean (0 warnings) - Unit tests: all 7 pass - ASan build: passes, found 4 pre-existing memory leaks in scanner tests (noted for follow-up) ## Impact Every PR now automatically gates on format, static analysis, strict warnings, and memory/thread safety — reducing human review rounds from 3-4 to 0-1 for mechanical issues.

TapTap added 1 commit 2026-07-19 15:38:00 +02:00

ci: add static analysis, sanitizers, and formatting enforcement ... 02266710fb

Add Tier 1 review automation to catch issues before human review:

- Add cppcheck and clang-format to CI lint job
- Add sanitizer matrix (ASan + TSan) CI job
- Enable -Wextra -Wpedantic -Werror in CI build
- Add SANITIZER and STRICT_WARNINGS CMake options
- Add .clang-format for consistent code style
- Update Dockerfile with cppcheck and clang-format
- Fix sign-compare and unused-parameter warnings for -Werror

TapTap added 1 commit 2026-07-19 15:57:39 +02:00

fix: reformat codebase and fix const-correctness for CI lint ... 23b1d6660c

- Reformat all C/H files to match .clang-format (LLVM style)
- Fix 26 cppcheck const-correctness warnings (constParameterPointer,
  constVariablePointer, constVariable)
- Update function declarations in headers to match const parameters

TapTap added 1 commit 2026-07-19 16:01:50 +02:00

ci: use fastsync-ci:v7 image with cppcheck and clang-format pre-installed 5ede5c8bec

TapTap added 1 commit 2026-07-19 16:07:00 +02:00

fix: reformat queue.c/queue.h after const-correctness change f899085de4

TapTap added 1 commit 2026-07-19 16:09:15 +02:00

ci: use find|xargs for clang-format to fix directory handling f6eeb69d77

TapTap added 1 commit 2026-07-19 16:13:50 +02:00

fix: const-correct receive_incremental_check for cppcheck 256e1d7567

TapTap added 1 commit 2026-07-19 16:16:29 +02:00

fix: const-correct local Config* in multiprocessing.c for cppcheck ba6d42ea2d

TapTap added 1 commit 2026-07-19 16:22:19 +02:00

fix: scanner leak, remove TSan (Docker incompatible) ... 5dfc262cf7

- Fix ArrayList leak in directory_scanner_next when no files found
- Remove TSan from CI matrix (Docker 'unexpected memory mapping')
- Keep ASan which now passes clean

TapTap commented 2026-07-19 16:32:29 +02:00

Author

Owner

Copy Link

Copy Source

Architect Review: PR #24 — "ci: add static analysis, sanitizers, and formatting enforcement"

Verdict: PASS — No critical issues found.

Summary

Severity	Count
Critical	0
Warning	4
Style	6

---

Warnings (non-blocking)

W1: Sanitizers job skips integration tests .gitea/workflows/ci.yaml:57
The sanitizers job only runs unit tests, not integration tests. This leaves a gap in sanitizer coverage for server-side code paths. Consider adding a note to track this, or a follow-up to run a subset of integration tests under ASan.

W2: TSan removed (Docker incompatible) .gitea/workflows/ci.yaml:43-45
The sanitizer matrix is [address] only. TSan was removed due to Docker incompatibility. This is documented and reasonable, but TSan should be tracked as a follow-up — the multithreaded pipeline is exactly the kind of code that benefits from TSan. Suggest opening an issue to revisit TSan when Docker supports thread sanitizer (or running TSan outside Docker).

W3: Duplicate -g in CMakeLists.txt CMakeLists.txt:16-19
The sanitizer flags include -g redundantly — it is already in the base compile options on line 9 (add_compile_options(-Wall -g -O3)). Harmless but noisy. Fix: Remove -g from the sanitizer add_compile_options.

W4: Duplicate typedef in protocol.h src/shared/protocol.h:8,23

typedef struct ssl_st SSL;   // line 8
...
typedef struct ssl_st SSL;   // line 23 — duplicate

While C11 permits compatible duplicate typedefs, this is a code smell likely introduced by the formatting pass. Remove the second one on line 23.

---

Style Notes (informational)

S1: file(GLOB) anti-pattern CMakeLists.txt:41-44
Pre-existing. CMake documentation recommends against file(GLOB) for source collection. Worth noting for a future cleanup — switching to explicit source lists.

S2: compression_level parameter unused src/shared/compression.c:10
The compression_level parameter is accepted but never used (hardcodes zstd default). The (void)compression_level; cast silences the warning but hides the semantic gap. Consider using the parameter or documenting it is unused.

S3: off_t cast direction unusual src/shared/file.c:264

while ((unsigned long long)offset < file_size) {

The cast from off_t (signed) to unsigned long long is safe here but the more conventional approach is comparing in the same signed domain. Works correctly, just noting the pattern.

S4: Scanner leak fix is correct (positive finding) src/client/scanner.c:161
The new array_list_delete(chunk_data); when chunk_data->size == 0 is a genuine bug fix. Previously chunk_data was leaked on every call when no files matched criteria. Verified that array_list_delete handles size == 0 correctly.

S5: Pre-existing null check missing src/shared/metadata.c:35-36
No null check after malloc. Pre-existing but the const-correctness changes touched this file. Consider adding a null check in a follow-up.

S6: Missed const opportunities src/shared/protocol.h:25, src/shared/transport_tcp.h:30
send_str takes char* data but should take const char* data. Similarly client_connect takes char* host but should be const char* host. The const-correctness pass missed these.

---

Positive Findings

• All const-correctness changes are correct and well-applied across 13+ functions
• .clang-format config matches existing code style; formatting changes are consistent and safe
• CI pipeline structure (lint → build-and-test + sanitizers) is well-designed with correct dependency chain
• Dockerfile changes are minimal and correct
• Warning fixes are correct and do not change semantics

---

Recommendation

Merge. The PR is well-structured, warning fixes are correct, const-correctness improvements are safe, the scanner leak fix is a genuine bugfix, and the CI infrastructure is solid. The warnings above are minor and can be addressed in follow-ups. The duplicate typedef (W4) is the only thing worth fixing before merge if trivial — it is a one-line deletion.

## Architect Review: PR #24 — "ci: add static analysis, sanitizers, and formatting enforcement" **Verdict: PASS** — No critical issues found. ### Summary | Severity | Count | |----------|-------| | Critical | 0 | | Warning | 4 | | Style | 6 | --- ### Warnings (non-blocking) **W1: Sanitizers job skips integration tests** `.gitea/workflows/ci.yaml:57` The `sanitizers` job only runs unit tests, not integration tests. This leaves a gap in sanitizer coverage for server-side code paths. Consider adding a note to track this, or a follow-up to run a subset of integration tests under ASan. **W2: TSan removed (Docker incompatible)** `.gitea/workflows/ci.yaml:43-45` The sanitizer matrix is `[address]` only. TSan was removed due to Docker incompatibility. This is documented and reasonable, but TSan should be tracked as a follow-up — the multithreaded pipeline is exactly the kind of code that benefits from TSan. Suggest opening an issue to revisit TSan when Docker supports `thread` sanitizer (or running TSan outside Docker). **W3: Duplicate `-g` in CMakeLists.txt** `CMakeLists.txt:16-19` The sanitizer flags include `-g` redundantly — it is already in the base compile options on line 9 (`add_compile_options(-Wall -g -O3)`). Harmless but noisy. Fix: Remove `-g` from the sanitizer `add_compile_options`. **W4: Duplicate typedef in protocol.h** `src/shared/protocol.h:8,23` ```c typedef struct ssl_st SSL; // line 8 ... typedef struct ssl_st SSL; // line 23 — duplicate ``` While C11 permits compatible duplicate typedefs, this is a code smell likely introduced by the formatting pass. Remove the second one on line 23. --- ### Style Notes (informational) **S1: `file(GLOB)` anti-pattern** `CMakeLists.txt:41-44` Pre-existing. CMake documentation recommends against `file(GLOB)` for source collection. Worth noting for a future cleanup — switching to explicit source lists. **S2: `compression_level` parameter unused** `src/shared/compression.c:10` The `compression_level` parameter is accepted but never used (hardcodes zstd default). The `(void)compression_level;` cast silences the warning but hides the semantic gap. Consider using the parameter or documenting it is unused. **S3: `off_t` cast direction unusual** `src/shared/file.c:264` ```c while ((unsigned long long)offset < file_size) { ``` The cast from `off_t` (signed) to `unsigned long long` is safe here but the more conventional approach is comparing in the same signed domain. Works correctly, just noting the pattern. **S4: Scanner leak fix is correct (positive finding)** `src/client/scanner.c:161` The new `array_list_delete(chunk_data);` when `chunk_data->size == 0` is a genuine bug fix. Previously `chunk_data` was leaked on every call when no files matched criteria. Verified that `array_list_delete` handles `size == 0` correctly. **S5: Pre-existing null check missing** `src/shared/metadata.c:35-36` No null check after `malloc`. Pre-existing but the const-correctness changes touched this file. Consider adding a null check in a follow-up. **S6: Missed const opportunities** `src/shared/protocol.h:25`, `src/shared/transport_tcp.h:30` `send_str` takes `char* data` but should take `const char* data`. Similarly `client_connect` takes `char* host` but should be `const char* host`. The const-correctness pass missed these. --- ### Positive Findings - All const-correctness changes are correct and well-applied across 13+ functions - `.clang-format` config matches existing code style; formatting changes are consistent and safe - CI pipeline structure (lint → build-and-test + sanitizers) is well-designed with correct dependency chain - Dockerfile changes are minimal and correct - Warning fixes are correct and do not change semantics --- ### Recommendation **Merge.** The PR is well-structured, warning fixes are correct, const-correctness improvements are safe, the scanner leak fix is a genuine bugfix, and the CI infrastructure is solid. The warnings above are minor and can be addressed in follow-ups. The duplicate typedef (W4) is the only thing worth fixing before merge if trivial — it is a one-line deletion.

TapTap added 1 commit 2026-07-19 16:40:48 +02:00

fix: const-correct protocol send functions, remove duplicate typedef, null check ... dc7ada552b

- send_n_data: void* data → const void* data
- send_str: char* data → const char* data
- send_data: Data* data → const Data* data
- Remove duplicate typedef struct ssl_st SSL in protocol.h
- Add NULL check after malloc in directory_scanner_create

TapTap added 1 commit 2026-07-19 16:44:03 +02:00

fix: const-correct config_send and file_send_single_calls for cppcheck 971a070b65

TapTap added 1 commit 2026-07-19 16:59:01 +02:00

Merge remote-tracking branch 'origin/main' into ci/tier1-review-automation ... b9840b123d

# Conflicts:
#	src/client/client_send.c
#	src/shared/config.c
#	src/shared/file.c
#	src/shared/file.h
#	src/shared/protocol.h

TapTap added 1 commit 2026-07-19 17:04:06 +02:00

fix: apply clang-format to merged code from main d7b9d4dbbc

TapTap added 1 commit 2026-07-19 17:07:15 +02:00

fix: const-correct delta.c and use unread variable in test for cppcheck 413f69de90

TapTap added 1 commit 2026-07-19 17:15:45 +02:00

fix: set DELTA_INSTR_LITERAL type in deserialize and fix error path leaks 8531f9e4c1

TapTap added 1 commit 2026-07-19 17:16:00 +02:00

chore: remove build-asan from tracking, add to .gitignore 4982f9f47d

TapTap merged commit 8a222bffa6 into main 2026-07-19 18:10:07 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Defect or bug

enhancement

Feature or improvement

memory

Memory safety issue

needs-triage

Needs human review

performance

Performance issue

quality

Code quality improvement

security

Security vulnerability

threading

Threading or concurrency

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

TapTap

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TapTap/FastSync#24