fix(group): handle named annotation args in Java Spring route extraction#1834
Conversation
The Java HTTP plugin only matched positional `@RequestMapping("/path")`
syntax for class-level prefixes and method-level routes. Named argument
forms (`path = "/path"` and `value = "/path"`) produce an
`element_value_pair` AST node that the tree-sitter queries did not cover,
causing the class prefix to be lost and named-arg method routes to be
missed entirely during cross-repo contract extraction.
Add a second pattern to both SPRING_CLASS_PREFIX_PATTERNS and
SPRING_METHOD_ROUTE_PATTERNS matching the element_value_pair structure.
|
Someone is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 9984 tests passed 4 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Claude finished @magyargergo's task in 5m 5s —— View job PR #1834 Review — fix(group): handle named annotation args in Java Spring route extractionTasks
Review bar for this PRAdversarial production-readiness review. The query touches Spring contract extraction directly — a tiny overbroad capture poisons contract IDs across all Java Spring services analyzed by GitNexus. The bar is correctness-first, not CI-green. Problem being solvedSpring annotations accept named arguments in addition to the positional (shorthand) form. The existing positional patterns Current PR stateOpen. Single functional commit ( Merge status and mergeabilityThe branch has been synced with main (merge commit Repository history considered
Branch hygiene assessmentClean. One functional commit, one file, no unrelated churn, no reformatting of surrounding code. Understanding of the changeThe PR adds a second (element_value_pair
value: (string_literal) @prefix) // class bundle
(element_value_pair
value: (string_literal) @path) // method bundleFindingsFinding A — Query captures ALL named annotation attributes with string values, not just
|
| Annotation | Matched capture | Emitted route (bogus) |
|---|---|---|
@GetMapping(produces = "application/json") |
"application/json" |
http::GET::/application/json |
@GetMapping(name = "listUsers", value = "/users") |
"listUsers" AND "/users" |
http::GET::/listUsers (extra) + http::GET::/users (correct) |
@RequestMapping(headers = "X-Foo=bar", path = "/api") |
"X-Foo=bar" AND "/api" |
Both are captured; order-dependent — if headers is listed after path, "X-Foo=bar" is the final value in prefixByClassId for this class |
@PostMapping(consumes = "application/json", value = "/orders") |
both | http::POST::/application/json (extra) |
For the class prefix case, prefixByClassId.set(classNode.id, prefix) is called for every match. Since runCompiledPatterns iterates all matches in document order (tree-sitter match ordering), the last element_value_pair in the annotation sets the class prefix. So @RequestMapping(path = "/api", name = "myApi") would use "myApi" as the class prefix — stripping the correct prefix entirely and producing routes like http::GET::/myApi/users.
For the method route case, each element_value_pair match produces a separate HttpDetection via out.push(...), so you get one detection per named attribute with a string value — multiple bogus contracts per method.
Recommended fix: Add key: (identifier) @key (#match? @key "^(path|value)$") to both new element_value_pair patterns, exactly as topic-patterns/java.ts already does for topics and queues:
(element_value_pair
key: (identifier) @key (#match? @key "^(path|value)$")
value: (string_literal) @prefix)Blocks merge: YES — confirmed false positive route emission.
Finding B — value: field in tree-sitter query covers BOTH path = and value = forms ✅ Not a bug
Risk: The PR description states named arguments include both path = "/api" and value = "/api". There was a concern that value: (string_literal) in the query might only match the annotation member named value.
Evidence: value: in the query is a tree-sitter field name selector, not a string match. It selects the value field of element_value_pair, which is the right-hand side of any named argument — regardless of the key. So path = "/api" produces element_value_pair { key: "path", value: string_literal("/api") } and value = "/api" produces element_value_pair { key: "value", value: string_literal("/api") }. Both are captured by value: (string_literal).
Conclusion: Both path = and value = forms are handled. Finding B is not a bug — but it is precisely why Finding A is a confirmed problem: the over-matching is what enables both to work.
Blocks merge: NO
Finding C — No regression tests added ✴ BLOCKS MERGE
Risk: The PR changes only production TypeScript. Zero test files are modified. The existing http-route-extractor.test.ts tests use exclusively positional annotation forms (@RequestMapping("/api/v2"), @GetMapping("/users")). The bug being fixed — named annotation arguments — is not exercised by any existing test.
Evidence: git diff --name-only for the PR shows only gitnexus/src/core/group/extractors/http-patterns/java.ts. The existing test suite passes because it doesn't call the new code path at all. The new patterns can be deleted or broken without any test failing.
From DoD.md §2.7: "Tests cover the real changed path — they would fail if behavior, wiring, or contracts were broken, not only if a mock were misconfigured."
From DoD.md §6 "Not Done" signals: "The runtime path is not actually exercised by the tests."
Missing tests (minimum required):
- Class-level
@RequestMapping(path = "/api")→ class prefix extracted correctly - Class-level
@RequestMapping(value = "/api")→ class prefix extracted correctly - Method-level
@GetMapping(value = "/users")→ route extracted correctly - Method-level
@GetMapping(path = "/users")→ route extracted correctly - Mixed class named + method positional:
@RequestMapping(path = "/api") + @GetMapping("/users")→ combined path correct - Mixed class positional + method named:
@RequestMapping("/api") + @GetMapping(value = "/users")→ combined path correct - Non-route attributes should NOT produce routes:
@GetMapping(produces = "application/json")→ no provider detection - Multi-attribute:
@GetMapping(name = "listUsers", value = "/users")→ exactly one provider, path "/users" - Class multi-attribute with non-route key:
@RequestMapping(name = "myApi", path = "/api")→ prefix is "/api", not "myApi"
Recommended fix: Add focused tests to gitnexus/test/unit/group/http-route-extractor.test.ts in the 'provider extraction — source-scan fallback (Strategy B)' describe block.
Blocks merge: YES — per DoD §2.7 and §6.
Finding D — Array-valued Spring annotations still not handled ℹ️ Not blocking, pre-existing limitation
Risk: Spring path and value are String[]. @GetMapping({"/a", "/b"}) and @GetMapping(value = {"/a", "/b"}) are not handled.
Evidence: Both the existing positional patterns and the new named patterns only match string_literal, not array_initializer. This is a pre-existing gap — the PR does not regress it. The PR description does not claim array support.
Recommended fix: Document as a known limitation or track against #1225. Not required for this PR.
Blocks merge: NO
Finding E — Empty method path with named arg is consistent with positional behavior ✅ Not a bug
Risk: @GetMapping(value = "") on a method with a class prefix might produce a trailing-slash contract ID.
Evidence: joinPath(prefix, "") returns "/" + cleanPrefix + "/" — a trailing slash. This is identical to the positional @GetMapping("") behavior (pre-existing, not introduced here). No regression.
Blocks merge: NO
Finding F — Interface attribution unchanged ✅ Not a problem
Risk: Adding named-arg support to class-level @RequestMapping might expand false provider attribution for annotated interfaces.
Evidence: The new patterns use (class_declaration ...) — not (interface_declaration ...). Interface-annotated types are unchanged by this PR. No interaction with draft PR #1743.
Blocks merge: NO
Hidden Unicode / bidi check ✅ Clean
Non-ASCII characters in the changed file are decorative box-drawing characters (─, →) used in section-header comments, matching the pre-existing file style. No hidden bidi controls detected.
PR-specific assessment sections
1. Spring annotation semantics lane: Both path and value scalar string cases are handled (when the key filter is fixed). Array-valued forms (String[]) are not handled, but were never handled and the PR does not claim they are. This is acceptable scope.
2. Tree-sitter query correctness lane: FAILED. The value: field selector is a tree-sitter structural field name, not a filter on annotation member names. The query matches produces, consumes, headers, name, params, etc. — any named attribute with a string value. The sibling file topic-patterns/java.ts demonstrates the required key: constraint. The PR author appears to have confused the tree-sitter field selector syntax with annotation member name matching.
3. Group sync / contract matching lane: Bogus contracts like http::GET::/application/json and http::GET::/listUsers would be emitted and synced as provider contracts. These would never match real consumers, producing false unmatch reports — the opposite of the stated fix. In multi-attribute annotations where a non-route key precedes the route key (e.g., @RequestMapping(headers = "X-Foo=bar", path = "/api")), the class prefix could be set to "X-Foo=bar" instead of "/api", corrupting every route under that controller.
4. Regression compatibility lane: Existing positional patterns are untouched. Consumer extraction (RestTemplate, WebClient, OkHttp) is untouched. Graph-assisted strategy still takes precedence. No regression in existing behavior.
5. Tests and CI lane: CI is green because the existing tests don't exercise the new code path at all. The new code path has zero test coverage. This fails the DoD §2.7 bar explicitly.
6. Repository-history alignment lane: The PR is correctly narrow — it does not claim to solve constants, arrays, property placeholders, interfaces, or graph ingestion. It is consistent with the group source-scan plugin architecture introduced after PR #619 was closed.
Back-and-forth avoided by verifying
- Is
value:the tree-sitter field or the annotation member name? → Verified viatopic-patterns/java.tsin the same repo, which proves the field isvalue:(structural) and the key name is filtered separately viakey: (identifier) @key (#eq? @key "..."). - Does the existing test suite cover named args? → Verified by reading all test cases: only positional forms used in all Spring Java fixtures.
- Does the query actually handle
path =forms? → Yes, becausevalue:captures the right-hand side of any pair, includingpath = "/api"— but that same fact makes it captureproduces = "application/json"too. - Are merges/conflicts an issue? → Branch is cleanly merged with main; no conflict.
Open questions
None required. All key issues have been verified directly from code.
Final verdict
not production-ready
The new element_value_pair queries lack a key: field constraint, which exists in the adjacent topic-patterns/java.ts and is required in tree-sitter to restrict matching to annotation members named path or value. Without it, the query captures any named attribute with a string value — name, produces, consumes, headers, params — emitting bogus route contracts that will appear as unmatched providers in group sync, which is the opposite of the stated fix. The required change is one line per pattern (key: (identifier) @key (#match? @key "^(path|value)$")). Additionally, the PR ships with zero regression tests for the new code path; the existing tests pass only because they never exercise the named-arg queries. Both issues must be resolved before merge per DoD §2.7 and §6.
|
@henry201605 could you please look into claude's findings? 🙏 |
… regression tests Address Claude review on PR abhigyanpatwari#1834. The named-argument patterns added in 8b6fa6e used `value: (string_literal)` (a tree-sitter field selector for the right-hand side of element_value_pair), which matched ANY annotation member with a string value — not just `path`/`value`. Concrete fallout (without this fix): @GetMapping(produces = "application/json") → bogus http::GET::/application/json @GetMapping(name = "listUsers", value = "/users") → extra http::GET::/listUsers @RequestMapping(headers = "X-Foo=bar", path = "/api") → class prefix could be set to "X-Foo=bar" because prefixByClassId.set runs per match in document order, so the LAST element_value_pair wins. The sibling topic-patterns/java.ts already demonstrates the correct shape: constrain the `key:` field to the route member names. This commit: - Adds `key: (identifier) @key (#match? @key "^(path|value)$")` to both SPRING_CLASS_PREFIX_PATTERNS and SPRING_METHOD_ROUTE_PATTERNS named-arg queries. - Adds 9 regression tests under `provider extraction — source-scan fallback (Strategy B)`: * @RequestMapping(path = "/api/v3") class prefix * @RequestMapping(value = "/orders") class prefix * @GetMapping(value = "/users") method route * @PostMapping(path = "/users") method route * mixed: class named-arg + method positional * mixed: class positional + method named-arg * @GetMapping(produces = "application/json") → no provider emitted * @GetMapping(name = "listUsers", value = "/users") → exactly one provider with path "/users", no /listUsers route * @RequestMapping(path = "/api", name = "myApi") → prefix is /api, not myApi (verifies the class-prefix overwrite scenario) Tests: 42/42 pass in http-route-extractor.test.ts; 522/522 pass under test/unit/group; npx tsc --noEmit clean.
…st verbatim Claude review on PR abhigyanpatwari#1834 explicitly asked for the method-level `@GetMapping(path = "/users")` case. The previous commit covered it indirectly by exercising path= on @PostMapping (the Spring method annotations share the same query, so any verb proves the path= field is matched). Add a dedicated GET+path= test so the reviewer's checklist is satisfied 1:1, and keep the POST+path= case as a bonus verb-coverage test. Tests: 43/43 pass in http-route-extractor.test.ts.
|
@magyargergo thanks for the review — addressed in Finding A — (element_value_pair
key: (identifier) @key (#match? @key "^(path|value)$")
value: (string_literal) @prefix)This stops the query from capturing non-route attributes like Finding C — Added 10 regression tests under
Without the Local validation:
|
|
@henry201605 would it be difficult to add it to kotlin as well? |
|
@henry201605 Could you please raise a new PR for kotlin? 🙏 |
|
Hi @magyargergo, thanks for the nudge! Already raised as #1849 earlier today ( A quick summary so the review is easy:
Scope note (worth flagging up front): this PR is providers only. Kotlin-side consumer detection (RestTemplate / WebClient / OkHttp call sites) is intentionally deferred to a follow-up. The Kotlin call-site AST (infix calls, extension functions, Happy to iterate on anything that comes up in review — and I'll open the consumer-side follow-up PR right after this one lands if that ordering works for you. |
2 participants
Problem
The Java HTTP source-scan plugin fails to extract routes when Spring annotations
use named arguments:
Only the positional form
@RequestMapping("/translator")was handled.This causes cross-repo contract matching to fail — providers are extracted
without their class-level prefix (e.g.
/findtranslatedlistpageinstead of/translator/findtranslatedlistpage), sogroup syncreports them as unmatched.Root Cause
The tree-sitter AST differs between positional and named annotation arguments:
@RequestMapping("/api")→annotation_argument_list > string_literal@RequestMapping(path = "/api")→annotation_argument_list > element_value_pair > string_literalThe existing queries only matched the first shape.
Fix
Add a second pattern to both
SPRING_CLASS_PREFIX_PATTERNSandSPRING_METHOD_ROUTE_PATTERNSthat matcheselement_value_pair > value: (string_literal).Testing
http-route-extractor.test.tstests passnpx tsc --noEmitclean