Skip to content

docs: formalize .semgrepignore convention for pre-existing findings #33

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.

Sign up for GitHub

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

Merged
smartwatermelon merged 1 commit into from
Aug 3, 2026
Merged

docs: formalize .semgrepignore convention for pre-existing findings #33

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 71 additions & 9 deletions docs/local-code-review-options.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ a proprietary dataset.
**What it is**: Free CLI tool that reviews staged/unstaged changes locally before push.

**Install**:

```bash
curl -fsSL https://cli.coderabbit.ai/install.sh | sh
```
Expand All @@ -33,12 +34,14 @@ curl -fsSL https://cli.coderabbit.ai/install.sh | sh
one-click fixes. Has a Claude Code plugin (as of Feb 2026) for tighter integration.

**Strengths**:

- Zero-config — install and run
- Free tier available (rate-limited)
- Reviews uncommitted changes (no PR needed)
- Production-bug-aware: trained on broad codebase patterns

**Weaknesses**:

- Cloud-dependent: code is sent to CodeRabbit's servers for analysis
- Not self-hostable without Enterprise plan ($15k+/month, 500-seat minimum)
- Rate limits on free tier may be restrictive for heavy commit workflows
Expand All @@ -48,8 +51,9 @@ one-click fixes. Has a Claude Code plugin (as of Feb 2026) for tighter integrati
existing hooks. The cloud dependency is the main concern — our code leaves the machine.

**References**:
- https://www.coderabbit.ai/cli
- https://docs.coderabbit.ai/cli/cli-with-self-hosted-CodeRabbit

- <https://www.coderabbit.ai/cli>
- <https://docs.coderabbit.ai/cli/cli-with-self-hosted-CodeRabbit>

---

Expand All @@ -58,6 +62,7 @@ existing hooks. The cloud dependency is the main concern — our code leaves the
**What it is**: Open-source AI PR review agent. Fully self-hosted, bring your own LLM.

**Install**:

```bash
pip install pr-agent
export ANTHROPIC_API_KEY=your_key_here
Expand All @@ -68,13 +73,15 @@ Supports Anthropic, OpenAI, and local models via Ollama. You control what leaves
machine (only LLM API calls).

**Strengths**:

- Fully open source (Apache 2.0)
- Self-hosted at no software cost — you pay only LLM API charges
- Supports Claude as the backing model
- Rich review output: security, correctness, performance
- Can be integrated into git hooks or run on-demand

**Weaknesses**:

- Designed primarily for PR-URL-based review (GitHub/GitLab), not raw local diffs
- Ollama integration has a known config bug (Issue #2098) that can cause it to
ignore local endpoint settings and default to OpenAI — a blocker for air-gapped use
Expand All @@ -86,8 +93,9 @@ need some integration work to run against local diffs in our pre-commit hook rat
than requiring a PR URL. The Anthropic API cost is minimal per review.

**References**:
- https://github.com/qodo-ai/pr-agent
- https://qodo-merge-docs.qodo.ai/installation/locally/

- <https://github.com/qodo-ai/pr-agent>
- <https://qodo-merge-docs.qodo.ai/installation/locally/>

---

Expand All @@ -97,6 +105,7 @@ than requiring a PR URL. The Anthropic API cost is minimal per review.
known vulnerability and bug patterns. Fully offline, no AI, no cloud.

**Install**:

```bash
pip install semgrep
semgrep scan --config auto .
Expand All @@ -107,6 +116,7 @@ are written by Semgrep's security research team based on real-world production
incidents. Supports TypeScript/TSX/React.

**Strengths**:

- Entirely local — code never leaves the machine
- Deterministic: same input always produces same output (no LLM variance)
- Fast — runs in seconds, not minutes
Expand All @@ -116,6 +126,7 @@ incidents. Supports TypeScript/TSX/React.
- Excellent CI integration if we want both local and CI coverage

**Weaknesses**:

- Pattern-matching, not reasoning — catches known anti-patterns but not novel logic bugs
- React Native coverage is partial (React web is better supported)
- Cannot reason about cross-file data flow or architectural concerns
Expand All @@ -126,8 +137,59 @@ to pre-commit alongside existing hooks. It catches a different class of bugs
(known vulnerability patterns) that our AI reviewers might miss, and vice versa.

**References**:
- https://semgrep.dev/docs/getting-started/cli
- https://semgrep.dev/docs/semgrep-code/pro-rules

- <https://semgrep.dev/docs/getting-started/cli>
- <https://semgrep.dev/docs/semgrep-code/pro-rules>

### Convention: scoping out pre-existing findings with `.semgrepignore`

`semgrep scan --staged` is not a valid flag — it doesn't exist. Our pre-commit
hook therefore runs `semgrep scan --config auto --error --quiet .`, which scans
the whole tracked repo on every commit, not just the staged diff. This means
pre-existing findings on files unrelated to the current change can block a
commit that never touched those files (see
[smartwatermelon/dev-env#1](https://github.com/smartwatermelon/dev-env/issues/1)
for the full analysis of alternatives).

The recommended fix, validated in practice in `smartwatermelon/tensegrity`
(2026-04-09) when pre-existing findings in a standalone PoC/scratch HTML
directory — pulling Three.js from a CDN without SRI — blocked an unrelated
commit, is a per-repo `.semgrepignore` file at the repo root:

- Scope it narrowly — specific paths, not broad excludes. The goal is to mask
known, accepted findings on files that don't ship or aren't security-relevant,
not to weaken scanning generally.
- Put an inline comment above each entry explaining _why_ it's excluded (e.g.,
non-shipped scratch/PoC code, never loaded by the app, not exposed to users).
A bare path with no justification is not reviewable.
- Land it as its own `chore:` commit, separate from whatever cleanup or feature
work prompted the discovery — this keeps the carve-out easy to review and
easy to revert independently.

This is the recommended alternative to filtering the `semgrep scan` invocation
itself to staged-file targets (e.g. `git diff --cached --name-only`), which was
considered and rejected in issue #1 in favor of this approach, because
`.semgrepignore`:

1. Is explicit, reviewable, and version-controlled — the exclusion is visible
in the diff, not implicit in how the hook happens to invoke semgrep.
2. Travels with the repo, not the developer's machine — anyone who clones the
repo gets the same exclusion, so the hook behaves consistently everywhere.
3. Leaves security-critical paths scanned by default — only paths explicitly
listed are skipped, so a mistake here is narrow and visible in the diff.
4. Still surfaces new finding classes on non-excluded paths — only
pre-existing findings on the excluded path are masked, not future ones
elsewhere.

Example (illustrative, not tied to any specific repo):

```
# Standalone PoC demo pulling Three.js from a CDN without SRI hashes.
# Non-shipped scratch code: never bundled, never loaded by the app, not
# exposed to users. Pinning to integrity hashes here would be churn with
# no security benefit — excluding rather than fixing.
poc/three-js-demo/
```

---

Expand Down Expand Up @@ -157,7 +219,7 @@ via the `~/.config/git/hooks/pre-commit` hook. It's invoked through `run-review.
Analysis of recent PR reviews (#1028, #1030, #1032, #1042) reveals consistent gaps:

| Gap | Example | Root Cause |
|-----|---------|------------|
| ----- | --------- | ------------ |
| **Cross-file integration** | Feature mention removed from HomeScreen without checking discovery elsewhere | Per-commit scope can't see full feature surface |
| **Platform-specific paths** | Android code path untested when `Platform.OS` guard added | Diff review doesn't consider both platform paths |
| **Silent failure modes** | `ANDROID_HOME` silently passes empty string instead of failing | Requires reasoning about what happens when env vars are unset |
Expand Down Expand Up @@ -254,7 +316,7 @@ full file reads.
### Estimated Effort

| Enhancement | Effort | Impact |
|-------------|--------|--------|
| ------------- | -------- | -------- |
| 4A: Production patterns checklist | ~30 min (prompt edit) | High — directly addresses known gaps |
| 4B: Cross-file awareness prompts | ~15 min (prompt edit) | Medium — generates useful questions |
| 4C: Severity recalibration | ~15 min (prompt edit) | Medium — fewer false negatives |
Expand All @@ -267,7 +329,7 @@ Total: ~1 hour of work, no new tooling, no new dependencies, no cost increase.
## Comparison Matrix

| Criterion | CodeRabbit CLI | PR-Agent | Semgrep | Enhanced Adversarial |
|-----------|---------------|----------|---------|---------------------|
| ----------- | --------------- | ---------- | --------- | --------------------- |
| **Cost** | Free (rate-limited) | LLM API only | Free | Free (existing) |
| **Code leaves machine** | Yes (cloud) | Yes (LLM API) | No | Yes (LLM API) |
| **Setup effort** | 1 min | 30 min | 5 min | 1 hour |
Expand Down
Loading