-
Notifications
You must be signed in to change notification settings - Fork 64
tools: add Claude configuration and PR review command #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -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: <pr-number> | ||||||
| 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 <build_dir> -DCMAKE_BUILD_TYPE=<mode>` | ||||||
| - `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 <build_dir> | ||||||
| ``` | ||||||
|
|
||||||
| 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 <build_dir> unit # Unit tests | ||||||
| make -C <build_dir> e2e # End-to-end tests | ||||||
| make -C <build_dir> integration # Integration tests | ||||||
| make -C <build_dir> check # Style check and linter | ||||||
|
|
||||||
| ctest --test-dir build --output-on-failure -R <pattern> # Run a specific test | ||||||
|
||||||
| ctest --test-dir build --output-on-failure -R <pattern> # Run a specific test | |
| ctest --test-dir <build_dir> --output-on-failure -R <pattern> # Run a specific test |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,27 @@ | ||||||||||||||||||||||||||||
| # AI PR review | ||||||||||||||||||||||||||||
| name: AI PR Review | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||||
| push: | ||||||||||||||||||||||||||||
| branches: | ||||||||||||||||||||||||||||
| - 'ci_**' | ||||||||||||||||||||||||||||
|
Comment on lines
+5
to
+7
|
||||||||||||||||||||||||||||
| push: | |
| branches: | |
| - 'ci_**' |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow doesn't include permissions configuration, unlike the ci.yaml workflow which has a comprehensive permissions block. For security best practices, explicitly define the minimum required permissions for this workflow (at minimum, it may need 'contents: read' and 'pull-requests: write' if the Claude tool posts comments to PRs).
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow uses actions/checkout@v4 while other workflows in ci.yaml use actions/checkout@v2. This inconsistency could lead to unexpected behavior. Consider using the same version across all workflows for consistency, or ensure there's a specific reason for using v4 here.
| uses: actions/checkout@v4 | |
| uses: actions/checkout@v2 |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow attempts to checkout code before installing Node.js and Claude Code. If the command execution at line 27 needs to access repository files (which it likely does based on the review-pr.md content), the checkout step should remain. However, there's a potential issue: the checkout doesn't fetch the full PR context needed for the review. Consider adding fetch-depth: 0 to the checkout action and potentially using ref: refs/pull/${{ github.event.pull_request.number }}/merge to checkout the PR merge state.
| uses: actions/checkout@v4 | |
| uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 | |
| ref: refs/pull/${{ github.event.pull_request.number }}/merge |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package '@anthropic-ai/claude-code' may not be the correct or official package name for Claude CLI integration. The actual package name should be verified, as this could cause installation failures. Check Anthropic's official documentation for the correct package name to install Claude CLI tools.
| run: npm install -g @anthropic-ai/claude-code | |
| run: npm install -g @anthropic-ai/claude |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ANTHROPIC_API_KEY secret is being passed as an environment variable, which is correct. However, ensure that the secret 'ANTHROPIC_API_KEY' has been configured in the repository settings. The workflow will fail if this secret is not set, and there's no error handling or validation to provide a clear error message.
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow will fail when triggered by a push event because github.event.pull_request.number will be null/empty for push events. The workflow should either be restricted to only pull_request events, or the command should handle both push and pull_request events appropriately (e.g., by deriving the PR number differently for push events or skipping execution).
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command 'claude /review-pr' assumes the Claude CLI is installed and available in PATH, and that it can directly execute custom commands defined in the .claude/commands/ directory. The integration between the npm package installation and the actual Claude CLI invocation needs verification. Additionally, the command may need to be invoked from the repository root directory where the .claude/ directory exists.
| - 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 }} | |
| - name: Review PR | |
| env: | |
| ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} | |
| working-directory: ${{ github.workspace }} | |
| run: | | |
| npx -y @anthropic-ai/claude-code /review-pr ${{ github.event.pull_request.number }} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,3 +10,6 @@ | |
| __pycache__ | ||
| .cache | ||
| .clangd | ||
|
|
||
| # Local Claude instructions | ||
| CLAUDE.local.md | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| ```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` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instructions specify setting
build_dirto/tmp/bpfilter-pr-$ARGUMENTS, but then use the generic placeholder<build_dir>in subsequent commands. For clarity, either consistently use the explicit path/tmp/bpfilter-pr-$ARGUMENTSthroughout, or add a note explaining that<build_dir>refers to/tmp/bpfilter-pr-$ARGUMENTS.