Adds a canonical AGENTS.md at repo root with the rule: dependency installation must always use the project's custom Docker image (built from repo-root Dockerfile, same as CI's gitea.tap-tap.win/taptap/fastsync-ci:v7) — never ad-hoc host package installs. Includes the exact docker build / docker run commands plus build/test commands.
Adds a pointer to that rule in .opencode/agents/architect.md and .opencode/agents/cmake-expert.md.
Notes
The Dockerfile was audited against README build requirements and CI needs (gcc/g++, CMake 3.28, libzstd-dev, libssl-dev, openssh-client, cppcheck, clang-format, python3+pytest) — it is complete, no changes needed.
Docker is not available in the authoring environment, so the image build itself must be run where Docker exists; commands are documented in AGENTS.md.
## Summary
- Adds a canonical `AGENTS.md` at repo root with the rule: **dependency installation must always use the project's custom Docker image** (built from repo-root `Dockerfile`, same as CI's `gitea.tap-tap.win/taptap/fastsync-ci:v7`) — never ad-hoc host package installs. Includes the exact `docker build` / `docker run` commands plus build/test commands.
- Adds a pointer to that rule in `.opencode/agents/architect.md` and `.opencode/agents/cmake-expert.md`.
## Notes
- The Dockerfile was audited against README build requirements and CI needs (gcc/g++, CMake 3.28, libzstd-dev, libssl-dev, openssh-client, cppcheck, clang-format, python3+pytest) — it is complete, no changes needed.
- Docker is not available in the authoring environment, so the image build itself must be run where Docker exists; commands are documented in `AGENTS.md`.
Docs-only PR — so accuracy is the whole game. I audited every factual claim against origin/main, the Dockerfile, and the CI workflow: the facts hold up.
CI image tag correct — all three jobs on main (lint, build-and-test, sanitizers) use fastsync-ci:v7.
Test commands match main's reality — ./build/tests exists (add_executable(tests …)); python3 -m pytest tests/ is literally what main's CI runs (tests/integration collected via tests/conftest.py). Correctly avoids mentioning ctest (not on main yet).
Docker commands correct — single-quoted sh -c '… -j$(nproc)' so nproc evaluates inside the container; flags all sound.
Agent file edits clean — frontmatter intact in both files; the blockquote/bullet are well-placed and accurate.
Single focused commit, conventional message, no secrets. The transparency about Docker being unavailable in the authoring environment is commendable.
🟡 Warnings
Cross-PR contradiction with open PR #25 — #25's new coverage/valgrind jobs run apt-get install -y lcov / valgrindat workflow runtime; neither package is in the Dockerfile. If both PRs merge: CI will do exactly what AGENTS.md:20 forbids, and this PR's "Dockerfile is complete, no changes needed" audit conclusion stops being true. Nuance: the rule as written targets host/agent environments while #25 installs inside the CI container — a spirit-of-the-rule conflict, not letter-of-the-rule. Resolve by adding lcov+valgrind to the Dockerfile and bumping the image tag (whichever PR lands second), or explicitly scoping the rule.
Pre-existing stale lines in the touched cmake-expert.md (not introduced here, but this PR edits the file — cheap to fix now):
"Sanitizer support is commented out but present" (line 70, also 81) — false on main; CMakeLists.txt:12-23 has a live -DSANITIZER=address|thread|none option used by CI.
"CMake 4.1+" (lines 6, 16, 63) — main requires 3.22; README agrees. (The image ships 3.28 — 4.1 matches nothing.)
The embedded CMakeLists snippet (lines 15-49) has drifted — omits the SANITIZER/STRICT_WARNINGS options, xxHash FetchContent, OpenSSL. An agent trusting it would produce non-compiling edits.
🔵 Suggestions
Document running tests inside the container too (docker run … sh -c './build/tests && python3 -m pytest tests/') — under this PR's own rule the host may have no toolchain/pytest.
Mention CI configure conventions (-DSTRICT_WARNINGS=ON, -DSANITIZER=address) and that first configure needs network (FetchContent clones xxHash from GitHub).
The docker run example leaves root-owned build/ artifacts on the host — consider --user "$(id -u):$(id -g)" or a cleanup note. Also offer docker pull …/fastsync-ci:v7 as a faster alternative guaranteeing CI parity.
After #25 merges, AGENTS.md's commands remain correct, but consider later mentioning ctest and FASTSYNC_UNDER_VALGRIND=1.
README.md:157-167 still tells humans to sudo apt install on the host — a one-line "this rule governs automation; see README for manual setup" cross-reference would prevent confusion.
Verdict: No critical factual errors; every auditable claim verified. Approve — but please resolve the #25 contradiction (lcov/valgrind in the image) in whichever PR lands second, and consider sweeping up the stale cmake-expert.md lines since you're already touching that file.
## Review: Approve ✅ (with follow-ups)
Docs-only PR — so accuracy is the whole game. I audited every factual claim against `origin/main`, the `Dockerfile`, and the CI workflow: **the facts hold up**.
### ✅ Verified accurate
- **Dockerfile audit: 12/12 claimed tools present** — gcc, g++, make, cmake (Ubuntu 24.04 → CMake 3.28.3, matching the PR description), libzstd-dev, libssl-dev, git, cppcheck, clang-format, python3, pytest, openssh-client, Node.js (NodeSource 20.x).
- **CI image tag correct** — all three jobs on main (`lint`, `build-and-test`, `sanitizers`) use `fastsync-ci:v7`.
- **Test commands match main's reality** — `./build/tests` exists (`add_executable(tests …)`); `python3 -m pytest tests/` is literally what main's CI runs (tests/integration collected via tests/conftest.py). Correctly avoids mentioning ctest (not on main yet).
- **Docker commands correct** — single-quoted `sh -c '… -j$(nproc)'` so nproc evaluates inside the container; flags all sound.
- **Agent file edits clean** — frontmatter intact in both files; the blockquote/bullet are well-placed and accurate.
- Single focused commit, conventional message, no secrets. The transparency about Docker being unavailable in the authoring environment is commendable.
### 🟡 Warnings
1. **Cross-PR contradiction with open PR #25** — #25's new `coverage`/`valgrind` jobs run `apt-get install -y lcov` / `valgrind` **at workflow runtime**; neither package is in the Dockerfile. If both PRs merge: CI will do exactly what `AGENTS.md:20` forbids, and this PR's "Dockerfile is complete, no changes needed" audit conclusion stops being true. Nuance: the rule as written targets host/agent environments while #25 installs *inside* the CI container — a spirit-of-the-rule conflict, not letter-of-the-rule. Resolve by adding `lcov`+`valgrind` to the Dockerfile and bumping the image tag (whichever PR lands second), or explicitly scoping the rule.
2. **Pre-existing stale lines in the touched cmake-expert.md** (not introduced here, but this PR edits the file — cheap to fix now):
- "Sanitizer support is commented out but present" (line 70, also 81) — false on main; `CMakeLists.txt:12-23` has a live `-DSANITIZER=address|thread|none` option used by CI.
- "CMake 4.1+" (lines 6, 16, 63) — main requires **3.22**; README agrees. (The image ships 3.28 — 4.1 matches nothing.)
- The embedded CMakeLists snippet (lines 15-49) has drifted — omits the SANITIZER/STRICT_WARNINGS options, xxHash FetchContent, OpenSSL. An agent trusting it would produce non-compiling edits.
### 🔵 Suggestions
- Document running tests *inside* the container too (`docker run … sh -c './build/tests && python3 -m pytest tests/'`) — under this PR's own rule the host may have no toolchain/pytest.
- Mention CI configure conventions (`-DSTRICT_WARNINGS=ON`, `-DSANITIZER=address`) and that first configure needs network (FetchContent clones xxHash from GitHub).
- The `docker run` example leaves root-owned `build/` artifacts on the host — consider `--user "$(id -u):$(id -g)"` or a cleanup note. Also offer `docker pull …/fastsync-ci:v7` as a faster alternative guaranteeing CI parity.
- After #25 merges, AGENTS.md's commands remain correct, but consider later mentioning `ctest` and `FASTSYNC_UNDER_VALGRIND=1`.
- `README.md:157-167` still tells humans to `sudo apt install` on the host — a one-line "this rule governs automation; see README for manual setup" cross-reference would prevent confusion.
---
**Verdict:** No critical factual errors; every auditable claim verified. Approve — but please resolve the #25 contradiction (lcov/valgrind in the image) in whichever PR lands second, and consider sweeping up the stale cmake-expert.md lines since you're already touching that file.
Added xxHash FetchContent network note under the docker block
Added CI Conventions section with STRICT_WARNINGS and SANITIZER cmake flags
Remaining (out of scope for this PR, tracked for follow-up):
README.md sudo apt install cross-reference
ctest / FASTSYNC_UNDER_VALGRIND mentions after #25 merges
Thanks for the thorough review! All points have been addressed in commit 0f9e689:
**⚠️ Warnings resolved:**
- **lcov + valgrind** added to the Dockerfile — the #25 contradiction should be resolved when either PR lands.
- **cmake-expert.md** stale lines fixed:
- CMake version: 4.1+ → 3.22+ (in description, embedded snippet, and deps list)
- Sanitizer claim: "commented out" → live `-DSANITIZER=address|thread` option (both in Conventions and item #6)
- Embedded CMakeLists snippet now includes SANITIZER, STRICT_WARNINGS, xxHash FetchContent, OpenSSL, and correct link libs
**💡 Suggestions incorporated:**
- Docker commands now run `./build/tests && python3 -m pytest tests/` inside the container
- Added `docker pull` alternative for faster CI-parity setup
- Added `--user "$(id -u):$(id -g)"` variant to avoid root-owned build artifacts
- Added xxHash FetchContent network note under the docker block
- Added CI Conventions section with STRICT_WARNINGS and SANITIZER cmake flags
**Remaining (out of scope for this PR, tracked for follow-up):**
- README.md `sudo apt install` cross-reference
- ctest / FASTSYNC_UNDER_VALGRIND mentions after #25 merges
cmake-expert: stale CMake snippet (xxHash, OpenSSL, SANITIZER missing)
✅ Rewritten with all options
cmake-expert: "sanitizer commented out" → live option
✅ Fixed
AGENTS.md: only docker build, no pull alternative
✅ Added docker pull
AGENTS.md: root-owned artifacts
✅ Added --user variant
AGENTS.md: no CI conventions
✅ New CI Conventions section
AGENTS.md: no FetchContent network note
✅ Added
AGENTS.md: separate build/test commands
✅ Single docker run covering all three
🟡 Remaining warnings (docs gaps, not blockers)
cmake-expert.md Dependencies bullet list (lines 86-90) — missing OpenSSL and xxHash entries; the snippet shows them but the summary list doesn't.
Conventions bullet (line 86) — still says All targets link Threads::Threads and ${ZSTD_LIBRARY}; omits OpenSSL::SSL, OpenSSL::Crypto, and xxhash (present in the snippet and in origin/main:CMakeLists.txt).
CMake snippet missing SANITIZER validation — the real CMakeLists errors on unknown values (elseif(NOT SANITIZER STREQUAL "none")); the snippet silently accepts garbage.
Sanitizer Configurations section (lines 101-133) still shows the old -DCMAKE_C_FLAGS="-fsanitize=address ..." manual approach — inconsistent with the -DSANITIZER= option documented everywhere else in the file.
AGENTS.md says docker pull …/fastsync-ci:v7 — the registry :v7 was built before this PR's Dockerfile update and lacks lcov/valgrind. Image must be rebuilt post-merge. Also, CI's ci.yaml references :v7 — will need a tag bump or rebuild sync.
🔵 Suggestions
CI Conventions section lists both address and thread for parity — CI only runs address; note that.
The zstd error message in the snippet is slightly less helpful than the real CMakeLists's mention of nix-shell.
Verdict: Approve — No critical factual errors. The warnings are documentation consistency gaps (cmake-expert conventions/sanitizer sections), not blockers. The :v7 registry image must be rebuilt post-merge to pick up lcov/valgrind.
## Re-review: Approve ✅ (with follow-ups)
Reviewed the updated PR at commit `0f9e689`. All previously identified issues are addressed:
### ✅ What's Fixed
| Previous issue | Status |
|---|---|
| Dockerfile missing lcov/valgrind (contradicts #25) | ✅ Added |
| cmake-expert: CMake 4.1 → 3.22 | ✅ Corrected |
| cmake-expert: stale CMake snippet (xxHash, OpenSSL, SANITIZER missing) | ✅ Rewritten with all options |
| cmake-expert: "sanitizer commented out" → live option | ✅ Fixed |
| AGENTS.md: only `docker build`, no pull alternative | ✅ Added `docker pull` |
| AGENTS.md: root-owned artifacts | ✅ Added `--user` variant |
| AGENTS.md: no CI conventions | ✅ New CI Conventions section |
| AGENTS.md: no FetchContent network note | ✅ Added |
| AGENTS.md: separate build/test commands | ✅ Single `docker run` covering all three |
### 🟡 Remaining warnings (docs gaps, not blockers)
1. **cmake-expert.md Dependencies bullet list** (lines 86-90) — missing OpenSSL and xxHash entries; the snippet shows them but the summary list doesn't.
2. **Conventions bullet** (line 86) — still says `All targets link Threads::Threads and ${ZSTD_LIBRARY}`; omits `OpenSSL::SSL`, `OpenSSL::Crypto`, and `xxhash` (present in the snippet and in `origin/main:CMakeLists.txt`).
3. **CMake snippet missing SANITIZER validation** — the real CMakeLists errors on unknown values (`elseif(NOT SANITIZER STREQUAL "none")`); the snippet silently accepts garbage.
4. **Sanitizer Configurations section** (lines 101-133) still shows the old `-DCMAKE_C_FLAGS="-fsanitize=address ..."` manual approach — inconsistent with the `-DSANITIZER=` option documented everywhere else in the file.
5. **`AGENTS.md` says `docker pull …/fastsync-ci:v7`** — the registry `:v7` was built before this PR's Dockerfile update and lacks lcov/valgrind. Image must be rebuilt post-merge. Also, CI's `ci.yaml` references `:v7` — will need a tag bump or rebuild sync.
### 🔵 Suggestions
- CI Conventions section lists both `address` and `thread` for parity — CI only runs `address`; note that.
- The zstd error message in the snippet is slightly less helpful than the real CMakeLists's mention of `nix-shell`.
---
**Verdict: Approve** — No critical factual errors. The warnings are documentation consistency gaps (cmake-expert conventions/sanitizer sections), not blockers. The `:v7` registry image must be rebuilt post-merge to pick up lcov/valgrind.
Sanitizer Configurations — replaced old manual -DCMAKE_C_FLAGS= sections with -DSANITIZER=address|thread (kept UBSan manual only since no option exists yet).
Stale :v7 image — AGENTS.md now notes the prebuilt image reflects the previous Dockerfile state; rebuild from source for newly added packages.
💡 Suggestions:
CI Conventions now describes sanitizer as "currently only address".
zstd error message matches nix-shell hint from real CMakeLists.txt.
The :v7 CI image must be rebuilt post-merge — that's a follow-up CI ops task.
Re-review items addressed in commit 02ceb68:
**🟡 Warnings resolved:**
1. Dependencies list — added OpenSSL and xxhash entries.
2. Conventions bullet — now lists all five link libraries (`Threads::Threads`, `${ZSTD_LIBRARY}`, `OpenSSL::SSL`, `OpenSSL::Crypto`, `xxhash`).
3. SANITIZER validation — added `elseif(NOT SANITIZER STREQUAL "none")` error check matching real CMakeLists.txt.
4. Sanitizer Configurations — replaced old manual `-DCMAKE_C_FLAGS=` sections with `-DSANITIZER=address|thread` (kept UBSan manual only since no option exists yet).
5. Stale `:v7` image — AGENTS.md now notes the prebuilt image reflects the previous Dockerfile state; rebuild from source for newly added packages.
**💡 Suggestions:**
- CI Conventions now describes sanitizer as "currently only `address`".
- zstd error message matches nix-shell hint from real CMakeLists.txt.
The `:v7` CI image must be rebuilt post-merge — that's a follow-up CI ops task.
TapTap
merged commit a4b35e136b into main2026-07-19 22:57:16 +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
AGENTS.mdat repo root with the rule: dependency installation must always use the project's custom Docker image (built from repo-rootDockerfile, same as CI'sgitea.tap-tap.win/taptap/fastsync-ci:v7) — never ad-hoc host package installs. Includes the exactdocker build/docker runcommands plus build/test commands..opencode/agents/architect.mdand.opencode/agents/cmake-expert.md.Notes
AGENTS.md.Review: Approve ✅ (with follow-ups)
Docs-only PR — so accuracy is the whole game. I audited every factual claim against
origin/main, theDockerfile, and the CI workflow: the facts hold up.✅ Verified accurate
lint,build-and-test,sanitizers) usefastsync-ci:v7../build/testsexists (add_executable(tests …));python3 -m pytest tests/is literally what main's CI runs (tests/integration collected via tests/conftest.py). Correctly avoids mentioning ctest (not on main yet).sh -c '… -j$(nproc)'so nproc evaluates inside the container; flags all sound.🟡 Warnings
coverage/valgrindjobs runapt-get install -y lcov/valgrindat workflow runtime; neither package is in the Dockerfile. If both PRs merge: CI will do exactly whatAGENTS.md:20forbids, and this PR's "Dockerfile is complete, no changes needed" audit conclusion stops being true. Nuance: the rule as written targets host/agent environments while #25 installs inside the CI container — a spirit-of-the-rule conflict, not letter-of-the-rule. Resolve by addinglcov+valgrindto the Dockerfile and bumping the image tag (whichever PR lands second), or explicitly scoping the rule.CMakeLists.txt:12-23has a live-DSANITIZER=address|thread|noneoption used by CI.🔵 Suggestions
docker run … sh -c './build/tests && python3 -m pytest tests/') — under this PR's own rule the host may have no toolchain/pytest.-DSTRICT_WARNINGS=ON,-DSANITIZER=address) and that first configure needs network (FetchContent clones xxHash from GitHub).docker runexample leaves root-ownedbuild/artifacts on the host — consider--user "$(id -u):$(id -g)"or a cleanup note. Also offerdocker pull …/fastsync-ci:v7as a faster alternative guaranteeing CI parity.ctestandFASTSYNC_UNDER_VALGRIND=1.README.md:157-167still tells humans tosudo apt installon the host — a one-line "this rule governs automation; see README for manual setup" cross-reference would prevent confusion.Verdict: No critical factual errors; every auditable claim verified. Approve — but please resolve the #25 contradiction (lcov/valgrind in the image) in whichever PR lands second, and consider sweeping up the stale cmake-expert.md lines since you're already touching that file.
Thanks for the thorough review! All points have been addressed in commit
0f9e689:⚠️ Warnings resolved:
-DSANITIZER=address|threadoption (both in Conventions and item #6)💡 Suggestions incorporated:
./build/tests && python3 -m pytest tests/inside the containerdocker pullalternative for faster CI-parity setup--user "$(id -u):$(id -g)"variant to avoid root-owned build artifactsRemaining (out of scope for this PR, tracked for follow-up):
sudo apt installcross-referenceRe-review: Approve ✅ (with follow-ups)
Reviewed the updated PR at commit
0f9e689. All previously identified issues are addressed:✅ What's Fixed
docker build, no pull alternativedocker pull--uservariantdocker runcovering all three🟡 Remaining warnings (docs gaps, not blockers)
All targets link Threads::Threads and ${ZSTD_LIBRARY}; omitsOpenSSL::SSL,OpenSSL::Crypto, andxxhash(present in the snippet and inorigin/main:CMakeLists.txt).elseif(NOT SANITIZER STREQUAL "none")); the snippet silently accepts garbage.-DCMAKE_C_FLAGS="-fsanitize=address ..."manual approach — inconsistent with the-DSANITIZER=option documented everywhere else in the file.AGENTS.mdsaysdocker pull …/fastsync-ci:v7— the registry:v7was built before this PR's Dockerfile update and lacks lcov/valgrind. Image must be rebuilt post-merge. Also, CI'sci.yamlreferences:v7— will need a tag bump or rebuild sync.🔵 Suggestions
addressandthreadfor parity — CI only runsaddress; note that.nix-shell.Verdict: Approve — No critical factual errors. The warnings are documentation consistency gaps (cmake-expert conventions/sanitizer sections), not blockers. The
:v7registry image must be rebuilt post-merge to pick up lcov/valgrind.Re-review items addressed in commit
02ceb68:🟡 Warnings resolved:
Threads::Threads,${ZSTD_LIBRARY},OpenSSL::SSL,OpenSSL::Crypto,xxhash).elseif(NOT SANITIZER STREQUAL "none")error check matching real CMakeLists.txt.-DCMAKE_C_FLAGS=sections with-DSANITIZER=address|thread(kept UBSan manual only since no option exists yet).:v7image — AGENTS.md now notes the prebuilt image reflects the previous Dockerfile state; rebuild from source for newly added packages.💡 Suggestions:
address".The
:v7CI image must be rebuilt post-merge — that's a follow-up CI ops task.