tools: ast-grep: add static analysis rules

[pzmarzly]

If https://ast-grep.github.io/ is installed (e.g. via pip install ast-grep-cli), we can use it to enforce many of our coding standards. See #510.

In this PR, rules with existing violations were introduced with severity warning, and have a known_issues file that silence known occurrances (otherwise, we'd get a ton of spam on every CI run). As we refactor our codebase, we should change all of them to errors, and remove the ignore_known_issues mechanism.

Test

ast-grep scan --config tools/ast-grep/sgconfig.yml src/bfcli/ src/libbpfilter/
Output: https://gist.github.com/pzmarzly/2e7d84b21a3202d1e87710f39c2d3857

List of rules

• [warning] assert-pointer-params — Functions with pointer parameters must assert() them at entry.
• bf-prefix-public-functions — Non-static functions must use the bf_/bfc_ prefix (or _bf_/_bfc_ when static).
• double-pointer-output — Constructor bf_*_new() must take a TYPE ** output parameter for ownership transfer.
• [warning] doxygen-prefer-backticks — Prefer `name` over @p name, @ref name, @c name in Doxygen comments.
• [warning] doxygen-public-functions — Public function declarations in headers must have a preceding /** @brief ... */ comment.
• enum-bf-prefix — Named enums must use bf_/bfc_ prefix.
• enum-sentinel-format — Enums must end with a _BF_*_MAX / _BFC_*_MAX sentinel (with listed exceptions for bitmasks and kernel ABI mirrors).
• fd-init-minus-one — Variables declared with _cleanup_close_ must be initialized to -1 to avoid closing fd 0 on error paths.
• free-function-pattern — bf_*_free() / _bf_*_free() must return void and accept a double-pointer parameter.
• negative-errno-returns — Error returns must be negated (-ENOMEM, not ENOMEM).
• [warning] no-direct-free — Disallows raw free(); use freep() or a typed bf_*_free() cleanup function instead (with exempt callers).
• no-fprintf-stderr — Use logging macros (bf_err, bf_warn, …) instead of fprintf(stderr, …) / vfprintf(stderr, …).
• no-ifndef-guards — Headers must use #pragma once, not #ifndef/#define include guards.
• [warning] single-line-comment-style — Single-line comments must use //, reserving /* */ for multi-line (SPDX excluded).
• spdx-license-header — Every source file must contain an SPDX-License-Identifier comment.
• struct-bf-prefix — Struct definitions (with a body) must use the bf_/bfc_ prefix; external type references are ignored.

[github-actions]

Claude review of PR #502 (e3f4fe0)

Suggestions

• CMake undefined SG_PYYAML_RC — tests/check/CMakeLists.txt:70 — variable is uninitialized when ast-grep is not found, relying on fragile CMake coercion (fixed: now pre-initialized to 1)
• Missing bfc_ in exemption regexes — tools/ast-grep/rules/no-direct-free.yml:26 — exemption regex omits bfc_ prefixed cleanup functions (fixed: bfc_ patterns now included)
• JSON parse error handling — tools/ast-grep/ignore_known_issues.py:79 — json.loads has no error handling for malformed input; a non-JSON diagnostic line from ast-grep would crash the script with an opaque traceback
• Unpinned ast-grep version in CI — .github/workflows/ci.yaml:116 — pipx install --global ast-grep-cli without a version pin; a breaking upstream change to JSON schema would silently break CI
• CI runtime installation — .github/workflows/ci.yaml:115 — consider baking ast-grep and python3-pyyaml into the Fedora Dockerfile (matching how other build deps are handled) rather than installing on every run
• assert-pointer-params exemption sync — tools/ast-grep/rules/assert-pointer-params.yml:37 — freep is in bf-prefix-public-functions exclusion but not here; however freep is static inline so it never matches the rule anyway (non-issue)
• negative-errno-returns regex gaps — tools/ast-grep/rules/negative-errno-returns.yml:10 — regex ^E[A-Z0-9]{2,}$ is broader than just errno names, but no false positives currently exist in the codebase

Nits

• Dangling reference in note — tools/ast-grep/rules/negative-errno-returns.yml:7 — note says "See error-handling standard" but no such document exists; should reference doc/developers/style.rst
• Incomplete test coverage for no-fprintf-stderr — tools/ast-grep/rule-tests/no-fprintf-stderr-test.yml:23 — missing vfprintf invalid case and bf_logger_* valid case
• README claim about error-severity exit behavior — tools/ast-grep/known_issues/README.md:19 — states ast-grep scan exits non-zero on error matches, but --json=stream mode may exit 0 regardless; verify and update
• no-fprintf-stderr brittle exclusion — tools/ast-grep/rules/no-fprintf-stderr.yml:17 — exclusion regex enumerates specific function prefixes (acceptable given the small set)
• YAML note field style — cosmetic inconsistency in note field quoting style (acceptable given YAML equivalence)
• enum-sentinel exclusion docs — tools/ast-grep/rules/enum-sentinel-format.yml:33 — now has inline comment documenting exclusion criteria (fixed)

Workflow run

[yaakov-stein]

I have not really looked over the code at all, but one thought - would it be easier (for development, review, and not having an overbearing amount of warnings) to start with a single rule that we can begin warning on? After we've seen how that goes, we can begin adding more at a quicker pace.

[github-actions]

Claude: suggestion: pipx install --global ast-grep-cli is unpinned. A new upstream release could change rule semantics or introduce incompatibilities, breaking CI unexpectedly. Consider pinning to a specific version (e.g., pipx install --global ast-grep-cli==0.37.0) and bumping deliberately when needed.

[pzmarzly]

I think it's better to fix it when it happens, than to let the pinned version rot.

[github-actions]

Claude: suggestion: json.loads(raw) will raise JSONDecodeError on malformed input (e.g., a truncated stream from SIGPIPE or an ast-grep version mismatch mixing diagnostics into the JSON stream). The resulting Python traceback would not indicate which line failed or what the raw content was. Consider wrapping in a try/except json.JSONDecodeError that prints a descriptive message to stderr before re-raising, so CI failures from corrupted streams are easier to triage.

[github-actions]

Claude: nit: The exclusion regex ^(_bf_print_insn|bf_logger_).* enumerates a single specific function name rather than a pattern. If another function legitimately needs fprintf(stderr, ...) in the future (e.g., a debug dumper), the rule requires a source edit. Consider generalizing to ^(_bf_print_|bf_logger_).* or adding a comment explaining the maintenance expectation.

[github-actions]

Claude: nit: The note field uses a quoted string (note: "...") while most other rules in this PR use >- folded scalar style. The same inconsistency appears in no-fprintf-stderr.yml (line 5) and no-ifndef-guards.yml (line 7). Consider using >- for consistency:

note: >-
  bpfilter uses negative errno convention: return -ENOMEM, not ENOMEM.
  See error-handling standard.

[github-actions]

Claude: nit: The exclusion regex lists 8 specific enum names. Any new kernel-mirror or bitmask enum added to the codebase requires a manual rule edit to this regex. Consider adding a YAML comment above the regex documenting the criteria for inclusion ("bitmask enums and kernel ABI mirrors that intentionally omit sentinels"), so the maintenance contract is clear.

[pzmarzly]

@claude review

[github-actions]

Claude: suggestion: json.loads(raw) has no error handling. If ast-grep ever writes a non-JSON diagnostic line to stdout (e.g., on a version upgrade or internal warning), the script crashes with an opaque JSONDecodeError traceback. Consider wrapping in try/except:

try:
    d = json.loads(raw)
except json.JSONDecodeError:
    sys.stderr.write(f"warning: skipping non-JSON line: {raw!r}\n")
    continue

[github-actions]

Claude: suggestion: pipx install --global ast-grep-cli installs an unpinned version. A breaking upstream change to the JSON stream schema would silently break CI. Consider pinning to a known-good version (e.g., pipx install --global ast-grep-cli==X.Y.Z) and documenting the minimum supported version.

[github-actions]

Claude: suggestion: Consider baking pipx, python3-pyyaml, and ast-grep-cli into the Fedora Dockerfile rather than installing at CI runtime. Other build dependencies (clang-format, cmocka, etc.) are already pre-installed in the container image. Installing at runtime adds network latency and a failure point on every run.

[github-actions]

Claude: nit: The note says "See error-handling standard" but no document with that exact title exists. The relevant reference is the "Error codes" section in doc/developers/style.rst. Consider: "See doc/developers/style.rst, Error codes section."

[github-actions]

Claude: nit: The test only exercises fprintf(stderr, ...) in the invalid cases but the rule also matches vfprintf(stderr, $$$). Similarly, valid cases test the _bf_print_* exclusion but not the bf_logger_* exclusion. Adding test cases for both branches would improve coverage of the rule.

[github-actions]

Claude: nit: The README states that ast-grep scan exits non-zero on severity: error matches, causing pipeline failure before the filter runs. However, ast-grep scan --json=stream typically exits 0 regardless of findings (JSON output modes are designed for piping to tools). If this is incorrect, the documented guarantee about error-severity rules being zero-tolerance may not hold. Please verify the actual exit behavior and update accordingly.