feat(group): add Kotlin Spring HTTP consumer extraction#1855
Conversation
Follow-up to abhigyanpatwari#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 ✅
|
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 10001 tests passed 4 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Claude finished @magyargergo's task in 4m 9s —— View job Adversarial Production-Readiness Review — PR #1855Review Bar for This PRCombined Kotlin + Spring Boot + tree-sitter + GitNexus contract integration review. Standard: correct AST matching, no new false positives, stable contract IDs, meaningful CI-verified tests, no hidden Unicode, focused scope. Problem Being SolvedPR #1855 extends Current PR StateOpen, one commit ( Merge Status and MergeabilityCI bot reports all checks passed. Repository History Considered
Branch Hygiene AssessmentClean. One feature commit, two causally related files. No CI workflows, Docker, release, UI, or dependency churn outside the stated scope. Understanding of the Change
Findings1. Kotlin/tree-sitter AST CorrectnessRestTemplate query ( The tree-sitter-kotlin (fwcd 0.3.8) AST for The anchor WebClient short-form query ( OkHttp query ( Potential AST edge case — Named arguments not captured:
2. Spring Boot HTTP Client SemanticsRestTemplate mapping ( WebClient short form: OkHttp GET limitation — minor, non-blocking:
3. GitNexus Contract Integration
Plugin registration in Contract ID format: the orchestrator ( 4. Tests and CI5 new Kotlin consumer tests at
All tests go through CI evidence: 10000 tests passed, 4 skipped. The 4 listed skips are benchmarks + Ruby — not the 5 Kotlin consumer tests. The PR description reports 59/59 for Grammar availability gating — minor concern:
5. Hidden Unicode and HygieneNon-ASCII characters found:
6. Branch Scope and Unrelated ChurnDiff is +420/-9 across exactly 2 files: Back-and-Forth Avoided by Verifying
Open QuestionsNone that block merge. Final Verdictproduction-ready with minor follow-ups The implementation is correct for its stated scope: AST queries are well-formed for the fwcd tree-sitter-kotlin grammar, all three consumer patterns are faithfully adapted from the Java plugin with documented receiver-name trade-offs, contract IDs and |
|
@henry201605 could you please double check these findings before I merge it in? 🙏 |
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 ✅
|
@magyargergo Thanks for the careful review! 🙏 Went through both findings — quick notes below. Finding 1 (OkHttp POST-chain heuristic-default GET) — addressed in commit Finding 2 (silent test-skip when tree-sitter-kotlin is unavailable) — intentionally deferred. The same Local validation after the new commit:
Happy to make any further adjustments! |
…atwari#1884 Two minor follow-ups from the production-readiness review: F1 — Stale block comment at the top of the Kotlin consumer suite (was: "Three consumer flavors covered here ... long-form deferred to a follow-up"). Updated to "Four consumer flavors" and removed the deferred sentence — the deferral is resolved by this PR. The kotlin.ts file header was already updated; this brings the test file comment in sync. Per DoD §2.3 (no stale comments). F2 — Replaced `expect(wcConsumers.length).toBeGreaterThanOrEqual(4)` with `expect(wcConsumers).toHaveLength(4)` in the multi-verb test. The fixture is fully deterministic — exactly 4 long-form calls, no other consumer types — so an exact count assertion is the right shape per DoD §2.7 ("use toBe / toEqual for exact expectations"). Added a comment explaining what the assertion catches that the existing per-verb toBeDefined() checks would miss (accidental 5th consumer from a duplicate query firing or a regressed receiver constraint). F3 (HEAD/OPTIONS/TRACE negative test) is intentionally not added in this PR — same precedent as abhigyanpatwari#1855 where HEAD/OPTIONS/TRACE on the short form are also implicitly excluded without a pinning test. Happy to add one in a separate PR if maintainers want explicit pinning across both forms. F4 (CI on pre-merge SHA) is the maintainer's call — the merge from main is theirs to re-trigger CI on. The merge brings only Java consumer changes (PR abhigyanpatwari#1872) and Go provider changes (PR abhigyanpatwari#1886), both in entirely separate files from this PR's Kotlin work. Local validation: - test/unit/group/http-route-extractor.test.ts: 73/73 ✅ (66 from this PR pre-merge + 7 from PR abhigyanpatwari#1872 merged via main) - npx prettier --check (changed files): clean ✅
…action (#1884) * feat(group): add Kotlin Spring WebClient long-form HTTP consumer extraction Follow-up to #1855. Extends `kotlin.ts` with the long-form WebClient fluent chain that #1855 explicitly deferred: webClient.method(HttpMethod.GET).uri("/x").retrieve().awaitBody<T>() This pattern remains common in Kotlin Spring 4 → 5 migrations and in codebases that prefer the fluent verb-as-enum style. The short form (`webClient.get().uri("/x")`) was already supported in #1855. Approach: - Single deeper tree-sitter query (`WEB_CLIENT_LONG_PATTERNS`) that matches the full chain structurally — both `.method(HttpMethod.X)` and `.uri("...")` in one pattern. Verb is captured as the `simple_identifier` of the `HttpMethod.X` field access. - Verb is whitelisted to GET/POST/PUT/DELETE/PATCH (consistent with the short-form's `WEB_CLIENT_SHORT_TO_HTTP` map). - Receiver constraint `(#eq? @obj "webClient")` mirrors the short form and Java plugin heuristic. Out of scope (intentional): - Variable-bound verbs: `val verb = HttpMethod.PATCH; webClient.method(verb)...` Source-scan can't follow the binding without graph context. Pinned by an anti-overreach test. - HEAD/OPTIONS/TRACE: not in `WEB_CLIENT_SHORT_TO_HTTP` either — keeps polyglot symmetry with java.ts and the short form. Tests: 4 new cases under `consumer extraction — fetch patterns`, gated by tree-sitter-kotlin grammar availability. positive (3) - long form GET - long form POST / PUT / DELETE / PATCH (4 verbs in 1 fixture) - no double-emit pin (long-form chain produces exactly one consumer, not one from each query) anti-regression (1) - variable-bound verb does NOT match (graph-aware concern) The previous `'does NOT match Kotlin WebClient long form (deferred to follow-up)'` test from #1855 is replaced by these — the deferred state is now resolved. Reverse-validated: temporarily disabling the long-form emit makes exactly the 3 positive tests fail; the variable-bound-verb anti- regression test continues to pass (it pins behavior independent of the emit being on or off). Local validation: - test/unit/group/http-route-extractor.test.ts: 66/66 ✅ - test/unit/group: 546/546 ✅ - npx prettier --check (changed files): clean ✅ * test(group): address Claude review findings F1 and F2 on PR #1884 Two minor follow-ups from the production-readiness review: F1 — Stale block comment at the top of the Kotlin consumer suite (was: "Three consumer flavors covered here ... long-form deferred to a follow-up"). Updated to "Four consumer flavors" and removed the deferred sentence — the deferral is resolved by this PR. The kotlin.ts file header was already updated; this brings the test file comment in sync. Per DoD §2.3 (no stale comments). F2 — Replaced `expect(wcConsumers.length).toBeGreaterThanOrEqual(4)` with `expect(wcConsumers).toHaveLength(4)` in the multi-verb test. The fixture is fully deterministic — exactly 4 long-form calls, no other consumer types — so an exact count assertion is the right shape per DoD §2.7 ("use toBe / toEqual for exact expectations"). Added a comment explaining what the assertion catches that the existing per-verb toBeDefined() checks would miss (accidental 5th consumer from a duplicate query firing or a regressed receiver constraint). F3 (HEAD/OPTIONS/TRACE negative test) is intentionally not added in this PR — same precedent as #1855 where HEAD/OPTIONS/TRACE on the short form are also implicitly excluded without a pinning test. Happy to add one in a separate PR if maintainers want explicit pinning across both forms. F4 (CI on pre-merge SHA) is the maintainer's call — the merge from main is theirs to re-trigger CI on. The merge brings only Java consumer changes (PR #1872) and Go provider changes (PR #1886), both in entirely separate files from this PR's Kotlin work. Local validation: - test/unit/group/http-route-extractor.test.ts: 73/73 ✅ (66 from this PR pre-merge + 7 from PR #1872 merged via main) - npx prettier --check (changed files): clean ✅ * refactor(group): hoist Kotlin WebClient long-form verb regex to module scope Address @magyargergo's review request on PR #1884: > Can you please extract the regexp from the for loop? 🙏 (kotlin.ts:510) Compiles the verb whitelist `^(GET|POST|PUT|DELETE|PATCH)$` once at module load instead of every iteration of the long-form scan loop. Mirrors the placement and JSDoc style of the sibling `WEB_CLIENT_SHORT_TO_HTTP` constant. Behavior is unchanged — same verb whitelist, same exclusion of HEAD/OPTIONS/TRACE for symmetry with the short form. The 4 itKotlinConsumer long-form tests added in this PR continue to pass, and the variable-bound-verb anti-overreach test continues to pin the deliberate non-match. Local validation: - test/unit/group/http-route-extractor.test.ts: 77/77 ✅ - test/unit/group: 557/557 ✅ - npx prettier --check (changed file): clean ✅ --------- Co-authored-by: henry <zhangwei2017@unipus.cn> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Summary
Follow-up to #1849 (Kotlin providers, merged) — extends
http-patterns/kotlin.tswith three call-site patterns common in Kotlin Spring projects, completing 80% of real-world consumer detection coverage.restTemplate.getForObject("/x", ...)etc.getForObject→ GET,postForEntity→ POST,put→ PUT,delete→ DELETE,patchForObject→ PATCH)webClient.get().uri("/x")etc.get/post/put/delete/patch)Request.Builder().url("/x")The mappings (
REST_TEMPLATE_TO_HTTP,WEB_CLIENT_SHORT_TO_HTTP) mirror the Java plugin so polyglot repos coalesce on the samehttp::VERB::/pathcontract IDs.Out of scope
webClient.method(HttpMethod.X).uri("/y")long form is intentionally deferred. The verb sits on a siblingcall_expressiontwo hops away, so it needs walk-up logic rather than a flat tree-sitter query — that warrants its own focused PR. An anti-overreach test pins the current behavior so a future short-form change can't accidentally start matching the long form.Why these queries are correct
The Kotlin AST shape for
webClient.get().uri("/x")is:The query anchors on the OUTER call (
.uri(...)) and walks one level inward to constrain the inner suffix to a verb name. The leading.onvalue_argumentensures the path is the first positional argument.OkHttp uses the same outer/inner shape but with
Request/Builder/urlconstraints. RestTemplate is one level shallower (no chained verb hop).Receiver-name constraints (and the trade-off)
#eq? @obj "restTemplate"and#eq? @cls "Request"match the Java plugin's heuristic. A project that aliases the receiver under a different name (val template = ...; template.getForObject(...)) won't be picked up. This keeps false-positive rates low and is documented in the file header. Same trade-off the Java plugin already accepts.Tests
5 new cases under
consumer extraction — fetch patterns, gated by tree-sitter-kotlin grammar availability (matches the Provider gating from #1849).Positive (3)
Request.Builder().url("/x")Anti-regression (2)
.method(HttpMethod.X)produces no consumer (deferred-feature pin)I sanity-checked the constraint the other way: removing
(#eq? @obj "restTemplate")makes the receiver-name anti-regression test fail. Same shape as the validation done on #1834 and #1849.Local validation
http-route-extractor.test.ts— 59 / 59 ✅test/unit/group— 539 / 539 ✅npm run format:check— clean ✅