This is the old stale pattern that was already flagged in the PR #26 review. The real CMakeLists.txt uses a single SANITIZER cache variable with elseif branches — not three separate option() booleans. An agent that reads the "When Adding Sanitizer Support" section will produce wrong CMake code.
Fix: Replace lines 165–191 with instructions showing how to add a new value to the existing SANITIZER cache variable (e.g., add an elseif(SANITIZER STREQUAL "undefined") block). Or remove the section entirely — the correct pattern is already shown above and in AGENTS.md.
PR #27's AGENTS.md says "never add apt-get install to CI workflows — use the custom Docker image". PR #25 adds lcov/valgrind to the Dockerfile. This PR also adds them. Both PRs are correct individually, but whoever merges second should confirm the Dockerfile is clean.
No action needed from this PR — just flagging the overlap. The lcov/valgrind addition in the Dockerfile is the right approach per AGENTS.md.
New agents — well-structured
issue-creator: Good orchestrator pattern — dispatches sub-agents, collects findings, creates issues with gh issue create. Uses mode: subagent correctly.
feature-scout: Clear scan categories (TODOs, hardcoded values, protocol gaps), structured output format with severity/location/suggestion.
security-screener: Good attack-surface focus (buffer overflows, TLS, path traversal, crypto), separate from security-auditor (which does targeted TLS verification).
All three scanner agents have the correct "No findings" return format, severity guidelines, and the standard three footer sections (CI, Branch Strategy, Dependency Installation).
integrator.md — fix correct
The CI example now uses container: gitea.tap-tap.win/taptap/fastsync-ci:v7 instead of sudo apt-get install. Correct per AGENTS.md dependency rules.
Verdict: Approve with 1 fix needed. The stale ENABLE_ASAN/ENABLE_TSAN/ENABLE_UBSAN section in cmake-expert.md should be updated or removed — it contradicts the correct pattern shown earlier in the same file.
## Review
CI is green, new agents are well-structured, integrator fix correct. One stale section needs attention.
---
### 🔴 cmake-expert.md — stale "When Adding Sanitizer Support" section contradicts correct pattern
The document has two contradictory sanitizer instructions:
**Lines 148–163** (correct, "Sanitizer Configurations"):
```bash
cmake -B build -S . -DSANITIZER=address # correct
cmake -B build -S . -DSANITIZER=thread # correct
```
**Lines 165–191** (wrong, "When Adding Sanitizer Support to CMakeLists.txt"):
```cmake
option(ENABLE_ASAN "Enable AddressSanitizer" OFF)
option(ENABLE_TSAN "Enable ThreadSanitizer" OFF)
option(ENABLE_UBSAN "Enable UndefinedBehaviorSanitizer" OFF)
# ... then build with: cmake -B build -S . -DENABLE_ASAN=ON
```
This is the old stale pattern that was already flagged in the PR #26 review. The real CMakeLists.txt uses a single `SANITIZER` cache variable with `elseif` branches — not three separate `option()` booleans. An agent that reads the "When Adding Sanitizer Support" section will produce wrong CMake code.
**Fix:** Replace lines 165–191 with instructions showing how to add a new value to the existing `SANITIZER` cache variable (e.g., add an `elseif(SANITIZER STREQUAL "undefined")` block). Or remove the section entirely — the correct pattern is already shown above and in AGENTS.md.
---
### 🟡 Dockerfile — lcov/valgrind addition conflicts with PR #27's AGENTS.md rule
PR #27's AGENTS.md says "never add `apt-get install` to CI workflows — use the custom Docker image". PR #25 adds lcov/valgrind to the Dockerfile. This PR also adds them. Both PRs are correct individually, but whoever merges second should confirm the Dockerfile is clean.
No action needed from this PR — just flagging the overlap. The lcov/valgrind addition in the Dockerfile is the right approach per AGENTS.md.
---
### New agents — well-structured
- **issue-creator**: Good orchestrator pattern — dispatches sub-agents, collects findings, creates issues with `gh issue create`. Uses `mode: subagent` correctly.
- **feature-scout**: Clear scan categories (TODOs, hardcoded values, protocol gaps), structured output format with severity/location/suggestion.
- **security-screener**: Good attack-surface focus (buffer overflows, TLS, path traversal, crypto), separate from security-auditor (which does targeted TLS verification).
- **code-quality-guardian**: Clear quality categories (god functions, duplication, complexity), structured output.
All three scanner agents have the correct "No findings" return format, severity guidelines, and the standard three footer sections (CI, Branch Strategy, Dependency Installation).
---
### integrator.md — fix correct
The CI example now uses `container: gitea.tap-tap.win/taptap/fastsync-ci:v7` instead of `sudo apt-get install`. Correct per AGENTS.md dependency rules.
---
**Verdict: Approve with 1 fix needed.** The stale `ENABLE_ASAN/ENABLE_TSAN/ENABLE_UBSAN` section in cmake-expert.md should be updated or removed — it contradicts the correct pattern shown earlier in the same file.
The critical cmake-expert.md fix is clean — the stale ENABLE_ASAN/ENABLE_TSAN/ENABLE_UBSAN section is gone, replaced with correct SANITIZER cache variable documentation. All other changes since last review are the PR #26 merge (ci.yaml, client_cli.c, reviewer.md) which we already approved.
FetchContent block now correctly placed before Sanitizer option, matching real file
✅
Remaining minor items (non-blocking)
1. integrator.md line 110: inaccurate CI job description
2. **sanitizer** — ASan + UBSan build and test (separate job)
The CI matrix only has [address]. UBSan is not in the matrix. Should read "ASan build and test" or "Sanitizer build and test (address)".
2. cmake-expert.md: UBSan workaround section
The manual -DCMAKE_C_FLAGS="-fsanitize=undefined" workaround is documented with the accurate caveat "no -DSANITIZER=undefined option in CMakeLists.txt yet". This will become stale when PR #25 lands (which adds UBSan to the SANITIZER option + CI matrix), but is correct for now.
Verdict: Approve. All previous critical items fixed. The two minor items above are non-blocking — the integrator.md inaccuracy is cosmetic (agents will still produce correct CI YAML from the example), and the UBSan workaround is correctly caveated.
## Re-review: Approve ✅ (2 minor items)
The critical cmake-expert.md fix is clean — the stale `ENABLE_ASAN/ENABLE_TSAN/ENABLE_UBSAN` section is gone, replaced with correct `SANITIZER` cache variable documentation. All other changes since last review are the PR #26 merge (ci.yaml, client_cli.c, reviewer.md) which we already approved.
### Verified fixes
| Finding | Fix | Status |
|---|---|---|
| 🔴 cmake-expert: stale `ENABLE_ASAN` pattern | Replaced with "Sanitizer Integration" section showing correct `SANITIZER` cache variable + `elseif` guidance | ✅ |
| 🟡 cmake-expert: ordering vs real CMakeLists.txt | FetchContent block now correctly placed before Sanitizer option, matching real file | ✅ |
### Remaining minor items (non-blocking)
**1. integrator.md line 110: inaccurate CI job description**
```
2. **sanitizer** — ASan + UBSan build and test (separate job)
```
The CI matrix only has `[address]`. UBSan is not in the matrix. Should read "ASan build and test" or "Sanitizer build and test (address)".
**2. cmake-expert.md: UBSan workaround section**
The manual `-DCMAKE_C_FLAGS="-fsanitize=undefined"` workaround is documented with the accurate caveat "no `-DSANITIZER=undefined` option in CMakeLists.txt yet". This will become stale when PR #25 lands (which adds UBSan to the SANITIZER option + CI matrix), but is correct for now.
---
**Verdict: Approve.** All previous critical items fixed. The two minor items above are non-blocking — the integrator.md inaccuracy is cosmetic (agents will still produce correct CI YAML from the example), and the UBSan workaround is correctly caveated.
TapTap
merged commit 51a984871c into main2026-07-20 14:44:36 +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.
Summary
Improves the agentic workflow configuration with consistent rules across all agents.
Changes
issue-creator(mode: subagent) — orchestrator that dispatches sub-agents and creates GitHub issuesfeature-scout— scans for feature opportunities (TODOs, hardcoded values, rsync gaps)security-screener— scans for vulnerabilities (buffer overflows, TLS, path traversal)code-quality-guardian— scans for quality issues (god functions, duplication, complexity)Review
CI is green, new agents are well-structured, integrator fix correct. One stale section needs attention.
🔴 cmake-expert.md — stale "When Adding Sanitizer Support" section contradicts correct pattern
The document has two contradictory sanitizer instructions:
Lines 148–163 (correct, "Sanitizer Configurations"):
Lines 165–191 (wrong, "When Adding Sanitizer Support to CMakeLists.txt"):
This is the old stale pattern that was already flagged in the PR #26 review. The real CMakeLists.txt uses a single
SANITIZERcache variable withelseifbranches — not three separateoption()booleans. An agent that reads the "When Adding Sanitizer Support" section will produce wrong CMake code.Fix: Replace lines 165–191 with instructions showing how to add a new value to the existing
SANITIZERcache variable (e.g., add anelseif(SANITIZER STREQUAL "undefined")block). Or remove the section entirely — the correct pattern is already shown above and in AGENTS.md.🟡 Dockerfile — lcov/valgrind addition conflicts with PR #27's AGENTS.md rule
PR #27's AGENTS.md says "never add
apt-get installto CI workflows — use the custom Docker image". PR #25 adds lcov/valgrind to the Dockerfile. This PR also adds them. Both PRs are correct individually, but whoever merges second should confirm the Dockerfile is clean.No action needed from this PR — just flagging the overlap. The lcov/valgrind addition in the Dockerfile is the right approach per AGENTS.md.
New agents — well-structured
gh issue create. Usesmode: subagentcorrectly.All three scanner agents have the correct "No findings" return format, severity guidelines, and the standard three footer sections (CI, Branch Strategy, Dependency Installation).
integrator.md — fix correct
The CI example now uses
container: gitea.tap-tap.win/taptap/fastsync-ci:v7instead ofsudo apt-get install. Correct per AGENTS.md dependency rules.Verdict: Approve with 1 fix needed. The stale
ENABLE_ASAN/ENABLE_TSAN/ENABLE_UBSANsection in cmake-expert.md should be updated or removed — it contradicts the correct pattern shown earlier in the same file.Re-review: Approve ✅ (2 minor items)
The critical cmake-expert.md fix is clean — the stale
ENABLE_ASAN/ENABLE_TSAN/ENABLE_UBSANsection is gone, replaced with correctSANITIZERcache variable documentation. All other changes since last review are the PR #26 merge (ci.yaml, client_cli.c, reviewer.md) which we already approved.Verified fixes
ENABLE_ASANpatternSANITIZERcache variable +elseifguidanceRemaining minor items (non-blocking)
1. integrator.md line 110: inaccurate CI job description
The CI matrix only has
[address]. UBSan is not in the matrix. Should read "ASan build and test" or "Sanitizer build and test (address)".2. cmake-expert.md: UBSan workaround section
The manual
-DCMAKE_C_FLAGS="-fsanitize=undefined"workaround is documented with the accurate caveat "no-DSANITIZER=undefinedoption in CMakeLists.txt yet". This will become stale when PR #25 lands (which adds UBSan to the SANITIZER option + CI matrix), but is correct for now.Verdict: Approve. All previous critical items fixed. The two minor items above are non-blocking — the integrator.md inaccuracy is cosmetic (agents will still produce correct CI YAML from the example), and the UBSan workaround is correctly caveated.