Improve AGENTS.md with comprehensive development guide by soloestoy · Pull Request #3428 · valkey-io/valkey

soloestoy · 2026-04-01T06:34:40Z

No description provided.

Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>

hpatro

Overall LGTM. I feel the source code layout would be difficult to maintain. So, if it's not that helpful in your experience, I would prefer leaving it out.

hpatro · 2026-04-02T17:56:28Z

AGENTS.md

+|---|---|
+| Build fails with jemalloc errors | `make distclean && make` |
+| `commands.def` out of sync | `python utils/generate-command-code.py` |
+| `clang-format` CI check fails | `clang-format-18 -i <modified files>` |


hpatro · 2026-04-02T17:56:46Z

AGENTS.md

+- Never push directly to the `unstable` branch.
+- If your fork does not exist, create one before pushing.
+- All commits must include a DCO sign-off: `git commit -s`
+- For large features, open an Issue for discussion before submitting a PR.


Suggested change

- For large features, open an Issue for discussion before submitting a PR.

- For large features, open an issue for discussion before submitting a PR.

hpatro · 2026-04-02T17:58:05Z

AGENTS.md

+
+- Follow conventions in `DEVELOPMENT_GUIDE.md`.
+- CI enforces `clang-format-18` on `*.c`, `*.h`, `*.cpp`, `*.hpp`.
+- After modifying C/C++ files, run: `clang-format-18 -i <modified files>`


Let's use clang-format, I believe the symlink should exist to appropriate version.

Suggested change

- After modifying C/C++ files, run: `clang-format-18 -i <modified files>`

- After modifying C/C++ files, run: `clang-format -i <modified files>`

hpatro · 2026-04-02T17:59:34Z

AGENTS.md

+make -j4 BUILD_TLS=yes             # With TLS support
+make -j4 SANITIZER=address         # With AddressSanitizer


Shall we provide similar $(nproc) suggestion like you have done below for CMake ?

Suggested change

make -j4 BUILD_TLS=yes # With TLS support

make -j4 SANITIZER=address # With AddressSanitizer

make -j$(nproc) BUILD_TLS=yes # With TLS support

make -j$(nproc) SANITIZER=address # With AddressSanitizer

hpatro · 2026-04-02T18:00:47Z

AGENTS.md

+## Source code layout
+
+| Module | Key files | Description |
+|---|---|---|
+| Server core | server.c, server.h | Main loop, initialization, global state |
+| Networking | networking.c, anet.c, socket.c, connection.c, io_threads.c | TCP, connection management, multi-threaded I/O |
+| Event loop | ae.c, ae_epoll.c, ae_kqueue.c | Event-driven engine (epoll/kqueue backends) |
+| Data structures | dict.c, hashtable.c, sds.c, quicklist.c, listpack.c, intset.c, rax.c | Internal data structures |
+| Data type commands | t_string.c, t_hash.c, t_list.c, t_set.c, t_zset.c, t_stream.c | Per-type command implementations |
+| Persistence | rdb.c, aof.c | RDB snapshots and AOF journaling |
+| Replication | replication.c | Primary/replica replication |
+| Cluster | cluster.c, cluster_legacy.c, cluster_migrateslots.c | Cluster mode |
+| Sentinel | sentinel.c | High-availability sentinel |
+| Scripting | eval.c, script.c, scripting_engine.c | Lua scripting |
+| Modules | module.c, module.h | Module system |
+| Memory | zmalloc.c, defrag.c, lazyfree.c, allocator_defrag.c | Memory management |
+| Security | acl.c, tls.c | ACL and TLS |


Does this help ? Generally the agents are able to discover the right files for me.

madolson

My opinion is effectively restating https://www.philschmid.de/writing-good-agents, which tries to summarize the best practices from https://arxiv.org/abs/2602.11988. Less is more for AGENTS.md files. So mostly suggesting removing things.

madolson · 2026-04-02T19:12:43Z

AGENTS.md

+- Third-party dependencies in `deps/`: jemalloc, libvalkey, lua, linenoise, hdr_histogram, fast_float
+- Command definitions: `src/commands/*.json` (427+ entries in JSON format)
+
+## Source code layout


I don't think there is strong evidence that layouts help agents. Would consider removing this.

madolson · 2026-04-02T19:13:52Z

AGENTS.md

+- Comments should explain non-obvious behavior and rationale, not restate the code
+- Functions should have documentation comments
+
+**License header** — all new files must include:


Suggested change

**License header** — all new files must include:

**License header** — all new code files must include:

Conversely, should we add a style checker to enforce this?

madolson · 2026-04-02T19:14:45Z

AGENTS.md

+make valgrind                       # Valgrind-compatible build
+```
+
+**CMake build (alternative):**


Why alternatives? I think the research says you should be prescriptive to a single approach

madolson · 2026-04-02T19:15:46Z

AGENTS.md

+- Integration tests: `tests/` (Tcl)
+- Top-level `Makefile` forwards most targets into `src/Makefile`
+- Third-party dependencies in `deps/`: jemalloc, libvalkey, lua, linenoise, hdr_histogram, fast_float
+- Command definitions: `src/commands/*.json` (427+ entries in JSON format)


Suggested change

- Command definitions: `src/commands/*.json` (427+ entries in JSON format)

- Command definitions: `src/commands/*.json`

madolson · 2026-04-02T19:17:54Z

AGENTS.md

-Always push to the user's fork. Never push to the upstream valkey-io/valkey repository. Never push directly to unstable. If a user fork does not exist, ask the contributor to create one.
+
+- Always push to your personal fork. **Never push directly to `valkey-io/valkey`.**
+- Never push directly to the `unstable` branch.


You can't push to the unstable branch, it has branch protections.

Suggested change

- Never push directly to the `unstable` branch.

Helps avoiding it on the fork where the branch protection isn't active for everyone.

That's not a valkey specific thing though? Again, my opinion is we should include as little as possible that agents can't infer. Agents can infer that you shouldn't push to default branch.

Actually, I now wonder if we need the "never push directly to valkey-io" either, you can't push there either.

Improve AGENTS.md with comprehensive development guide

e0b4f5b

Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>

github-actions bot assigned soloestoy Apr 1, 2026

soloestoy requested review from hpatro and madolson April 1, 2026 09:16

hpatro reviewed Apr 2, 2026

View reviewed changes

madolson reviewed Apr 2, 2026

View reviewed changes

	\| `clang-format` CI check fails \| `clang-format-18 -i <modified files>` \|
	\| `clang-format` CI check fails \| `clang-format -i <modified files>` \|

	- For large features, open an Issue for discussion before submitting a PR.
	- For large features, open an issue for discussion before submitting a PR.

	- After modifying C/C++ files, run: `clang-format-18 -i <modified files>`
	- After modifying C/C++ files, run: `clang-format -i <modified files>`

		make -j4 BUILD_TLS=yes # With TLS support
		make -j4 SANITIZER=address # With AddressSanitizer

	License header — all new files must include:
	License header — all new code files must include:

	- Command definitions: `src/commands/*.json` (427+ entries in JSON format)
	- Command definitions: `src/commands/*.json`

Conversation

soloestoy commented Apr 1, 2026

Uh oh!

hpatro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants