From d11861d4b560ba51290bd1fead3dc4eade07819a Mon Sep 17 00:00:00 2001 From: Quentin Deslandes Date: Thu, 15 Jan 2026 17:53:42 +0100 Subject: [PATCH] tools: add Claude configuration and PR review command --- .claude/commands/review-pr.md | 164 ++++++++++++++++++++++++++++++++ .github/workflows/ai.yaml | 27 ++++++ .gitignore | 3 + CLAUDE.md | 173 ++++++++++++++++++++++++++-------- 4 files changed, 329 insertions(+), 38 deletions(-) create mode 100644 .claude/commands/review-pr.md create mode 100644 .github/workflows/ai.yaml diff --git a/.claude/commands/review-pr.md b/.claude/commands/review-pr.md new file mode 100644 index 000000000..dc0e3404a --- /dev/null +++ b/.claude/commands/review-pr.md @@ -0,0 +1,164 @@ +--- +allowed-tools: Bash(git *), Bash(make *), Bash(cmake *), Bash(ctest *), Bash(grep *), Bash(lcov *), Bash(sudo make *), Bash(gcov *), Bash(ls *), Grep, Read, Glob, Task +argument-hint: +description: Review a pull request +--- + +Review pull request #$ARGUMENTS + +## Setup + +1. Fetch the PR: `git fetch origin pull/$ARGUMENTS/head:pr-$ARGUMENTS` (try `upstream` if `origin` fails) +2. Create a worktree: `git worktree add /tmp/bpfilter-pr-$ARGUMENTS pr-$ARGUMENTS` +3. Get commit info: `git log main..pr-$ARGUMENTS --oneline` +4. Get the diff: `git diff --stat main...pr-$ARGUMENTS` +5. Read the style guide @doc/developers/style.rst + +## Review steps + +- Build and tests should be done in both mode `debug` and `release`. +- Configure: `cmake -S . -B -DCMAKE_BUILD_TYPE=` + - `build_dir`: `/tmp/bpfilter-pr-$ARGUMENTS` + - Use `-DWITH_COVERAGE=1` for coverage information + +### Code review + +Review code changes for quality and safety. + +- Warn on ABI/API breakage (in libbpfilter) +- No buffer overflows +- Input validation at boundaries +- No command injection risks +- No hardcoded credentials +- Pay extra attention to the new BPF bytecode generated, be careful about: register misuse, sub-optimal constructs + +Report: +1. Issues found, grouped by severity + 1. Issues that must be addressed + 2. Suggestions for improvement + 3. Minor style notes +2. Overall status (PASS/FAIL) + +### Build + +Configure and build the project, reporting any issues. + +Build: +```bash +make -C +``` + +Report: +1. Configuration status +2. Warning count +3. List all warnings with file:line +4. List all errors with context +5. Build status (PASS/FAIL) + +### Test + +Test: +```bash +make -C unit # Unit tests +make -C e2e # End-to-end tests +make -C integration # Integration tests +make -C check # Style check and linter + +ctest --test-dir build --output-on-failure -R # Run a specific test +``` + +- New functions should be tested using unit tests for libbpfilter, end-to-end tests for matchers +- New matchers should have a corresponding E2E test in `tests/e2e/matchers/` +- Use your best judgement to assess if a given function should be tested or not + +Report: +1. Which suites were run +2. Pass/fail counts +3. Any failures with their output +4. Overall status (PASS/FAIL) + +### Style + +Do not run the check target again, use your knowledge of the style guide. + +Report: +1. Style violations +2. Overall status (PASS/FAIL) + +### Documentation + +Generate documentation: +```bash +make -C doc +``` + +- Documentation should be generated without warning or issue +- Important or complex functions should be documented +- Do not document trivial functions (e.g. getters, setters) + +Report: +1. Any warnings or errors +2. Overall status (PASS/FAIL) + +### Coverage + +Generate coverage information: +```bash +make -C coverage +``` + +- Requires `-DWITH_COVERAGE=1` and unit tests to have run +- Only analyse the coverage of the lines changed in the PR +- Use gcov to check specific file coverage: `gcov -p /src/libbpfilter/CMakeFiles/libbpfilter.dir/.o` +- New lines: minimum 70% covered +- New functions: 100% covered + +Report: +1. New lines coverage (percentage per function) +2. New functions coverage (list uncovered functions) +3. Overall status (PASS/FAIL) + +### Commit + +Validate commit messages against project guidelines. + +Get commit message: +```bash +git log -1 --format='%s' +git log -1 --format='%b' +``` + +Get changed files: +```bash +git diff --stat ^.. +``` + +Report: +1. Overall status (PASS/FAIL) + +## Cleanup + +After review, clean up the worktree and branch: +1. `git worktree remove /tmp/bpfilter-pr-$ARGUMENTS` +2. `git branch -D pr-$ARGUMENTS` + +## Output format + +Structure the final report as follows: + +1. **Description**: A short paragraph describing what the PR does +2. **Code Review**: Issues grouped by severity (must address / suggestions / minor notes) +3. **Build**: Table with Mode, Status, Warnings, Errors columns +4. **Test**: Table with Suite, Passed, Failed columns +5. **Style**: PASS/FAIL with any violations listed +6. **Documentation**: PASS/FAIL with any warnings +7. **Coverage**: Table showing coverage percentage per new function +8. **Commit**: PASS/FAIL with validation details +9. **Overall Status**: PASS, FAIL, or PASS (conditional) with summary + +Guidelines: +- Use markdown tables for build and test results +- Reference issues with `function_name` in `file:line` format +- Focus on what should be improved, ignore what is already good +- Only report issues for which you have high confidence +- Use "PASS (conditional)" when code is correct but improvements are recommended diff --git a/.github/workflows/ai.yaml b/.github/workflows/ai.yaml new file mode 100644 index 000000000..4140e0c9c --- /dev/null +++ b/.github/workflows/ai.yaml @@ -0,0 +1,27 @@ +# AI PR review +name: AI PR Review + +on: + push: + branches: + - 'ci_**' + pull_request: + types: [opened, synchronize] + +jobs: + review: + runs-on: [ "ubuntu-24.04" ] + container: ghcr.io/facebook/bpfilter:fedora-43-x64 + name: "AI PR review" + steps: + - name: Checkout bpfilter + uses: actions/checkout@v4 + - name: Install Node.js and npm + run: dnf install -y nodejs npm + - name: Install Claude Code + run: npm install -g @anthropic-ai/claude-code + - name: Review PR + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + run: | + claude /review-pr ${{ github.event.pull_request.number }} \ No newline at end of file diff --git a/.gitignore b/.gitignore index 1b4884497..e6f0f82db 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,6 @@ __pycache__ .cache .clangd + +# Local Claude instructions +CLAUDE.local.md \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index c5373243f..840920916 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,47 +1,144 @@ # Instructions for Claude -## Communication +## Project overview + +bpfilter is an eBPF-based packet filtering framework that translates filtering rules into optimized BPF programs. Licensed under GPLv2, maintained by Meta. + +**Components:** +- `libbpfilter` - Core library with public API for filtering logic +- `bpfilter` - Daemon that generates and manages BPF programs +- `bfcli` - CLI for defining filtering rules + +**Requirements:** Linux 6.6+, libbpf 1.2+, libnl-3 -### Tone requirements -- Conversational: target kernel experts, not beginners -- Factual: no drama, just technical observations -- Questions: frame as questions about the code, not accusations -- Terminology: call issues "regressions" not "bugs" or "critical" +## Directory structure -### Question phrasing -- ❌ "Did you corrupt memory here?" -- ✅ "Can this corrupt memory?" -- ❌ "Does this loop have a bounds checking issue?" -- ✅ "Does this code overflow xyz[]?" +``` +src/ +├── libbpfilter/ # Core library (shared object) +│ ├── include/bpfilter/ # Public API headers +│ └── *.c # Implementation (chain, matcher, rule, hook, set, bpf, btf...) +├── bpfilter/ # Daemon +│ ├── cgen/ # BPF code generation engine +│ │ ├── matcher/ # Packet matcher codegen (ip4, ip6, tcp, udp, icmp, meta, set) +│ │ └── prog/ # Program linking (link, map) +│ ├── xlate/ # Rule translation (cli, ipt/, nft/) +│ └── bpf/ # eBPF stub programs +├── bfcli/ # CLI (parser.y, lexer.l, opts, print, chain, ruleset) +└── external/ # External deps (mpack) -### Formatting Rules +tests/ +├── unit/ # cmocka tests for libbpfilter API +├── e2e/ # Bash scripts testing full filtering behavior +├── integration/ # API stability tests +├── check/ # clang-tidy and clang-format validation +└── harness/ # Test utilities (test.h, mock.h, fake.h) -- Reference functions by name, not line numbers -- Use call chains for clarity: funcA()→funcB() +doc/ +├── usage/ # User guides (bfcli, daemon, iptables, nftables) +└── developers/ # Dev docs (build, style, tests, modules/) +``` + +## Communication + +### Tone +- Target system development and network experts, not beginners +- Factual observations, no drama +- Frame issues as questions about code, not accusations +- Call issues "regressions" not "bugs" or "critical" ## Building and testing -Building and testing bpfilter should only be performed using instructions detailed in this section: - -- Configure CMake: `cmake -S $SOURCE_DIR -B $BUILD_DIR -DCMAKE_BUILD_TYPE=$TYPE -DWITH_COVERAGE=$COVERAGE`, with: - - `SOURCE_DIR`: the base directory of the repository - - `BUILD_DIR`: usually `SOURCE_DIR/build`, unless specific otherwise - - `TYPE`: `debug` or `release`, use `debug` during development - - `COVERAGE`: 0 or 1, use `1` when `TYPE=debug` -- Build the project: `make -C $BUILD_DIR` -- Run the tests: `make -C $BUILD_DIR test`, the `test_bin` target should be build prior -- Run a specific test suite: `ctest --test-dir $BUILD_DIR --output-on-failure -L $SUITE`, with `SUITE` either `unit`, `integration, `check`, or `e2e` -- Run a specific test: `ctest --test-dir $BUILD_DIR --output-on-failure -R $TEST`, with `TEST` the path to the test file from `tests/` (excluded) and `/` replaced with `.` -- Collect the coverage results: `make -C build coverage`, the tests should be run prior -- Generate the documentation (includes the coverage report): `make -C build doc`, the coverage results should be collected prior - -## Reviewing changes - -When reviewing changes: -- Use git diff to identify changes -- Manually find function definitions and relationships with grep and other tools -- Document any missing context that affects review quality -- Ensure the changes build, and tests succeed, no build error or warning should be introduced, no test failure either -- New code lines should be covered by unit tests (at least 70% of new lines, and 100% of new functions) -- Ensure changes matches the commit message -- Focus on what should be improved, do not explain what is good \ No newline at end of file +```bash +# Configure (use debug + coverage during development) +cmake -S . -B build -DCMAKE_BUILD_TYPE=debug -DWITH_COVERAGE=1 + +# Build +make -C build + +# Run all tests (build test_bin first) +make -C build test_bin test + +# Run specific suite: unit, e2e, integration, check +make -C build unit e2e integration check + +# Run specific test (path from tests/, replace / with .) +ctest --test-dir build --output-on-failure -R e2e.matchers.ip4 + +# Coverage and docs +make -C build coverage +make -C build doc +``` + +**Build options:** +- `-DNO_DOCS=1`, `-DNO_TESTS=1`, `-DNO_CHECKS=1`, `-DNO_BENCHMARKS=1` + +## Code style + +Enforced by `.clang-format` (run `make -C build check` or `make -C build fixstyle`). CI uses latest Fedora's ClangFormat version. See `doc/developers/style.rst` for complete guidelines. + +- 4 spaces (no tabs), 80 char line limit +- String literals: don't split (easier to grep) + +### Naming +- Functions/variables: `bf_` prefix, lowercase with underscores (`bf_chain_new()`) +- Static/internal: leading underscore (`_bf_ctx_free()`) +- CLI utilities: `bfc_` prefix +- Macros: uppercase (`EMIT()`, `TAKE_PTR()`, `ARRAY_SIZE()`) +- Enum values: uppercase with enum prefix (`BF_LOG_DBG`) +- Sentinel values: `_*_MAX` suffix (`_BF_LOG_MAX`) + +### Functions +- Return `0` on success, negative errno on failure (`-ENOMEM`, `-EINVAL`, `-EEXIST`) +- Cleanup functions: return `void`, take double pointer, set `*ptr` to `NULL` +- Error checking: `if (r)` or `if (r < 0)` +- Use `assert()` for pointer preconditions only + +### Memory management +- Use `__attribute__((cleanup))` extensively +- Cleanup macros: `_free_*` for heap, `_clean_*` for stack +- Ownership transfer: `TAKE_PTR()`, `TAKE_FD()`, `TAKE_STRUCT()` + +### Logging +- Levels: `bf_dbg()`, `bf_info()`, `bf_warn()`, `bf_err()`, `bf_abort()` +- Log and return: `bf_err_r(-ENOMEM, "message")` + +### Comments +- Single-line: `//` +- Multi-line: `/* */` with aligned asterisks, close on last text line +- Doxygen: `@brief`, `@param`, `@return`; skip trivial getters/setters +- Doxygen multi-line: first and last lines empty (unlike regular comments) + +### Includes +Use `#pragma once` for header guards. Prefer forward declarations over includes when only a pointer is needed. + +### Commit messages +Format: `component: subcomponent: short description` +- Components: `lib`, `daemon`, `cli`, `tests`, `build`, `tools`, `doc` +- Lowercase, imperative mood, no period, under 72 chars +- Description explains "why", code shows "what" +- No reference to Claude or Claude as co-author + +Examples: +``` +lib: matcher: add meta.flow_hash matcher +daemon: cgen: link: add support for dual-stack Netfilter chains +tests: e2e: fix end-to-end tests leaving files behind +``` + +## Testing requirements + +**Unit tests** (`tests/unit/`): cmocka framework, test every public libbpfilter function + +**E2E tests** (`tests/e2e/`): Bash scripts, test complete filtering behavior with namespace isolation + +**Coverage:** +- New lines: minimum 70% covered +- New functions: 100% covered +- Generate report: `make -C build coverage` + +## Allowed short identifiers + +From `.clang-tidy`: +- Variables: `_`, `i`, `fd`, `r`, `j0`-`j9`, `op`, `ns`, `n` +- Parameters: `ip`, `fd`, `op`, `id`, `cb`, `ns`, `n`