docs: add AGENTS.md with custom-image dependency install rule #27

Merged

TapTap merged 3 commits from feat/custom-image-deps-rule into main 2026-07-19 22:57:16 +02:00

Conversation 4 Commits 3 Files Changed 4

+104 -34

TapTap commented 2026-07-19 20:44:48 +02:00

Owner

Copy Link

Copy Source

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.

## 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`.

TapTap added 1 commit 2026-07-19 20:44:48 +02:00

docs: add AGENTS.md with custom-image dependency install rule a7bb6454a4

TapTap referenced this pull request 2026-07-19 21:46:15 +02:00

testing: add comprehensive test suite for test-driven development #25

TapTap commented 2026-07-19 21:46:21 +02:00

Author

Owner

Copy Link

Copy Source

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.

## 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.

TapTap added 1 commit 2026-07-19 22:37:28 +02:00

fix: address PR review — lcov/valgrind in Dockerfile, correct cmake-expert.md, expand AGENTS.md 0f9e689e39

TapTap commented 2026-07-19 22:37:37 +02:00

Author

Owner

Copy Link

Copy Source

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

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

TapTap commented 2026-07-19 22:47:58 +02:00

Author

Owner

Copy Link

Copy Source

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.

## 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.

TapTap added 1 commit 2026-07-19 22:54:03 +02:00

fix: re-review cleanup — deps/conventions/sanitizer sections in cmake-expert, AGENTS.md notes 02ceb6828b

TapTap commented 2026-07-19 22:54:10 +02:00

Author

Owner

Copy Link

Copy Source

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.

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 main 2026-07-19 22:57:16 +02:00

TapTap deleted branch feat/custom-image-deps-rule 2026-07-19 22:57:22 +02:00

TapTap referenced this pull request 2026-07-20 14:40:54 +02:00

Improve agentic workflow — CI rules, branch strategy, deprecation rules, and new agents #28

TapTap referenced this pull request 2026-07-20 17:30:38 +02:00

ci: fix double CI runs on PR pushes #75

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

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TapTap/FastSync#27