fix(rules/java): repair invalid query in security-interface-impl#11
fix(rules/java): repair invalid query in security-interface-impl#11
Conversation
The third pattern in `java-security-interface-impl` used `name:
(type_identifier)` inside `scoped_type_identifier`, but per
tree-sitter-java 0.23.5 that node has no fields — only unnamed children.
This caused the rule to fail to compile and be skipped at scan time
("Query error at 18:9. Impossible pattern"), so any class implementing
a fully-qualified Spring Security interface was missed.
Drop the field designator and anchor the capture to the last
type_identifier (the actual scoped name) with `.`. Also adds:
- A test fixture for the fully-qualified form, plus four matcher.rs
unit tests so `cargo test` exercises this rule (it had none).
`cargo run -- rules validate` now reports "All 32 rules validated
successfully." Scans of vbench-server and endorsed_resume no longer
emit the WARN.
Refs #6.
📝 WalkthroughWalkthroughUpdates a Java security interface implementation rule to capture fully-qualified interface names and extends test coverage with four new unit tests verifying detection of Spring Security interface implementations and negative cases. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Review Summary
This PR effectively resolves a critical query compilation error in the security-interface-impl rule. The changes are well-structured and properly tested.
Changes Verified
✅ Fixed invalid name: field designator in tree-sitter query that was causing compilation failure
✅ Added proper anchor syntax (.) to capture the correct type identifier
✅ Added comprehensive test coverage (4 new unit tests) covering plain, scoped, generic, and negative cases
✅ New test fixture for fully-qualified interface names
✅ Tests follow established patterns in the codebase
Validation
The PR description shows thorough validation:
- All 32 rules validate successfully
- All 89 tests pass
- Previously failing scans now work without warnings
No blocking issues found. This is a clean bug fix with appropriate test coverage.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/scanner/matcher.rs (1)
480-543: Optional: consolidate these into a table-driven test.All four tests share the same harness and assertions shape; parameterizing cases would reduce duplication and make future variants easier to add.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scanner/matcher.rs` around lines 480 - 543, These four unit tests (java_security_interface_impl_matches, java_security_interface_impl_scoped_matches, java_security_interface_impl_generic_matches, java_security_interface_impl_no_false_positive) are duplicate harnesses — refactor into a single table-driven test that iterates test cases calling parse_and_match_java with the snippet and include_str!("../../rules/java/security-interface-impl.toml"), asserting either findings.is_empty() or !findings.is_empty() per case; use a Vec of (case_name, source_snippet, expect_match: bool) and iterate asserting with the case_name to preserve per-case failure messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/scanner/matcher.rs`:
- Around line 480-543: These four unit tests
(java_security_interface_impl_matches,
java_security_interface_impl_scoped_matches,
java_security_interface_impl_generic_matches,
java_security_interface_impl_no_false_positive) are duplicate harnesses —
refactor into a single table-driven test that iterates test cases calling
parse_and_match_java with the snippet and
include_str!("../../rules/java/security-interface-impl.toml"), asserting either
findings.is_empty() or !findings.is_empty() per case; use a Vec of (case_name,
source_snippet, expect_match: bool) and iterate asserting with the case_name to
preserve per-case failure messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 369d98dd-6d3a-4ab9-b8ac-4ffaaf9cb179
📒 Files selected for processing (2)
rules/java/security-interface-impl.tomlsrc/scanner/matcher.rs
Summary
The third pattern in
rules/java/security-interface-impl.tomlusedname: (type_identifier)insidescoped_type_identifier, but per tree-sitter-java 0.23.5node-types.jsonthat node has no fields — only unnamed children of typesannotation,generic_type,marker_annotation,scoped_type_identifier, andtype_identifier.Result: the entire rule failed to compile, was skipped at scan time, and emitted a
WARNon every Java scan ("Query error at 18:9. Impossible pattern"). Any class implementing a fully-qualified Spring Security interface (e.g.implements org.springframework.security.core.userdetails.UserDetailsService) was silently missed.Changes
rules/java/security-interface-impl.tomlname:field designator from thescoped_type_identifierpattern.type_identifierwith.so we capture the actual scoped name (e.g.UserDetailsService) rather than the package prefix.implements org.springframework.security.core.userdetails.UserDetailsService.src/scanner/matcher.rs— add fourcargo testunit tests for this rule (it had none, which is why the bug went unnoticed):java_security_interface_impl_matches(plain)java_security_interface_impl_scoped_matches(fully-qualified)java_security_interface_impl_generic_matches(AuthorizationManager<...>)java_security_interface_impl_no_false_positive(implements Serializable)Verification
Before this change, scans on the issue's sample tarballs printed the WARN. After, they don't.
Refs #6 (the broken-rule half of the issue; the larger custom-authz-service detection is tracked separately).
Notes
While running
cargo run -- rules test, I noticedjava-access-decision-voter[1]is failing pre-existing — it expectsextends AbstractAccessDecisionManagerto match but the query doesn't. Out of scope here; flagging for a follow-up.Test plan
cargo fmt -- --checkcargo clippy --tests -- -D warningscargo testcargo run -- rules validatecargo run -- rules test— only pre-existingjava-access-decision-voter[1]failure remainsSummary by CodeRabbit
Tests
Bug Fixes