Skip to content

fix(cicd): scope semgrep filesystem-deletion rule to exclude src/hooks/ - #2143

Closed
maxmilian wants to merge 1 commit into
rtk-ai:developfrom
maxmilian:fix/cicd-semgrep-exclude-hooks
Closed

maxmilian wants to merge 1 commit into
rtk-ai:developfrom
maxmilian:fix/cicd-semgrep-exclude-hooks

Conversation

@maxmilian

Copy link
Copy Markdown
Contributor

Summary

Root cause

.semgrep.yml rule filesystem-deletion matches fs::remove_file(...) / remove_dir_all(...) repo-wide. CI runs semgrep scan --config .semgrep.yml --baseline-commit <base> --error, so any PR adding a new deletion to src/hooks/init.rs (e.g. a new agent's uninstall path) produces a fresh finding and fails the scan — a guaranteed false positive for every future agent-uninstall PR (e.g. Pi support #1741). The existing ~10 deletions in init.rs only pass because they predate the baseline.

Fix

Add paths.exclude: src/hooks/ to the rule, so it flags deletions in filter modules (the surprising case) but not in the hooks/init cleanup code (the expected case).

Verification (semgrep before/after)

Ran semgrep scan --config .semgrep.yml locally:

Target Before (no exclude) After (exclude src/hooks/)
src/hooks/init.rs 10 findings 0
whole src/hooks/ (flagged) 0
src/core/tee.rs (filter module) 1 1 (still flagged)

The rule still catches deletions outside src/hooks/, confirming the exclude isn't over-broad.

Scope

Config-only, 3 lines. No Rust changes, so no unit test applies — the behavior is verified by the semgrep runs above. Does not touch any other rule.

The filesystem-deletion rule matches fs::remove_file/remove_dir_all with no
paths scoping, so it flags src/hooks/ — yet its own message states deletion
is "Expected in hooks/init cleanup". Since CI runs semgrep with
--baseline-commit --error, every new agent-uninstall PR that adds a deletion
to src/hooks/init.rs trips a false positive and fails the scan.

Add paths.exclude: src/hooks/ so the rule reflects its stated intent —
flag deletions in filter modules, not in hooks/init cleanup code.

Fixes rtk-ai#1954
@maxmilian

Copy link
Copy Markdown
Contributor Author

Ran a fresh-context independent review (skeptical, diff + issue only). Verdict: approve, no blockers. Confirmed:

  • The paths:/exclude: block is valid semgrep schema, indented as a sibling key of filesystem-deletion (not nested under a pattern), and mirrors the existing ureq-outside-telemetry rule's paths block. filesystem-deletion is the last rule in the file, so there's no risk of leaking into an adjacent rule.
  • Bare src/hooks/ is the idiomatic recursive directory exclude (src/hooks/* would be non-recursive; ** redundant).
  • Security tradeoff reviewed: the rule is WARNING-level and its stated intent is exactly "expected in hooks/init cleanup", so a directory-level exclude is proportionate. Narrowing to init.rs only would re-introduce false positives from integrity.rs/hook_cmd.rs (which also contain expected uninstall deletions). Left as directory-level deliberately.

@maxmilian

Copy link
Copy Markdown
Contributor Author

Closing this to reduce my own open-PR footprint here, not because the change is wrong.

I opened seven PRs across 26–28 May, which was too many at once from someone with no merge history in this repo — and this one has had no review activity in the two months since. I'd rather leave a short, meaningful queue than a wall of stale PRs on your review list, so I'm keeping only #2105 (approved, single file) and #2142 (backed by the user report in #1830).

The branch stays on my fork and this is trivially reopenable if it's ever wanted — no work is lost.

@maxmilian maxmilian closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

semgrep: filesystem-deletion rule lacks paths:exclude for src/hooks/

1 participant