Copilot instructions

1---
2description: 'Code review instructions for Dragonfly'
3applyTo: '**'
4excludeAgent: ["coding-agent"]
5---
6 
7# Dragonfly Code Review Instructions
8 
9Dragonfly is a high-performance, Redis-compatible in-memory data store written in C++20 with a unique shared-nothing, fiber-based architecture. Code reviews must prioritize correctness, security, and architectural compliance specific to this threading model.
10 
11## Review Priorities
12 
13### 🔴 CRITICAL (Block merge immediately)
14 
15**Threading Model Violations** (causes deadlocks/crashes):
16- ❌ **NEVER** use `std::thread`, `std::mutex`, `std::condition_variable`, or standard library threading primitives
17- ✅ **ALWAYS** use fiber-aware equivalents: `util::fb2::Mutex`, `util::fb2::Fiber`, `util::fb2::CondVar` from `util/fibers/`
18 
19**Architecture Violations**:
20- ❌ Cross-shard data access without proper synchronization
21- ✅ Per-shard operations only (see `src/server/db_slice.cc` for patterns)
22 
23**Security Vulnerabilities**:
24- Authentication/authorization bypass in ACL code (`src/server/acl/`)
25- Exposed secrets, credentials in code or logs
26- Buffer overflows, use-after-free, memory safety issues
27 
28**Correctness Issues**:
29- Race conditions in fiber scheduling
30- Logic errors in transaction handling (`src/server/transaction.cc`)
31- Data corruption risks in DashTable operations (`src/core/dash.h`)
32 
33### 🟡 IMPORTANT (Requires discussion)
34 
35**Code Quality**:
36- Missing error handling (should return `OpStatus` from `facade/op_status.h`)
37- Obvious memory leaks (check ASAN reports)
38- Performance bottlenecks in hot paths (unnecessary allocations, N+1 patterns)
39 
40**Test Coverage**:
41- New features without tests (both C++ unit tests and Python integration tests)
42- Changes to critical paths (transactions, replication, cluster) without test coverage
43- Modified code that fails existing tests
44 
45**Style Violations** (severe only):
46- Not following naming conventions: `snake_case` variables, `PascalCase` functions, `kPascalCase` constants
47- Code that won't pass pre-commit hooks (clang-format, 100 char limit)
48 
49### 🟢 SUGGESTIONS (Non-blocking, comment only if obvious)
50 
51- Over-engineering: adding abstraction layers, feature flags, or configurability not requested
52- Missing comments on complex fiber synchronization logic
53- Premature optimization without profiling
54 
55## Dragonfly-Specific Patterns
56 
57### ✅ DO: Correct Patterns
58 
59**Threading & Synchronization**:
60```cpp
61// ✅ CORRECT: Fiber-aware mutex
62util::fb2::Mutex mutex_;
63std::lock_guard<util::fb2::Mutex> lock(mutex_);
64 
65// ✅ CORRECT: Fiber-aware operations
66util::fb2::Fiber fb = util::fb2::Fiber("name", [&] { /* work */ });
67```
68 
69 
70**Per-Shard Design**:
71```cpp
72// ✅ CORRECT: Operate on shard-local data
73void DbSlice::SomeOperation() {
74  // Access only this shard's data
75  auto& db_slice = cntx->ns->GetCurrentDbSlice();
76}
77```
78 
79### ❌ DON'T: Anti-Patterns
80 
81**Threading**:
82```cpp
83// ❌ WRONG: Standard library threading (causes deadlocks!)
84std::mutex mutex_;
85std::thread worker;
86std::condition_variable cv_;
87```
88 
89**Global State**:
90```cpp
91// ❌ WRONG: Global mutable state (breaks shared-nothing architecture)
92static std::unordered_map<string, int> global_cache;
93```
94 
95**Build Commands**:
96- ❌ Don't suggest `./tools/docker/build.sh` or `make` for incremental builds
97- ✅ Use `cd build-dbg && ninja <target>` instead
98 
99## Code Review Checklist
100 
101When reviewing Dragonfly code, verify:
102 
1031. **Architecture Compliance**:
104   - [ ] No standard library threading primitives (`std::thread`, `std::mutex`)
105   - [ ] No global mutable state
106   - [ ] Fiber-aware synchronization used correctly
107   - [ ] Follows per-shard, shared-nothing design
108 
1092. **Security**:
110   - [ ] No OWASP vulnerabilities (injection, XSS, auth bypass)
111   - [ ] No hardcoded secrets or credentials
112   - [ ] Input validation on command arguments
113   - [ ] Safe memory operations (no buffer overflows)
114 
1153. **Testing**:
116   - [ ] New functionality has test coverage
117   - [ ] Tests build and pass: `cd build-dbg && ninja <test> && ./<test>`
118   - [ ] No test regressions
119 
1204. **Style & Formatting**:
121   - [ ] Follows naming conventions (snake_case vars, PascalCase functions)
122   - [ ] Will pass pre-commit checks (clang-format, 100 char limit)
123   - [ ] Code compiles without warnings (CI uses `-Werror`)
124 
1255. **Helio Submodule**:
126   - [ ] No direct edits to `helio/` directory (it's a git submodule)
127 
128## Common False Positives to Ignore
129 
130These are **NOT** issues in Dragonfly's design. Do not comment on:
131 
1321. **Single-threaded-looking code**: Per-shard operations intentionally avoid locks
1332. **Custom allocators**: mimalloc is used intentionally for performance
1343. **Manual memory management**: Required for performance-critical paths
1354. **Complex template metaprogramming**: DashTable uses advanced C++20 features
1365. **Missing const**: Not always applicable in high-performance code
137 
138## Review Style Guidelines
139 
1401. **Be specific**: Reference file:line, explain WHY it's wrong
1412. **Show examples**: Demonstrate the correct pattern with code
1423. **Prioritize**: Security and correctness over style
1434. **Link to docs**: Reference `docs/df-share-nothing.md`, `docs/transaction.md`, etc.
1445. **Be concise**: Dragonfly team values focused, actionable feedback
145 
146## Example Review Comments
147 
148**❌ BAD - Too noisy**:
149> "Consider using auto here for type inference"
150 
151**✅ GOOD - Actionable and specific**:
152> "🔴 CRITICAL: Line 42 uses `std::mutex`. This will cause fiber deadlocks. Replace with `util::fb2::Mutex` from helio/util/fibers/. See src/server/set_family.cc:123 for correct pattern."
153 
154**✅ GOOD - Security focused**:
155> "🔴 SECURITY: Line 58 doesn't validate `user_input` before passing to eval(). Vulnerable to command injection. Add validation or use SafeEval()."
156 
157**✅ GOOD - Architecture violation**:
158> "🟡 ARCHITECTURE: Line 91 accesses global `cache_map`. Dragonfly uses shared-nothing design - each shard must have its own cache. See docs/df-share-nothing.md"
159 
160---
161 
162**Key Files Reference**: See AGENTS.md for complete codebase structure, build commands, and testing procedures.
163