TapTap/FastSync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: update AI automation agents, add reviewer agent, enhance CI #26

Merged
TapTap merged 3 commits from feat/ai-automation-updates into main 2026-07-20 14:38:52 +02:00
Conversation 3 Commits 3 Files Changed 7
+263 -87
TapTap commented 2026-07-19 19:28:52 +02:00
Owner
Copy Link
Copy Source

Changes

Agents Updated

  • cmake-expert: Fixed stale CMake version (4.1 to 3.22), added OpenSSL and xxHash dependencies to match actual CMakeLists.txt
  • integrator: Updated stale test.py references to tests/integration/, fixed CI job descriptions
  • test-writer: Updated integration test patterns to reference modular test structure (tests/integration/, conftest.py, common.py)

New Agent

  • reviewer: Comprehensive PR reviewer that evaluates code correctness, build system, CI/CD, configuration, documentation, and overall PR quality

CI Enhanced

  • sanitizer job: ASan + UBSan build and test (catches memory errors and undefined behavior)
  • clang-tidy job: Static analysis on C source files (catches bugprone patterns and clang-analyzer issues)
## Changes ### Agents Updated - **cmake-expert**: Fixed stale CMake version (4.1 to 3.22), added OpenSSL and xxHash dependencies to match actual CMakeLists.txt - **integrator**: Updated stale test.py references to tests/integration/, fixed CI job descriptions - **test-writer**: Updated integration test patterns to reference modular test structure (tests/integration/, conftest.py, common.py) ### New Agent - **reviewer**: Comprehensive PR reviewer that evaluates code correctness, build system, CI/CD, configuration, documentation, and overall PR quality ### CI Enhanced - **sanitizer job**: ASan + UBSan build and test (catches memory errors and undefined behavior) - **clang-tidy job**: Static analysis on C source files (catches bugprone patterns and clang-analyzer issues)
TapTap commented 2026-07-19 21:00:51 +02:00
Author
Owner
Copy Link
Copy Source

Review: Request changes ⛔

Reviewed the actual PR diff (against origin/main): .gitea/workflows/ci.yaml + the four .opencode/agents/*.md files. Verified by reading the code, a clean build, unit tests, and an ASan+UBSan build & run.

(Note: my first pass accidentally diffed against a stale local main and pulled in the already-merged delta-transfer code from #22. Findings below are scoped to this PR; delta-code findings are noted separately at the end.)

🔴 Critical

1. The sanitizer job never fails — and it is currently masking real bugs in the tree

.gitea/workflows/ci.yaml — both test steps in the sanitizer job end with || true:

      - name: Unit Tests
        run: ./build/tests || true

      - name: Integration Tests
        run: python3 -m pytest tests/ -v --tb=short || true

ASan/UBSan exit non-zero on any finding, so || true turns this job into a permanent green check. Commit 4a5bf67 justifies this with "pre-existing leaks expected" — but this isn't hypothetical. I built this branch with the job's exact flags (-fsanitize=address,undefined -fno-omit-frame-pointer -g) and ran ./build/tests:

  • Leaks from the new delta code (merged in #22): test_delta_serialize_roundtrip leaks literal buffers allocated at src/shared/delta.c:388. Root cause: delta_deserialize() never sets .type in the DELTA_OP_LITERAL branch (compare the match branch at delta.c:365), so delta_destroy() can't tell literals from matches and never frees them. That's an uninitialized-field bug, not just a leak.
  • Pre-existing test_scanner leaks (~4.3 KB in 10 allocations).

So the job as written hides both pre-existing issues and a real uninitialized-memory bug. Please either:

  • remove || true and fix the reported leaks, or
  • keep gating but add an LSAN suppression file for the known pre-existing leaks (LSAN_OPTIONS=suppressions=...) so anything new fails the build.

🟡 Warnings

2. clang-tidy job only fails on error: — analyzer findings are warning:

The enabled check groups (bugprone-*, clang-analyzer-*, misc-*) emit warning:, not error:, so grep -q " error:" will essentially never trigger on their findings — the job only fails on hard compile errors. If gating on analyzer findings is intended, grep for warning: too (possibly with a baseline). If warnings-as-informational is the intent, a comment saying so would help.

(compile_commands.json is fine — CMAKE_EXPORT_COMPILE_COMMANDS ON is set in CMakeLists.txt:5.)

3. apt-get install -y clang-tidy || true + silent skip

Combined with #2, the whole clang-tidy job can no-op without failing. The echo "clang-tidy not available, skipping" is good; consider also ::warning annotations or a step summary so a skipped job is visible in the UI.

🔵 Suggestions

  • Squash the empty ci: trigger workflow run commit (2ffd324).
  • Fix the pre-existing test_scanner leaks so the sanitizer job can run unmasked.

✅ What looks good

  • Agent docs are accurate. cmake-expert now correctly says CMake 3.22 (matches CMakeLists.txt:1) and documents the OpenSSL/xxHash/FetchContent deps; integrator and test-writer correctly reference tests/integration/ + pytest instead of the removed test.py; no stale paths found.
  • The new reviewer.md agent is well-formed (valid frontmatter, sensible review dimensions).
  • CI job structure (checkout → configure → build → test) is sound; no secrets introduced.

⚠️ Out of scope for this PR, but verified while testing the sanitizer job

The ASan build + live wire tests surfaced serious issues in the already-merged delta-transfer code from #22 (present on main). Filing these here so they don't get lost — recommend a follow-up issue/PR:

  1. Remotely-triggerable heap buffer overflow in delta_apply() (src/shared/delta.c:415–438): no out_pos + len <= new_file_size check before the memcpys; the final size check fires after the corruption. Reproduced end-to-end with a malicious-client PoC against an ASan server (WRITE of size 16 ... delta_apply ← receive_delta_file ← receive_incremental_check). Unauthenticated in TCP mode.
  2. Server protocol desync after any post-signature failure (src/shared/file.c:171–277, 314–332): once STATUS_DELTA_SIGNATURE is sent, any failure makes the server send STATUS_NEXT a second time and hang waiting for a full file the client never sends. Reproduced live.
  3. Client desync when delta_compute() returns NULL (src/client/client_send.c:59–61): returns without sending STATUS_NEXT while the server blocks in receive_status.
  4. delta_deserialize() error paths leak prior literal allocations (4 paths missing cleanup); no instruction_count sanity check (~137 GB transient malloc from a 12-byte payload); delta_compute block scan is O(file_size × block_count) — impractical at the 256 MB ceiling.

Happy to open a dedicated issue with full repro details if you want.

Verdict: The agent changes are merge-ready, but the PR's headline feature — the sanitizer job — is neutralized by || true and is actively hiding a real uninitialized-memory bug. Please fix finding #1 before merge; #2/#3 at your discretion.

## Review: Request changes ⛔ Reviewed the actual PR diff (against `origin/main`): `.gitea/workflows/ci.yaml` + the four `.opencode/agents/*.md` files. Verified by reading the code, a clean build, unit tests, and an ASan+UBSan build & run. *(Note: my first pass accidentally diffed against a stale local `main` and pulled in the already-merged delta-transfer code from #22. Findings below are scoped to this PR; delta-code findings are noted separately at the end.)* --- ### 🔴 Critical **1. The sanitizer job never fails — and it is currently masking real bugs in the tree** `.gitea/workflows/ci.yaml` — both test steps in the `sanitizer` job end with `|| true`: ```yaml - name: Unit Tests run: ./build/tests || true - name: Integration Tests run: python3 -m pytest tests/ -v --tb=short || true ``` ASan/UBSan exit non-zero on any finding, so `|| true` turns this job into a permanent green check. Commit `4a5bf67` justifies this with "pre-existing leaks expected" — but this isn't hypothetical. I built this branch with the job's exact flags (`-fsanitize=address,undefined -fno-omit-frame-pointer -g`) and ran `./build/tests`: - **Leaks from the new delta code** (merged in #22): `test_delta_serialize_roundtrip` leaks literal buffers allocated at `src/shared/delta.c:388`. Root cause: `delta_deserialize()` never sets `.type` in the `DELTA_OP_LITERAL` branch (compare the match branch at `delta.c:365`), so `delta_destroy()` can't tell literals from matches and never frees them. That's an uninitialized-field bug, not just a leak. - Pre-existing `test_scanner` leaks (~4.3 KB in 10 allocations). So the job as written hides both pre-existing issues **and** a real uninitialized-memory bug. Please either: - remove `|| true` and fix the reported leaks, or - keep gating but add an LSAN suppression file for the known pre-existing leaks (`LSAN_OPTIONS=suppressions=...`) so anything *new* fails the build. --- ### 🟡 Warnings **2. clang-tidy job only fails on ` error:` — analyzer findings are `warning:`** The enabled check groups (`bugprone-*`, `clang-analyzer-*`, `misc-*`) emit `warning:`, not `error:`, so `grep -q " error:"` will essentially never trigger on their findings — the job only fails on hard compile errors. If gating on analyzer findings is intended, grep for `warning:` too (possibly with a baseline). If warnings-as-informational is the intent, a comment saying so would help. (`compile_commands.json` is fine — `CMAKE_EXPORT_COMPILE_COMMANDS ON` is set in `CMakeLists.txt:5`.) **3. `apt-get install -y clang-tidy || true` + silent skip** Combined with #2, the whole clang-tidy job can no-op without failing. The `echo "clang-tidy not available, skipping"` is good; consider also `::warning` annotations or a step summary so a skipped job is visible in the UI. --- ### 🔵 Suggestions - Squash the empty `ci: trigger workflow run` commit (`2ffd324`). - Fix the pre-existing `test_scanner` leaks so the sanitizer job can run unmasked. --- ### ✅ What looks good - **Agent docs are accurate.** cmake-expert now correctly says CMake 3.22 (matches `CMakeLists.txt:1`) and documents the OpenSSL/xxHash/FetchContent deps; integrator and test-writer correctly reference `tests/integration/` + pytest instead of the removed `test.py`; no stale paths found. - The new `reviewer.md` agent is well-formed (valid frontmatter, sensible review dimensions). - CI job structure (checkout → configure → build → test) is sound; no secrets introduced. --- ### ⚠️ Out of scope for this PR, but verified while testing the sanitizer job The ASan build + live wire tests surfaced serious issues in the **already-merged** delta-transfer code from #22 (present on `main`). Filing these here so they don't get lost — recommend a follow-up issue/PR: 1. **Remotely-triggerable heap buffer overflow in `delta_apply()`** (`src/shared/delta.c:415–438`): no `out_pos + len <= new_file_size` check before the `memcpy`s; the final size check fires *after* the corruption. Reproduced end-to-end with a malicious-client PoC against an ASan server (`WRITE of size 16 ... delta_apply ← receive_delta_file ← receive_incremental_check`). Unauthenticated in TCP mode. 2. **Server protocol desync after any post-signature failure** (`src/shared/file.c:171–277`, `314–332`): once `STATUS_DELTA_SIGNATURE` is sent, any failure makes the server send `STATUS_NEXT` a *second* time and hang waiting for a full file the client never sends. Reproduced live. 3. **Client desync when `delta_compute()` returns NULL** (`src/client/client_send.c:59–61`): returns without sending `STATUS_NEXT` while the server blocks in `receive_status`. 4. `delta_deserialize()` error paths leak prior literal allocations (4 paths missing cleanup); no `instruction_count` sanity check (~137 GB transient malloc from a 12-byte payload); `delta_compute` block scan is O(file_size × block_count) — impractical at the 256 MB ceiling. Happy to open a dedicated issue with full repro details if you want. --- **Verdict:** The agent changes are merge-ready, but the PR's headline feature — the sanitizer job — is neutralized by `|| true` and is actively hiding a real uninitialized-memory bug. Please fix finding #1 before merge; #2/#3 at your discretion.
TapTap force-pushed feat/ai-automation-updates from 4a5bf67b59 to 20a26e2f5a 2026-07-19 21:04:36 +02:00 Compare
TapTap force-pushed feat/ai-automation-updates from 20a26e2f5a to 64b8a1a950 2026-07-19 21:09:38 +02:00 Compare
TapTap force-pushed feat/ai-automation-updates from 64b8a1a950 to 326aa5d08b 2026-07-19 22:38:22 +02:00 Compare
TapTap commented 2026-07-19 22:59:37 +02:00
Author
Owner
Copy Link
Copy Source

Re-review: Request changes ⛔

The force-push addressed the previous || true issue (now uses LSAN_OPTIONS=suppressions=.lsan-suppressions.txt instead). Verified locally: ASan+UBSan build clean, unit tests pass, 39/49 integration tests pass with LSAN suppression.

🔴 Critical

1. cmake-expert.md introduces misleading commented-out sanitizer lines (.opencode/agents/cmake-expert.md:24-25,88)

The diff adds these new lines to the CMake snippet:

# add_compile_options(-Wall -g -O1 -fsanitize=address)
# add_link_options(-fsanitize=address)

And line 88 still says "Sanitizer support is commented out but present". This is false — origin/main's CMakeLists.txt has a live -DSANITIZER=address|thread|none option block, not commented-out lines. This PR introduces wrong code to match a wrong claim. Any agent reading this doc would produce non-standard builds.

Fix: Replace the commented-out lines with the actual SANITIZER option block from CMakeLists, and update the text to reference -DSANITIZER=address.

2. "When Adding Sanitizer Support" uses ENABLE_ASAN/ENABLE_TSAN pattern (.opencode/agents/cmake-expert.md:172-198)

The section shows three separate option(ENABLE_ASAN ...) / option(ENABLE_TSAN ...) / option(ENABLE_UBSAN ...) flags. The project uses a single -DSANITIZER=address|thread|none cache variable. An agent following this doc would add non-matching boilerplate. Fix: Update to recommend the existing set(SANITIZER ...) pattern, or remove the section.

3. integrator.md YAML example uses stale v6 tag and raw flags (.opencode/agents/integrator.md:113-128)

  • Line 118: container: gitea.tap-tap.win/taptap/fastsync-ci:v6 — actual CI uses :v7
  • Lines 122-125: -DCMAKE_C_FLAGS="-fsanitize=address,undefined ..." — origin/main supports -DSANITIZER=address
  • Missing: the symlink step and integration test with LSAN suppression that the ci.yaml now has

Agents copying this example for new CI jobs would produce broken YAML.

🟡 Warnings

4. clang-tidy job never gates (.gitea/workflows/ci.yaml:75-79) — grep then just echoes a message; the step passes regardless of findings. Compare with lint job's --Werror/--error-exitcode=1. If intentionally informational, add a comment. Otherwise, gate on errors.

5. "Fix leaks" claim is overstated — the only "fix" is .lsan-suppressions.txt suppressing config_create. The actual leak (246 bytes in 4 allocations, confirmed by ASan) is still present. Rename the commit or add config_destroy() before return in client_cli.c.

6. integrator.md example YAML omits the symlink + integration test steps — any agent adding a sanitizer-like job from this doc would produce broken YAML.

🔵 Suggestions

  • In test-writer.md, clarify that conftest.py is at tests/conftest.py (not tests/integration/conftest.py) for discoverability.
  • UBSan pre-existing alignment warnings in chunk.c:98,135 — outside PR scope, but tech debt.

✅ What's Fixed

  • || true is gone — integration tests now run properly with LSAN_OPTIONS=suppressions=...
  • LSAN suppression correctly targets only the config_create leak (verified: 39/49 integration tests pass with it, 25 fail without)
  • cmake_minimum_required 4.1→3.22, xxHash/OpenSSL/ZSTD checks added
  • test.py → tests/integration/ in integrator.md and test-writer.md ✅
  • Symlink step is correct and necessary (common.py references ./build/)

Verdict: Request changes. The || true removal and LSAN suppression are real progress, but the cmake-expert.md actively introduces new inaccuracies (commented-out sanitizer lines that don't match reality), the integrator.md example uses stale :v6 and wrong flags, and the clang-tidy job runs without gating. Fix the three critical doc issues before merge.

## Re-review: Request changes ⛔ The force-push addressed the previous `|| true` issue (now uses `LSAN_OPTIONS=suppressions=.lsan-suppressions.txt` instead). Verified locally: ASan+UBSan build clean, unit tests pass, 39/49 integration tests pass with LSAN suppression. --- ### 🔴 Critical **1. cmake-expert.md introduces misleading commented-out sanitizer lines** (`.opencode/agents/cmake-expert.md:24-25,88`) The diff **adds** these new lines to the CMake snippet: ```cmake # add_compile_options(-Wall -g -O1 -fsanitize=address) # add_link_options(-fsanitize=address) ``` And line 88 still says "Sanitizer support is commented out but present". **This is false** — `origin/main`'s `CMakeLists.txt` has a **live** `-DSANITIZER=address|thread|none` option block, not commented-out lines. This PR introduces wrong code to match a wrong claim. Any agent reading this doc would produce non-standard builds. **Fix:** Replace the commented-out lines with the actual SANITIZER option block from CMakeLists, and update the text to reference `-DSANITIZER=address`. **2. "When Adding Sanitizer Support" uses `ENABLE_ASAN`/`ENABLE_TSAN` pattern** (`.opencode/agents/cmake-expert.md:172-198`) The section shows three separate `option(ENABLE_ASAN ...)` / `option(ENABLE_TSAN ...)` / `option(ENABLE_UBSAN ...)` flags. The project uses a **single** `-DSANITIZER=address|thread|none` cache variable. An agent following this doc would add non-matching boilerplate. **Fix:** Update to recommend the existing `set(SANITIZER ...)` pattern, or remove the section. **3. integrator.md YAML example uses stale `v6` tag and raw flags** (`.opencode/agents/integrator.md:113-128`) - Line 118: `container: gitea.tap-tap.win/taptap/fastsync-ci:v6` — actual CI uses **`:v7`** - Lines 122-125: `-DCMAKE_C_FLAGS="-fsanitize=address,undefined ..."` — origin/main supports **`-DSANITIZER=address`** - Missing: the symlink step and integration test with LSAN suppression that the ci.yaml now has Agents copying this example for new CI jobs would produce broken YAML. --- ### 🟡 Warnings **4. clang-tidy job never gates** (`.gitea/workflows/ci.yaml:75-79`) — `grep` then just echoes a message; the step passes regardless of findings. Compare with lint job's `--Werror`/`--error-exitcode=1`. If intentionally informational, add a comment. Otherwise, gate on errors. **5. "Fix leaks" claim is overstated** — the only "fix" is `.lsan-suppressions.txt` suppressing `config_create`. The actual leak (246 bytes in 4 allocations, confirmed by ASan) is still present. Rename the commit or add `config_destroy()` before `return` in `client_cli.c`. **6. integrator.md example YAML omits the symlink + integration test steps** — any agent adding a sanitizer-like job from this doc would produce broken YAML. --- ### 🔵 Suggestions - In test-writer.md, clarify that `conftest.py` is at `tests/conftest.py` (not `tests/integration/conftest.py`) for discoverability. - UBSan pre-existing alignment warnings in `chunk.c:98,135` — outside PR scope, but tech debt. ### ✅ What's Fixed - **`|| true` is gone** — integration tests now run properly with `LSAN_OPTIONS=suppressions=...` - LSAN suppression correctly targets only the `config_create` leak (verified: 39/49 integration tests pass with it, 25 fail without) - `cmake_minimum_required` 4.1→3.22, xxHash/OpenSSL/ZSTD checks added - `test.py` → `tests/integration/` in integrator.md and test-writer.md ✅ - Symlink step is correct and necessary (common.py references `./build/`) --- **Verdict: Request changes.** The `|| true` removal and LSAN suppression are real progress, but the cmake-expert.md actively introduces *new* inaccuracies (commented-out sanitizer lines that don't match reality), the integrator.md example uses stale `:v6` and wrong flags, and the clang-tidy job runs without gating. Fix the three critical doc issues before merge.
TapTap added 3 commits 2026-07-20 14:29:16 +02:00
feat: update AI automation agents, add reviewer agent, fix leaks, enhance CI bd17f50f9b 
Agents:
- cmake-expert: fix stale CMake version (4.1 -> 3.22), add OpenSSL/xxHash deps
- integrator: fix stale test.py references to tests/integration/, update CI docs
- test-writer: update integration test patterns for modular test structure
- reviewer: new comprehensive PR reviewer (code, build, CI, docs, quality)

Bug fixes:
- delta.c: set instructions[i].type = DELTA_INSTR_LITERAL in deserialize
- scanner.c: free ArrayList when directory_scanner_next returns NULL

CI:
- Add sanitizer job (ASan + UBSan) with LSAN suppressions for pre-existing leaks
- Add clang-tidy job with proper warning/error detection
- Remove || true masking (fixes now make sanitizer useful)

Cleanup:
- gitignore build-asan/
- .lsan-suppressions.txt for known CLI config leaks
ci: symlink build dir for sanitizer integration tests c7f34eaf9d
fix: address PR review — doc accuracy, clang-tidy info, config leak fix
CI / lint (push) Successful in 7s
Details
CI / lint (pull_request) Successful in 7s
Details
CI / build-and-test (push) Successful in 54s
Details
CI / clang-tidy (push) Successful in 6s
Details
CI / sanitizers (address) (push) Successful in 59s
Details
CI / build-and-test (pull_request) Successful in 55s
Details
CI / clang-tidy (pull_request) Successful in 5s
Details
CI / sanitizers (address) (pull_request) Successful in 57s
Details
5c686b0655 
Review findings addressed:

Critical:
- cmake-expert.md: replace commented-out sanitizer lines with actual
  SANITIZER cache variable block, fix ENABLE_ASAN/TSAN/UBSAN pattern
- integrator.md: update YAML example to v7, use -DSANITIZER= instead
  of raw flags, add symlink + LSAN suppression steps

Warnings:
- ci.yaml: rename clang-tidy step to 'informational, non-blocking',
  add ::warning:: workflow command for visibility
- client_cli.c: add goto cleanup pattern to free config on error paths
  (skip for multithreaded path where pipeline_context_sender_destroy
  already owns config)

Suggestions:
- test-writer.md: clarify conftest.py is at tests/conftest.py
- .lsan-suppressions.txt: remove config_create suppression (leak fixed)
TapTap force-pushed feat/ai-automation-updates from 9b2d697256 to 5c686b0655 2026-07-20 14:29:16 +02:00 Compare
TapTap commented 2026-07-20 14:37:46 +02:00
Author
Owner
Copy Link
Copy Source

Re-review 2: Approve ✅

All 3 critical + 3 warning items from the last review are fixed. The config leak is now actually fixed (not just suppressed), docs corrected, and the LSAN suppression file emptied.

Verified fixes

Finding Fix Status
🔴 cmake-expert: stale commented-out sanitizer lines Removed; live SANITIZER option block added ✅
🔴 cmake-expert: wrong ENABLE_ASAN/ENABLE_TSAN pattern Replaced with "Sanitizer Integration" showing actual set(SANITIZER ...) ✅
🔴 integrator.md: stale v6 tag + raw flags Updated to v7, -DSANITIZER=, symlink + integration test steps documented ✅
🟡 clang-tidy never gates Now labeled "informational, non-blocking", uses ::warning:: annotation, adds "no issues found" for clean case ✅
🟡 "Fix leaks" overstated Config leak now actually fixed in client_cli.c (goto cleanup + config_delete); LSAN suppressions emptied ✅
🟡 integrator example missing symlink + LSAN steps Now documented with explanation of why symlink is needed ✅

Config leak fix

src/client/client_cli.c now properly frees config via goto cleanup + config_delete(config) on all early-exit paths. The config_owned_by_pipeline flag prevents double-free when send_files/send_files_multithreaded takes ownership. Clean, idiomatic pattern. LSAN suppressions file emptied to just a format comment — the actual leak is gone.

cmake-expert.md quality

The doc now accurately describes the project's sanitizer approach:

  • SANITIZER cache variable with address/thread/none
  • Correct conventions bullet: all 5 link libraries listed
  • Dependencies include OpenSSL and xxHash with v0.8.3 version
  • "Sanitizer Integration" section shows the actual pattern and correctly notes UBSan isn't yet supported (PR #25 adds it)
  • "To add support for a new sanitizer, add an elseif block" — clear and actionable

integrator.md quality

The YAML example now matches real CI structure: v7 tag, -DSANITIZER= configure, symlink step, unit + integration tests with LSAN suppression. Includes explanation of why the symlink is needed. Agents following this for new CI jobs will produce correct YAML.

Verdict: Approve. All previous critical and warning items addressed with clean, targeted changes. No new issues found. The config leak fix is a genuine improvement to client_cli.c (not just a suppression), and the agent docs now accurately reflect the project's current CMake and CI patterns.

## Re-review 2: Approve ✅ All 3 critical + 3 warning items from the last review are fixed. The config leak is now **actually fixed** (not just suppressed), docs corrected, and the LSAN suppression file emptied. ### Verified fixes | Finding | Fix | Status | |---|---|---| | 🔴 cmake-expert: stale commented-out sanitizer lines | Removed; live SANITIZER option block added | ✅ | | 🔴 cmake-expert: wrong `ENABLE_ASAN`/`ENABLE_TSAN` pattern | Replaced with "Sanitizer Integration" showing actual `set(SANITIZER ...)` | ✅ | | 🔴 integrator.md: stale `v6` tag + raw flags | Updated to `v7`, `-DSANITIZER=`, symlink + integration test steps documented | ✅ | | 🟡 clang-tidy never gates | Now labeled "informational, non-blocking", uses `::warning::` annotation, adds "no issues found" for clean case | ✅ | | 🟡 "Fix leaks" overstated | Config leak now **actually fixed** in `client_cli.c` (goto cleanup + `config_delete`); LSAN suppressions emptied | ✅ | | 🟡 integrator example missing symlink + LSAN steps | Now documented with explanation of why symlink is needed | ✅ | ### Config leak fix `src/client/client_cli.c` now properly frees `config` via `goto cleanup` + `config_delete(config)` on all early-exit paths. The `config_owned_by_pipeline` flag prevents double-free when `send_files`/`send_files_multithreaded` takes ownership. Clean, idiomatic pattern. LSAN suppressions file emptied to just a format comment — the actual leak is gone. ### cmake-expert.md quality The doc now accurately describes the project's sanitizer approach: - SANITIZER cache variable with `address`/`thread`/`none` - Correct conventions bullet: all 5 link libraries listed - Dependencies include OpenSSL and xxHash with v0.8.3 version - "Sanitizer Integration" section shows the actual pattern and correctly notes UBSan isn't yet supported (PR #25 adds it) - "To add support for a new sanitizer, add an `elseif` block" — clear and actionable ### integrator.md quality The YAML example now matches real CI structure: `v7` tag, `-DSANITIZER=` configure, symlink step, unit + integration tests with LSAN suppression. Includes explanation of why the symlink is needed. Agents following this for new CI jobs will produce correct YAML. --- **Verdict: Approve.** All previous critical and warning items addressed with clean, targeted changes. No new issues found. The config leak fix is a genuine improvement to `client_cli.c` (not just a suppression), and the agent docs now accurately reflect the project's current CMake and CI patterns.
TapTap merged commit afa5aeca37 into main 2026-07-20 14:38:52 +02:00
TapTap deleted branch feat/ai-automation-updates 2026-07-20 14:38:56 +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
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: TapTap/FastSync#26
Delete Branch "feat/ai-automation-updates"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?