Api integration - #7
harshitha711 wants to merge 9 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Qodana for JVM3 new problems were found
☁️ View the detailed Qodana report Contact Qodana teamContact us at qodana-support@jetbrains.com
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
src/main/java/com/modernizer/orchestrator_service/clients/EpamDialClient.java (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
apiVersionas a constant.The value never changes per instance. Static analysis flags the field for this reason. Consider making it a
private static finalconstant, or externalize it as a property if the DIAL API version must be configurable per environment.♻️ Proposed change
- private final String apiVersion = "2023-12-01-preview"; + private static final String API_VERSION = "2023-12-01-preview";🤖 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/main/java/com/modernizer/orchestrator_service/clients/EpamDialClient.java` at line 21, Update the apiVersion field in EpamDialClient to be a private static final constant, preserving its existing value and access level.Source: Linters/SAST tools
src/main/java/com/modernizer/orchestrator_service/api/LlmOrchestrationController.java (1)
23-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo tests cover the new endpoint.
This PR adds a public endpoint, a routing service, and five provider clients with request translation, but the diff contains no tests. The plain-text-to-JSON conversion at lines 49-62 and the translation logic in
ClaudeClientandGeminiClientare the highest-value targets. Add@WebMvcTestcoverage for the controller and unit tests for each translation method.I can generate the initial test classes if you want.
🤖 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/main/java/com/modernizer/orchestrator_service/api/LlmOrchestrationController.java` around lines 23 - 28, Add `@WebMvcTest` coverage for LlmOrchestrationController, including the new public endpoint and its plain-text-to-JSON conversion, and add unit tests for every provider translation method, with particular coverage for ClaudeClient and GeminiClient. Mock routing/dependent services as needed and verify translated request payloads and controller responses.src/main/resources/application.yml (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or document the now-dead Google GenAI configuration.
This change excludes
GoogleGenAiChatAutoConfiguration, butspring.ai.google.genai.*at lines 48-56 remains, andbuild.gradlestill declaresspring-ai-starter-model-google-genai. The remaining block is now inert configuration that suggests an active integration. If Gemini access now goes throughGeminiClient, remove the stale block, or add a comment that states why the properties are kept.🤖 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/main/resources/application.yml` around lines 7 - 11, Update the Google GenAI configuration in application.yml: if Gemini access now uses GeminiClient, remove the stale spring.ai.google.genai.* properties and the corresponding unused dependency; otherwise retain the properties and add a clear comment explaining why they remain despite excluding GoogleGenAiChatAutoConfiguration.src/main/java/com/modernizer/orchestrator_service/clients/OpenAiClient.java (1)
34-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared OpenAI-shaped client logic.
OpenAiClient,CopilotClient, andEpamDialClientrepeat the same flow: build a POST, checkstatusCode() >= 400, readchoices[0].message.content, and throw on parse failure.ClaudeClientandGeminiClientrepeat the base-url trimming andisActive()logic too. Extract an abstract base class or a small helper that takes the URL, the auth headers, and a response extractor. This keeps a future change, such as adding request timeouts or retry handling, to one place.This is a follow-up refactor, not a merge blocker.
🤖 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/main/java/com/modernizer/orchestrator_service/clients/OpenAiClient.java` around lines 34 - 56, Extract the duplicated request and response-handling flow used by OpenAiClient, CopilotClient, and EpamDialClient into a shared base class or helper. Centralize POST construction, authentication headers, statusCode() error handling, and choices[0].message.content extraction while allowing each client to provide its URL and auth headers. Also reuse shared base-url trimming and isActive() logic across ClaudeClient and GeminiClient, without changing client-specific response extraction.
🤖 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.
Inline comments:
In `@build.gradle`:
- Around line 229-230: Remove the `runtimeOnly 'com.h2database:h2'` declaration
from the main runtime configuration in `build.gradle`, leaving H2 available only
through `testRuntimeOnly` unless a dedicated local-profile configuration is
explicitly required.
In
`@src/main/java/com/modernizer/orchestrator_service/api/LlmOrchestrationController.java`:
- Around line 63-74: Update the JSON handling in LlmOrchestrationController to
catch parsing failures separately, log the exception at debug level, and return
an HTTP 400 response for malformed JSON instead of forwarding formattedPayload
unchanged. Preserve the existing default-model insertion for valid JSON missing
model.
- Around line 46-62: Replace the manual JSON construction in the raw-prompt
branch of LlmOrchestrationController with Jackson serialization, constructing
the request object and user message through the existing ObjectMapper or
equivalent JSON API. Preserve the gpt-4o model and prompt content while letting
Jackson escape backslashes, quotes, and all control characters safely; avoid
string formatting for payload generation.
- Around line 80-92: Update handleClientErrors and handleNetworkErrors to avoid
Map.of with nullable exception messages by using non-null fallback messages. In
handleNetworkErrors, log the detailed upstream exception server-side while
returning a generic client-safe message, and restore the thread interrupt status
when ex is an InterruptedException before responding with BAD_GATEWAY.
In `@src/main/java/com/modernizer/orchestrator_service/clients/ClaudeClient.java`:
- Around line 96-107: In the message-building logic of ClaudeClient, replace the
redundant role ternary with direct preservation of the assistant role while
retaining the user fallback for other roles. Before setting claudeRoot’s
messages in the request-building method, reject an empty claudeMessages
collection with a clear error message so requests containing only system or
developer messages fail locally.
In
`@src/main/java/com/modernizer/orchestrator_service/clients/CopilotClient.java`:
- Around line 35-44: Replace the direct HTTP request in
CopilotClient.chatCompletions with a supported Copilot SDK/CLI integration, or
implement a documented compatible provider contract before enabling the client.
Remove reliance on EXTERNAL_COPILOT_API_KEY and the Bearer Authorization header
as Copilot authentication, while preserving the chat-completions behavior
through the supported integration.
In `@src/main/java/com/modernizer/orchestrator_service/clients/GeminiClient.java`:
- Around line 70-76: The translation methods in
src/main/java/com/modernizer/orchestrator_service/clients/GeminiClient.java:70-76
and
src/main/java/com/modernizer/orchestrator_service/clients/ClaudeClient.java:83-89
must validate that messages is a non-empty array instead of casting
root.path("messages"), throwing IOException for invalid input, and must preserve
text from array-shaped content parts. Extract the shared messages validation and
content extraction into one helper reused by GeminiClient and ClaudeClient so
both translate methods behave consistently.
- Around line 38-40: Update GeminiClient’s model extraction/validation to accept
only values matching ^gemini-[A-Za-z0-9._-]+$ (falling back to gemini-2.5-pro or
rejecting invalid input), preventing path injection. Build the generateContent
URL without the key query parameter and send apiKey via the x-goog-api-key
request header. In the IOException handling path, stop appending resp.body() so
the controller’s returned exception message does not expose the response body.
In
`@src/main/java/com/modernizer/orchestrator_service/clients/HttpClientConfig.java`:
- Around line 16-19: Update the shared HttpClient in
HttpClientConfig.httpClient() to configure an explicit connect timeout, and add
a bounded HttpRequest.timeout to every provider request, including the request
built in OpenAiClient.chatCompletions. Use the existing provider client
request-building methods and apply a consistent appropriate timeout so
httpClient.send cannot wait indefinitely.
In
`@src/main/java/com/modernizer/orchestrator_service/clients/LlmOrchestrator.java`:
- Around line 13-35: Update LlmOrchestrator to accept and store an explicit
defaultProvider, prefer it when routeChat receives a blank provider, and
otherwise select the first client using a deterministic ordering rather than
HashMap iteration. Normalize provider names with Locale.ROOT in both the
constructor’s clients map key and routeChat lookup.
In `@src/main/java/com/modernizer/orchestrator_service/clients/OpenAiClient.java`:
- Around line 14-15: Update the `@ConditionalOnProperty` annotation on
OpenAiClient and the corresponding four provider client classes to prevent bean
creation when the configured API key is blank, using a blank-aware condition
such as `@ConditionalOnExpression` or removing the annotations consistently.
Preserve bean creation only when the provider API key contains non-whitespace
content.
---
Nitpick comments:
In
`@src/main/java/com/modernizer/orchestrator_service/api/LlmOrchestrationController.java`:
- Around line 23-28: Add `@WebMvcTest` coverage for LlmOrchestrationController,
including the new public endpoint and its plain-text-to-JSON conversion, and add
unit tests for every provider translation method, with particular coverage for
ClaudeClient and GeminiClient. Mock routing/dependent services as needed and
verify translated request payloads and controller responses.
In
`@src/main/java/com/modernizer/orchestrator_service/clients/EpamDialClient.java`:
- Line 21: Update the apiVersion field in EpamDialClient to be a private static
final constant, preserving its existing value and access level.
In `@src/main/java/com/modernizer/orchestrator_service/clients/OpenAiClient.java`:
- Around line 34-56: Extract the duplicated request and response-handling flow
used by OpenAiClient, CopilotClient, and EpamDialClient into a shared base class
or helper. Centralize POST construction, authentication headers, statusCode()
error handling, and choices[0].message.content extraction while allowing each
client to provide its URL and auth headers. Also reuse shared base-url trimming
and isActive() logic across ClaudeClient and GeminiClient, without changing
client-specific response extraction.
In `@src/main/resources/application.yml`:
- Around line 7-11: Update the Google GenAI configuration in application.yml: if
Gemini access now uses GeminiClient, remove the stale spring.ai.google.genai.*
properties and the corresponding unused dependency; otherwise retain the
properties and add a clear comment explaining why they remain despite excluding
GoogleGenAiChatAutoConfiguration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9334ebd1-4b74-44b6-a99a-852e8cd2fa84
📒 Files selected for processing (12)
.env.examplebuild.gradlesrc/main/java/com/modernizer/orchestrator_service/api/LlmOrchestrationController.javasrc/main/java/com/modernizer/orchestrator_service/clients/ClaudeClient.javasrc/main/java/com/modernizer/orchestrator_service/clients/CopilotClient.javasrc/main/java/com/modernizer/orchestrator_service/clients/EpamDialClient.javasrc/main/java/com/modernizer/orchestrator_service/clients/GeminiClient.javasrc/main/java/com/modernizer/orchestrator_service/clients/HttpClientConfig.javasrc/main/java/com/modernizer/orchestrator_service/clients/LlmOrchestrator.javasrc/main/java/com/modernizer/orchestrator_service/clients/LlmService.javasrc/main/java/com/modernizer/orchestrator_service/clients/OpenAiClient.javasrc/main/resources/application.yml
💤 Files with no reviewable changes (1)
- .env.example
| testRuntimeOnly 'com.h2database:h2' | ||
| runtimeOnly 'com.h2database:h2' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not put H2 on the production runtime classpath.
Line 230 adds H2 to the main runtime configuration. The application targets PostgreSQL, so H2 is not needed at runtime. H2 has a record of high-severity advisories, and its presence can also enable unintended auto-configuration paths. If a test or local profile needs H2, keep only the testRuntimeOnly entry at line 229, or move it to a dedicated local source set.
🛡️ Proposed fix
testRuntimeOnly 'com.h2database:h2'
- runtimeOnly 'com.h2database:h2'If the intent is to support an H2-backed local profile, tell me and I can propose a profile-scoped configuration instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testRuntimeOnly 'com.h2database:h2' | |
| runtimeOnly 'com.h2database:h2' | |
| testRuntimeOnly 'com.h2database:h2' |
🤖 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 `@build.gradle` around lines 229 - 230, Remove the `runtimeOnly
'com.h2database:h2'` declaration from the main runtime configuration in
`build.gradle`, leaving H2 available only through `testRuntimeOnly` unless a
dedicated local-profile configuration is explicitly required.
| String formattedPayload = body.trim(); | ||
|
|
||
| // Case A: Input is raw plain text prompt | ||
| if (!formattedPayload.startsWith("{")) { | ||
| formattedPayload = | ||
| """ | ||
| { | ||
| "model": "gpt-4o", | ||
| "messages": [ | ||
| { | ||
| "role": "user", | ||
| "content": "%s" | ||
| } | ||
| ] | ||
| } | ||
| """ | ||
| .formatted(formattedPayload.replace("\"", "\\\"").replace("\n", "\\n")); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Manual string escaping allows JSON injection.
Line 62 escapes only " and \n. It does not escape backslashes or other control characters. A prompt that contains \ immediately before " produces \\", which closes the JSON string early. The caller can then inject arbitrary members into the request, for example an extra system message or a different model. Input with a tab or another control character produces invalid JSON.
Build the payload with Jackson instead of string formatting.
🛡️ Proposed fix
if (!formattedPayload.startsWith("{")) {
- formattedPayload =
- """
- {
- "model": "gpt-4o",
- "messages": [
- {
- "role": "user",
- "content": "%s"
- }
- ]
- }
- """
- .formatted(formattedPayload.replace("\"", "\\\"").replace("\n", "\\n"));
+ ObjectNode payload = objectMapper.createObjectNode();
+ payload.put("model", "gpt-4o");
+ ObjectNode message = objectMapper.createObjectNode();
+ message.put("role", "user");
+ message.put("content", formattedPayload);
+ payload.putArray("messages").add(message);
+ formattedPayload = objectMapper.writeValueAsString(payload);
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String formattedPayload = body.trim(); | |
| // Case A: Input is raw plain text prompt | |
| if (!formattedPayload.startsWith("{")) { | |
| formattedPayload = | |
| """ | |
| { | |
| "model": "gpt-4o", | |
| "messages": [ | |
| { | |
| "role": "user", | |
| "content": "%s" | |
| } | |
| ] | |
| } | |
| """ | |
| .formatted(formattedPayload.replace("\"", "\\\"").replace("\n", "\\n")); | |
| String formattedPayload = body.trim(); | |
| // Case A: Input is raw plain text prompt | |
| if (!formattedPayload.startsWith("{")) { | |
| ObjectNode payload = objectMapper.createObjectNode(); | |
| payload.put("model", "gpt-4o"); | |
| ObjectNode message = objectMapper.createObjectNode(); | |
| message.put("role", "user"); | |
| message.put("content", formattedPayload); | |
| payload.putArray("messages").add(message); | |
| formattedPayload = objectMapper.writeValueAsString(payload); | |
| } else { |
🤖 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/main/java/com/modernizer/orchestrator_service/api/LlmOrchestrationController.java`
around lines 46 - 62, Replace the manual JSON construction in the raw-prompt
branch of LlmOrchestrationController with Jackson serialization, constructing
the request object and user message through the existing ObjectMapper or
equivalent JSON API. Preserve the gpt-4o model and prompt content while letting
Jackson escape backslashes, quotes, and all control characters safely; avoid
string formatting for payload generation.
| } else { | ||
| // Case B: Input is JSON but might be missing the "model" parameter | ||
| try { | ||
| JsonNode jsonNode = objectMapper.readTree(formattedPayload); | ||
| if (!jsonNode.has("model") || jsonNode.path("model").asText().isBlank()) { | ||
| ((ObjectNode) jsonNode).put("model", "gpt-4o"); | ||
| formattedPayload = objectMapper.writeValueAsString(jsonNode); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // Fall back to original formattedPayload if parsing fails | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject malformed JSON instead of forwarding it.
Line 71 catches every exception and forwards the original body unchanged. A payload that starts with { but is not valid JSON then reaches the provider, which returns an error that surfaces as HTTP 502. Return HTTP 400 for a malformed body so the caller sees the real cause. Log the parse failure at debug level rather than discarding it.
🤖 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/main/java/com/modernizer/orchestrator_service/api/LlmOrchestrationController.java`
around lines 63 - 74, Update the JSON handling in LlmOrchestrationController to
catch parsing failures separately, log the exception at debug level, and return
an HTTP 400 response for malformed JSON instead of forwarding formattedPayload
unchanged. Preserve the existing default-model insertion for valid JSON missing
model.
| /** Centralized local error handler for invalid providers or configurations. */ | ||
| @ExceptionHandler({IllegalArgumentException.class, IllegalStateException.class}) | ||
| public ResponseEntity<Map<String, String>> handleClientErrors(RuntimeException ex) { | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(Map.of("error", "Invalid Request", "message", ex.getMessage())); | ||
| } | ||
|
|
||
| /** Centralized exception handler for upstream network/parsing errors. */ | ||
| @ExceptionHandler({IOException.class, InterruptedException.class}) | ||
| public ResponseEntity<Map<String, String>> handleNetworkErrors(Exception ex) { | ||
| return ResponseEntity.status(HttpStatus.BAD_GATEWAY) | ||
| .body(Map.of("error", "Upstream Service Failure", "message", ex.getMessage())); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Both handlers can throw, and one leaks upstream response bodies.
Three problems exist in these handlers.
First, Map.of rejects null values. ex.getMessage() is null for many exceptions, including an InterruptedException raised with no message. The handler then throws NullPointerException and the caller receives HTTP 500 instead of the intended status.
Second, the provider clients embed the full upstream response body in the IOException message, for example OpenAiClient line 47 and GeminiClient line 51. Line 91 returns that message to the caller. Upstream bodies can carry provider-side account and request detail. Log the body server-side and return a generic message.
Third, the handler consumes InterruptedException without restoring the interrupt flag. Downstream code then cannot observe the cancellation.
🛡️ Proposed fix
`@ExceptionHandler`({IllegalArgumentException.class, IllegalStateException.class})
public ResponseEntity<Map<String, String>> handleClientErrors(RuntimeException ex) {
return ResponseEntity.status(HttpStatus.BAD_REQUEST)
- .body(Map.of("error", "Invalid Request", "message", ex.getMessage()));
+ .body(
+ Map.of(
+ "error",
+ "Invalid Request",
+ "message",
+ Objects.requireNonNullElse(ex.getMessage(), "Invalid request.")));
}
`@ExceptionHandler`({IOException.class, InterruptedException.class})
public ResponseEntity<Map<String, String>> handleNetworkErrors(Exception ex) {
+ if (ex instanceof InterruptedException) {
+ Thread.currentThread().interrupt();
+ }
+ log.error("Upstream LLM provider call failed", ex);
return ResponseEntity.status(HttpStatus.BAD_GATEWAY)
- .body(Map.of("error", "Upstream Service Failure", "message", ex.getMessage()));
+ .body(
+ Map.of(
+ "error",
+ "Upstream Service Failure",
+ "message",
+ "The upstream LLM provider did not return a valid response."));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Centralized local error handler for invalid providers or configurations. */ | |
| @ExceptionHandler({IllegalArgumentException.class, IllegalStateException.class}) | |
| public ResponseEntity<Map<String, String>> handleClientErrors(RuntimeException ex) { | |
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | |
| .body(Map.of("error", "Invalid Request", "message", ex.getMessage())); | |
| } | |
| /** Centralized exception handler for upstream network/parsing errors. */ | |
| @ExceptionHandler({IOException.class, InterruptedException.class}) | |
| public ResponseEntity<Map<String, String>> handleNetworkErrors(Exception ex) { | |
| return ResponseEntity.status(HttpStatus.BAD_GATEWAY) | |
| .body(Map.of("error", "Upstream Service Failure", "message", ex.getMessage())); | |
| } | |
| /** Centralized local error handler for invalid providers or configurations. */ | |
| `@ExceptionHandler`({IllegalArgumentException.class, IllegalStateException.class}) | |
| public ResponseEntity<Map<String, String>> handleClientErrors(RuntimeException ex) { | |
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | |
| .body( | |
| Map.of( | |
| "error", | |
| "Invalid Request", | |
| "message", | |
| Objects.requireNonNullElse(ex.getMessage(), "Invalid request."))); | |
| } | |
| /** Centralized exception handler for upstream network/parsing errors. */ | |
| `@ExceptionHandler`({IOException.class, InterruptedException.class}) | |
| public ResponseEntity<Map<String, String>> handleNetworkErrors(Exception ex) { | |
| if (ex instanceof InterruptedException) { | |
| Thread.currentThread().interrupt(); | |
| } | |
| log.error("Upstream LLM provider call failed", ex); | |
| return ResponseEntity.status(HttpStatus.BAD_GATEWAY) | |
| .body( | |
| Map.of( | |
| "error", | |
| "Upstream Service Failure", | |
| "message", | |
| "The upstream LLM provider did not return a valid response.")); | |
| } |
🤖 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/main/java/com/modernizer/orchestrator_service/api/LlmOrchestrationController.java`
around lines 80 - 92, Update handleClientErrors and handleNetworkErrors to avoid
Map.of with nullable exception messages by using non-null fallback messages. In
handleNetworkErrors, log the detailed upstream exception server-side while
returning a generic client-safe message, and restore the thread interrupt status
when ex is an InterruptedException before responding with BAD_GATEWAY.
| } else { | ||
| ObjectNode claudeMsg = objectMapper.createObjectNode(); | ||
| claudeMsg.put("role", "assistant".equalsIgnoreCase(role) ? "assistant" : "user"); | ||
| claudeMsg.put("content", content); | ||
| claudeMessages.add(claudeMsg); | ||
| } | ||
| } | ||
|
|
||
| if (!systemPrompt.isEmpty()) { | ||
| claudeRoot.put("system", systemPrompt.toString()); | ||
| } | ||
| claudeRoot.set("messages", claudeMessages); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Simplify the role ternary and reject an empty messages array.
Line 98 maps "assistant" to "assistant", so the ternary always yields its input for that branch. Write the intent directly.
Line 107 also sets messages even when every incoming message had the system or developer role. The Anthropic Messages API rejects an empty messages array, so the caller receives an upstream 400 mapped to HTTP 502. Fail early with a clear message instead.
♻️ Proposed change
- claudeMsg.put("role", "assistant".equalsIgnoreCase(role) ? "assistant" : "user");
+ claudeMsg.put("role", "assistant".equalsIgnoreCase(role) ? "assistant" : "user");
+ // consider: String claudeRole = "assistant".equalsIgnoreCase(role) ? "assistant" : "user";
}
}
+ if (claudeMessages.isEmpty()) {
+ throw new IOException("Claude request requires at least one user or assistant message.");
+ }
if (!systemPrompt.isEmpty()) {🤖 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/main/java/com/modernizer/orchestrator_service/clients/ClaudeClient.java`
around lines 96 - 107, In the message-building logic of ClaudeClient, replace
the redundant role ternary with direct preservation of the assistant role while
retaining the user fallback for other roles. Before setting claudeRoot’s
messages in the request-building method, reject an empty claudeMessages
collection with a clear error message so requests containing only system or
developer messages fail locally.
| String model = extractModel(bodyJson, "gemini-2.5-pro"); | ||
| String translatedPayload = translateToGeminiPayload(bodyJson); | ||
| String url = baseUrl + "/v1beta/models/" + model + ":generateContent?key=" + apiKey; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Gemini generateContent REST API x-goog-api-key header authentication
💡 Result:
To authenticate REST API requests to the Gemini generateContent endpoint, you must include your API key in the request header using the x-goog-api-key key [1][2][3]. When making a POST request, include the header as follows: -H "x-goog-api-key: YOUR_API_KEY" [1][4][2] Example using cURL: curl "https://generativelanguage.googleapis.com/v1beta/models/gemini-3.6-flash:generateContent" \ -H "Content-Type: application/json" \ -H "x-goog-api-key: YOUR_API_KEY" \ -X POST \ -d '{ "contents": [ { "parts": [ {"text": "Explain how AI works in a few words"} ] } ] }' [1][4][2] Key considerations for API key usage: - Obtaining Keys: You can generate and manage your API keys through Google AI Studio [2][5][6]. - Key Types: New keys created in AI Studio are typically "authorization" (auth) keys, which are bound to a Google Cloud service account for better security and access control [7][5]. - Security: Avoid exposing your API key in public repositories. For local development, it is standard practice to store the key in an environment variable (e.g., GEMINI_API_KEY) and reference it in your requests [5][6]. - Alternative Authentication: While the API key header is the standard method for most implementations, you can also use OAuth 2.0 if your application requires stricter access controls or needs to access user-specific data [8].
Citations:
- 1: https://ai.google.dev/gemini-api/docs/generate-content/api-key
- 2: https://ai.google.dev/api
- 3: https://ai.google.dev/gemini-api/docs/generate-content/text-generation
- 4: https://ai.google.dev/gemini-api/docs/generate-content/get-started
- 5: https://ai.google.dev/gemini-api/docs/interactions/api-key
- 6: https://ai.google.dev/gemini-api/docs/get-started
- 7: https://ai.google.dev/gemini-api/docs/api-key
- 8: https://ai.google.dev/gemini-api/docs/oauth
🏁 Script executed:
#!/bin/bash
set -e
file="src/main/java/com/modernizer/orchestrator_service/clients/GeminiClient.java"
ast-grep outline "$file" --lang java
printf '\n--- GeminiClient.java ---\n'
cat -n "$file"
printf '\n--- related symbols ---\n'
rg -n "extractModel|IOException|GeminiClient|apiKey|generateContent|baseUrl" src/main/java src/test 2>/dev/null | head -200Repository: sarojswain680/Phoenix-orchestrator-svc
Length of output: 18332
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- controller exception handling ---'
cat -n src/main/java/com/modernizer/orchestrator_service/api/LlmOrchestrationController.java | sed -n '30,105p'
printf '%s\n' '--- Gemini-related tests and configuration ---'
rg -n -i "gemini|model|api-key|base-url" src/test src/main/resources README.md .github 2>/dev/null | head -240
printf '%s\n' '--- all GeminiClient references ---'
rg -n "GeminiClient|extractModel|chatCompletions\\(" src/main src/test 2>/dev/null | head -240Repository: sarojswain680/Phoenix-orchestrator-svc
Length of output: 7878
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from urllib.parse import urlsplit
base = "https://generativelanguage.googleapis.com"
models = [
"gemini-2.5-pro",
"gemini/../other-model",
"not-gemini/../other-model",
]
for model in models:
url = f"{base}/v1beta/models/{model}:generateContent?key=secret"
print(model, "=>", urlsplit(url).path, "query=", urlsplit(url).query)
PYRepository: sarojswain680/Phoenix-orchestrator-svc
Length of output: 458
Move the Gemini API key to the request header and constrain model.
contains("gemini") accepts path separators such as gemini/../other-model, allowing caller-controlled path injection. Use an allow-list or reject values that do not match ^gemini-[A-Za-z0-9._-]+$. Build the URL without the key query parameter and send apiKey in x-goog-api-key. Also avoid appending resp.body() to IOException, because the controller returns ex.getMessage() to the caller.
🤖 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/main/java/com/modernizer/orchestrator_service/clients/GeminiClient.java`
around lines 38 - 40, Update GeminiClient’s model extraction/validation to
accept only values matching ^gemini-[A-Za-z0-9._-]+$ (falling back to
gemini-2.5-pro or rejecting invalid input), preventing path injection. Build the
generateContent URL without the key query parameter and send apiKey via the
x-goog-api-key request header. In the IOException handling path, stop appending
resp.body() so the controller’s returned exception message does not expose the
response body.
| ArrayNode openAiMessages = (ArrayNode) root.path("messages"); | ||
| ArrayNode contents = objectMapper.createArrayNode(); | ||
| StringBuilder systemPrompt = new StringBuilder(); | ||
|
|
||
| for (JsonNode msg : openAiMessages) { | ||
| String role = msg.path("role").asText(); | ||
| String content = msg.path("content").asText(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unvalidated messages handling in both translation methods. Both clients cast root.path("messages") directly to ArrayNode and read content with asText(). A request without a messages field yields a MissingNode, and the cast throws ClassCastException. No @ExceptionHandler in LlmOrchestrationController covers that type, so the caller receives HTTP 500 instead of HTTP 400. A request with array-shaped content loses its text, because asText() returns an empty string for a non-textual node.
src/main/java/com/modernizer/orchestrator_service/clients/GeminiClient.java#L70-L76: replace the cast with a check such asJsonNode messages = root.path("messages"); if (!messages.isArray() || messages.isEmpty()) { throw new IOException("Request must contain a non-empty 'messages' array."); }, and extract text from array-shapedcontentparts instead of callingasText()at line 76.src/main/java/com/modernizer/orchestrator_service/clients/ClaudeClient.java#L83-L89: apply the same guarded read ofmessagesintranslateToClaudePayload, and apply the samecontentextraction at line 89.
Extract the guard and the content extraction into one shared helper so the two clients cannot diverge.
📍 Affects 2 files
src/main/java/com/modernizer/orchestrator_service/clients/GeminiClient.java#L70-L76(this comment)src/main/java/com/modernizer/orchestrator_service/clients/ClaudeClient.java#L83-L89
🤖 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/main/java/com/modernizer/orchestrator_service/clients/GeminiClient.java`
around lines 70 - 76, The translation methods in
src/main/java/com/modernizer/orchestrator_service/clients/GeminiClient.java:70-76
and
src/main/java/com/modernizer/orchestrator_service/clients/ClaudeClient.java:83-89
must validate that messages is a non-empty array instead of casting
root.path("messages"), throwing IOException for invalid input, and must preserve
text from array-shaped content parts. Extract the shared messages validation and
content extraction into one helper reused by GeminiClient and ClaudeClient so
both translate methods behave consistently.
| @Bean | ||
| public HttpClient httpClient() { | ||
| return HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1).build(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a connect timeout to the shared HttpClient.
The client has no connect timeout, and none of the provider clients set HttpRequest.timeout(...). httpClient.send then blocks the calling request thread until the OS-level TCP timeout. A slow or unreachable provider can exhaust the servlet thread pool.
Set a connect timeout here, and set a per-request timeout in each provider client.
🛡️ Proposed fix
`@Bean`
public HttpClient httpClient() {
- return HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1).build();
+ return HttpClient.newBuilder()
+ .version(HttpClient.Version.HTTP_1_1)
+ .connectTimeout(Duration.ofSeconds(10))
+ .build();
}Apply the request timeout in each client, for example in OpenAiClient.chatCompletions:
HttpRequest req =
HttpRequest.newBuilder()
.uri(URI.create(url))
.timeout(Duration.ofSeconds(60))
// ...
.build();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Bean | |
| public HttpClient httpClient() { | |
| return HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1).build(); | |
| } | |
| `@Bean` | |
| public HttpClient httpClient() { | |
| return HttpClient.newBuilder() | |
| .version(HttpClient.Version.HTTP_1_1) | |
| .connectTimeout(Duration.ofSeconds(10)) | |
| .build(); | |
| } |
🤖 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/main/java/com/modernizer/orchestrator_service/clients/HttpClientConfig.java`
around lines 16 - 19, Update the shared HttpClient in
HttpClientConfig.httpClient() to configure an explicit connect timeout, and add
a bounded HttpRequest.timeout to every provider request, including the request
built in OpenAiClient.chatCompletions. Use the existing provider client
request-building methods and apply a consistent appropriate timeout so
httpClient.send cannot wait indefinitely.
| public LlmOrchestrator(List<LlmService> services) { | ||
| // Only register clients that actually have a valid API key configured | ||
| this.clients = | ||
| services.stream() | ||
| .filter(LlmService::isActive) | ||
| .collect( | ||
| Collectors.toMap( | ||
| service -> service.getProviderName().toLowerCase(), service -> service)); | ||
| } | ||
|
|
||
| public String routeChat(String provider, String bodyJson) | ||
| throws IOException, InterruptedException { | ||
|
|
||
| String activeProvider = provider; | ||
|
|
||
| // Auto-detect: Pick the first active client registered from your .env | ||
| if (activeProvider == null || activeProvider.isBlank()) { | ||
| if (clients.isEmpty()) { | ||
| throw new IllegalStateException( | ||
| "No active LLM providers are configured in your .env file!"); | ||
| } | ||
| activeProvider = clients.keySet().iterator().next(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Auto-selection picks an arbitrary provider.
Collectors.toMap returns a HashMap, so clients.keySet() has no defined order. Line 34 therefore selects a provider determined by hash order, not by intent. Two environments with the same set of keys can route to different providers, and the selection can change when a provider name is added or renamed.
Add an explicit default provider property, and fall back to a deterministic order.
🛠️ Proposed fix
- public LlmOrchestrator(List<LlmService> services) {
+ private final String defaultProvider;
+
+ public LlmOrchestrator(
+ List<LlmService> services,
+ `@Value`("${external.default-provider:}") String defaultProvider) {
+ this.defaultProvider = defaultProvider;
// Only register clients that actually have a valid API key configured
this.clients =
services.stream()
.filter(LlmService::isActive)
.collect(
Collectors.toMap(
- service -> service.getProviderName().toLowerCase(), service -> service));
+ service -> service.getProviderName().toLowerCase(Locale.ROOT),
+ service -> service,
+ (a, b) -> {
+ throw new IllegalStateException(
+ "Duplicate LLM provider name: " + a.getProviderName());
+ },
+ TreeMap::new));
}Then prefer defaultProvider before falling back to the first key at line 34.
Note the toLowerCase() calls at lines 20 and 37 also use the default locale. Use Locale.ROOT to avoid locale-dependent case mapping.
🤖 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/main/java/com/modernizer/orchestrator_service/clients/LlmOrchestrator.java`
around lines 13 - 35, Update LlmOrchestrator to accept and store an explicit
defaultProvider, prefer it when routeChat receives a blank provider, and
otherwise select the first client using a deterministic ordering rather than
HashMap iteration. Normalize provider names with Locale.ROOT in both the
constructor’s clients map key and routeChat lookup.
| @Service | ||
| @ConditionalOnProperty(prefix = "external.openai", name = "api-key") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Spring Boot ConditionalOnProperty behavior when property value is an empty string and havingValue is not set
💡 Result:
When the havingValue attribute is not explicitly set in @ConditionalOnProperty, the annotation defaults to a condition where the property must be present in the Environment and its value must not be equal to the string "false" [1][2]. If the property is set to an empty string (e.g., my.property=), the condition will evaluate to true, because an empty string is not equal to "false" [1][2]. ### Summary of Behavior - Default condition (havingValue not set): Matches if the property exists and its value is not "false" [1][3]. - With empty string value (""): Since "" != "false", the condition matches [1][2]. If you need the condition to exclude empty strings, @ConditionalOnProperty does not provide a built-in attribute to enforce this [4]. To achieve this, you should use @ConditionalOnExpression with SpEL (Spring Expression Language) to explicitly check that the property is not empty [5]. Example using SpEL: @ConditionalOnExpression("!T(org.springframework.util.StringUtils).isEmpty('${my.property:}')") public class MyConfiguration {... } This approach ensures the bean is only created if the property is present and contains non-empty text [5]. Alternatively, for complex scenarios, implementing a custom @Conditional is the recommended path [5][4].
Citations:
- 1: https://docs.spring.io/spring-boot/api/java/org/springframework/boot/autoconfigure/condition/ConditionalOnProperty.html
- 2: https://docs.spring.io/spring-boot/3.4.4/api/java/org/springframework/boot/autoconfigure/condition/ConditionalOnProperty.html
- 3: https://docs.spring.io/spring-boot/docs/2.7.18/api/org/springframework/boot/autoconfigure/condition/ConditionalOnProperty.html
- 4: Allow @ConditionalOnProperty NOT to match if the property value is empty spring-projects/spring-boot#16721
- 5: https://stackoverflow.com/questions/46118782/spel-conditionalonproperty-string-property-empty-or-nulll
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(OpenAiClient|EpamDialClient|GeminiClient|ClaudeClient|CopilotClient|application\.yml)$'
printf '%s\n' '--- annotations and activation logic ---'
rg -n -C 5 '`@ConditionalOnProperty`|isActive\(|external\.(openai|epam|gemini|claude|copilot)|api-key' src/main/java src/main/resources
printf '%s\n' '--- targeted file sizes ---'
for f in $(git ls-files | rg '(^|/)(OpenAiClient|EpamDialClient|GeminiClient|ClaudeClient|CopilotClient|application\.yml)$'); do
printf '%s ' "$f"; wc -l < "$f"
doneRepository: sarojswain680/Phoenix-orchestrator-svc
Length of output: 21658
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- routing and bean-consumer context ---'
rg -n -C 8 'List<LlmService>|Map<.*LlmService|LlmService|isActive|provider' src/main/java | head -n 260
printf '%s\n' '--- build/runtime versions ---'
rg -n -C 3 '<(java|maven.compiler|spring-boot|spring-cloud)[^>]*>|<artifactId>spring-boot-starter' pom.xml build.gradle settings.gradle gradle.properties 2>/dev/null || true
printf '%s\n' '--- exact client declarations ---'
for f in src/main/java/com/modernizer/orchestrator_service/clients/{OpenAiClient,EpamDialClient,GeminiClient,ClaudeClient,CopilotClient}.java; do
[ -f "$f" ] && sed -n '1,35p' "$f"
done
printf '%s\n' '--- deterministic condition probe ---'
python3 - <<'PY'
import re
from pathlib import Path
config = Path("src/main/resources/application.yml").read_text()
props = re.findall(r'^\s{4}([a-z-]+): \$\{([^:}]+):\}', config, re.M)
print("placeholder-backed api-key properties:")
for name, env in props:
if name == "api-key":
print(f" {env} -> empty default; resolved default value = {''!r}")
def conditional_on_property_matches(present, value, having_value=None, match_if_missing=False):
if not present:
return match_if_missing
if having_value is None:
return value.lower() != "false"
return value.lower() == having_value.lower()
for value in ("", " ", "false", "some-key"):
print(f"value={value!r}: default ConditionalOnProperty={conditional_on_property_matches(True, value)}; isActive={bool(value.strip())}")
print("client annotation/property pairs:")
for path in Path("src/main/java/com/modernizer/orchestrator_service/clients").glob("*Client.java"):
text = path.read_text()
m = re.search(r'`@ConditionalOnProperty`\(prefix = "([^"]+)", name = "([^"]+)"\)', text)
if m:
key = f"{m.group(1)}.{m.group(2)}"
print(f" {path.name}: {key}")
PYRepository: sarojswain680/Phoenix-orchestrator-svc
Length of output: 35073
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- LLM orchestration implementation ---'
fd -t f . src/main/java | rg 'Llm|Orchestrat'
for f in $(fd -t f . src/main/java | rg 'Llm|Orchestrat'); do
printf '%s\n' "--- $f ---"
sed -n '1,240p' "$f"
done
printf '%s\n' '--- configuration and dependency files ---'
git ls-files | rg '(^|/)(pom.xml|build.gradle|build.gradle.kts|gradle.properties|application\.(properties|yml|yaml))$' | xargs -r -n1 sh -c 'echo "--- $0 ---"; sed -n "1,180p" "$0"'Repository: sarojswain680/Phoenix-orchestrator-svc
Length of output: 18875
Make the provider condition reject blank API keys.
application.yml assigns each API-key property an empty-string default. Without havingValue, @ConditionalOnProperty matches that value and rejects only "false". The five client beans are therefore created with blank keys, then removed later by LlmOrchestrator.isActive(). Remove these annotations or replace them with a blank-key condition, such as @ConditionalOnExpression("!'${external.openai.api-key:}'.isBlank()"), for each client.
🤖 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/main/java/com/modernizer/orchestrator_service/clients/OpenAiClient.java`
around lines 14 - 15, Update the `@ConditionalOnProperty` annotation on
OpenAiClient and the corresponding four provider client classes to prevent bean
creation when the configured API key is blank, using a blank-aware condition
such as `@ConditionalOnExpression` or removing the annotations consistently.
Preserve bean creation only when the provider API key contains non-whitespace
content.
2 participants
📝 Description
Closes #
🔧 Type of Change
🧩 Affected Pipeline Stage(s)
✅ Author Checklist
./gradlew clean build)${...})🔒 Security Considerations
🧪 How Was This Tested?
Summary by CodeRabbit