diff --git a/propose/CLIENT-ROLE-RENAME-PROPOSE.md b/propose/CLIENT-ROLE-RENAME-PROPOSE.md new file mode 100644 index 0000000..05f5954 --- /dev/null +++ b/propose/CLIENT-ROLE-RENAME-PROPOSE.md @@ -0,0 +1,516 @@ +# Propose: `FEIGN_CLIENT` role → `CLIENT` role + `HTTP_CLIENT` capability + +**Status:** Draft (2026-05-06) +**Author:** Dmitry (with Computer) +**Companion docs:** +- `propose/FEIGN-NOT-AN-EXPOSER-PROPOSE.md` (PR #25, merged) — orthogonal +- `propose/CROSS-SERVICE-RESOLUTION-FLAG-PROPOSE.md` (PR #26, merged) — orthogonal +- `plans/PLAN-FEIGN-NOT-AN-EXPOSER.md` — implements PR-F1 (must ship before this) +- `plans/PLAN-CROSS-SERVICE-RESOLUTION-FLAG.md` — implements PR-G1 (must ship before this) + +**Supersedes:** `propose/DEFERRED-REST-CLIENT-MIGRATION-PROPOSE.md` +(deleted in this branch — its 2026-04-26 conclusion that +"`REST_CLIENT` should be a capability, not a role rename" is reversed +by this proposal because the architecture has moved on: capabilities +already exist, brownfield overrides already exist, and the +`@CodebaseRole` annotation already lets users assign primary roles +honestly). + +## TL;DR + +Rename the role enum value `FEIGN_CLIENT` → `CLIENT` and add a new +capability `HTTP_CLIENT`. When `@FeignClient` is detected, emit role +`CLIENT` AND capability `HTTP_CLIENT` (instead of just role +`FEIGN_CLIENT`). + +Keep auto-promotion **scoped exactly as today** — only `@FeignClient` +interfaces are auto-promoted to `CLIENT` role. RestTemplate-using +classes, WebClient-using classes, and other HTTP outbound patterns +keep their existing role (typically `SERVICE` or `COMPONENT`) and can +opt into `CLIENT` via brownfield annotations (`@CodebaseRole(role="CLIENT")` ++ `@CodebaseCapability(capability="HTTP_CLIENT")`) on a case-by-case +basis. + +The motivation is **annotation honesty for brownfield**. Today, a user +who wants to mark a `RestTemplate`-using class as an HTTP client must +annotate it `@CodebaseRole(role="FEIGN_CLIENT")` — which lies (the +class is not a Feign client). After this change, the same user +annotates `@CodebaseRole(role="CLIENT") @CodebaseCapability(capability="HTTP_CLIENT")` +and the annotation is true. + +`KAFKA_PRODUCER`/`RABBIT_PRODUCER`/`JMS_PRODUCER`/`STREAM_PRODUCER` +**already exist** today as a unified `MESSAGE_PRODUCER` capability +(`ast_java.py:117-122`), auto-detected from injected types. No async +rename is needed. + +~105 LOC. Ontology bump (likely 8 → 9, depending on PR-G1's bump +landing first). **Hard rename — no deprecation alias.** The MCP +bundle has no users yet; breaking changes are explicitly allowed. +`FEIGN_CLIENT` is removed from `VALID_ROLES` in the same PR; brownfield +YAML/annotation inputs that still say `FEIGN_CLIENT` fail validation +with a clear error pointing at the new vocabulary. + +## 1. Why this is the right shape + +### 1.1 The annotation-honesty problem (brownfield) + +Today's `ROLE_ANNOTATIONS` (`ast_java.py:77-92`): + +```python +ROLE_ANNOTATIONS: dict[str, str] = { + "RestController": "CONTROLLER", + "Controller": "CONTROLLER", + "Service": "SERVICE", + "Repository": "REPOSITORY", + "Component": "COMPONENT", + "Configuration": "CONFIG", + "Entity": "ENTITY", + "MappedSuperclass": "ENTITY", + "Embeddable": "ENTITY", + "FeignClient": "FEIGN_CLIENT", # ← the leak + "Mapper": "MAPPER", +} +``` + +`FEIGN_CLIENT` is **library-specific**. Every other role describes an +architectural concept (service, repository, controller, entity) that +is independent of the library. A `@FeignClient` interface is, at the +architectural level, an **HTTP client** that happens to be implemented +via Spring Cloud OpenFeign. + +This causes a real problem in brownfield codebases. When a user has a +`RestTemplate`-using class that they want to mark as an HTTP client — +to make it visible to "list all HTTP clients in svc-a"-style queries — +the only way today is: + +```java +@CodebaseRole(role = "FEIGN_CLIENT") // ← lie +@Service +class UserApiClient { + private final RestTemplate restTemplate; + // calls user-svc over HTTP +} +``` + +The annotation literally says "this is a Feign client" when it is +not. For a brownfield system whose entire value proposition is +**making the codebase legible to humans and AI agents**, the role +label is a public contract. Lying in annotations is a smell that +propagates: code reviewers see `FEIGN_CLIENT` and look for a +`@FeignClient` interface that doesn't exist. + +### 1.2 What the rename buys + +With `CLIENT` as the role name + `HTTP_CLIENT` as the capability: + +```java +@CodebaseRole(role = "CLIENT") +@CodebaseCapability(capability = "HTTP_CLIENT") +@Service +class UserApiClient { + private final RestTemplate restTemplate; +} +``` + +Now the annotation is honest: "this class plays the **client** role, +with **HTTP** as the protocol." A future async client gets the same +treatment with `@CodebaseCapability(capability="MESSAGE_PRODUCER")` — +a capability that already exists in the system. + +### 1.3 What the rename does NOT buy (be honest about scope) + +The role label is **almost never** consulted in resolution paths: + +- **Cross-service matcher** (`build_ast_graph.py:1620-1648`): reads + `route.kind` and `OutgoingCall.client_kind`, not role +- **PR #25's EXPOSES gate**: reads `route.kind`, not role +- **PR-G1's brownfield_only flag**: reads `route_source_layer` and + `resolution_strategy`, not role +- **Outgoing call extraction** (`ast_java.py:1818, 1901`): reads + injected type names (`RestTemplate`, `WebClient`), not role + +The role label IS consulted in: + +- **`search_lancedb.py:188`** `_ROLE_SCORE_WEIGHTS["FEIGN_CLIENT"] = 0.06` + (ranking nudge for behavioural search) +- **`kuzu_queries.py:956`** `_FLOW_STAGES[2]` includes `FEIGN_CLIENT` + (trace_flow integration stage) +- **`server.py:1415`** `entry_roles = [..., "FEIGN_CLIENT"]` + (codebase_search entry-role filter) +- **MCP tool `role` enum strings** in `server.py:687, 1138` + +These are surface-level — a rename is mechanical, not architectural. + +So **be clear about what this proposal is**: it is a vocabulary +cleanup that makes brownfield annotations honest. It is **not** a +new resolution mechanism, **not** a richer cross-service edge model, +**not** auto-promotion of more class types. + +## 2. Goals + +- **G1.** Rename `FEIGN_CLIENT` → `CLIENT` in `ROLE_ANNOTATIONS` + (`ast_java.py:90`). +- **G2.** When `@FeignClient` is detected, emit role `CLIENT` AND + capability `HTTP_CLIENT`. The capability is added to a new + `_TYPE_ANN_TO_CAPABILITY` entry (`ast_java.py:111-114`) — same + detector path as `EXCEPTION_HANDLER`. +- **G3.** All call sites that key on `FEIGN_CLIENT` literal switch + to `CLIENT`. List in §5. +- **G4.** Brownfield YAML and `@CodebaseRole`/`@CodebaseCapability` + annotations accept the new vocabulary (`CLIENT` role, + `HTTP_CLIENT` capability). `FEIGN_CLIENT` is removed from + `VALID_ROLES` — any brownfield input still using it fails + validation with an error that names the replacement + (`CLIENT` role + `HTTP_CLIENT` capability). +- **G5.** Ontology bump (7→8 if PR-G1 hasn't landed; 8→9 if it has). +- **G6.** No change to auto-promotion scope. RestTemplate-using + and WebClient-using classes keep their existing role. +- **G7.** `MESSAGE_PRODUCER` capability already exists and covers + Kafka/Rabbit/JMS/Stream/event publishers. **No change** to + async detection. + +## 3. Non-goals + +- **NG1.** Auto-promoting `RestTemplate`/`WebClient`/`RestClient` + users to `CLIENT` role. Deferred until after real-project test + shows it's needed. +- **NG2.** Auto-promoting `KafkaTemplate`-injecting classes to + `CLIENT` role. They already get capability `MESSAGE_PRODUCER` + via the existing `_INJECTED_TYPES_TO_CAPABILITY` table. +- **NG3.** Maintaining backwards compatibility for `FEIGN_CLIENT` as + a brownfield input. The MCP bundle has no users yet — `FEIGN_CLIENT` + is dropped from `VALID_ROLES` in v1. No alias, no warning, no + two-phase deprecation. Validation error points users at the + replacement. +- **NG4.** Changing `CONTROLLER` to be the inbound counterpart of + `CLIENT` (i.e., extending `CONTROLLER` to async listeners). + Separate proposal (parked). +- **NG5.** Changing how `@FeignClient` is **routed** (`kind=http_consumer` + Route emission unchanged, EXPOSES gate from PR-F1 unchanged, + cross-service matcher unchanged). +- **NG6.** Adding `HTTP_CLIENT` as a sub-classifier under role + `CLIENT` for downstream queries to distinguish protocol. The + capability already does this. + +## 4. Current state (verified 2026-05-06) + +### 4.1 Role detection today + +| Class signal | Role assigned today | Where | +|---|---|---| +| `@FeignClient interface X` | `FEIGN_CLIENT` | `ast_java.py:90` | +| `@RestController class X` | `CONTROLLER` | `ast_java.py:79` | +| `class X { RestTemplate rt; }` | `SERVICE` (or whatever stereotype) | unchanged by RestTemplate injection | +| `class X { KafkaTemplate kt; }` | `SERVICE` (or whatever stereotype) + capability `MESSAGE_PRODUCER` | `ast_java.py:117` | +| `class X { WebClient wc; }` | `SERVICE` (or whatever stereotype) | unchanged by WebClient injection | + +### 4.2 Capability detection today (verified) + +`MESSAGE_PRODUCER` already exists and is auto-detected from injected +types (`ast_java.py:116-122`): + +```python +_INJECTED_TYPES_TO_CAPABILITY: dict[str, str] = { + "KafkaTemplate": "MESSAGE_PRODUCER", + "RabbitTemplate": "MESSAGE_PRODUCER", + "JmsTemplate": "MESSAGE_PRODUCER", + "StreamBridge": "MESSAGE_PRODUCER", + "ApplicationEventPublisher": "MESSAGE_PRODUCER", +} +``` + +`MESSAGE_LISTENER` covers the consumer side (`ast_java.py:101-107`) +via method-level annotations. So the async picture is **already +complete and unified** — no work needed there. + +`EXCEPTION_HANDLER` is the existing example of a type-annotation-driven +capability (`ast_java.py:111-114`), and is the pattern this proposal +mirrors for `HTTP_CLIENT`. + +### 4.3 Where `FEIGN_CLIENT` literal is referenced + +Verified count: **5 production files, ~12 references**: + +| File | Lines | Purpose | +|---|---|---| +| `ast_java.py` | 90 | `ROLE_ANNOTATIONS` — the source | +| `kuzu_queries.py` | 956, 965, 975 | `_FLOW_STAGES[2]` integration tier; `_ENTRYPOINT_ROLES`; `trace_flow` docstring | +| `search_lancedb.py` | 188 | `_ROLE_SCORE_WEIGHTS["FEIGN_CLIENT"] = 0.06` | +| `server.py` | 49, 687, 1138, 1335, 1339, 1415 | MCP tool docstrings, `role` enum strings, entry-role filter | +| `tests/test_lancedb_e2e.py` | 342 | One assertion | + +Plus docs: `README.md`, `CODEBASE_REQUIREMENTS.md`. Doc sweep is +straightforward. + +### 4.4 Brownfield input today + +`graph_enrich.py:375` uses `VALID_ROLES` (derived from +`ROLE_ANNOTATIONS.values() + {"DTO"}`) to validate brownfield YAML +inputs. `_INJECT_FIELD_ANNOTATIONS` and `VALID_CAPABILITIES` +(`java_ontology.py:18-25`) already cover the capability validator +side. + +After the rename, `VALID_ROLES` automatically contains `CLIENT` +(no extra wiring), and `VALID_CAPABILITIES` automatically contains +`HTTP_CLIENT` once added to `_TYPE_ANN_TO_CAPABILITY`. + +## 5. Design + +### 5.1 Source-of-truth rename + +`ast_java.py:90`: + +```python +# BEFORE: +"FeignClient": "FEIGN_CLIENT", + +# AFTER: +"FeignClient": "CLIENT", +``` + +`ast_java.py:111-114`, extend `_TYPE_ANN_TO_CAPABILITY`: + +```python +_TYPE_ANN_TO_CAPABILITY: dict[str, str] = { + "ControllerAdvice": "EXCEPTION_HANDLER", + "RestControllerAdvice": "EXCEPTION_HANDLER", + "FeignClient": "HTTP_CLIENT", # ← new +} +``` + +This is the canonical pattern: a class annotated `@FeignClient` now +gets: +- Role `CLIENT` via `ROLE_ANNOTATIONS` +- Capability `HTTP_CLIENT` via `_TYPE_ANN_TO_CAPABILITY` + +`VALID_ROLES` and `VALID_CAPABILITIES` (`java_ontology.py:16-25`) +update automatically — they are derived sets. + +### 5.2 Mechanical literal updates + +#### `kuzu_queries.py:956` (`_FLOW_STAGES`) + +```python +# BEFORE: +("FEIGN_CLIENT", "REPOSITORY", "MAPPER"), + +# AFTER: +("CLIENT", "REPOSITORY", "MAPPER"), +``` + +#### `kuzu_queries.py:965` (`_ENTRYPOINT_ROLES`) + +```python +# BEFORE: +"CONTROLLER", "COMPONENT", "SERVICE", "FEIGN_CLIENT", + +# AFTER: +"CONTROLLER", "COMPONENT", "SERVICE", "CLIENT", +``` + +#### `kuzu_queries.py:975` (docstring) + +Update inline reference. + +#### `search_lancedb.py:188` (`_ROLE_SCORE_WEIGHTS`) + +```python +# BEFORE: +"FEIGN_CLIENT": 0.06, + +# AFTER: +"CLIENT": 0.06, +``` + +Same numeric weight — the rename does not change behavioural-search +ranking. (Open question [TBD-3]: is `0.06` still the right weight +when the role now also includes brownfield-annotated RestTemplate +clients? Recommend keep `0.06` for v1; revisit only if users complain.) + +#### `server.py` (~6 sites) + +Replace `FEIGN_CLIENT` → `CLIENT` in: +- Tool docstring (`server.py:49, 1335, 1339`) +- `role` enum descriptions (`server.py:687, 1138`) +- `entry_roles` list (`server.py:1415`) + +#### `tests/test_lancedb_e2e.py:342` + +```python +# BEFORE: +s["symbol"]["role"] in {"CONTROLLER", "COMPONENT", "SERVICE", "FEIGN_CLIENT"} + +# AFTER: +s["symbol"]["role"] in {"CONTROLLER", "COMPONENT", "SERVICE", "CLIENT"} +``` + +### 5.3 Brownfield validation behaviour (no alias) + +`FEIGN_CLIENT` is removed from `VALID_ROLES` in the same PR. The +existing validator in `graph_enrich.py` already raises on unknown +roles — we just rely on that path. The error message lists the +allowed roles, which is enough to redirect users: + +```python +# Existing pattern in graph_enrich.py (no new code): +if role not in VALID_ROLES: + raise ValueError( + f"role_overrides: {role!r} is not a valid role for " + f"{annotation_or_fqn!r}. Valid roles: {sorted(VALID_ROLES)}" + ) +``` + +Users with existing YAML like: + +```yaml +role_overrides: + fqn: + "com.example.UserApiClient": "FEIGN_CLIENT" +``` + +get a hard validation error and must update to: + +```yaml +role_overrides: + fqn: + "com.example.UserApiClient": "CLIENT" +capability_overrides: + fqn: + "com.example.UserApiClient": ["HTTP_CLIENT"] +``` + +This is a deliberate breaking change: the MCP bundle has no users +yet, so the alias machinery would only add complexity without +protecting anyone. Clean v1. + +### 5.4 Ontology version + +If PR-G1 has merged before this PR, bump 8 → 9. +If PR-G1 has not merged, bump 7 → 8 and PR-G1's bump becomes 8 → 9. + +The bump is required because existing graphs have rows with +`role = "FEIGN_CLIENT"` — incompatible with the new `VALID_ROLES`. +Document "rebuild to apply" in the PR description (same migration +story as every other graph-shape change). + +### 5.5 Documentation + +- README: rename role table; mention `CLIENT` + `HTTP_CLIENT` capability; + document the `MESSAGE_PRODUCER` capability that already exists for + symmetry. +- `CODEBASE_REQUIREMENTS.md`: rename references. +- `propose/DEFERRED-REST-CLIENT-MIGRATION-PROPOSE.md`: **delete** (this + proposal supersedes it; the rename-vs-capability decision is + reversed by current architecture). + +## 6. Risks and mitigations + +| # | Risk | Likelihood | Mitigation | +|---|---|---|---| +| 1 | Existing YAML overrides use `FEIGN_CLIENT` and break | N/A | No users yet. Hard rename is explicitly allowed. Validation error points at replacement (§5.3). | +| 2 | Old graphs' rows have `role="FEIGN_CLIENT"` | High | Ontology bump documented. Rebuild required. Same migration story as past breaking changes. | +| 3 | Downstream consumers of MCP tool `role` enum (e.g., LLM prompts) hardcode `FEIGN_CLIENT` | Low | The MCP `role` field is documented; this is a vocabulary change. Bump tool description visibly. Note in README "breaking change". | +| 4 | Behavioural-search ranking changes silently | Low | Same numeric weight (`0.06`). [TBD-2] flags it for review if user reports drift. | +| 5 | `trace_flow` integration stage misclassifies brownfield-annotated RestTemplate clients | Medium | They get role `CLIENT`, which IS in `_FLOW_STAGES[2]`. No regression — actually an improvement (they were `SERVICE` before, missed the integration tier). | +| 6 | Auto-promotion scope is unchanged but users assume it expanded | Medium | Document explicitly in README and PR description: "Only `@FeignClient` interfaces auto-promote to `CLIENT`. RestTemplate/WebClient users keep their existing role; opt in via brownfield annotations." | + +## 7. Verification + +### Tests + +| # | Test | Asserts | +|---|---|---| +| 1 | `test_feign_client_emits_client_role` | `@FeignClient interface X` → `Symbol.role == "CLIENT"`, `"HTTP_CLIENT" in Symbol.capabilities` | +| 2 | `test_no_legacy_feign_client_role_in_graph` | After build, `MATCH (s:Symbol) WHERE s.role = 'FEIGN_CLIENT' RETURN count(s)` returns 0 | +| 3 | `test_resttemplate_class_unchanged` | Class with `RestTemplate` field but no `@FeignClient` → role unchanged from today's behaviour (e.g., `SERVICE`), no `HTTP_CLIENT` capability auto-added | +| 4 | `test_brownfield_feign_client_role_rejected` | YAML with `role_overrides.fqn: "com.x.Y": "FEIGN_CLIENT"` → build raises `ValueError` whose message contains `'CLIENT'` (so the user is redirected). Same behaviour for `@CodebaseRole(role="FEIGN_CLIENT")` in source. | +| 5 | `test_brownfield_client_role_accepted` | YAML with `role_overrides.fqn: "com.x.Y": "CLIENT"` → built graph has `role="CLIENT"`. No warning. | +| 6 | `test_brownfield_http_client_capability_accepted` | YAML with `capability_overrides.fqn: "com.x.Y": ["HTTP_CLIENT"]` → built graph has `"HTTP_CLIENT" in capabilities`. No warning. | +| 7 | `test_message_producer_capability_unchanged` | Class injecting `KafkaTemplate` → `"MESSAGE_PRODUCER" in capabilities` (regression: this proposal does NOT touch async) | +| 8 | `test_trace_flow_includes_client_in_stage_2` | trace_flow walks through `_FLOW_STAGES[2]` correctly with the new `CLIENT` literal | +| 9 | `test_codebase_search_entry_roles_includes_client` | `entry_roles` filter accepts `CLIENT` and excludes `FEIGN_CLIENT` (ensures server.py:1415 was updated) | + +Test count target: existing baseline + 9. Combined with PR-F1 (+6) and PR-G1 (+8), total target: **~289 passed, 4 skipped** (266 baseline + 6 + 8 + 9). Note: test #4 changed from "alias translated" to "role rejected" — same test slot, different assertion. + +### Manual evidence + +```bash +cd /home/user/workspace/user-rag + +rm -rf /tmp/check_client && \ + python build_ast_graph.py --source-root tests/fixtures/cross_service_smoke \ + --kuzu-path /tmp/check_client --verbose 2>&1 | tail -10 + +# Verify: +python -c " +import kuzu +db = kuzu.Database('/tmp/check_client'); conn = kuzu.Connection(db) +print('=== role distribution ===') +r = conn.execute('MATCH (s:Symbol) WHERE s.role IS NOT NULL RETURN s.role, count(s) ORDER BY s.role') +while r.has_next(): print(' ', r.get_next()) + +print('=== HTTP_CLIENT capability holders ===') +r = conn.execute(\"MATCH (s:Symbol) WHERE 'HTTP_CLIENT' IN s.capabilities RETURN s.fqn\") +while r.has_next(): print(' ', r.get_next()) +" +# Expected: +# role distribution includes CLIENT (the BFeignClient interface) +# role distribution does NOT include FEIGN_CLIENT +# HTTP_CLIENT capability holders include com.smoke.a.BFeignClient +``` + +## 8. Sequencing + +This proposal **must ship after** PR-F1 (Feign-not-an-exposer) and +PR-G1 (cross_service_resolution flag). Reasons: + +1. PR-F1 and PR-G1 are reviewed against today's familiar `FEIGN_CLIENT` + vocabulary. Stacking the rename first complicates two PRs that are + otherwise simple. +2. The rename's ontology bump composes cleanly on top of PR-G1's + bump (7→8 → 8→9). Reverse order requires renumbering. +3. The fixture assertions in PR-F1 ("`BFeignClient` does not appear + in EXPOSES") are unchanged regardless of role naming. Same for + PR-G1's brownfield gating. + +So the queue is: +1. Plans PR (#27) merges → cursor prompts for PR-F1, PR-G1 +2. PR-F1 ships +3. PR-G1 ships +4. Real-project test in `brownfield_only` mode (data-gathering) +5. **This proposal** drafted PR (covered by this doc); plan & implementing PR drafted next +6. (Parallel) review of #24 multi-attribution and any HTTP_CLIENT + role unification expansion based on real-project data + +## 9. [TBD] + +| # | Decision | Notes | +|---|----------|-------| +| 1 | Should `@CodebaseRole(role="CLIENT")` without `@CodebaseCapability(capability="HTTP_CLIENT")` be accepted as-is, or auto-add the capability? | Recommend accept as-is (no auto-add). The user explicitly opted in to `CLIENT` role; they may have a non-HTTP client (e.g., gRPC) where `HTTP_CLIENT` would be wrong. Capabilities are independent. | +| 2 | Is `_ROLE_SCORE_WEIGHTS["CLIENT"] = 0.06` still right when the role can include brownfield-annotated RestTemplate clients? | Recommend keep `0.06` for v1. Revisit only if behavioural-search drift is reported. | +| 3 | Should this proposal be folded into a single PR with PR-F1 / PR-G1 (smaller surface) or kept as PR-H1 (separate ontology bump)? | Recommend separate PR-H1. Combining muddies review. | +| 4 | Should we also rename `_ROLE_SCORE_WEIGHTS` key? | Yes — covered in §5.2. | +| 5 | Add a test that checks `VALID_ROLES` contains `CLIENT` but not `FEIGN_CLIENT`? | Recommend yes — guards against accidental re-introduction. Test #2 covers the graph-level check; add a unit test on `VALID_ROLES` directly. | + +## 10. References + +- `ast_java.py:90` — `ROLE_ANNOTATIONS["FeignClient"]` (the rename target) +- `ast_java.py:111-114` — `_TYPE_ANN_TO_CAPABILITY` (where `HTTP_CLIENT` is added) +- `ast_java.py:116-122` — `_INJECTED_TYPES_TO_CAPABILITY` (existing + `MESSAGE_PRODUCER` precedent — no change needed) +- `java_ontology.py:18-25` — `VALID_CAPABILITIES` (auto-derived; updates + for free) +- `kuzu_queries.py:956, 965, 975` — `_FLOW_STAGES`, `_ENTRYPOINT_ROLES`, + trace_flow docstring +- `search_lancedb.py:188` — `_ROLE_SCORE_WEIGHTS` +- `server.py:49, 687, 1138, 1335, 1339, 1415` — MCP tool docstrings, + `role` enum strings, `entry_roles` list +- `tests/test_lancedb_e2e.py:342` — one e2e assertion +- `graph_enrich.py:375, 408, 446, 733-735` — brownfield role validation + (existing `VALID_ROLES` check rejects `FEIGN_CLIENT` for free) +- `propose/DEFERRED-REST-CLIENT-MIGRATION-PROPOSE.md` — **superseded + and deleted** by this proposal +- `propose/FEIGN-NOT-AN-EXPOSER-PROPOSE.md` (PR #25) — orthogonal + bug fix; ships first via PR-F1 +- `propose/CROSS-SERVICE-RESOLUTION-FLAG-PROPOSE.md` (PR #26) — + orthogonal flag; ships before this via PR-G1 diff --git a/propose/DEFERRED-REST-CLIENT-MIGRATION-PROPOSE.md b/propose/DEFERRED-REST-CLIENT-MIGRATION-PROPOSE.md deleted file mode 100644 index 0600506..0000000 --- a/propose/DEFERRED-REST-CLIENT-MIGRATION-PROPOSE.md +++ /dev/null @@ -1,183 +0,0 @@ -# Deferred `FEIGN_CLIENT` → `REST_CLIENT` migration — proposal - -Status: **not implemented, intentionally deferred**. This document captures -the agreed direction so the next iteration can pick it up coherently with -the cross-service tracing work that gates it. - -## Why this is needed - -The current role taxonomy in `ast_java.py` recognises only `@FeignClient` as -an outbound HTTP integration: - -```python -ROLE_ANNOTATIONS = { - ... - "FeignClient": "FEIGN_CLIENT", - ... -} -``` - -The role name is tied to one specific Spring Cloud annotation. From an -end-user point of view this is leaky: the question being answered (in -`trace_flow`, `_ROLE_SCORE_WEIGHTS`, integration-stage seeding) is *"is this -a remote-service client?"*, not *"which library generated the proxy?"*. As -soon as the codebase mixes Feign with `@HttpExchange` (Spring 6 declarative -HTTP), Retrofit, raw `RestTemplate` / `WebClient` / `RestClient` (Spring -6.1+), or hand-rolled `OkHttpClient` callers, every one of those edges -becomes invisible to the integration stage of `trace_flow`. - -For cross-service tracing this is not a cosmetic gap — **a missing edge -silently breaks the chain**. Spurious edges can be filtered by the consumer; -missing edges cannot be recovered. - -## Why this is deferred (and not implemented now) - -The right scope of `REST_CLIENT` is determined by what **cross-service -tracing** actually needs to consume. Until that consumer exists we can -only guess at the right detector aggressiveness. Concretely: - -- The annotation-driven detectors (`@FeignClient`, `@HttpExchange`, - `@RetrofitClient`) are zero-false-positive and cheap. They are safe to - ship at any time. -- The injection-driven detectors (`RestTemplate`, `WebClient`, `RestClient`, - `OkHttpClient`, JDK `HttpClient`) are necessary for completeness but - produce noise: any `@Service` that makes one outbound call gets pulled - into the integration stage, displacing genuine orchestrators. - -Without a cross-service tracer to validate against, we cannot tell whether -the noisy detectors help or hurt. Shipping them speculatively would -destabilise current behavioural-search ranking for users who do *not* yet -have a tracer. - -The capabilities model (see `plans/PLAN-CAPABILITIES-MODEL.md`) resolves -the noise problem cleanly: keep `role = SERVICE` for the orchestrator, -add `REST_CLIENT` as a **capability** so cross-service tracing can find -the edge without the role-bias table downgrading the class. -**`REST_CLIENT` should therefore be implemented as a capability, not a -role rename, when the time comes.** - -## Migration shape (when picked up) - -### Pre-conditions (must be true before starting) - -1. The capabilities model from `plans/PLAN-CAPABILITIES-MODEL.md` is - merged. `TypeDecl.capabilities`, `Symbol.capabilities` (Kuzu), - `SymbolHit.capabilities`, `list_by_capability` MCP tool all exist. -2. A cross-service tracing consumer exists (or is being implemented in the - same change set) that demonstrably uses `REST_CLIENT` as input. - -### Detector set - -Two tiers, gated independently: - -#### Tier 1 — annotation-driven (always safe, ship first) - -| Annotation | Source | Notes | -|---|---|---| -| `@FeignClient` | Spring Cloud OpenFeign | already detected today | -| `@HttpExchange` | Spring 6 declarative HTTP | type-level on the interface | -| `@RetrofitClient` | spring-boot-starter-retrofit | community starter | - -#### Tier 2 — injection-driven (gated behind a flag, validate before defaulting on) - -A type acquires the `REST_CLIENT` capability if it injects (field, ctor -param, or Lombok-RAC final field) any of: - -| Type | Library | -|---|---| -| `RestTemplate` | Spring Web | -| `WebClient` | Spring WebFlux | -| `RestClient` | Spring Web 6.1+ | -| `OkHttpClient` | Square OkHttp | -| `HttpClient` | JDK 11+ `java.net.http` | - -Tier 2 is opt-in via the brownfield config (see -`plans/PLAN-BROWNFIELD-ROLE-OVERRIDES.md`): - -```yaml -rest_client_detection: - injection_based: true # default false until cross-service tracing validated -``` - -### Decision: `REST_CLIENT` as **capability**, not a role rename - -Do not rename the `FEIGN_CLIENT` role to `REST_CLIENT`. Instead: - -1. Stop emitting `FEIGN_CLIENT` as a primary role (i.e. remove the - `"FeignClient": "FEIGN_CLIENT"` entry from `ROLE_ANNOTATIONS`). -2. Emit `REST_CLIENT` as a **capability** populated by the detectors above. -3. The primary role for a `@FeignClient` interface falls back to its - stereotype if any (`@Component`, etc.) or `OTHER`. Most `@FeignClient`s - are interfaces with no other annotation, so `OTHER + capability=REST_CLIENT` - is the typical resulting tag set. - -Why capability rather than rename: - -- A `@Service` that injects `WebClient` is genuinely both a service and a - REST client. Forcing one role discards information. -- The `_ROLE_SCORE_WEIGHTS` table in `search_lancedb.py` tunes ranking on a - *single* axis. Treating a service-with-outbound-calls as `REST_CLIENT` - would silently demote it from `0.08` → `0.06`. That is wrong for - behavioural search. -- Multiple capabilities compose naturally: a `@FeignClient` interface that - is also `@CircuitBreaker`-wrapped at the type level can carry both - `REST_CLIENT` and (future) `RESILIENCE_PATTERN` capabilities. - -## Call sites to update - -| File | Change | -|---|---| -| `ast_java.py` | Drop `FeignClient` from `ROLE_ANNOTATIONS`. Add `REST_CLIENT` to the capability detector tables (annotation set + injection-type set). | -| `search_lancedb.py` | Drop `"FEIGN_CLIENT": 0.06` from `_ROLE_SCORE_WEIGHTS`. Decide whether `REST_CLIENT` capability gets a *capability-bias* (parallel table) — recommend **no** for now; let cross-service tracing surface them via explicit capability filter rather than ranking nudge. | -| `kuzu_queries.py` | Replace `FEIGN_CLIENT` in `_FLOW_STAGES[2]` with a capability-aware predicate: `(s.role IN ['REPOSITORY','MAPPER'] OR 'REST_CLIENT' IN s.capabilities)`. Same for `_ENTRYPOINT_ROLES` if `REST_CLIENT` proxies are ever entrypoints (rare — usually they are *called from* services). | -| `server.py` | Remove `FEIGN_CLIENT` from the `role` enum strings in `codebase_search`, `list_by_role`, and the `trace_flow` docstring. Update mention of "CONTROLLER → SERVICE → REPOSITORY/FEIGN" to "CONTROLLER → SERVICE → REPOSITORY/REST_CLIENT". | -| `README.md`, `CODEBASE_REQUIREMENTS.md` | Doc sweep. | -| `tests/test_lancedb_e2e.py` | Update any assertions that key on `FEIGN_CLIENT`. | - -## Ontology version - -This is a breaking schema change for the Kuzu graph (existing rows have -`role = "FEIGN_CLIENT"` that no longer maps cleanly). Bump -`ONTOLOGY_VERSION` (will already be `3` after the capabilities plan; this -change bumps it to `4`). - -The graph **must** be rebuilt; no online migration is provided. This is -acceptable per project policy ("breaking changes are allowed"). - -## Test plan - -1. **Unit (annotation tier).** Fixture file with a `@FeignClient` interface, - an `@HttpExchange` interface, and a plain interface. Assert: - - both annotated interfaces have `REST_CLIENT` in `capabilities` - - neither has `role = "FEIGN_CLIENT"` (the role is gone) - - the plain interface has no `REST_CLIENT` capability. - -2. **Unit (injection tier, when enabled).** Fixture `@Service` with a - `WebClient` field. Assert `role = "SERVICE"`, `capabilities` contains - `REST_CLIENT`. With injection-tier disabled (default), the same fixture - yields `capabilities = []`. - -3. **End-to-end.** Reuse the e2e test pattern in `tests/test_lancedb_e2e.py`. - Add a synthetic two-service fixture where service A's `@Service` - injects a `@FeignClient` for service B. Assert that: - - `list_by_capability("REST_CLIENT", microservice="service-a")` returns - the Feign interface. - - `trace_flow("...", microservice="service-a")` reaches the Feign - interface in stage 2. - -4. **Regression.** All current `FEIGN_CLIENT`-keyed e2e assertions must - keep passing under the new spelling (`REST_CLIENT` capability). - -## Out of scope - -- Inbound HTTP detection beyond `@RestController` / `@Controller` (covered by - existing primary roles). -- gRPC clients — separate `GRPC_CLIENT` capability if the codebase warrants it. -- Service-mesh-injected clients (Istio sidecars, etc.) — invisible to the AST. -- Cross-service tracing itself — that is the *consumer* of this change and - has its own design. - -## Decision log - -- 2026-04-26: Agreed to defer until cross-service tracing exists. Agreed - `REST_CLIENT` is a capability, not a role rename. Recorded in this file.