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.
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
- 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
- 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
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.txtCMakeLists.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.hsrc/shared/protocol.h:8,23
typedefstructssl_stSSL;// line 8
...typedefstructssl_stSSL;// 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-patternCMakeLists.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 unusedsrc/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 unusualsrc/shared/file.c:264
while((unsignedlonglong)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 missingsrc/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 opportunitiessrc/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.
- 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
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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:
Build System (CMakeLists.txt)
Code Quality
Verification
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.
Architect Review: PR #24 — "ci: add static analysis, sanitizers, and formatting enforcement"
Verdict: PASS — No critical issues found.
Summary
Warnings (non-blocking)
W1: Sanitizers job skips integration tests
.gitea/workflows/ci.yaml:57The
sanitizersjob 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-45The 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 supportsthreadsanitizer (or running TSan outside Docker).W3: Duplicate
-gin CMakeLists.txtCMakeLists.txt:16-19The sanitizer flags include
-gredundantly — it is already in the base compile options on line 9 (add_compile_options(-Wall -g -O3)). Harmless but noisy. Fix: Remove-gfrom the sanitizeradd_compile_options.W4: Duplicate typedef in protocol.h
src/shared/protocol.h:8,23While 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-patternCMakeLists.txt:41-44Pre-existing. CMake documentation recommends against
file(GLOB)for source collection. Worth noting for a future cleanup — switching to explicit source lists.S2:
compression_levelparameter unusedsrc/shared/compression.c:10The
compression_levelparameter 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_tcast direction unusualsrc/shared/file.c:264The cast from
off_t(signed) tounsigned long longis 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:161The new
array_list_delete(chunk_data);whenchunk_data->size == 0is a genuine bug fix. Previouslychunk_datawas leaked on every call when no files matched criteria. Verified thatarray_list_deletehandlessize == 0correctly.S5: Pre-existing null check missing
src/shared/metadata.c:35-36No 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:30send_strtakeschar* databut should takeconst char* data. Similarlyclient_connecttakeschar* hostbut should beconst char* host. The const-correctness pass missed these.Positive Findings
.clang-formatconfig matches existing code style; formatting changes are consistent and safeRecommendation
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.