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)
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:
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.)
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:
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.
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.
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.
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.
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.
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
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)
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.
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
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 main2026-07-20 14:38:52 +02:00
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.
Changes
Agents Updated
New Agent
CI Enhanced
Review: Request changes ⛔
Reviewed the actual PR diff (against
origin/main):.gitea/workflows/ci.yaml+ the four.opencode/agents/*.mdfiles. 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
mainand 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 thesanitizerjob end with|| true:ASan/UBSan exit non-zero on any finding, so
|| trueturns this job into a permanent green check. Commit4a5bf67justifies 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:test_delta_serialize_roundtripleaks literal buffers allocated atsrc/shared/delta.c:388. Root cause:delta_deserialize()never sets.typein theDELTA_OP_LITERALbranch (compare the match branch atdelta.c:365), sodelta_destroy()can't tell literals from matches and never frees them. That's an uninitialized-field bug, not just a leak.test_scannerleaks (~4.3 KB in 10 allocations).So the job as written hides both pre-existing issues and a real uninitialized-memory bug. Please either:
|| trueand fix the reported leaks, orLSAN_OPTIONS=suppressions=...) so anything new fails the build.🟡 Warnings
2. clang-tidy job only fails on
error:— analyzer findings arewarning:The enabled check groups (
bugprone-*,clang-analyzer-*,misc-*) emitwarning:, noterror:, sogrep -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 forwarning:too (possibly with a baseline). If warnings-as-informational is the intent, a comment saying so would help.(
compile_commands.jsonis fine —CMAKE_EXPORT_COMPILE_COMMANDS ONis set inCMakeLists.txt:5.)3.
apt-get install -y clang-tidy || true+ silent skipCombined with #2, the whole clang-tidy job can no-op without failing. The
echo "clang-tidy not available, skipping"is good; consider also::warningannotations or a step summary so a skipped job is visible in the UI.🔵 Suggestions
ci: trigger workflow runcommit (2ffd324).test_scannerleaks so the sanitizer job can run unmasked.✅ What looks good
CMakeLists.txt:1) and documents the OpenSSL/xxHash/FetchContent deps; integrator and test-writer correctly referencetests/integration/+ pytest instead of the removedtest.py; no stale paths found.reviewer.mdagent is well-formed (valid frontmatter, sensible review dimensions).⚠️ 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:delta_apply()(src/shared/delta.c:415–438): noout_pos + len <= new_file_sizecheck before thememcpys; 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.src/shared/file.c:171–277,314–332): onceSTATUS_DELTA_SIGNATUREis sent, any failure makes the server sendSTATUS_NEXTa second time and hang waiting for a full file the client never sends. Reproduced live.delta_compute()returns NULL (src/client/client_send.c:59–61): returns without sendingSTATUS_NEXTwhile the server blocks inreceive_status.delta_deserialize()error paths leak prior literal allocations (4 paths missing cleanup); noinstruction_countsanity check (~137 GB transient malloc from a 12-byte payload);delta_computeblock 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
|| trueand is actively hiding a real uninitialized-memory bug. Please fix finding #1 before merge; #2/#3 at your discretion.4a5bf67b59to20a26e2f5a20a26e2f5ato64b8a1a95064b8a1a950to326aa5d08bRe-review: Request changes ⛔
The force-push addressed the previous
|| trueissue (now usesLSAN_OPTIONS=suppressions=.lsan-suppressions.txtinstead). 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:
And line 88 still says "Sanitizer support is commented out but present". This is false —
origin/main'sCMakeLists.txthas a live-DSANITIZER=address|thread|noneoption 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_TSANpattern (.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|nonecache variable. An agent following this doc would add non-matching boilerplate. Fix: Update to recommend the existingset(SANITIZER ...)pattern, or remove the section.3. integrator.md YAML example uses stale
v6tag and raw flags (.opencode/agents/integrator.md:113-128)container: gitea.tap-tap.win/taptap/fastsync-ci:v6— actual CI uses:v7-DCMAKE_C_FLAGS="-fsanitize=address,undefined ..."— origin/main supports-DSANITIZER=addressAgents copying this example for new CI jobs would produce broken YAML.
🟡 Warnings
4. clang-tidy job never gates (
.gitea/workflows/ci.yaml:75-79) —grepthen 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.txtsuppressingconfig_create. The actual leak (246 bytes in 4 allocations, confirmed by ASan) is still present. Rename the commit or addconfig_destroy()beforereturninclient_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
conftest.pyis attests/conftest.py(nottests/integration/conftest.py) for discoverability.chunk.c:98,135— outside PR scope, but tech debt.✅ What's Fixed
|| trueis gone — integration tests now run properly withLSAN_OPTIONS=suppressions=...config_createleak (verified: 39/49 integration tests pass with it, 25 fail without)cmake_minimum_required4.1→3.22, xxHash/OpenSSL/ZSTD checks addedtest.py→tests/integration/in integrator.md and test-writer.md ✅./build/)Verdict: Request changes. The
|| trueremoval 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:v6and wrong flags, and the clang-tidy job runs without gating. Fix the three critical doc issues before merge.9b2d697256to5c686b0655Re-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
ENABLE_ASAN/ENABLE_TSANpatternset(SANITIZER ...)v6tag + raw flagsv7,-DSANITIZER=, symlink + integration test steps documented::warning::annotation, adds "no issues found" for clean caseclient_cli.c(goto cleanup +config_delete); LSAN suppressions emptiedConfig leak fix
src/client/client_cli.cnow properly freesconfigviagoto cleanup+config_delete(config)on all early-exit paths. Theconfig_owned_by_pipelineflag prevents double-free whensend_files/send_files_multithreadedtakes 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:
address/thread/noneelseifblock" — clear and actionableintegrator.md quality
The YAML example now matches real CI structure:
v7tag,-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.