Add AI assistant documentation and commands (#8785)

* Add AI assistant documentation and commands Adds structured documentation for AI coding assistants: - CLAUDE.md / AGENTS.md: Lightweight entry points with critical rules - .ai/: Shared knowledge base (CODE_REVIEW.md, DEVELOPMENT.md, ISSUES.md) - .claude/commands/: Claude Code skills for review, issue, release - .github/copilot-instructions.md: GitHub Copilot instructions Supports Claude Code, OpenAI Codex, and GitHub Copilot with modular, pointer-based structure for maintainability. Includes guidelines for AI assistants to prompt developers about updating these docs after receiving feedback, creating a continuous improvement loop. * Add parallel development tip with git worktrees * Address review feedback - Add missing details to DEVELOPMENT.md: fork-specific testing, database backends, cross-compilation targets, make test-release - Simplify AGENTS.md to pointer to CLAUDE.md (Codex can read files) * Address review feedback - Add priority signaling: Critical vs Important vs Good Practices - Restore actionable file references (canonical_head.rs, test_utils.rs, etc.) - Add Rayon CPU oversubscription context - Add tracing span guidelines - Simplify AGENTS.md to pointer * Address review feedback and remove Copilot instructions - Restore anti-patterns section (over-engineering, unnecessary complexity) - Restore design principles (simplicity first, high cohesion) - Add architecture guidance (dependency bloat, schema migrations, backwards compat) - Improve natural language guidance for AI comments - Add try_read lock pattern - Remove copilot-instructions.md (can't follow file refs, untestable)
2026-07-04 05:14:33 +00:00 · 2026-02-10 17:41:57 +11:00
parent 7e275f8dc2
commit 8948159a40
8 changed files with 930 additions and 303 deletions
--- a/.ai/CODE_REVIEW.md
+++ b/.ai/CODE_REVIEW.md
@@ -0,0 +1,277 @@
+# Lighthouse Code Review Guidelines
+
+Code review guidelines based on patterns from Lighthouse maintainers.
+
+## Core Principles
+
+- **Correctness** over clever code
+- **Clarity** through good documentation and naming
+- **Safety** through proper error handling and panic avoidance
+- **Maintainability** for long-term health
+
+## Critical: Consensus Crate (`consensus/` excluding `types/`)
+
+**Extra scrutiny required** - bugs here cause consensus failures.
+
+### Requirements
+
+1. **Safe Math Only**
+   ```rust
+   // NEVER
+   let result = a + b;
+
+   // ALWAYS
+   let result = a.saturating_add(b);
+   // or use safe_arith crate
+   let result = a.safe_add(b)?;
+   ```
+
+2. **Zero Panics**
+   - No `.unwrap()`, `.expect()`, array indexing `[i]`
+   - Return `Result` or `Option` instead
+
+3. **Deterministic Behavior**
+   - Identical results across all platforms
+   - No undefined behavior
+
+## Panic Avoidance (All Code)
+
+```rust
+// NEVER at runtime
+let value = option.unwrap();
+let item = array[1];
+
+// ALWAYS
+let value = option.ok_or(Error::Missing)?;
+let item = array.get(1)?;
+
+// Only acceptable during startup for CLI/config validation
+let flag = matches.get_one::<String>("flag")
+    .expect("Required due to clap validation");
+```
+
+## Code Clarity
+
+### Variable Naming
+```rust
+// BAD - ambiguous
+let bb = ...;
+let bl = ...;
+
+// GOOD - clear
+let beacon_block = ...;
+let blob = ...;
+```
+
+### Comments
+- Explain the "why" not just the "what"
+- All `TODO` comments must link to a GitHub issue
+- Remove dead/commented-out code
+
+## Error Handling
+
+### Don't Silently Swallow Errors
+```rust
+// BAD
+self.store.get_info().unwrap_or(None)
+
+// GOOD
+self.store.get_info().unwrap_or_else(|e| {
+    error!(self.log, "Failed to read info"; "error" => ?e);
+    None
+})
+```
+
+### Check Return Values
+Ask: "What happens if this returns `Ok(Failed)`?" Don't ignore results that might indicate failure.
+
+## Performance & Concurrency
+
+### Lock Safety
+- Document lock ordering requirements
+- Keep lock scopes narrow
+- Seek detailed review for lock-related changes
+- Use `try_read` when falling back to an alternative is acceptable
+- Use blocking `read` when alternative is more expensive (e.g., state reconstruction)
+
+### Async Patterns
+```rust
+// NEVER block in async context
+async fn handler() {
+    expensive_computation(); // blocks runtime
+}
+
+// ALWAYS spawn blocking
+async fn handler() {
+    tokio::task::spawn_blocking(|| expensive_computation()).await?;
+}
+```
+
+### Rayon
+- Use scoped rayon pools from beacon processor
+- Avoid global thread pool (causes CPU oversubscription)
+
+## Review Process
+
+### Focus on Actionable Issues
+
+**Limit to 3-5 key comments.** Prioritize:
+1. Correctness issues - bugs, race conditions, panics
+2. Missing test coverage - especially edge cases
+3. Complex logic needing documentation
+4. API design concerns
+
+**Don't comment on:**
+- Minor style issues
+- Things caught by CI (formatting, linting)
+- Nice-to-haves that aren't important
+
+### Keep Comments Natural and Minimal
+
+**Tone**: Natural and conversational, not robotic.
+
+**Good review comment:**
+```
+Missing test coverage for the None blobs path. The existing test at
+`store_tests.rs:2874` still provides blobs. Should add a test passing
+None to verify backfill handles this correctly.
+```
+
+**Good follow-up after author addresses comments:**
+```
+LGTM, thanks!
+```
+or
+```
+Thanks for the updates, looks good!
+```
+
+**Avoid:**
+- Checklists or structured formatting (✅ Item 1 fixed...)
+- Repeating what was fixed (makes it obvious it's AI-generated)
+- Headers, subsections, "Summary" sections
+- Verbose multi-paragraph explanations
+
+### Use Natural Language
+
+```
+BAD (prescriptive):
+"This violates coding standards which strictly prohibit runtime panics."
+
+GOOD (conversational):
+"Should we avoid `.expect()` here? This gets called in hot paths and
+we typically try to avoid runtime panics outside of startup."
+```
+
+### Verify Before Commenting
+
+- If CI passes, trust it - types/imports must exist
+- Check the full diff, not just visible parts
+- Ask for verification rather than asserting things are missing
+
+## Common Review Patterns
+
+### Fork-Specific Changes
+- Verify production fork code path unchanged
+- Check SSZ compatibility (field order)
+- Verify rollback/error paths handle edge cases
+
+### API Design
+- Constructor signatures should be consistent
+- Avoid `Option` parameters when value is always required
+
+### Concurrency
+- Lock ordering documented?
+- Potential deadlocks?
+- Race conditions?
+
+### Error Handling
+- Errors logged?
+- Edge cases handled?
+- Context provided with errors?
+
+## Deep Review Techniques
+
+### Verify Against Specifications
+- Read the actual spec in `./consensus-specs/`
+- Compare formulas exactly
+- Check constant values match spec definitions
+
+### Trace Data Flow End-to-End
+For new config fields:
+1. Config file - Does YAML contain the field?
+2. Config struct - Is it parsed with serde attributes?
+3. apply_to_chain_spec - Is it actually applied?
+4. Runtime usage - Used correctly everywhere?
+
+### Check Error Handling Fallbacks
+Examine every `.unwrap_or()`, `.unwrap_or_else()`:
+- If the fallback triggers, does code behave correctly?
+- Does it silently degrade or fail loudly?
+
+### Look for Incomplete Migrations
+When a PR changes a pattern across the codebase:
+- Search for old pattern - all occurrences updated?
+- Check test files - often lag behind implementation
+
+## Architecture & Design
+
+### Avoid Dependency Bloat
+- Question whether imports add unnecessary dependencies
+- Consider feature flags for optional functionality
+- Large imports when only primitives are needed may warrant a `core` or `primitives` feature
+
+### Schema Migrations
+- Database schema changes require migrations
+- Don't forget to add migration code when changing stored types
+- Review pattern: "Needs a schema migration"
+
+### Backwards Compatibility
+- Consider existing users when changing behavior
+- Document breaking changes clearly
+- Prefer additive changes when possible
+
+## Anti-Patterns to Avoid
+
+### Over-Engineering
+- Don't add abstractions until needed
+- Keep solutions simple and focused
+- "Three similar lines of code is better than a premature abstraction"
+
+### Unnecessary Complexity
+- Avoid feature flags for simple changes
+- Don't add fallbacks for scenarios that can't happen
+- Trust internal code and framework guarantees
+
+### Premature Optimization
+- Optimize hot paths based on profiling, not assumptions
+- Document performance considerations but don't over-optimize
+
+### Hiding Important Information
+- Don't use generic variable names when specific ones are clearer
+- Don't skip logging just to keep code shorter
+- Don't omit error context
+
+## Design Principles
+
+### Simplicity First
+Question every layer of abstraction:
+- Is this `Arc` needed, or is the inner type already `Clone`?
+- Is this `Mutex` needed, or can ownership be restructured?
+- Is this wrapper type adding value or just indirection?
+
+If you can't articulate why a layer of abstraction exists, it probably shouldn't.
+
+### High Cohesion
+Group related state and behavior together. If two fields are always set together, used together, and invalid without each other, they belong in a struct.
+
+## Before Approval Checklist
+
+- [ ] No panics: No `.unwrap()`, `.expect()`, unchecked array indexing
+- [ ] Consensus safe: If touching consensus crate, all arithmetic is safe
+- [ ] Errors logged: Not silently swallowed
+- [ ] Clear naming: Variable names are unambiguous
+- [ ] TODOs linked: All TODOs have GitHub issue links
+- [ ] Tests present: Non-trivial changes have tests
+- [ ] Lock safety: Lock ordering is safe and documented
+- [ ] No blocking: Async code doesn't block runtime
--- a/.ai/DEVELOPMENT.md
+++ b/.ai/DEVELOPMENT.md
@@ -0,0 +1,200 @@
+# Lighthouse Development Guide
+
+Development patterns, commands, and architecture for AI assistants and contributors.
+
+## Development Commands
+
+**Important**: Always branch from `unstable` and target `unstable` when creating pull requests.
+
+### Building
+
+- `make install` - Build and install Lighthouse in release mode
+- `make install-lcli` - Build and install `lcli` utility
+- `cargo build --release` - Standard release build
+- `cargo build --bin lighthouse --features "gnosis,slasher-lmdb"` - Build with specific features
+
+### Testing
+
+- `make test` - Full test suite in release mode
+- `make test-release` - Run tests using nextest (faster parallel runner)
+- `cargo nextest run -p <package>` - Run tests for specific package (preferred for iteration)
+- `cargo nextest run -p <package> <test_name>` - Run individual test
+- `FORK_NAME=electra cargo nextest run -p beacon_chain` - Run tests for specific fork
+- `make test-ef` - Ethereum Foundation test vectors
+
+**Fork-specific testing**: `beacon_chain` and `http_api` tests support fork-specific testing via `FORK_NAME` env var when `beacon_chain/fork_from_env` feature is enabled.
+
+**Note**: Full test suite takes ~20 minutes. Prefer targeted tests when iterating.
+
+### Linting
+
+- `make lint` - Run Clippy with project rules
+- `make lint-full` - Comprehensive linting including tests
+- `cargo fmt --all && make lint-fix` - Format and fix linting issues
+- `cargo sort` - Sort dependencies (enforced on CI)
+
+## Architecture Overview
+
+Lighthouse is a modular Ethereum consensus client with two main components:
+
+### Beacon Node (`beacon_node/`)
+
+- Main consensus client syncing with Ethereum network
+- Beacon chain state transition logic (`beacon_node/beacon_chain/`)
+- Networking, storage, P2P communication
+- HTTP API for validator clients
+- Entry point: `beacon_node/src/lib.rs`
+
+### Validator Client (`validator_client/`)
+
+- Manages validator keystores and duties
+- Block proposals, attestations, sync committee duties
+- Slashing protection and doppelganger detection
+- Entry point: `validator_client/src/lib.rs`
+
+### Key Subsystems
+
+| Subsystem | Location | Purpose |
+|-----------|----------|---------|
+| Consensus Types | `consensus/types/` | Core data structures, SSZ encoding |
+| Storage | `beacon_node/store/` | Hot/cold database (LevelDB, RocksDB, REDB backends) |
+| Networking | `beacon_node/lighthouse_network/` | Libp2p, gossipsub, discovery |
+| Fork Choice | `consensus/fork_choice/` | Proto-array fork choice |
+| Execution Layer | `beacon_node/execution_layer/` | EL client integration |
+| Slasher | `slasher/` | Optional slashing detection |
+
+### Utilities
+
+- `account_manager/` - Validator account management
+- `lcli/` - Command-line debugging utilities
+- `database_manager/` - Database maintenance tools
+
+## Code Quality Standards
+
+### Panic Avoidance (Critical)
+
+**Panics should be avoided at all costs.**
+
+```rust
+// NEVER at runtime
+let value = some_result.unwrap();
+let item = array[1];
+
+// ALWAYS prefer
+let value = some_result?;
+let item = array.get(1)?;
+
+// Only acceptable during startup
+let config = matches.get_one::<String>("flag")
+    .expect("Required due to clap validation");
+```
+
+### Consensus Crate Safety (`consensus/` excluding `types/`)
+
+Extra scrutiny required - bugs here cause consensus failures.
+
+```rust
+// NEVER standard arithmetic
+let result = a + b;
+
+// ALWAYS safe math
+let result = a.saturating_add(b);
+// or
+use safe_arith::SafeArith;
+let result = a.safe_add(b)?;
+```
+
+Requirements:
+- Use `saturating_*` or `checked_*` operations
+- Zero panics - no `.unwrap()`, `.expect()`, or `array[i]`
+- Deterministic behavior across all platforms
+
+### Error Handling
+
+- Return `Result` or `Option` instead of panicking
+- Log errors, don't silently swallow them
+- Provide context with errors
+
+### Async Patterns
+
+```rust
+// NEVER block in async context
+async fn handler() {
+    expensive_computation(); // blocks runtime
+}
+
+// ALWAYS spawn blocking
+async fn handler() {
+    tokio::task::spawn_blocking(|| expensive_computation()).await?;
+}
+```
+
+### Concurrency
+
+- **Lock ordering**: Document lock ordering to avoid deadlocks. See [`canonical_head.rs:9-32`](beacon_node/beacon_chain/src/canonical_head.rs) for excellent example documenting three locks and safe acquisition order.
+- Keep lock scopes narrow
+- Seek detailed review for lock-related changes
+
+### Rayon Thread Pools
+
+Avoid using the rayon global thread pool - it causes CPU oversubscription when beacon processor has fully allocated all CPUs to workers. Use scoped rayon pools started by beacon processor for computationally intensive tasks.
+
+### Tracing Spans
+
+- Avoid spans on simple getter methods (performance overhead)
+- Be cautious of span explosion with recursive functions
+- Use spans per meaningful computation step, not every function
+- **Never** use `span.enter()` or `span.entered()` in async tasks
+
+### Documentation
+
+- All `TODO` comments must link to a GitHub issue
+- Prefer line comments (`//`) over block comments
+- Keep comments concise, explain "why" not "what"
+
+## Logging Levels
+
+| Level | Use Case |
+|-------|----------|
+| `crit` | Lighthouse may not function - needs immediate attention |
+| `error` | Moderate impact - expect user reports |
+| `warn` | Unexpected but recoverable |
+| `info` | High-level status - not excessive |
+| `debug` | Developer events, expected errors |
+
+## Testing Patterns
+
+- **Unit tests**: Single component edge cases
+- **Integration tests**: Use [`BeaconChainHarness`](beacon_node/beacon_chain/src/test_utils.rs) for end-to-end workflows
+- **Sync components**: Use [`TestRig`](beacon_node/network/src/sync/tests/mod.rs) pattern with event-based testing
+- **Mocking**: `mockall` for unit tests, `mockito` for HTTP APIs
+- **Adapter pattern**: For testing `BeaconChain` dependent components, create adapter structs. See [`fetch_blobs/tests.rs`](beacon_node/beacon_chain/src/fetch_blobs/tests.rs)
+- **Local testnet**: See `scripts/local_testnet/README.md`
+
+## Build Notes
+
+- Full builds take 5+ minutes - use large timeouts (300s+)
+- Use `cargo check` for faster iteration
+- MSRV documented in `Cargo.toml`
+
+### Cross-compilation
+
+- `make build-x86_64` - Cross-compile for x86_64 Linux
+- `make build-aarch64` - Cross-compile for ARM64 Linux
+- `make build-riscv64` - Cross-compile for RISC-V 64-bit Linux
+
+## Parallel Development
+
+For working on multiple branches simultaneously, use git worktrees:
+
+```bash
+git worktree add -b my-feature ../lighthouse-my-feature unstable
+```
+
+This creates a separate working directory without needing multiple clones. To save disk space across worktrees, configure a shared target directory:
+
+```bash
+# In .cargo/config.toml at your workspace root
+[build]
+target-dir = "/path/to/shared-target"
+```
--- a/.ai/ISSUES.md
+++ b/.ai/ISSUES.md
@@ -0,0 +1,130 @@
+# GitHub Issue & PR Guidelines
+
+Guidelines for creating well-structured GitHub issues and PRs for Lighthouse.
+
+## Issue Structure
+
+### Start with Description
+
+Always begin with `## Description`:
+
+```markdown
+## Description
+
+We presently prune all knowledge of non-canonical blocks once they conflict with
+finalization. The pruning is not always immediate, fork choice currently prunes
+once the number of nodes reaches a threshold of 256.
+
+It would be nice to develop a simple system for handling messages relating to
+blocks that are non-canonical.
+```
+
+**Guidelines:**
+- First paragraph: problem and brief solution
+- Provide context about current behavior
+- Link to related issues, PRs, or specs
+- Be technical and specific
+
+### Steps to Resolve (when applicable)
+
+```markdown
+## Steps to resolve
+
+I see two ways to fix this: a strict approach, and a pragmatic one.
+
+The strict approach would only check once the slot is finalized. This would have
+0 false positives, but would be slower to detect missed blocks.
+
+The pragmatic approach might be to only process `BeaconState`s from the canonical
+chain. I don't have a strong preference between approaches.
+```
+
+**Guidelines:**
+- Don't be overly prescriptive - present options
+- Mention relevant constraints
+- It's okay to say "I don't have a strong preference"
+
+### Optional Sections
+
+- `## Additional Info` - Edge cases, related issues
+- `## Metrics` - Performance data, observations
+- `## Version` - For bug reports
+
+## Code References
+
+**Use GitHub permalinks with commit hashes** so code renders properly:
+
+```
+https://github.com/sigp/lighthouse/blob/261322c3e3ee/beacon_node/beacon_processor/src/lib.rs#L809
+```
+
+Get commit hash: `git rev-parse unstable`
+
+For line ranges: `#L809-L825`
+
+## Writing Style
+
+### Be Natural and Concise
+- Direct and objective
+- Precise technical terminology
+- Avoid AI-sounding language
+
+### Be Honest About Uncertainty
+- Don't guess - ask questions
+- Use tentative language when appropriate ("might", "I think")
+- Present multiple options without picking one
+
+### Think About Trade-offs
+- Present multiple approaches
+- Discuss pros and cons
+- Consider backward compatibility
+- Note performance implications
+
+## Labels
+
+**Type:** `bug`, `enhancement`, `optimization`, `code-quality`, `security`, `RFC`
+
+**Component:** `database`, `HTTP-API`, `fork-choice`, `beacon-processor`, etc.
+
+**Effort:** `good first issue`, `low-hanging-fruit`, `major-task`
+
+## Pull Request Guidelines
+
+```markdown
+## Description
+
+[What does this PR do? Why is it needed? Be concise and technical.]
+
+Closes #[issue-number]
+
+## Additional Info
+
+[Breaking changes, performance impacts, migration steps, etc.]
+```
+
+### Commit Messages
+
+Format:
+- First line: Brief summary (imperative mood)
+- Blank line
+- Additional details if needed
+
+```
+Add custody info API for data columns
+
+Implements `/lighthouse/custody/info` endpoint that returns custody group
+count, custodied columns, and earliest available data column slot.
+```
+
+## Anti-Patterns
+
+- Vague descriptions without details
+- No code references when describing code
+- Premature solutions without understanding the problem
+- Making claims without validating against codebase
+
+## Good Examples
+
+- https://github.com/sigp/lighthouse/issues/6120
+- https://github.com/sigp/lighthouse/issues/4388
+- https://github.com/sigp/lighthouse/issues/8216