Skip to content

fix(rules+cache): post_match_filter ID mismatch + cache key missing provider/base_url - #479

Merged
ajianaz merged 1 commit into
developfrom
fix/rule-id-mismatch-and-cache-key
Aug 4, 2026
Merged

fix(rules+cache): post_match_filter ID mismatch + cache key missing provider/base_url#479
ajianaz merged 1 commit into
developfrom
fix/rule-id-mismatch-and-cache-key

Conversation

@ajianaz

@ajianaz ajianaz commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

What

Fixes two bugs found during codebase audit:

Bug 1: post_match_filter rule ID mismatch

  • Builtin rule 'sec-hardcoded-secret' (in rules/builtin.rs) was never filtered by post_match_filter
  • The match arm only checked "crypto/hardcoded-secret" (the ID used by security_scanner.rs)
  • Result: false positive suppression for empty strings, Svelte $state(), bind: patterns did not work when the builtin rule engine matched hardcoded secrets

Bug 2: Cache key missing provider and base_url

  • cache_key() only hashed diff + model + temperature
  • Switching providers (e.g., ZAI to OpenAI) with the same model name returned stale cache from the previous provider
  • Different base_url (e.g., proxy vs direct) also shared the same cache entry

Why

  • Bug 1: Users get false positive findings for patterns like let password = "" or let formSecret = $state("") -- exactly what issue False positives: SVG xmlns, form variable names, and duplicate rule engines #357 was meant to fix, but only half the code paths were covered
  • Bug 2: Silent correctness issue -- cached reviews from a different provider are semantically incorrect and may have different quality, system prompts, or token limits

Testing

  • cargo test --bin cora -- 781 pass, 0 fail (5 new tests added)
  • cargo clippy --bin cora -- -D warnings -- clean
  • cargo test --test "*" -- 6 integration pass
  • Cora pre-commit review -- No issues found

New tests:

  • builtin_rule_id_secret_empty_string_is_false_positive -- verifies sec-hardcoded-secret ID is now filtered
  • builtin_rule_id_secret_svelte_state_is_false_positive -- verifies Svelte $state() suppression works for builtin ID
  • builtin_rule_id_secret_actual_hardcoded_is_real_finding -- verifies real secrets still detected
  • cache_key_differs_for_different_providers -- verifies provider isolation
  • cache_key_differs_for_different_base_urls -- verifies base_url isolation

@ajianaz
ajianaz force-pushed the fix/rule-id-mismatch-and-cache-key branch from f9159e3 to 18da077 Compare August 4, 2026 10:10
…provider/base_url to cache key

Bug 1: post_match_filter ID mismatch (sec-hardcoded-secret vs crypto/hardcoded-secret)
- Builtin rule 'sec-hardcoded-secret' was never filtered by post_match_filter
  because the match arm only checked 'crypto/hardcoded-secret' (security_scanner.rs ID)
- Added 'sec-hardcoded-secret' as an alias in the match arm
- Added regression tests for builtin rule ID

Bug 2: Cache key missing provider and base_url
- cache_key() only hashed diff+model+temperature
- Switching providers with same model name returned stale cache from previous provider
- Added provider and base_url to cache_key, get_cached_review, save_cached_review
- Added regression tests for provider/base_url cache isolation

Tests: 781 pass (5 new), clippy clean, 6 integration pass
@ajianaz
ajianaz force-pushed the fix/rule-id-mismatch-and-cache-key branch from 18da077 to c44724c Compare August 4, 2026 10:41
@ajianaz
ajianaz merged commit 072bd23 into develop Aug 4, 2026
13 checks passed
@ajianaz
ajianaz deleted the fix/rule-id-mismatch-and-cache-key branch August 4, 2026 10:47
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.

1 participant