feat(group): add Kotlin Spring HTTP route extraction (named + positional)#1849
Conversation
…nal) Mirror the Java Spring named-argument fix for Kotlin Spring Boot controllers. Adds a new `http-patterns/kotlin.ts` plugin behind the optional `tree-sitter-kotlin` grammar, registered for `.kt`/`.kts`. Both annotation forms produce providers: @RequestMapping("/api") / @GetMapping("/users") @RequestMapping(path = "/api") / @GetMapping(value = "/users") @RequestMapping(value = "/api") / @GetMapping(path = "/users") The Kotlin AST (fwcd/tree-sitter-kotlin) shares one node type (`value_argument`) for positional and named forms, so the queries are split: - positional: anchors `string_literal` as the first named child of `value_argument` via the immediate-child anchor `.` - named: explicitly captures `simple_identifier` and constrains it to `^(path|value)$` via `#match?`, mirroring the same safety bar enforced by `http-patterns/java.ts` and `topic-patterns/java.ts`. Without this constraint the query would also capture non-route attributes like `produces`, `consumes`, `headers`, `name`, `params`. `tree-sitter-kotlin` is an optionalDependency (parser-loader.ts, parse-worker.ts pattern). When the native binding is unavailable the plugin exports `null` and `index.ts` skips registering `.kt`/`.kts` so the orchestrator stays healthy. Scope: providers only. Consumer detection (RestTemplate, WebClient, OkHttp) on Kotlin call-site ASTs differs enough from Java's `method_invocation` shape to warrant a separate, focused PR. Tests: 11 new cases under `provider extraction — source-scan fallback (Strategy B)`, gated by the kotlin grammar availability. positive (8) - class @RequestMapping("/api/v1") (positional) - class @RequestMapping(path = "/api/v2") - class @RequestMapping(value = "/orders") - method @GetMapping(value = "/users") - method @GetMapping(path = "/users") - method @PostMapping(path = "/users") - mixed: class named-arg + method positional - mixed: class positional + method named-arg anti-regression (3) - @GetMapping(produces = "application/json") emits no provider - @GetMapping(name = "x", value = "/users") emits exactly one provider - @RequestMapping(path = "/api", name = "myApi") prefix stays /api Reverse-validated: removing the `(#match? @key "^(path|value)$")` constraint causes precisely the 3 anti-regression tests to fail. Local validation: - test/unit/group/http-route-extractor.test.ts: 54/54 - test/unit/group: 534/534 - npx tsc --noEmit: clean (modulo the pre-existing TS2339 in user-defined-conversions.ts merged from main, unrelated)
|
Someone is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
✨ PR AutofixFound fixable formatting / unused-import issues across 94 changed lines. Comment |
|
Claude finished @magyargergo's task in 5m 30s —— View job Production-Readiness Review: PR #1849 — Kotlin Spring HTTP Route ExtractionReview complete. Review bar for this PRThree lenses: (1) GitNexus core maintainer — source-scan/group contract extraction, optional parser loading, test/CI wiring, DoD enforcement; (2) Kotlin expert — tree-sitter-kotlin AST behavior, annotation argument forms, optional native grammar; (3) Spring Boot expert — route annotation semantics, false-positive resistance, path joining. Problem being solvedGitNexus Strategy B (source-scan) had no Kotlin Spring Boot support. PR #1834 (merged) fixed Java named-arg Spring route extraction ( Current PR state
Merge status and mergeability❌ Not mergeable — All other jobs pass: Repository history considered
Branch hygiene assessmentClean feature PR. One commit, three domain-consistent files. No unrelated formatting churn in unchanged sections. No hidden merge-from-main commits. No The Prettier failure is in new code only ( Understanding of the change
FindingsFinding 1: Risk: MERGE BLOCKER Evidence found: CI job The PR Autofix bot (run Recommended fix: Run Blocks merge: yes PR-specific assessment sectionsA. Runtime registration and optional grammar safety✅ Confirmed safe.
B. Kotlin AST/query correctness✅ Confirmed correct for the claimed scope. Positional anchor ( Named query key constraint ( No double-match: For any given
C. Spring Boot route semantics✅ Scope is correctly limited and does not overclaim. The PR description explicitly lists what is supported (class positional + named prefix, method positional + named for D. False-positive/false-negative behavior✅ Confirmed correct via direct query analysis and confirmed by anti-regression tests. For
For
For
E. Test coverage and CI reality✅ Tests actually ran in CI — not silently skipped. The ubuntu/coverage log confirms: Coverage for Uncovered lines: 49 (the The 3 grammar-absent behaviors are tested implicitly: when F. Packaging/install behavior✅ G. Regression risk to existing providers✅ Zero regression. The changes to H. Hygiene/security✅ No hidden Unicode characters found in any of the 3 changed files. No bidirectional controls, zero-width spaces, or BOM markers. CodeQL passed. Gitleaks passed. Dependency Review passed. The Back-and-forth avoided by verifying
Open questions that remain only if unavoidableNone — all mandatory verification points were resolved. Final verdict❌ not production-readyOne merge blocker: Everything else is production-quality: the optional dependency is correctly declared, the tree-sitter query anchoring is correct, false-positive resistance is verified through both code analysis and anti-regression tests, the Kotlin tests actually ran and passed in CI (not silently skipped), path joining is identical to the Java plugin, the null-grammar fallback is safe, and there is zero regression risk to existing language plugins. Fix required: Comment |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 9995 tests passed 4 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
@henry201605 could you please look into claude's findings above? |
|
@magyargergo Thanks for flagging — and apologies, I should have run Already addressed in commit Looks like the only remaining red is the Vercel deploy authorization, same as on #1834. Let me know if there's anything else you'd like adjusted! |
Sorry I meant these findings |
|
No worries at all! 🙏 My reply just above (commit |
Address Claude review on PR abhigyanpatwari#1855 (Finding 1). The OkHttp query in `kotlin.ts:OK_HTTP_PATTERNS` matches the `.url("/x")` sub-expression of a builder chain, but the verb is encoded on a separate sibling call (`.post(body)` / `.delete()` / ...). The query intentionally does not walk the chain to recover the verb — it emits `method: 'GET'` for every match, mirroring the Java plugin's `OK_HTTP_PATTERNS` (java.ts). Concretely: `Request.Builder().url("/x").post(body).build()` becomes `http::GET::/x`, not `http::POST::/x`. This is an already-accepted Java parity heuristic, but it was untested on the Kotlin side. This commit: - Adds an anti-overreach test pinning the current behavior: * exactly one consumer is emitted with method=GET * no second http::POST::/x consumer appears - Documents the limitation in kotlin.ts as a "Known limitation" block tied to the test, so a future verb-walk implementation has to update the comment in lockstep with the assertion. Rationale for not implementing verb-walk in this PR: - Verb-walk requires walking sibling call_expression nodes (the `.post(body)` chain), which is the same shape as the deferred WebClient long-form work - Java has the same limitation in production today; fixing only Kotlin would create polyglot drift - A coordinated future PR can add verb-walk to both plugins at once and update both comments + the pin tests together Finding 2 (silent test-skip when tree-sitter-kotlin grammar is unavailable) is intentionally NOT addressed here — same gating pattern was accepted in abhigyanpatwari#1849 for Provider tests, and a coordinated follow-up should add a CI sentinel covering both Provider and Consumer suites in one place. Local validation: - test/unit/group/http-route-extractor.test.ts: 60/60 ✅ - test/unit/group: 540/540 ✅ - npm run format:check: clean ✅
* feat(group): add Kotlin Spring HTTP consumer extraction Follow-up to #1849 (Kotlin providers). Extends `http-patterns/kotlin.ts` with three call-site patterns common in Kotlin Spring projects: - RestTemplate: `restTemplate.getForObject("/x", ...)` and the full verb family (getForObject/getForEntity → GET, postForObject/postForEntity → POST, put → PUT, delete → DELETE, patchForObject → PATCH). Mirrors the Java plugin's `REST_TEMPLATE_TO_HTTP` map so polyglot repos coalesce on a single contract id. - WebClient short form: `webClient.get().uri("/x")` and the `.post()` / `.put()` / `.delete()` / `.patch()` siblings. The chain parses as two nested `call_expression` nodes; the query anchors on the outer `.uri(...)` and walks one level inward to constrain the verb. - OkHttp: `Request.Builder().url("/x")`. Kotlin parses `Request.Builder()` as a `call_expression` whose callee is a `navigation_expression` (not Java's `object_creation_expression`), so the query shape differs from `java.ts` but the receiver/method constraints (`Request` / `Builder` / `url`) and emitted contract format match. Out of scope: `webClient.method(HttpMethod.X).uri("/y")` long form. The verb sits on a sibling `call_expression` two hops away, so it needs a walk-up helper rather than a flat tree-sitter query. A dedicated anti-overreach test pins the current behavior so a future short-form change can't accidentally start matching the long form. Receiver name constraints (`#eq? @obj "restTemplate"`, `#eq? @cls "Request"`) match the Java plugin's heuristic — a project that aliases the receiver under a different name won't be picked up. This trade-off keeps false-positive rates low and is documented in the file header. Tests: 5 new cases under `consumer extraction — fetch patterns`, gated by tree-sitter-kotlin grammar availability. positive (3) - RestTemplate verbs (5 calls × 5 verbs) - WebClient short-form verbs (5 calls × 5 verbs) - OkHttp Request.Builder().url("/x") anti-regression (2) - WebClient long form `.method(HttpMethod.X)` produces no consumer (deferred-feature pin) - non-restTemplate receiver does not match (receiver-name pin) Reverse-validated: removing the `(#eq? @obj "restTemplate")` constraint causes the receiver-name anti-regression test to fail. Local validation: - test/unit/group/http-route-extractor.test.ts: 59/59 ✅ - test/unit/group: 539/539 ✅ - npm run format:check: clean ✅ * test(group): pin Kotlin OkHttp POST-chain heuristic-default GET behavior Address Claude review on PR #1855 (Finding 1). The OkHttp query in `kotlin.ts:OK_HTTP_PATTERNS` matches the `.url("/x")` sub-expression of a builder chain, but the verb is encoded on a separate sibling call (`.post(body)` / `.delete()` / ...). The query intentionally does not walk the chain to recover the verb — it emits `method: 'GET'` for every match, mirroring the Java plugin's `OK_HTTP_PATTERNS` (java.ts). Concretely: `Request.Builder().url("/x").post(body).build()` becomes `http::GET::/x`, not `http::POST::/x`. This is an already-accepted Java parity heuristic, but it was untested on the Kotlin side. This commit: - Adds an anti-overreach test pinning the current behavior: * exactly one consumer is emitted with method=GET * no second http::POST::/x consumer appears - Documents the limitation in kotlin.ts as a "Known limitation" block tied to the test, so a future verb-walk implementation has to update the comment in lockstep with the assertion. Rationale for not implementing verb-walk in this PR: - Verb-walk requires walking sibling call_expression nodes (the `.post(body)` chain), which is the same shape as the deferred WebClient long-form work - Java has the same limitation in production today; fixing only Kotlin would create polyglot drift - A coordinated future PR can add verb-walk to both plugins at once and update both comments + the pin tests together Finding 2 (silent test-skip when tree-sitter-kotlin grammar is unavailable) is intentionally NOT addressed here — same gating pattern was accepted in #1849 for Provider tests, and a coordinated follow-up should add a CI sentinel covering both Provider and Consumer suites in one place. Local validation: - test/unit/group/http-route-extractor.test.ts: 60/60 ✅ - test/unit/group: 540/540 ✅ - npm run format:check: clean ✅ --------- Co-authored-by: henry <zhangwei2017@unipus.cn>
2 participants
Summary
Follow-up to #1834 (now merged) — extends the Spring named-argument HTTP route extraction to Kotlin Spring Boot controllers, as suggested by @magyargergo in #1834 (comment).
A new
http-patterns/kotlin.tsplugin sits behind the optionaltree-sitter-kotlingrammar, registered for.kt/.kts. Both annotation forms produce providers:Why two queries
The Kotlin AST (fwcd/tree-sitter-kotlin) shares one node type —
value_argument— for both positional and named forms, with an optional leadingsimple_identifier "="distinguishing them. So the queries split:string_literalas the first named child ofvalue_argumentvia the immediate-child anchor.simple_identifierand constrains it to^(path|value)$via#match?, mirroring the same safety bar enforced byhttp-patterns/java.tsandtopic-patterns/java.tsafter the fix(group): handle named annotation args in Java Spring route extraction #1834 reviewWithout the
key:constraint, the named query would also capture non-route attributes likeproduces,consumes,headers,name,params— emitting bogus contracts identical to the Finding A failure mode flagged on #1834.Optional dependency
tree-sitter-kotlinis anoptionalDependency. When the native binding is unavailable (e.g. CI environments where the build fails), the plugin exportsnullandindex.tsskips registering.kt/.kts, so the orchestrator stays healthy.This mirrors the existing pattern used by
parse-worker.tsandparser-loader.ts.Out of scope
Kotlin consumer detection (RestTemplate / WebClient / OkHttp) is intentionally not included here. The Kotlin call-site ASTs differ enough from Java's
method_invocationshape that I felt a focused follow-up PR would be easier to review than bundling it. Happy to open that next.Tests
11 new cases under
provider extraction — source-scan fallback (Strategy B), gated by the kotlin grammar availability:Positive (8)
@RequestMapping("/api/v1")(positional)@RequestMapping(path = "/api/v2")@RequestMapping(value = "/orders")@GetMapping(value = "/users")@GetMapping(path = "/users")@PostMapping(path = "/users")Anti-regression (3)
@GetMapping(produces = "application/json")emits no provider@GetMapping(name = "x", value = "/users")emits exactly one provider@RequestMapping(path = "/api", name = "myApi")prefix stays/api, nevermyApiI sanity-checked the constraint the other way too: removing
(#match? @key "^(path|value)$")makes precisely the 3 anti-regression tests fail — same shape as the Java fix.Local validation
http-route-extractor.test.ts— 54 / 54 ✅test/unit/group— 534 / 534 ✅npx tsc --noEmit— clean ✅