feat: javax/jakarta annotation coverage, JAX-RS rule, Route category#80
Merged
Conversation
Java annotation rules previously matched only bare-identifier names, so fully-qualified usages like `@jakarta.annotation.security.RolesAllowed` and the `javax.*` siblings were silently missed across the Jakarta EE 9 package migration boundary. Extend each annotation rule's `name:` field to accept `scoped_identifier` alongside `identifier`, mirroring the alternation already in `access-decision-voter.toml` and `security-interface-impl.toml`. Predicate matching stays on the trailing identifier so `eq`/`match` rules apply uniformly. Add a new `jaxrs-endpoint.toml` rule covering `@GET`/`@POST`/`@PUT`/ `@DELETE`/`@PATCH`/`@HEAD`/`@OPTIONS`/`@Path` from both `javax.ws.rs.*` and `jakarta.ws.rs.*` so unprotected REST endpoints are visible to the structural pass. Inline `[[rule.tests]]` cover bare and fully-qualified forms (jakarta, javax, framework-qualified) on every touched rule.
Surface the package-prefix of fully-qualified Java annotations as a structured `Finding.provenance` field so downstream tooling can split `javax.*` (legacy) vs `jakarta.*` (modern) call sites without grepping the snippet text. Rule TOML grows a new opt-in `provenance_capture` field naming the capture whose matched text should flow onto the finding; `compile_rule` validates the name resolves to a real query capture. Bare-identifier annotation matches leave provenance unset because the package isn't resolvable from the call site alone. All ten Java annotation rules updated by the prior commit plus the new `jaxrs-endpoint.toml` rule now expose an `@anno_scope` capture on the scoped-identifier branch and declare `provenance_capture = "anno_scope"`. The Finding shim folds the new field through serde with `#[serde(default)]` so legacy persisted findings keep loading.
The /commit flow used to only check whether HEAD was `main`. That missed the common case where someone is parked on a feature branch named for a different issue/PR than the work being committed (e.g. the #71 branch when implementing #58). Tighten the branch-check step to also cover that case and the already-merged-upstream case, and add the cherry-pick-and-stash-pop recipe so the new branch picks up any prior commits that belong to the new work.
JAX-RS / Jakarta REST `@GET`, `@POST`, `@Path` etc. were landing as `middleware`, conflating route declarations with route-level auth guards. The annotations only assert "this is an endpoint" — the absence of a paired auth annotation is what matters — so they belong in their own bucket. Add `AuthCategory::Route`, wire it through the slug, Display, MCP tool / resource enums, deep-scan system prompt, and output schema. Both engines get TODO-style default templates since route declarations don't generate concrete policies on their own. Reclassify `java-jaxrs-endpoint` from `middleware` to `route` and swap the awkward inner-class fixture for a class-level `@Path` placement.
CI ran only fmt/clippy/test, never `cargo run -- rules validate` or
`rules test` — so broken policy templates and rule fixtures could
land on main unflagged. Three cedar templates (java-roles-allowed-array,
java-shiro-requires-roles-array, csharp-aspnet-authorize-roles) had
been failing template validation on main without anyone noticing.
Root cause was in `validate_template`: bare `{{var}}` placeholders were
substituted with the literal identifier `placeholder_value`, which is
fine in identifier position (`principal.{{attribute}}`) but invalid
inside a set literal — `[{{cedar_roles_set}}]` became
`[placeholder_value]`, which Cedar rejects. Detect the set-context
shape `[{{var}}]` and substitute `["placeholder"]` so the rendered
template parses cleanly.
Add the two missing CI steps so this class of regression can't repeat.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds optional package-namespace provenance to findings, introduces an AuthCategory::Route and a JAX-RS endpoint rule, broadens Java annotation rules to accept fully-qualified/scoped identifiers and capture provenance, updates scanner compilation/execution, adds route templates and Cedar placeholder handling, extends deep prompt/schema/tooling, updates tests, CI, and commit guidance. ChangesAuthorization Provenance and Route Detection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR delivers substantial improvements to Java annotation coverage and policy validation while maintaining backward compatibility. The implementation is well-tested with 417 unit tests passing and all 80 rules validated successfully.
Key Additions:
- Comprehensive javax/jakarta annotation coverage for JSR-250, Spring Security, Shiro, and JAX-RS
- New
Routecategory for HTTP endpoint declarations (JAX-RS annotations) - Provenance field to track package prefixes for migration reporting
- Cedar template validation fix for set placeholders
- CI validation steps for rules and policy templates
Code Quality:
- Extensive test coverage with unit tests and integration tests
- Proper backward compatibility through
FindingShimdeserialization - Clear documentation and well-structured code
- All validation steps passing in CI
The changes are production-ready and ready to merge.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/deep/context.rs (1)
176-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard marker append when
max_charsis smaller than marker + imports budgetAt Line 176–183, if
max_charsis very small,snippet_budgetsaturates to0but the code still appendsTRUNCATION_MARKER, sosnippet.len() + imports_lencan exceedmax_chars. This breaks the stated combined-budget guarantee.Proposed fix
- let snippet_budget = max_chars - .saturating_sub(TRUNCATION_MARKER.len()) - .saturating_sub(imports_len); - if snippet.len() > snippet_budget { - let cut = snippet.floor_char_boundary(snippet_budget); - snippet.truncate(cut); - snippet.push_str(TRUNCATION_MARKER); - } + let marker_fits = max_chars.saturating_sub(imports_len) >= TRUNCATION_MARKER.len(); + let snippet_budget = if marker_fits { + max_chars + .saturating_sub(TRUNCATION_MARKER.len()) + .saturating_sub(imports_len) + } else { + max_chars.saturating_sub(imports_len) + }; + + if snippet.len() > snippet_budget { + let cut = snippet.floor_char_boundary(snippet_budget); + snippet.truncate(cut); + if marker_fits { + snippet.push_str(TRUNCATION_MARKER); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/deep/context.rs` around lines 176 - 183, The current truncation logic can append TRUNCATION_MARKER even when snippet_budget is smaller than the marker length, violating the combined max_chars + imports_len guarantee; change the block that computes snippet_budget and truncates to only append TRUNCATION_MARKER when snippet_budget >= TRUNCATION_MARKER.len(), otherwise truncate the snippet to at most snippet_budget (possibly empty) without appending the marker. Update the code around snippet_budget, snippet.floor_char_boundary, snippet.truncate and snippet.push_str to check snippet_budget >= TRUNCATION_MARKER.len() before calling push_str and use the proper cut computed from snippet_budget (not 0) so final snippet.len() + imports_len <= max_chars.
🧹 Nitpick comments (2)
rules/java/shiro-requires-authentication.toml (1)
50-57: 💤 Low valueConsider adding FQ fixtures for
RequiresGuestandRequiresUserto match coverage ofRequiresAuthentication.The predicate covers all three names equally, but only
RequiresAuthenticationhas a qualified-form test. Two extra[[rule.tests]]blocks would give symmetrical coverage for the full annotation family the rule declares.🧪 Suggested fixtures
+[[rule.tests]] +input = """ +public class SecureController { + `@org.apache.shiro.authz.annotation.RequiresGuest` + public void guestAction() { } +} +""" +expect_match = true + +[[rule.tests]] +input = """ +public class SecureController { + `@org.apache.shiro.authz.annotation.RequiresUser` + public void userAction() { } +} +""" +expect_match = true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rules/java/shiro-requires-authentication.toml` around lines 50 - 57, Add fully-qualified annotation fixtures for RequiresGuest and RequiresUser to match the existing qualified-form test for RequiresAuthentication: duplicate the existing [[rule.tests]] block that uses the FQ form (e.g., `@org.apache.shiro.authz.annotation.RequiresAuthentication`) and replace the annotation name with org.apache.shiro.authz.annotation.RequiresGuest and org.apache.shiro.authz.annotation.RequiresUser respectively so the predicate is exercised for all three annotation names (RequiresAuthentication, RequiresGuest, RequiresUser).src/cedar/templates.rs (1)
135-145: 💤 Low valueConsider adding a unit test for the new
AuthCategory::Routearm.
default_template(AuthCategory::Middleware),AuthCategory::Ownership, etc. all have direct test coverage in this module. The newRoutearm is the only addition without one. TheBusinessRule/Custompattern is already a precedent without tests, but for a brand-new arm a one-liner assertion prevents silent regressions if the stub text is accidentally clobbered.🧪 Suggested test addition
+ #[test] + fn default_stub_route_returns_commented_todo() { + let stub = generate_default_stub(AuthCategory::Route, ""); + assert!(stub.contains("TODO: route declaration")); + assert!(!stub.contains("permit (")); // must be commented out + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cedar/templates.rs` around lines 135 - 145, Add a unit test that exercises default_template(AuthCategory::Route) and asserts its returned string contains the expected stub comment (the TODO route declaration stub) to prevent regressions; locate the enum AuthCategory and the default_template function in templates.rs, create a test in the existing tests module (or add a new #[cfg(test)] mod) that calls default_template(AuthCategory::Route) and compares or contains-matches the exact template text used for the Route arm.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/deep/context.rs`:
- Around line 176-183: The current truncation logic can append TRUNCATION_MARKER
even when snippet_budget is smaller than the marker length, violating the
combined max_chars + imports_len guarantee; change the block that computes
snippet_budget and truncates to only append TRUNCATION_MARKER when
snippet_budget >= TRUNCATION_MARKER.len(), otherwise truncate the snippet to at
most snippet_budget (possibly empty) without appending the marker. Update the
code around snippet_budget, snippet.floor_char_boundary, snippet.truncate and
snippet.push_str to check snippet_budget >= TRUNCATION_MARKER.len() before
calling push_str and use the proper cut computed from snippet_budget (not 0) so
final snippet.len() + imports_len <= max_chars.
---
Nitpick comments:
In `@rules/java/shiro-requires-authentication.toml`:
- Around line 50-57: Add fully-qualified annotation fixtures for RequiresGuest
and RequiresUser to match the existing qualified-form test for
RequiresAuthentication: duplicate the existing [[rule.tests]] block that uses
the FQ form (e.g., `@org.apache.shiro.authz.annotation.RequiresAuthentication`)
and replace the annotation name with
org.apache.shiro.authz.annotation.RequiresGuest and
org.apache.shiro.authz.annotation.RequiresUser respectively so the predicate is
exercised for all three annotation names (RequiresAuthentication, RequiresGuest,
RequiresUser).
In `@src/cedar/templates.rs`:
- Around line 135-145: Add a unit test that exercises
default_template(AuthCategory::Route) and asserts its returned string contains
the expected stub comment (the TODO route declaration stub) to prevent
regressions; locate the enum AuthCategory and the default_template function in
templates.rs, create a test in the existing tests module (or add a new
#[cfg(test)] mod) that calls default_template(AuthCategory::Route) and compares
or contains-matches the exact template text used for the Route arm.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ea7f7c9-5fe8-4f44-8c30-3374088e4285
📒 Files selected for processing (33)
.agents/commands/commit.md.github/workflows/ci.ymlrules/java/authorized-annotation.tomlrules/java/jaxrs-endpoint.tomlrules/java/shiro-requires-authentication.tomlrules/java/shiro-requires-permissions.tomlrules/java/shiro-requires-roles-array.tomlrules/java/shiro-requires-roles.tomlrules/java/spring-permit-all.tomlrules/java/spring-preauthorize.tomlrules/java/spring-roles-allowed-array.tomlrules/java/spring-roles-allowed.tomlrules/java/spring-secured.tomlsrc/cedar/grouping.rssrc/cedar/templates.rssrc/cedar/validator.rssrc/deep/candidate.rssrc/deep/context.rssrc/deep/finding.rssrc/deep/merge.rssrc/deep/prompt.rssrc/mcp/resources.rssrc/mcp/tools.rssrc/output/json.rssrc/output/text.rssrc/rego/grouping.rssrc/rego/templates.rssrc/rules/embedded.rssrc/rules/mod.rssrc/scanner/matcher.rssrc/types.rstests/deep_http_integration.rstests/deep_subprocess_integration.rs
- Guard truncation marker append in deep context when budget is too small - Add FQ-form Shiro fixtures for RequiresGuest and RequiresUser - Add unit test for Cedar Route default template
Contributor
Author
|
@coderabbitai Addressed in 279ece3:
|
|
Tip For best results, initiate chat on the files or code changes.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the javax/jakarta migration coverage gap in Java annotation rules, adds a JAX-RS endpoint rule, introduces a
Routeauth category for endpoint surface declarations, and lands the Phase BPolicyGeneratorrefactor that collapses parallel Rego/Cedar fields. Also closes a CI gap that let broken policy templates land on main unflagged.Changes
scoped_identifierforms —@RolesAllowedand@jakarta.annotation.security.RolesAllowedboth fire. Newjava-jaxrs-endpointrule covers@GET/@POST/@PUT/@DELETE/@PATCH/@HEAD/@OPTIONS/@Pathfromjavax.ws.rs.*andjakarta.ws.rs.*.provenance_capture = "anno_scope"so the package prefix (jakarta.annotation.security) flows ontoFinding.provenance. Consumers split on the head segment forjavax-vs-jakartamigration reports without grepping snippets. Compile-time validates the capture name resolves.AuthCategory::Route: new bucket for HTTP route declarations (JAX-RS@GET, future Spring@RequestMapping, etc.). Reclassifiesjava-jaxrs-endpointfrommiddlewaretoroutesince these annotations only assert "endpoint exists" — the absence of a paired auth annotation is the signal. Wired through MCP tools/resources, deep-scan prompt, and output schema.PolicyGeneratortrait (refactor: extract PolicyGenerator trait and collapse parallel engine fields #79): collapses the parallel Rego/Cedar codepaths in extract/MCP/grouping behind one engine-keyed dispatch surface.Findingnow carries aVec<PolicyOutput>instead ofrego_stub/cedar_stub; serde shim folds legacy JSON into the new shape so persisted findings keep loading.cargo run -- rules validateandrules testnow run in CI. Three cedar templates (java-roles-allowed-array,java-shiro-requires-roles-array,csharp-aspnet-authorize-roles) had been failing template validation on main. Root cause was invalidate_template: bare{{var}}placeholders were substituted with the literal identifierplaceholder_value, invalid inside Cedar set literals. Detect the[{{var}}]shape and substitute["placeholder"]instead.Test plan
cargo fmt --checkcargo clippy --all-features -- -D warningscargo test --all-features— 417 unit + 18 + 6 + 6 + 8 integration passedcargo run -- rules validate— All 80 rules validated successfullycargo run -- rules test— 393 fixtures passed, 0 failedprovenanceround-trips through serde for bothjavax.*andjakarta.*qualified annotationsrego_stub/cedar_stubJSON deserializes intopolicy_outputsFixes #58
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores