diff --git a/.gitea/workflows/ci.yaml b/.gitea/workflows/ci.yaml index dafb4be..f1ccc36 100644 --- a/.gitea/workflows/ci.yaml +++ b/.gitea/workflows/ci.yaml @@ -55,3 +55,28 @@ jobs: - name: Unit Tests run: ./build-${{ matrix.sanitizer }}/tests + + - name: Integration Tests + run: LSAN_OPTIONS=suppressions=.lsan-suppressions.txt python3 -m pytest tests/ -v --tb=short + + clang-tidy: + runs-on: ubuntu-latest + container: gitea.tap-tap.win/taptap/fastsync-ci:v7 + needs: lint + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Configure (generate compile_commands.json) + run: cmake -B build -S . + + - name: Run clang-tidy (informational, non-blocking) + run: | + find src/ -name '*.c' | xargs clang-tidy -p build \ + --checks='-*,bugprone-*,clang-analyzer-*,misc-*,-misc-no-recursion' \ + 2>&1 | tee clang-tidy-output.txt + if grep -q -E " error:| warning:" clang-tidy-output.txt; then + echo "::warning::clang-tidy found issues — review the output above" + else + echo "clang-tidy: no issues found" + fi diff --git a/.lsan-suppressions.txt b/.lsan-suppressions.txt new file mode 100644 index 0000000..0107c8c --- /dev/null +++ b/.lsan-suppressions.txt @@ -0,0 +1,6 @@ +# LSAN suppressions for FastSync +# Add suppression entries here for known pre-existing leaks that cannot be +# fixed immediately. Remove entries as leaks are fixed. +# +# Example format: +# leak:function_name diff --git a/.opencode/agents/cmake-expert.md b/.opencode/agents/cmake-expert.md index b616652..a50ca07 100644 --- a/.opencode/agents/cmake-expert.md +++ b/.opencode/agents/cmake-expert.md @@ -22,6 +22,10 @@ set(CMAKE_C_STANDARD_REQUIRED ON) add_compile_options(-Wall -g -O3) +include(FetchContent) +FetchContent_Declare(xxhash GIT_REPOSITORY https://github.com/Cyan4973/xxHash GIT_TAG v0.8.3 SOURCE_SUBDIR cmake_unofficial) +FetchContent_MakeAvailable(xxhash) + # Sanitizer option set(SANITIZER "none" CACHE STRING "Sanitizer to enable (address, thread, none)") set_property(CACHE SANITIZER PROPERTY STRINGS address thread none) @@ -40,10 +44,6 @@ if(STRICT_WARNINGS) add_compile_options(-Wextra -Wpedantic -Werror) endif() -include(FetchContent) -FetchContent_Declare(xxhash GIT_REPOSITORY https://github.com/Cyan4973/xxHash GIT_TAG v0.8.3 SOURCE_SUBDIR cmake_unofficial) -FetchContent_MakeAvailable(xxhash) - set(THREADS_PREFER_PTHREAD_FLAG ON) find_package(Threads REQUIRED) @@ -51,6 +51,7 @@ find_library(ZSTD_LIBRARY zstd) if(NOT ZSTD_LIBRARY) message(FATAL_ERROR "zstd library not found. Ensure it is in your nix-shell!") endif() + find_package(OpenSSL REQUIRED) file(GLOB SHARED_SRCS "src/shared/*.c") @@ -76,14 +77,15 @@ target_link_libraries(tests PRIVATE Threads::Threads ${ZSTD_LIBRARY} OpenSSL::SS src/shared/ — shared libraries (globbed as SHARED_SRCS) src/client/ — client sources (globbed as CLIENT_SRCS) src/server/ — server sources (globbed as SERVER_SRCS) -tests/ — test sources (globbed as TEST_SRCS) +tests/ — unit test sources (globbed as TEST_SRCS) +tests/integration/ — Python pytest integration tests ``` ### Dependencies - **zstd** — found via `find_library(ZSTD_LIBRARY zstd)` +- **OpenSSL** — found via `find_package(OpenSSL REQUIRED)` (TLS 1.2+ transport) +- **xxHash** — fetched via `FetchContent` from GitHub (delta transfer hashing, v0.8.3) - **pthreads** — found via `find_package(Threads REQUIRED)` -- **OpenSSL** — found via `find_package(OpenSSL REQUIRED)` -- **xxhash** — fetched via `FetchContent` from GitHub (v0.8.3) - **C11 standard** — required - **CMake 3.22+** — minimum version @@ -162,31 +164,19 @@ cmake --build build -j$(nproc) ./build/tests ``` -## When Adding Sanitizer Support to CMakeLists.txt +## Sanitizer Integration -Use CMake options for cleaner integration: +The project uses a single `SANITIZER` cache variable in `CMakeLists.txt`: ```cmake -option(ENABLE_ASAN "Enable AddressSanitizer" OFF) -option(ENABLE_TSAN "Enable ThreadSanitizer" OFF) -option(ENABLE_UBSAN "Enable UndefinedBehaviorSanitizer" OFF) - -if(ENABLE_ASAN) - add_compile_options(-fsanitize=address -fno-omit-frame-pointer) - add_link_options(-fsanitize=address) -endif() - -if(ENABLE_TSAN) - add_compile_options(-fsanitize=thread) - add_link_options(-fsanitize=thread) -endif() - -if(ENABLE_UBSAN) - add_compile_options(-fsanitize=undefined) - add_link_options(-fsanitize=undefined) -endif() +set(SANITIZER "none" CACHE STRING "Sanitizer to enable (address, thread, none)") +set_property(CACHE SANITIZER PROPERTY STRINGS address thread none) ``` +Supported values: `address`, `thread`, `none`. Unknown values trigger `FATAL_ERROR`. -Then build with: +Build with: ```bash -cmake -B build -S . -DENABLE_ASAN=ON +cmake -B build -S . -DSANITIZER=address +cmake --build build -j$(nproc) ``` + +To add support for a new sanitizer (e.g., UBSan), add an `elseif(SANITIZER STREQUAL "undefined")` block following the existing `address`/`thread` pattern. diff --git a/.opencode/agents/integrator.md b/.opencode/agents/integrator.md index b7b34bf..f2abda4 100644 --- a/.opencode/agents/integrator.md +++ b/.opencode/agents/integrator.md @@ -16,12 +16,12 @@ Design integration tests that verify the full transfer pipeline works end-to-end - Custom framework in `tests/test_utils.h` - Run: `./build/tests` -### 2. Integration Tests (existing — `test.py`) +### 2. Integration Tests (existing — `tests/integration/`) - Full transfer pipeline: client → server → verify - Multiple configurations (TCP, SSH, TLS, compression, multithreading) - Network shaping (LAN, WAN profiles) - Feature tests (dry run, archive, exclude, delete, incremental, bandwidth limit) -- Run: `python3 test.py` +- Run: `python3 -m pytest tests/ -v --tb=short` ### 3. New: Focused Integration Tests When adding new features or fixing bugs, write targeted integration tests. @@ -106,35 +106,36 @@ test ! -f /tmp/dst/.../extra.txt ### Gitea Workflow Structure (`.gitea/workflows/ci.yaml`) The project uses Gitea Actions. Key jobs: -1. **Build** — compile on push/PR -2. **Unit tests** — run `./build/tests` -3. **Integration tests** — run `python3 test.py` (light mode) -4. **Sanitizer builds** — ASan, TSan variants +1. **build-and-test** — compile, unit tests, integration tests on push/PR +2. **sanitizer** — ASan + UBSan build and test (separate job) +3. **clang-tidy** — static analysis on C source files ### Adding a New CI Job ```yaml jobs: sanitizer: runs-on: ubuntu-latest + container: gitea.tap-tap.win/taptap/fastsync-ci:v7 steps: - uses: actions/checkout@v4 - - name: Install dependencies - run: sudo apt-get update && sudo apt-get install -y libzstd-dev libssl-dev - - name: Build with ASan - run: | - cmake -B build -S . \ - -DCMAKE_C_FLAGS="-fsanitize=address -fno-omit-frame-pointer" \ - -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address" - cmake --build build -j$(nproc) - - name: Run tests - run: ./build/tests + - name: Configure + run: cmake -B build-${{ matrix.sanitizer }} -S . -DSANITIZER=${{ matrix.sanitizer }} + - name: Build + run: cmake --build build-${{ matrix.sanitizer }} -j$(nproc) + - name: Symlink for integration tests + run: ln -sf build-${{ matrix.sanitizer }} build + - name: Unit Tests + run: ./build-${{ matrix.sanitizer }}/tests + - name: Integration Tests + run: LSAN_OPTIONS=suppressions=.lsan-suppressions.txt python3 -m pytest tests/ -v --tb=short ``` +The symlink step is required because `tests/conftest.py` expects `./build` to exist. ## Verification Checklist After any code change: - [ ] Unit tests pass: `./build/tests` -- [ ] Integration tests pass: `python3 test.py` (light mode at minimum) +- [ ] Integration tests pass: `python3 -m pytest tests/ -v --tb=short` - [ ] Build clean: no warnings with `-Wall` - [ ] No memory errors: ASan clean - [ ] No thread errors: TSan clean (if threading involved) diff --git a/.opencode/agents/reviewer.md b/.opencode/agents/reviewer.md new file mode 100644 index 0000000..cbcc5ee --- /dev/null +++ b/.opencode/agents/reviewer.md @@ -0,0 +1,134 @@ +--- +description: Reviews pull requests comprehensively — code correctness, CI/CD validity, configuration, documentation, and overall PR quality. Use when the user says "review PR", "review this PR", or wants a comprehensive code review. +mode: subagent +--- + +You are a comprehensive PR reviewer for the FastSync project — a high-performance file synchronization system written in C11. + +## Your Role + +Review pull requests holistically. You go beyond just C code review — you evaluate CI/CD impact, configuration changes, documentation accuracy, and overall PR quality. You are the final gatekeeper before merge. + +## Review Dimensions + +### 1. C Code Review + +Review all changed `.c` and `.h` files for: + +**Memory Safety** +- Every `malloc`/`calloc` has a matching `free` on all code paths (including error paths) +- No use-after-free, no double-free +- Null checks after allocation before use +- Correct buffer sizes (strlen + 1 for null terminators) +- `Data` objects created/destroyed properly via `data_create()`/`data_destroy()` + +**Thread Safety** +- Shared state accessed under proper mutex protection (C11 ``) +- No race conditions on queue operations +- Condition variable signals under lock +- No deadlock potential (consistent lock ordering) +- `done` flags checked properly in consumer loops + +**Security** +- No `strcpy`/`strcat`/`sprintf` — use `snprintf` with bounds +- `malloc` size calculations don't overflow +- Path traversal prevention (`..` in filenames) +- TLS error codes checked after `SSL_read`/`SSL_write` +- No hardcoded certificates, keys, or credentials +- Received file permissions validated (no SUID/SGID injection) + +**Protocol Safety** +- `send_n_data` / `receive_n_data` return values checked +- Status codes validated before use +- Config serialization/deserialization handles partial reads + +**Logic Errors** +- Off-by-one in loops/buffers +- Incorrect size calculations +- Wrong enum values or comparisons +- Missing break statements in switch + +### 2. Build System Review + +If `CMakeLists.txt` is changed: +- Dependencies properly declared with `find_package` or `FetchContent` +- New targets follow existing patterns (link flags, include dirs) +- No duplicate source file additions +- Sanitizer options not accidentally enabled for release builds +- Minimum CMake version is 3.22 + +### 3. CI/CD Review + +If `.gitea/workflows/ci.yaml` is changed: +- Workflow syntax is valid +- New jobs have proper `runs-on` and `container` specifications +- Test commands are correct and will pass +- No secrets or credentials exposed +- Steps are in correct order (checkout before build) + +### 4. Configuration & Documentation Review + +If agents (`.opencode/agents/`), skills (`.opencode/skills/`), or docs are changed: +- References to file paths are accurate (e.g., `test.py` no longer exists, use `tests/integration/`) +- CMake version references match actual `CMakeLists.txt` (3.22, not 4.1) +- Dependencies listed match actual build requirements (zstd, OpenSSL, xxHash) +- Commands in examples actually work +- No stale references to removed files or changed APIs + +### 5. PR Quality + +- Commit messages are clear and follow project conventions +- PR description explains what changed and why +- Changes are focused — not mixing unrelated concerns +- No unnecessary file changes (formatting-only diffs on unchanged code) +- Test coverage for new functionality + +## Review Checklist + +For each PR, evaluate: + +- [ ] All changed C files reviewed for memory/thread/protocol/security +- [ ] Build system changes validated +- [ ] CI/CD changes verified (if any) +- [ ] Agent/skill/doc changes checked for accuracy +- [ ] No secrets, keys, or credentials committed +- [ ] Commit history is clean and meaningful +- [ ] New features have test coverage +- [ ] Breaking changes documented +- [ ] Backward compatibility maintained (protocol version field) + +## Output Format + +``` +=== PR REVIEW SUMMARY === +Branch: +Files reviewed: +Dimensions checked: code, build, CI, docs, quality + +=== FINDINGS === + +[CRITICAL] src/shared/protocol.c:142 — memory + Potential buffer overflow in config deserialization + Fix: Add bounds check before memcpy + +[WARNING] src/client/client_send.c:87 — thread + Queue accessed without lock in error path + Fix: Acquire mtx before queue_destroy + +[STYLE] .opencode/agents/cmake-expert.md:5 — docs + References CMake 4.1 but project uses 3.22 + Fix: Update version reference + +=== VERDICT === +[PASS] No critical issues found — safe to merge + — or — +[FAIL] critical issues must be fixed before merge +``` + +## Rules +- Report ALL issues — don't filter or minimize +- Be specific about line numbers and fix suggestions +- Separate critical from warnings from style +- Check that the PR actually compiles (review CMake changes carefully) +- If agents/docs are changed, verify every reference is current +- Be constructive — suggest fixes, not just problems diff --git a/.opencode/agents/test-writer.md b/.opencode/agents/test-writer.md index 97c336f..5b7869b 100644 --- a/.opencode/agents/test-writer.md +++ b/.opencode/agents/test-writer.md @@ -165,34 +165,28 @@ int main() { ## Integration Test Patterns -When writing integration tests (Python-based), follow the pattern in `test.py`: +When writing integration tests (Python-based), follow the patterns in `tests/integration/`: +- `common.py` — shared helpers (server lifecycle, file verification, transfer utilities) +- `test_preflight.py` — preflight checks and configuration validation +- `test_tcp.py` — TCP transport tests +- `test_ssh.py` — SSH transport tests +- `test_tls.py` — TLS transport tests +- `test_features.py` — feature-specific tests (delete, exclude, incremental, etc.) + +Use `tests/conftest.py` fixtures for server setup/teardown (note: the file is at `tests/conftest.py`, not `tests/integration/conftest.py`). ### Minimal Integration Test ```python -def test_basic_transfer(): +def test_basic_transfer(tmp_path): # Setup - source = create_test_files() - dest = tempfile.mkdtemp() - - # Start server - server = subprocess.Popen(["./build/server"], ...) - time.sleep(0.5) - - # Run client - result = subprocess.run( - ["./build/client", "--source-dir", source, - "--dest-dir", dest, "--save-to-disk"], - capture_output=True, text=True - ) - assert result.returncode == 0 - - # Verify - mismatches, missing = verify_transfer(source, dest) - assert not mismatches - assert not missing - - # Cleanup - server.terminate() + source = tmp_path / "src" + dest = tmp_path / "dst" + source.mkdir() + dest.mkdir() + (source / "file.txt").write_text("test content") + + # Start server and run client (use fixtures from conftest.py) + # Verify with helper from common.py ``` ### Edge Case Tests to Write diff --git a/src/client/client_cli.c b/src/client/client_cli.c index eaeddf8..2b5d979 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -73,6 +73,7 @@ int main(int argc, char* argv[]) { Config* config = config_create(str_dup(PROTOCOL_VERSION), NULL, NULL, save_to_disk, false, false, false, false, 5, false, 0); + int exit_code = 0; int positional_args[2]; int positional_count = 0; @@ -80,7 +81,7 @@ int main(int argc, char* argv[]) { for (int i = 1; i < argc; i++) { if (strcmp(argv[i], "--help") == 0) { print_usage(); - return 0; + goto cleanup; } else if (strcmp(argv[i], "-a") == 0 || strcmp(argv[i], "--archive") == 0) { config->use_compression = true; config->use_multithreading = true; @@ -165,11 +166,13 @@ int main(int argc, char* argv[]) { unsigned long long kbps = strtoull(argv[++i], &end, 10); if (errno != 0 || *end != '\0' || kbps == 0) { fprintf(stderr, "Error: --bwlimit must be a positive integer\n"); - return 1; + exit_code = 1; + goto cleanup; } if (kbps > ULLONG_MAX / 1024) { fprintf(stderr, "Error: --bwlimit value too large\n"); - return 1; + exit_code = 1; + goto cleanup; } io_set_bwlimit(kbps * 1024); log_message(LOG_LEVEL_INFO, "Set bandwidth limit to %llu KB/s", kbps); @@ -195,14 +198,16 @@ int main(int argc, char* argv[]) { } else if (argv[i][0] == '-') { fprintf(stderr, "Unknown option: %s\n", argv[i]); print_usage(); - return 1; + exit_code = 1; + goto cleanup; } else { if (positional_count < 2) positional_args[positional_count++] = i; else { fprintf(stderr, "Unexpected argument: %s\n", argv[i]); print_usage(); - return 1; + exit_code = 1; + goto cleanup; } } } @@ -218,7 +223,8 @@ int main(int argc, char* argv[]) { } else if (positional_count == 1) { fprintf(stderr, "Error: missing destination argument\n"); print_usage(); - return 1; + exit_code = 1; + goto cleanup; } else { if (!config->send_directory && env_source) config->send_directory = str_dup((char*)env_source); @@ -229,22 +235,26 @@ int main(int argc, char* argv[]) { if (!config->send_directory || !config->receive_root_directory) { fprintf(stderr, "Error: source and destination directories are required\n"); print_usage(); - return 1; + exit_code = 1; + goto cleanup; } if (config->use_sendfile && (config->use_chunk_serialization || config->use_compression)) { fprintf(stderr, "Error: -f/--sendfile cannot be combined with -c (compression) or -s (chunk " "serialization)\n"); - return 1; + exit_code = 1; + goto cleanup; } if (config->transport == TRANSPORT_SSH && config->use_sendfile) { fprintf(stderr, "Error: -f/--sendfile is not supported with SSH transport\n"); - return 1; + exit_code = 1; + goto cleanup; } if (config->use_incremental && config->use_chunk_serialization) { fprintf(stderr, "Error: --incremental is not supported with -s (chunk serialization)\n"); - return 1; + exit_code = 1; + goto cleanup; } if (config->use_incremental && !config->use_metadata) { @@ -254,15 +264,18 @@ int main(int argc, char* argv[]) { if (config->use_delta && !config->use_incremental) { fprintf(stderr, "Error: --delta requires --incremental\n"); - return 1; + exit_code = 1; + goto cleanup; } if (config->use_delta && config->use_chunk_serialization) { fprintf(stderr, "Error: --delta cannot be combined with -s (chunk serialization)\n"); - return 1; + exit_code = 1; + goto cleanup; } if (config->use_delta && config->use_sendfile) { fprintf(stderr, "Error: --delta cannot be combined with -f (sendfile)\n"); - return 1; + exit_code = 1; + goto cleanup; } if (config->use_delta && !config->use_metadata) { log_message(LOG_LEVEL_INFO, "Enabling metadata preservation for --delta"); @@ -272,12 +285,18 @@ int main(int argc, char* argv[]) { if (config->use_tls) { if (!config->tls_cert || !config->tls_key) { fprintf(stderr, "Error: --tls requires --cert and --key\n"); - return 1; + exit_code = 1; + goto cleanup; } tls_global_init(); } if (config->use_multithreading) - return send_files_multithreaded(config); - return send_files(config); + exit_code = send_files_multithreaded(config); + else + exit_code = send_files(config); + +cleanup: + config_delete(config); + return exit_code; }