greptile: Add Greptile configuration for automated code review on PRs.#21482
greptile: Add Greptile configuration for automated code review on PRs.#21482suphawitwanl wants to merge 1 commit into
Conversation
config.json details
Settings:
- Strictness 2, status check enabled, re-reviews on every push
- Comment types: logic, syntax, best_practices, security
Rule structure:
Each rule has an id, description, scope, and severity.
- Scope: glob patterns that limit which files the rule applies to
(e.g. **/*.c, **/*.yang, lib/**)
- Severity: high (P0/P1 — must fix), medium (P2 — should fix), low
(NOTE — suggestion)
Rules cover:
- Memory safety: XMALLOC/XCALLOC/XFREE enforcement, no raw malloc/free
- String safety: no strcpy/strcat/sprintf, enforce strlcpy/snprintf
- Banned functions: no system(), no fork()+exec()
- Added popen() to banned-functions rule (same issue with SIGINT to be
ignored and break daemon shutdown)
- Typesafe containers: reject legacy linklist.h/hash.h, enforce lib/typesafe.h
- CLI conventions: DEFPY required, YANG model required for YANGified daemons
- YANG/northbound: API-agnostic callbacks, JSON output backed by YANG models
- Packet parsing: bounds checking on all network message parsing
- RCU safety: lock discipline, no direct XFREE on RCU-protected data
- Logging: zlog_* only, debug guarded by CLI flags
- Licensing: SPDX headers, GPL compatibility checks
- Formatting: include order, whitespace discipline, printfrr usage
- Commit messages: subsystem prefix, imperative mood, Signed-off-by
- Topotest coverage (P0): blocks merge if daemon code changes without
tests/topotests/ changes
- User docs (P0): blocks merge if CLI/YANG changes without doc/user/ updates
- Mark User docs as Prority 1 checks to run first
- Flag doc updates as mandatory for features
- Topotest coverage: check for coverage as priority 2 (after used docs)
- Flag Topotests coverage as mandatory for features
Signed-off-by: Suphawit Wanlaung <suphawitw@netdef.org>
Greptile SummaryThis PR introduces
|
| Filename | Overview |
|---|---|
| .greptile/config.json | Adds comprehensive Greptile review config with 44 rules covering FRR coding standards; minor scope gap in mandatory-topotest (does not exclude tests/**) and a stale node_modules ignore pattern. |
| .greptile/files.json | Lists context files (developer docs, accords) for the AI reviewer; content is consistent with the style-guide entries already present in .db-rules.md, no issues found. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
PR[Pull Request opened / updated] --> GREPTILE[Greptile triggered\ntriggerOnUpdates: true]
GREPTILE --> IGNORE{File in\nignorePatterns?}
IGNORE -- Yes --> SKIP[Skip file]
IGNORE -- No --> PRIO1
subgraph PRIO1 [Priority 1 — user-docs-cli-changes]
P1CHK{.c/.h/.yang changed\nwithout doc/user/ changes?}
P1CHK -- Yes --> P1FLAG[Flag P0: Missing user docs]
P1CHK -- No --> PRIO2
end
subgraph PRIO2 [Priority 2 — mandatory-topotest]
P2CHK{.c/.h/.yang changed\nwithout tests/topotests/ changes?}
P2CHK -- Yes --> P2FLAG[Flag P0: No topotest coverage]
P2CHK -- No --> RULES
end
subgraph RULES [Remaining 42 rules evaluated]
R1[memory-allocation / string-safety / banned-functions]
R2[logging-api / printfrr-usage / assert-usage]
R3[typesafe-containers / rcu-safety / event-loop-blocking]
R4[spdx-license / yang-schema-license / license-compatibility]
R5[cli-defpy / yang-json-output / northbound-yang-callbacks]
R6[commit-title / commit-body / squash-policy]
end
RULES --> OUTPUT[Review comment output\nP0 / P1 / P2 / NOTE]
P1FLAG --> OUTPUT
P2FLAG --> OUTPUT
OUTPUT --> SC[Status check posted\nstatusCheck: true]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: .greptile/config.json
Line: 321-324
Comment:
**`mandatory-topotest` scope matches test C files — potential false positives**
The scope `["**/*.c", "**/*.h", "**/*.yang"]` matches C files that reside under `tests/` (e.g., `tests/lib/test_stream.c`). A PR that only modifies an existing unit-test helper in `tests/lib/` without touching `tests/topotests/` will have this rule applied to those files, which could trigger a false P0 flag even though no daemon feature code changed.
The rule text says "with the exception of files under tests/" but that exclusion only lives in the prose — it is not enforced by the scope glob. If Greptile evaluates the rule per-file without full-PR context, the prose exclusion may be missed.
Consider tightening the scope to exclude the tests directory explicitly (if negation patterns are supported):
```suggestion
"scope": ["**/*.c", "**/*.h", "**/*.yang", "!tests/**"],
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: .greptile/config.json
Line: 6
Comment:
**`node_modules/**` ignore pattern is irrelevant for FRR**
FRR is a pure C project with no JavaScript/npm toolchain. `node_modules/**` will never match any file in this repository. While harmless, it signals that this config was derived from a template rather than written specifically for FRR. Consider removing it to keep the patterns meaningful and auditable.
```suggestion
"ignorePatterns": ["**/*.generated.*", "**/*.pb.cc", "**/*.pb.h", "**/*.grpc.pb.cc", "**/*.grpc.pb.h", ".git/**"],
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: .greptile/config.json
Line: 236-240
Comment:
**`user-docs-cli-changes` scope — intentional Python exclusion worth confirming**
The scope `["**/*.c", "**/*.h", "**/*.yang"]` correctly covers C and YANG changes. The rule text explicitly notes Python files are excluded to avoid false positives on topotest/tooling PRs. This is fine as documented, but worth a callout: if FRR ever gains YANG-backed CLI defined via Python bindings, this scope would silently miss those changes. No change needed now — just ensure the exclusion remains intentional as the codebase evolves.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "greptile: Add Greptile configuration for..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "rule": "PRIORITY 2 CHECK — RUN THIS IMMEDIATELY AFTER PRIORITY 1. If any .c, .h, or .yang file (with the exception of files under tests/) is modified with new features or behavior changes, and there are NO changes to any file under tests/topotests/, you MUST flag this as P0 severity with label 'P0': 'No topotest coverage for new feature code. Topotests are mandatory for new features per FRR policy. This blocks merge — no exceptions.' This is the most critical check and MUST appear before all other findings. Do NOT flag for simple one-line bug fixes, typo corrections, documentation-only changes, or pure refactors that do not change behavior.", | ||
| "scope": ["**/*.c", "**/*.h", "**/*.yang"], | ||
| "severity": "high" | ||
| } |
There was a problem hiding this comment.
mandatory-topotest scope matches test C files — potential false positives
The scope ["**/*.c", "**/*.h", "**/*.yang"] matches C files that reside under tests/ (e.g., tests/lib/test_stream.c). A PR that only modifies an existing unit-test helper in tests/lib/ without touching tests/topotests/ will have this rule applied to those files, which could trigger a false P0 flag even though no daemon feature code changed.
The rule text says "with the exception of files under tests/" but that exclusion only lives in the prose — it is not enforced by the scope glob. If Greptile evaluates the rule per-file without full-PR context, the prose exclusion may be missed.
Consider tightening the scope to exclude the tests directory explicitly (if negation patterns are supported):
| "rule": "PRIORITY 2 CHECK — RUN THIS IMMEDIATELY AFTER PRIORITY 1. If any .c, .h, or .yang file (with the exception of files under tests/) is modified with new features or behavior changes, and there are NO changes to any file under tests/topotests/, you MUST flag this as P0 severity with label 'P0': 'No topotest coverage for new feature code. Topotests are mandatory for new features per FRR policy. This blocks merge — no exceptions.' This is the most critical check and MUST appear before all other findings. Do NOT flag for simple one-line bug fixes, typo corrections, documentation-only changes, or pure refactors that do not change behavior.", | |
| "scope": ["**/*.c", "**/*.h", "**/*.yang"], | |
| "severity": "high" | |
| } | |
| "scope": ["**/*.c", "**/*.h", "**/*.yang", "!tests/**"], |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .greptile/config.json
Line: 321-324
Comment:
**`mandatory-topotest` scope matches test C files — potential false positives**
The scope `["**/*.c", "**/*.h", "**/*.yang"]` matches C files that reside under `tests/` (e.g., `tests/lib/test_stream.c`). A PR that only modifies an existing unit-test helper in `tests/lib/` without touching `tests/topotests/` will have this rule applied to those files, which could trigger a false P0 flag even though no daemon feature code changed.
The rule text says "with the exception of files under tests/" but that exclusion only lives in the prose — it is not enforced by the scope glob. If Greptile evaluates the rule per-file without full-PR context, the prose exclusion may be missed.
Consider tightening the scope to exclude the tests directory explicitly (if negation patterns are supported):
```suggestion
"scope": ["**/*.c", "**/*.h", "**/*.yang", "!tests/**"],
```
How can I resolve this? If you propose a fix, please make it concise.| "id": "user-docs-cli-changes", | ||
| "rule": "PRIORITY 1 CHECK — RUN THIS FIRST. If the PR adds, removes, or modifies CLI commands, YANG models, or any user-facing interface, and there are NO changes to files under doc/user/, you MUST flag this as P0 severity with label 'P0': 'Missing user documentation update. User-facing CLI and YANG changes require doc/user/ updates per FRR policy. This blocks merge — no exceptions.' This is the most critical check and MUST appear before all other findings. This applies to new DEFPY/DEFUN commands, changed command syntax, new show commands, new JSON output fields, and YANG schema changes. New BGP features go in the BGP chapter, not a new chapter. New protocol daemons get their own chapter. Note: Python files are intentionally excluded from scope to avoid false positives on topotest and tooling PRs — FRR CLI is defined in C via DEFPY/DEFUN macros. Do NOT flag for internal-only refactors, simple bug fixes, or changes that do not alter user-visible behavior.", | ||
| "scope": ["**/*.c", "**/*.h", "**/*.yang"], | ||
| "severity": "high" | ||
| }, |
There was a problem hiding this comment.
user-docs-cli-changes scope — intentional Python exclusion worth confirming
The scope ["**/*.c", "**/*.h", "**/*.yang"] correctly covers C and YANG changes. The rule text explicitly notes Python files are excluded to avoid false positives on topotest/tooling PRs. This is fine as documented, but worth a callout: if FRR ever gains YANG-backed CLI defined via Python bindings, this scope would silently miss those changes. No change needed now — just ensure the exclusion remains intentional as the codebase evolves.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .greptile/config.json
Line: 236-240
Comment:
**`user-docs-cli-changes` scope — intentional Python exclusion worth confirming**
The scope `["**/*.c", "**/*.h", "**/*.yang"]` correctly covers C and YANG changes. The rule text explicitly notes Python files are excluded to avoid false positives on topotest/tooling PRs. This is fine as documented, but worth a callout: if FRR ever gains YANG-backed CLI defined via Python bindings, this scope would silently miss those changes. No change needed now — just ensure the exclusion remains intentional as the codebase evolves.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
mjstapp
left a comment
mjstapp
left a comment
There was a problem hiding this comment.
there are a number of items in this config that don't seem quite correct - how did you come up with these?
my concern is that the greptile will produce noisy output, which will be ignored, and that will make its feedback less useful
This was done from our Thai Employees based on the developer docs in the git. |
|
Personally I like that someone is starting to think about how to make greptile be better focused on things we care about. Although, I am not a huge fan of changing everything at once. It's a recipe for having to fight the tool in ways that we might not be prepared for. I am for breaking this up into much smaller chunks rolling something out and seeing what happens over a couple days -> week and then doing the next chunk of it. |
|
I've cloned the FRR with this greptile config and submitted some of the recent PRs to it to see the differences in the Greptile Review. Here is an overview with links to the PR here and the clone PR with the different Greptile review on the test Repo:
|
|
@mwinter-osr mirrored two of my open refactoring PRs on opensourcerouting/frr to run Greptile with the proposed config. I compared those reviews to the ones produced by the current config on the live FRRouting/frr repo.
Detailed breakdown: osr #233 (speed-via-dplane, mirror of #19412):
Ratio: 3 real / 2 false positives out of 5 inline comments. osr #234 (VRF cleanup, mirror of #20318):
Ratio: 1 useful / 1 false positive out of 2 inline comments. Takeaways
Net: proposed config is a significant improvement. |
|
Closing this PR to keep it as a record of all the proposed rules. We'll open a new PR with just a small subset of the rules |
5 participants
config.json details
Settings:
Rule structure:
Each rule has an id, description, scope, and severity.
Rules cover: