Repository navigation
Conversation
This reverts commit c6d13d6.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/main/java/com/raditha/graph/ApacheAgeGraphStore.java`:
- Around line 115-117: The methods persistNode, flushEdges, clearGraph, and
executeQuery currently swallow all exceptions and only log them; change their
error handling so failures are visible to callers by either rethrowing a runtime
exception or returning an explicit failure indicator—preferably rethrow a new
RuntimeException (including a clear message and the caught exception as the
cause) from each catch block in persistNode, flushEdges, clearGraph, and
executeQuery so callers can detect and handle write/query failures; ensure any
callers are updated if needed to handle the propagated RuntimeException.
In `@src/main/java/com/raditha/graph/GraphStoreFactory.java`:
- Around line 81-91: The code currently does an unchecked cast when retrieving
nested configs in createNeo4jStore (and the corresponding AGE branch), which can
throw ClassCastException for malformed scalar values; add a defensive helper
method getSubMap(Map<String,Object> cfg, String key) that checks instanceof Map
and returns the cast map or an empty Map.of() otherwise, then replace the direct
casts in createNeo4jStore (and the AGE factory) to call getSubMap(graphConfig,
"neo4j") / getSubMap(graphConfig, "age") and keep the rest of the logic
(getString(...)) unchanged so malformed configs safely fall back to defaults.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`:
- Around line 568-589: The method extractJsonFromCodeBlocks currently breaks out
when any line ends with '}' or ']', which truncates nested JSON; update this
logic to track brace/bracket depth instead: introduce an integer depth variable,
increment on '{' or '[', decrement on '}' or ']', update depth while appending
lines (when starting to capture via foundJson/inCodeBlock), and only break when
depth <= 0 indicating the outermost structure closed; ensure variables
referenced are depth, inCodeBlock, foundJson, and jsonBuilder in the existing
method.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIServiceTest.java`:
- Around line 122-140: The test testExtractJsonFromCodeBlocks_WithNestedJson
currently only uses contains(...) so it misses truncation; update it to assert
structural validity by parsing the extracted result with a JSON parser (e.g.,
ObjectMapper.readTree) or by comparing against the full expected JSON string,
calling AbstractAIService.extractJsonFromCodeBlocks(response) and then asserting
the parsed tree is non-null or equals the expected JSON structure to ensure the
outer braces aren’t dropped.
🧹 Nitpick comments (9)
src/main/java/sa/com/cloudsolutions/antikythera/examples/AnnotationFinder.java (3)
43-44: Static mutable state reduces testability and reusability.
finalSearchTermandisSimpleModeare mutable static fields, which means this class cannot be safely reused or tested in parallel. Consider passing these as method parameters through the call chain instead of relying on class-level static state.
40-41: Consider using SLF4J for error reporting.The
@SuppressWarnings("java:S106")suppression is reasonable for CLI stdout output, but the error messages onSystem.err(lines 59, 107–112) would benefit from using an SLF4J logger for consistency. As per coding guidelines, "Use SLF4J logger fields with@Slf4jannotation or manual logger initialization in Java classes".
128-133: Minor: redundantendsWithcheck whensearchTermis fully qualified.When
searchTermis already a FQN (e.g.,org.springframework.stereotype.Service), line 131'sendsWith("." + searchTerm)will almost never match anything meaningful since annotation names are unlikely to have a prefix before a full package path. Not a bug, but a no-op branch in that scenario.src/main/java/com/raditha/graph/ApacheAgeGraphStore.java (1)
108-108: Avoid creating a newObjectMapperon every call — reuse a shared instance.
ObjectMapperis thread-safe and expensive to construct. It's instantiated inline inpersistNode,flushEdges(per edge!), andexecuteQuery. Extract it to aprivate static finalfield.♻️ Proposed fix
+ private static final com.fasterxml.jackson.databind.ObjectMapper OBJECT_MAPPER = + new com.fasterxml.jackson.databind.ObjectMapper(); + private static final String BASE_LABEL = "CodeElement";Then replace all three
new com.fasterxml.jackson.databind.ObjectMapper().writeValueAsString(params)calls withOBJECT_MAPPER.writeValueAsString(params).Also applies to: 152-152, 219-219
src/test/java/com/raditha/graph/GraphStoreFactoryTest.java (1)
74-86: Consider using JUnit 5@TempDirto simplify temp file lifecycle.The manual
createTempFile+try/finally/deleteIfExistspattern works but is verbose.@TempDirhandles cleanup automatically and is the idiomatic JUnit 5 approach.♻️ Example with `@TempDir`
+import org.junit.jupiter.api.io.TempDir; +import java.nio.file.Path; + class GraphStoreFactoryTest { + `@TempDir` + Path tempDir; + `@ParameterizedTest`(name = "{0}") `@MethodSource`("neo4jStoreTestCases") void testNeo4jStoreCreation(String testName, String yamlConfig, Class<? extends GraphStore> expectedClass) throws Exception { - Path config = Files.createTempFile("graph-config", ".yml"); - try { - Files.writeString(config, yamlConfig); - try (GraphStore store = GraphStoreFactory.createGraphStore(config.toFile())) { - assertInstanceOf(expectedClass, store); - } - } finally { - Files.deleteIfExists(config); - } + Path config = tempDir.resolve("graph-config.yml"); + Files.writeString(config, yamlConfig); + try (GraphStore store = GraphStoreFactory.createGraphStore(config.toFile())) { + assertInstanceOf(expectedClass, store); + } }src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java (4)
414-425:getConfigIntandgetConfigDoublepropagate uncaughtNumberFormatExceptionon malformed string values.
Integer.parseInt(line 422) andDouble.parseDouble(line 438) throwNumberFormatExceptionif the config string is not a valid number. Since config values may come from YAML files or environment variables, a malformed value would surface as an unhandled runtime exception instead of falling back to the default.♻️ Proposed fix — catch and fall back to default
} else if (value instanceof String str) { - return Integer.parseInt(str); + try { + return Integer.parseInt(str); + } catch (NumberFormatException e) { + logger.warn("Invalid integer config value for key '{}': '{}'", key, str); + return defaultValue; + } }Apply the same pattern to
getConfigDouble.Also applies to: 430-442
606-618: Blind fallback to the first field value may silently misparse non-array responses.Lines 615–618 grab the first field of an unknown object and treat it as the recommendations array. If the LLM returns an unexpected schema (e.g.,
{"error": "rate limited"}), this silently proceeds with a non-array node, which is then caught by the!responseArray.isArray()guard — but only after potentially confusing log output. Consider logging the unexpected structure here for debuggability.
536-563:extractJsonFromResponseusesindexOf/lastIndexOfwhich can mis-pair brackets across disjoint structures.If the response contains multiple separate JSON objects or arrays (e.g., prose with
[see above]and then{"key": "val"}), the first[and last]may not belong to the same structure. This is an inherent limitation of the simple bracket-matching approach. It works for typical single-structure LLM responses, but worth a comment or a TODO for robustness if multi-structure responses are expected.
338-372: LGTM —cloneClassSignatureandcloneNodeListare clean utility methods.Deliberately omitting members from the clone is a sound approach for creating a stub compilation unit. The
@SuppressWarnings("unchecked")annotation is missing on the cast at line 390, but this is a minor nit.Also applies to: 383-393
| } catch (Exception e) { | ||
| logger.error("Failed to persist node: {}", signature, e); | ||
| } |
There was a problem hiding this comment.
Silent exception swallowing in persistNode can hide data-loss bugs.
persistNode catches all exceptions and only logs them. The caller has no indication the node was never written, which can lead to dangling edge references later. The same pattern repeats in flushEdges (Line 161), clearGraph (Line 181), and executeQuery (Line 241). Consider either propagating a runtime exception or returning a success/failure indicator so callers can react.
🤖 Prompt for AI Agents
In `@src/main/java/com/raditha/graph/ApacheAgeGraphStore.java` around lines 115 -
117, The methods persistNode, flushEdges, clearGraph, and executeQuery currently
swallow all exceptions and only log them; change their error handling so
failures are visible to callers by either rethrowing a runtime exception or
returning an explicit failure indicator—preferably rethrow a new
RuntimeException (including a clear message and the caught exception as the
cause) from each catch block in persistNode, flushEdges, clearGraph, and
executeQuery so callers can detect and handle write/query failures; ensure any
callers are updated if needed to handle the propagated RuntimeException.
| @SuppressWarnings("unchecked") | ||
| private static GraphStore createNeo4jStore(Map<String, Object> graphConfig, int batchSize) { | ||
| Map<String, Object> neo4jConfig = (Map<String, Object>) graphConfig.getOrDefault(DEFAULT_TYPE, Map.of()); | ||
|
|
||
| String uri = getString(neo4jConfig, "uri", DEFAULT_NEO4J_URI); | ||
| String username = getString(neo4jConfig, "username", DEFAULT_NEO4J_USERNAME); | ||
| String password = getString(neo4jConfig, "password", ""); | ||
| String database = getString(neo4jConfig, "database", DEFAULT_NEO4J_DATABASE); | ||
|
|
||
| return new Neo4jGraphStore(uri, username, password, database, batchSize); | ||
| } |
There was a problem hiding this comment.
Unchecked cast to Map<String, Object> can throw ClassCastException on malformed config.
Lines 83 and 95 cast the value of "neo4j" / "age" keys to Map<String, Object>. If a user writes a scalar (e.g., neo4j: true) instead of a nested map, this will throw an unhandled ClassCastException at runtime. An instanceof check or a defensive helper would make this more robust.
🛡️ Proposed defensive helper
+ `@SuppressWarnings`("unchecked")
+ private static Map<String, Object> getSubMap(Map<String, Object> config, String key) {
+ Object value = config.get(key);
+ if (value instanceof Map) {
+ return (Map<String, Object>) value;
+ }
+ return Map.of();
+ }Then use getSubMap(graphConfig, "neo4j") and getSubMap(graphConfig, "age") instead of the raw casts.
🤖 Prompt for AI Agents
In `@src/main/java/com/raditha/graph/GraphStoreFactory.java` around lines 81 - 91,
The code currently does an unchecked cast when retrieving nested configs in
createNeo4jStore (and the corresponding AGE branch), which can throw
ClassCastException for malformed scalar values; add a defensive helper method
getSubMap(Map<String,Object> cfg, String key) that checks instanceof Map and
returns the cast map or an empty Map.of() otherwise, then replace the direct
casts in createNeo4jStore (and the AGE factory) to call getSubMap(graphConfig,
"neo4j") / getSubMap(graphConfig, "age") and keep the rest of the logic
(getString(...)) unchanged so malformed configs safely fall back to defaults.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| MavenHelper mavenHelper = new MavenHelper(); | ||
| mavenHelper.readPomFile(); | ||
| mavenHelper.buildJarPaths(); |
There was a problem hiding this comment.
Suggestion: Exceptions thrown by the Maven helper (for example when pom.xml is missing or invalid) are no longer caught, causing the CLI to abort instead of proceeding with the previously supported "limited resolution" mode; this breaks analysis for non-Maven projects or projects with a bad pom file. [logic error]
Severity Level: Major ⚠️
- ❌ KnowledgeGraphCLI fails entirely when pom.xml missing or invalid.
- ⚠️ Limited-resolution analysis mode no longer reachable for bad pom.xml.
- ⚠️ Non-Maven or misconfigured projects cannot generate knowledge graphs.| MavenHelper mavenHelper = new MavenHelper(); | |
| mavenHelper.readPomFile(); | |
| mavenHelper.buildJarPaths(); | |
| try { | |
| MavenHelper mavenHelper = new MavenHelper(); | |
| mavenHelper.readPomFile(); | |
| mavenHelper.buildJarPaths(); | |
| } catch (Exception e) { | |
| logger.warn("Could not load Maven dependencies (pom.xml not found or invalid). Proceeding with limited resolution.", e); | |
| } |
Steps of Reproduction ✅
1. Build and run the CLI entry point `com.raditha.graph.KnowledgeGraphCLI.main` from
`src/main/java/com/raditha/graph/KnowledgeGraphCLI.java:39-47`, providing a valid
`graph.yml` but pointing `--base-path` (or positional project path) to a Java project
directory that does not contain a valid `pom.xml` (e.g., non-Maven project or project with
malformed pom).
2. `main` calls `parseArgs()` at `KnowledgeGraphCLI.java:49-85` to resolve `configPath`
and `basePath`, then invokes `new KnowledgeGraphCLI().run(options.basePath(),
options.configPath())` at line 42.
3. Inside `run(...)` at `KnowledgeGraphCLI.java:96-133`, configuration is loaded and
`AbstractCompiler.reset()` is executed (line 110), after which a `MavenHelper` is
instantiated and `readPomFile()` / `buildJarPaths()` are called at lines 112-114 to
resolve Maven dependencies based on the target project.
4. When `pom.xml` is missing or invalid for the target project,
`MavenHelper.readPomFile()` or `buildJarPaths()` (external class
`sa.com.cloudsolutions.antikythera.parser.MavenHelper`, used similarly in
`src/test/java/com/raditha/graph/KnowledgeGraphIntegrationTest.java:96-99` and declared to
throw `XmlPullParserException`) will throw an exception, which now propagates out of
`run(...)`, is caught only in `main`'s broad `catch (Exception e)` at lines 40-45, causes
the log message "Analysis failed" and `System.exit(1)`, and prevents the CLI from
continuing with the previously supported "limited resolution" mode that was implemented by
the removed try/catch around the MavenHelper block in the old hunk.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/raditha/graph/KnowledgeGraphCLI.java
**Line:** 112:114
**Comment:**
*Logic Error: Exceptions thrown by the Maven helper (for example when pom.xml is missing or invalid) are no longer caught, causing the CLI to abort instead of proceeding with the previously supported "limited resolution" mode; this breaks analysis for non-Maven projects or projects with a bad pom file.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`:
- Around line 417-428: getConfigInt currently calls Integer.parseInt(str)
without handling NumberFormatException; update getConfigInt to catch
NumberFormatException around Integer.parseInt and return defaultValue on parse
failure (preserve existing behavior for Integer instances), and apply the same
pattern in getConfigDouble by wrapping Double.parseDouble(str) in a try-catch
that returns the provided defaultValue on parse errors. Ensure both methods
still handle null config and non-string/non-number types by returning
defaultValue, and reference the existing methods getConfigInt and
getConfigDouble when making the changes.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/AnnotationFinderTest.java`:
- Around line 186-189: The current assertions in AnnotationFinderTest use weak
disjunctions that are trivially true; replace the two assertions that check
signature.contains("items") || signature.contains("List") and
signature.contains("counts") || signature.contains("Map") with stronger checks
that verify the parameter name is paired with its type (e.g., ensure signature
contains both "List" and "items" together and both "Map" and "counts" together)
or, even better, assert equality against the exact expected signature string
produced by the method under test (refer to the local variable signature in the
test to construct the expected value).
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/OpenAIServiceTest.java`:
- Around line 471-490: The test method
testExtractJsonFromResponse_MultipleFormats uses string literals with escaped
"\\n" and escaped quotes, so it doesn't exercise the code-block parsing path in
openAIService.extractJsonFromResponse; change the codeBlockResponse and
plainResponse inputs to use actual newline characters and normal JSON quotes
(e.g., "```json\n[{\"test\": \"value\"}]\n```" and "Here is the result:
[{\"test\": \"value\"}] end") and update the corresponding expected assertions
to match unescaped JSON ("[{\"test\": \"value\"}]"); leave the no-JSON and null
checks as-is.
- Around line 197-206: The parameterized test testExtractJsonFromResponse in
OpenAIServiceTest is using CSV-escaped strings (\\n, \\") which produce literal
backslashes instead of real newlines/quotes; replace the `@CsvSource` with a
`@MethodSource`: add a static provider method (e.g.,
provideExtractJsonFromResponseCases or provideExtractJsonCases) that returns
Stream<Arguments> containing the input strings with actual newlines and real
quotes (e.g., "Here is the JSON response:\n[{\"test\": \"value\"}]\nEnd of
response." etc.) and the expected JSON outputs, then change the test signature
to `@ParameterizedTest` `@MethodSource`("provideExtractJsonFromResponseCases") void
testExtractJsonFromResponse(String input, String expected) to ensure realistic
IA response formats are used.
🧹 Nitpick comments (10)
src/test/java/com/raditha/perf/JavaParserMethodFindingBenchmark.java (2)
33-51: Visitor never stops after finding the target — full traversal every time.The visitor calls
super.visit(md, targetMethodName)unconditionally, so it walks everyMethodDeclarationin the AST even after a match. This negates the main advantage of the visitor pattern (early exit) and makes the benchmark comparison less meaningful. For overloaded methods,foundMethodis also silently overwritten with the last match.Additionally,
reset()(line 48) is dead code — a newMethodFinderVisitoris allocated on every iteration infindMethodUsingVisitor().♻️ Proposed fix: stop after first match and reuse the visitor
private static class MethodFinderVisitor extends VoidVisitorAdapter<String> { private MethodDeclaration foundMethod; `@Override` public void visit(MethodDeclaration md, String targetMethodName) { + if (foundMethod != null) { + return; // stop traversal after first match + } if (md.getNameAsString().equals(targetMethodName)) { foundMethod = md; + return; } super.visit(md, targetMethodName); }And reuse the visitor instance in the benchmark loop:
private MethodDeclaration findMethodUsingVisitor() { - MethodFinderVisitor visitor = new MethodFinderVisitor(); + visitor.reset(); classDeclaration.accept(visitor, TARGET_METHOD_NAME); return visitor.getFoundMethod(); }
90-143: Hand-rolled microbenchmark is susceptible to JIT/GC noise.
System.nanoTime()loops without fork isolation, GC pauses control, or statistical analysis can produce misleading results. Consider using JMH for reliable microbenchmarks — it handles warmup, dead-code elimination, and result statistics out of the box.src/test/java/sa/com/cloudsolutions/antikythera/examples/AnnotationFinderTest.java (1)
384-386: Assertion tests local string concatenation, not the SUT.Lines 385-386 construct a string locally and then assert it equals a hardcoded value — this validates nothing about
AnnotationFinder. Only the assertion on Lines 389-390 (callingbuildMethodSignature) actually exercises the SUT. Consider removing the trivially-true assertion or replacing it with a call to an actualAnnotationFindermethod that produces simple-mode output.src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java (2)
598-640: Index-based correlation between AI response array and query list is fragile.Line 631 assumes a 1:1 positional mapping between
responseArrayelements andbatch.getQueries(). If the AI returns fewer or more elements, or reorders them, recommendations silently attach to the wrong query. The&& i < queries.size()guard prevents an AIOOBE but drops extra recommendations without logging.Consider logging a warning when sizes don't match, so mismatches are visible during debugging.
🔍 Suggested improvement
List<RepositoryQuery> queries = batch.getQueries(); + if (responseArray.size() != queries.size()) { + logger.warn("AI returned {} recommendations but batch has {} queries — correlating by index", + responseArray.size(), queries.size()); + } for (int i = 0; i < responseArray.size() && i < queries.size(); i++) {
86-90:analyzeQueryBatchdoesn't guard againstnullor empty batch.If
batchisnullorbatch.getQueries()is empty, the call chain proceeds into payload building and an actual API call, wasting tokens. A quick guard at the entry point would avoid unnecessary API calls.src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceTest.java (1)
70-72:MockitoAnnotations.openMocks(this)return value is not closed.
openMocksreturns anAutoCloseablethat should be closed after each test to release mock resources. Consider using@ExtendWith(MockitoExtension.class)instead, or storing and closing the handle in@AfterEach.♻️ Preferred approach
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; +@ExtendWith(MockitoExtension.class) class GeminiAIServiceTest { ... `@BeforeEach` void setUp() throws IOException { - MockitoAnnotations.openMocks(this); geminiAIService = new GeminiAIService(); ... }src/test/java/sa/com/cloudsolutions/antikythera/examples/OpenAIServiceTest.java (1)
42-86: Significant duplication withGeminiAIServiceTest.
setUpAll(),setUp(),createTestQueryBatch(), and several test methods (testBuildTableSchemaString,testBuildTableSchemaString_NullTable,testExtractJsonFromResponse_MultipleFormats,testExtractRecommendedColumnOrder_NestedClass) are nearly identical to theirGeminiAIServiceTestcounterparts. Consider extracting shared setup and helpers into a common base test class or utility to reduce maintenance burden.Also applies to: 267-286
src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenAIService.java (1)
34-39: Double-brace initialization creates an anonymousLinkedHashMapsubclass.This is a well-known Java anti-pattern: the
{{ }}initializer creates an anonymous inner class that holds a reference to the enclosing class, slightly inflates the class count, and can confuse serialization. UseMap.ofentries fed into aLinkedHashMapconstructor or a static factory method instead.♻️ Suggested alternative
- private static final Map<String, ModelPricing> MODEL_PRICING = new LinkedHashMap<>() {{ - put("gpt-4o-mini", new ModelPricing(0.150, 0.600, 0.25)); - put("gpt-4-turbo", new ModelPricing(10.00, 30.00, 0.25)); - put(GPT_4_O, new ModelPricing(2.50, 10.00, 0.25)); - put("gpt-4", new ModelPricing(30.00, 60.00, 0.25)); - }}; + private static final Map<String, ModelPricing> MODEL_PRICING; + static { + Map<String, ModelPricing> m = new LinkedHashMap<>(); + m.put("gpt-4o-mini", new ModelPricing(0.150, 0.600, 0.25)); + m.put("gpt-4-turbo", new ModelPricing(10.00, 30.00, 0.25)); + m.put(GPT_4_O, new ModelPricing(2.50, 10.00, 0.25)); + m.put("gpt-4", new ModelPricing(30.00, 60.00, 0.25)); + MODEL_PRICING = Map.copyOf(m); // or Collections.unmodifiableMap(m) to preserve order + }Note:
Map.copyOfdoes not preserve insertion order. UseCollections.unmodifiableMap(m)if iteration order matters (it does for thecontains()lookup).src/main/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIService.java (2)
127-151:sendApiRequestretry/timeout logic is nearly identical toOpenAIService.sendApiRequest.Both implementations share the same pattern: read
initialRetryCount, compare withretryCount, add 30s on retry, build anHttpRequest, and delegate toexecuteHttpRequest. The only differences are the URL construction and auth header. Consider extracting the common timeout-adjustment and retry-count logic into the base class, with subclasses providing only the request-building specifics (URL, headers).♻️ Sketch
In
AbstractAIService:protected abstract HttpRequest buildHttpRequest(String payload, int timeoutSeconds); protected String sendApiRequest(String payload, int retryCount) throws IOException, InterruptedException { int timeoutSeconds = getConfigInt("timeout_seconds", 90); int initialRetryCount = getConfigInt("initial_retry_count", 0); if (retryCount < initialRetryCount) { timeoutSeconds += 30; logger.info("Retrying with extra 30s (total: {}s)", timeoutSeconds); } HttpRequest request = buildHttpRequest(payload, timeoutSeconds); return executeHttpRequest(request, payload, retryCount); }
142-142: API key appended as URL query parameter — standard for Gemini but could leak in access logs or exception stack traces.
?key=in the URL means the API key may appear in HTTP access logs, exception messages, or any toString() of the URI. This is Google's documented approach for Gemini, but be mindful of logging the request URL at debug level.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/test/java/sa/com/cloudsolutions/antikythera/examples/AnnotationFinderTest.java (3)
318-319: Nit: prefer importingjava.util.Listandjava.util.HashSetat the top of the file.These types are used inline with fully-qualified names while other
java.utiltypes (Set,Stream) are already imported.Suggested fix
Add to the import block:
import java.util.HashSet; import java.util.List;Then replace inline usages:
- java.util.List<MethodDeclaration> methods = classDecl.getMethods(); - Set<String> seen = new java.util.HashSet<>(); + List<MethodDeclaration> methods = classDecl.getMethods(); + Set<String> seen = new HashSet<>();(Also applies to Line 354–355.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sa/com/cloudsolutions/antikythera/examples/AnnotationFinderTest.java` around lines 318 - 319, Add imports for java.util.List and java.util.HashSet at the top of the file and replace the fully-qualified inline usages with the simple types: change java.util.List<MethodDeclaration> methods = classDecl.getMethods(); to List<MethodDeclaration> methods = classDecl.getMethods(); and change Set<String> seen = new java.util.HashSet<>(); to Set<String> seen = new HashSet<>(); (also update the other occurrences mentioned around the same area, e.g., the usages at lines flagged 354–355) so the file consistently uses imports for these java.util types.
386-392: Lines 387–388 assert local string concatenation, notAnnotationFinderbehavior.The "simple mode" assertion constructs a string locally and then checks it equals a hardcoded value — this tests Java string concatenation, not any
AnnotationFindermethod. Only the "detailed mode" assertion on Line 391–392 actually exercisesbuildMethodSignature. Consider either removing the trivial assertion or replacing it with a call to an actualAnnotationFinderAPI that produces simple-mode output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sa/com/cloudsolutions/antikythera/examples/AnnotationFinderTest.java` around lines 386 - 392, The test currently asserts a locally constructed string instead of exercising AnnotationFinder; replace the trivial "simpleOutput" concatenation and assertEquals call with a call into AnnotationFinder that produces simple-mode output (e.g., call AnnotationFinder.buildSimpleMethodSignature(method) or the equivalent simple-mode API on AnnotationFinder that returns "com.example.TestClass#testMethod"), keeping the detailed-mode assertion that uses AnnotationFinder.buildMethodSignature(method) intact.
296-368: "Simple mode" tests replicate production logic inline rather than testing anAnnotationFinderAPI.These tests manually iterate methods, check annotations, and build output strings — essentially reimplementing the expected tool behavior in the test itself. If
AnnotationFinderexposes a method for simple-mode output (or will in the future), these tests should call that method directly. Otherwise, the tests are validating their own inline logic rather than the tool's behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sa/com/cloudsolutions/antikythera/examples/AnnotationFinderTest.java` around lines 296 - 368, The tests reimplement simple-mode behavior inline instead of exercising AnnotationFinder; update both tests to call a single AnnotationFinder API that returns the simple-mode outputs (e.g., add or use a method like AnnotationFinder.getSimpleModeOutputs(ClassOrInterfaceDeclaration) or AnnotationFinder.generateSimpleModeOutputs(CompilationUnit/ClassOrInterfaceDeclaration)) and assert against that result; if such a method does not exist, add a small API on AnnotationFinder (name it getSimpleModeOutputs or generateSimpleModeOutputs) that takes the class declaration (or compilation unit) and returns a Set<String> of "ClassName#methodName" for methods with the Test annotation, then replace the per-test forEach/hasAnnotation/string-building logic with calls to that method and assert its size/contents.src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceConfigTest.java (1)
218-228: Add a test for explicitlynullconfig values.The method has tests for missing keys and non-string values, but not for keys that exist with
nullvalues. Whenconfig.put("api_key", null)andgetConfigString("api_key", default)is called, the null value fails theinstanceof Stringcheck, causing a fallthrough to environment variable checks before returning the default. While the outcome is the same as a missing key, the code path differs. A focused test would clarify expected behavior:📝 Suggested test
`@Test` void testGetConfigString_NullValueInConfig() { Map<String, Object> config = new HashMap<>(); config.put("api_key", null); service.setConfig(config); String result = service.getConfigString("api_key", "default-key"); assertEquals("default-key", result, "Should return default when config value is null"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceConfigTest.java` around lines 218 - 228, Add a focused unit test in GeminiAIServiceConfigTest to cover keys present with null values: create a test method (e.g., testGetConfigString_NullValueInConfig) that does Map<String,Object> config = new HashMap<>(); config.put("api_key", null); service.setConfig(config); then call service.getConfigString("api_key", "default-key") and assertEquals("default-key", result). This ensures getConfigString's behavior for a null config entry is verified (similar style and assertions as the existing testGetConfigString_NonStringConfigValue).src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenAIService.java (1)
94-116: Re-readinginitial_retry_countfrom config on every retry call is slightly wasteful but not incorrect.Line 98 calls
getConfigInt("initial_retry_count", 0)on every invocation ofsendApiRequest(payload, retryCount), including recursive retry calls. Since config doesn't change between retries, this is redundant but harmless. Mentioning for awareness only — no change needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenAIService.java` around lines 94 - 116, The code repeatedly calls getConfigInt("initial_retry_count", 0) inside sendApiRequest(String payload, int retryCount), which is redundant across recursive retries; either leave as-is (no change required) or cache the value once and reuse it — e.g., read initial_retry_count earlier (call getConfigInt in the caller or a constructor) and pass it into sendApiRequest or store it in a private field so sendApiRequest uses the cached initialRetryCount rather than calling getConfigInt on every invocation.pom.xml (1)
136-146: Misleading XML comment — PostgreSQL dependency placed under the Neo4j comment.The comment on line 136 reads
<!-- Neo4j Java Driver for Knowledge Graph storage -->but lines 137–141 declare the PostgreSQL JDBC driver. This is confusing for future maintainers. Add a separate comment for the PostgreSQL dependency.Proposed fix
- <!-- Neo4j Java Driver for Knowledge Graph storage --> - <dependency> + <!-- PostgreSQL JDBC Driver for Apache AGE graph storage --> + <dependency> <groupId>org.postgresql</groupId> <artifactId>postgresql</artifactId> <version>42.7.2</version> </dependency> + <!-- Neo4j Java Driver for Knowledge Graph storage --> <dependency> <groupId>org.neo4j.driver</groupId> <artifactId>neo4j-java-driver</artifactId> <version>5.18.0</version> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 136 - 146, Update the misleading XML comment so each dependency has the correct descriptive comment: move or add a comment describing the PostgreSQL JDBC driver above the org.postgresql:postgresql dependency and keep the Neo4j comment above the org.neo4j.driver:neo4j-java-driver dependency; ensure the comment text clearly states "PostgreSQL JDBC driver" for the postgres dependency and "Neo4j Java Driver for Knowledge Graph storage" for the neo4j dependency so maintainers can unambiguously identify org.postgresql:postgresql and org.neo4j.driver:neo4j-java-driver.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 137-141: Update the three Maven dependencies shown by replacing
the artifact versions: bump artifactId "postgresql" from 42.7.2 to 42.7.10;
upgrade "testcontainers-neo4j" to either 2.0.3 (preferred) or at minimum 1.21.4;
and update "system-stubs-jupiter" to 2.1.8 (or if you prefer to keep 2.1.x, add
explicit dependencyManagement or dependency overrides for AssertJ >= 3.27.7 and
Jetty >= 9.4.57 to mitigate the transitive CVEs). Ensure the pom's dependency
entries for artifactId postgresql, testcontainers-neo4j, and
system-stubs-jupiter are updated accordingly and run mvn dependency:tree to
verify the transitive versions were resolved as intended.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`:
- Around line 539-566: The current extractJsonFromResponse method can return
substrings with mismatched brackets because it uses indexOf/lastIndexOf; update
extractJsonFromResponse to (1) prefer extracting JSON via
extractJsonFromCodeBlocks(response) first when present, and otherwise when using
the bracket-based substring logic (arrayStart/arrayEnd or objectStart/objectEnd)
validate the candidate by attempting to parse it with the existing objectMapper
(e.g., call objectMapper.readTree(candidate) inside a try/catch) and only return
the candidate if parsing succeeds; if parsing fails, continue to the next
extraction strategy or return null. Ensure you reference extractJsonFromResponse
and extractJsonFromCodeBlocks and use the objectMapper.readTree validation to
avoid returning malformed JSON.
- Around line 571-604: The extractJsonFromCodeBlocks method can prematurely
break if foundJson is set before any opening brace/bracket is seen; add a
boolean flag (e.g., hasOpenedStructure = false) initialized alongside
braceDepth/bracketDepth, set to true whenever you increment braceDepth or
bracketDepth (when encountering '{' or '['), and change the break condition to
require hasOpenedStructure && braceDepth == 0 && bracketDepth == 0; update
references to foundJson, braceDepth, bracketDepth and the loop in
extractJsonFromCodeBlocks accordingly so the parser only exits after at least
one opening structure was seen and subsequently closed.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/OpenAIServiceTest.java`:
- Around line 72-88: The test uses MockitoAnnotations.openMocks(this) in setUp()
which returns an AutoCloseable that is never closed; replace manual mock init by
annotating the test class (OpenAIServiceTest) with
`@ExtendWith`(MockitoExtension.class) and remove the
MockitoAnnotations.openMocks(this) call from the setUp() method so the
MockitoExtension manages mock lifecycle automatically (no need to call/close
AutoCloseable).
---
Duplicate comments:
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`:
- Around line 58-67: The constructor AbstractAIService currently mutates the
global StaticJavaParser configuration (via new ParserConfiguration() and
StaticJavaParser.setConfiguration(parserConfig)) on every instantiation, causing
a thread-unsafe race; change this to a one-time initialization by moving the
ParserConfiguration/StaticJavaParser.setConfiguration(...) into a static
initializer or dedicated bootstrap method that runs once (e.g., a static block
in AbstractAIService or a static init method called on class load), and leave
the AbstractAIService() constructor to only initialize instance fields
(objectMapper, lastTokenUsage, systemPrompt) so the global parser config is not
overwritten per instance.
- Around line 417-428: getConfigInt and getConfigDouble currently call
Integer.parseInt(str) and Double.parseDouble(str) directly which will throw
NumberFormatException for non-numeric strings; wrap the string-parse logic in a
try-catch that catches NumberFormatException (and optionally
NullPointerException) and returns the provided default on failure, and trim the
string before parsing; update the methods getConfigInt(...) and
getConfigDouble(...) to perform the safe parse-with-catch and return default
when parsing fails.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/AnnotationFinderTest.java`:
- Around line 186-194: Replace the loose contains-based assertions in
AnnotationFinderTest (the assertions using signature.contains(...) for
"process", "items", "List", "counts", "Map" and the startsWith/endsWith checks)
with a single exact equality assertion against the expected signature string;
build the expected string (e.g. "process(List<String> items, Map<String,Integer>
counts)" or whatever the canonical formatting used by other tests is) and call
assertEquals(expected, signature) to ensure exact match, updating the assertion
message accordingly so the test follows the same exact-equality pattern as the
other parameterized cases.
---
Nitpick comments:
In `@pom.xml`:
- Around line 136-146: Update the misleading XML comment so each dependency has
the correct descriptive comment: move or add a comment describing the PostgreSQL
JDBC driver above the org.postgresql:postgresql dependency and keep the Neo4j
comment above the org.neo4j.driver:neo4j-java-driver dependency; ensure the
comment text clearly states "PostgreSQL JDBC driver" for the postgres dependency
and "Neo4j Java Driver for Knowledge Graph storage" for the neo4j dependency so
maintainers can unambiguously identify org.postgresql:postgresql and
org.neo4j.driver:neo4j-java-driver.
In `@src/main/java/sa/com/cloudsolutions/antikythera/examples/OpenAIService.java`:
- Around line 94-116: The code repeatedly calls
getConfigInt("initial_retry_count", 0) inside sendApiRequest(String payload, int
retryCount), which is redundant across recursive retries; either leave as-is (no
change required) or cache the value once and reuse it — e.g., read
initial_retry_count earlier (call getConfigInt in the caller or a constructor)
and pass it into sendApiRequest or store it in a private field so sendApiRequest
uses the cached initialRetryCount rather than calling getConfigInt on every
invocation.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/AnnotationFinderTest.java`:
- Around line 318-319: Add imports for java.util.List and java.util.HashSet at
the top of the file and replace the fully-qualified inline usages with the
simple types: change java.util.List<MethodDeclaration> methods =
classDecl.getMethods(); to List<MethodDeclaration> methods =
classDecl.getMethods(); and change Set<String> seen = new java.util.HashSet<>();
to Set<String> seen = new HashSet<>(); (also update the other occurrences
mentioned around the same area, e.g., the usages at lines flagged 354–355) so
the file consistently uses imports for these java.util types.
- Around line 386-392: The test currently asserts a locally constructed string
instead of exercising AnnotationFinder; replace the trivial "simpleOutput"
concatenation and assertEquals call with a call into AnnotationFinder that
produces simple-mode output (e.g., call
AnnotationFinder.buildSimpleMethodSignature(method) or the equivalent
simple-mode API on AnnotationFinder that returns
"com.example.TestClass#testMethod"), keeping the detailed-mode assertion that
uses AnnotationFinder.buildMethodSignature(method) intact.
- Around line 296-368: The tests reimplement simple-mode behavior inline instead
of exercising AnnotationFinder; update both tests to call a single
AnnotationFinder API that returns the simple-mode outputs (e.g., add or use a
method like AnnotationFinder.getSimpleModeOutputs(ClassOrInterfaceDeclaration)
or
AnnotationFinder.generateSimpleModeOutputs(CompilationUnit/ClassOrInterfaceDeclaration))
and assert against that result; if such a method does not exist, add a small API
on AnnotationFinder (name it getSimpleModeOutputs or generateSimpleModeOutputs)
that takes the class declaration (or compilation unit) and returns a Set<String>
of "ClassName#methodName" for methods with the Test annotation, then replace the
per-test forEach/hasAnnotation/string-building logic with calls to that method
and assert its size/contents.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/GeminiAIServiceConfigTest.java`:
- Around line 218-228: Add a focused unit test in GeminiAIServiceConfigTest to
cover keys present with null values: create a test method (e.g.,
testGetConfigString_NullValueInConfig) that does Map<String,Object> config = new
HashMap<>(); config.put("api_key", null); service.setConfig(config); then call
service.getConfigString("api_key", "default-key") and
assertEquals("default-key", result). This ensures getConfigString's behavior for
a null config entry is verified (similar style and assertions as the existing
testGetConfigString_NonStringConfigValue).
| <dependency> | ||
| <groupId>org.postgresql</groupId> | ||
| <artifactId>postgresql</artifactId> | ||
| <version>42.7.2</version> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PostgreSQL JDBC driver latest version 2025 2026 security
💡 Result:
-
Latest PostgreSQL JDBC (pgjdbc) release (Feb 2026):
42.7.10(listed as the current release; also published to Maven Central on Feb 11, 2026). [1], [2]- Maven coordinate:
org.postgresql:postgresql:42.7.10[2]
- Maven coordinate:
-
Key 2025 security item: CVE-2025-49146 (MITM risk when
channelBinding=requirecould incorrectly allow non-channel-binding auth methods). Fixed in 42.7.7; upgrading to ≥ 42.7.7 mitigates this, so 42.7.10 is safe with respect to this issue. [3], [4]
Recommendation: if you’re on any 42.7.4–42.7.6, upgrade to 42.7.10 (or at minimum 42.7.7+). [3], [4]
Sources: [1] pgJDBC site “Latest Releases”, [2] Maven Repository versions list, [3] PostgreSQL.org security news post, [4] pgJDBC 42.7.7 changelog.
🌐 Web query:
testcontainers neo4j latest stable version
💡 Result:
For Testcontainers for Java (Neo4j module), the latest stable Maven Central release is:
org.testcontainers:testcontainers-neo4j:2.0.3(published Dec 15, 2025) [1]
Note: there’s also the older/legacy artifact org.testcontainers:neo4j, whose latest release is 1.21.4 (published Dec 16, 2025) [2]
Sources: [1], [2]
🌐 Web query:
system-stubs-jupiter latest stable version uk.org.webcompere
💡 Result:
Latest stable version of uk.org.webcompere:system-stubs-jupiter is 2.1.8 (released 2025-04-05). [1], [2], [3]
Maven coordinate
<dependency>
<groupId>uk.org.webcompere</groupId>
<artifactId>system-stubs-jupiter</artifactId>
<version>2.1.8</version>
</dependency>🌐 Web query:
CVE PostgreSQL JDBC driver 42.7.2
💡 Result:
For PostgreSQL JDBC (pgjdbc) v42.7.2:
- CVE fixed by 42.7.2: CVE-2024-1597 (SQL injection possible only when using the non-default connection property
preferQueryMode=simpleand a specific vulnerable SQL pattern). 42.7.2 is the patched release. (github.com) - If you are already on 42.7.2: you are not vulnerable to CVE-2024-1597 (assuming you’re actually running 42.7.2+). (nvd.nist.gov)
- Related later pgjdbc CVE (not 42.7.2-specific): CVE-2025-49146 affects 42.7.4–42.7.6 and is fixed in 42.7.7 (channel binding “required” not enforced correctly). This is not reported as affecting 42.7.2. (postgresql.org)
Citations:
- 1: https://github.com/advisories/GHSA-24rp-q3w6-vc56?utm_source=openai
- 2: https://nvd.nist.gov/vuln/detail/cve-2024-1597?utm_source=openai
- 3: https://www.postgresql.org/about/news/postgresql-jdbc-4277-security-update-for-cve-2025-49146-3088/?utm_source=openai
🌐 Web query:
CVE testcontainers-neo4j 1.20.4
💡 Result:
No CVE (or GitHub Security Advisory) is currently published specifically for the Java artifact org.testcontainers:neo4j (often referred to as “testcontainers-neo4j”) version 1.20.4—it’s mainly a thin wrapper for running the official Neo4j Docker image in tests. [1][2] Snyk also reports no direct vulnerabilities for the core org.testcontainers:testcontainers package (note: this does not cover vulnerabilities in transitive deps or the container image you run). [3]
What you do need to check for CVEs is the Neo4j Docker image tag you use with Neo4jContainer(...), because Neo4j publishes CVEs affecting certain Neo4j versions (e.g., CVE-2026-1622, CVE-2026-1337, etc.). [4]
Sources: [1] org.testcontainers:neo4j 1.20.4 on Maven Central index [2] Testcontainers Neo4j module docs (uses official Neo4j image) [3] Snyk page for org.testcontainers:testcontainers [4] Neo4j security advisories page.
🌐 Web query:
CVE system-stubs-jupiter 2.1.6
💡 Result:
uk.org.webcompere:system-stubs-jupiter:2.1.6 itself does not appear to have a CVE assigned; the CVEs typically reported against it are from its (test) dependency tree (notably AssertJ and Jetty). In Maven Repository, 2.1.6 is flagged due to dependency CVEs including CVE-2026-24400, CVE-2024-9823, CVE-2024-8184, CVE-2024-13009. [1]
Why 2.1.6 gets flagged (key vulnerable deps it brings in):
- AssertJ Core 3.24.2 → CVE-2026-24400 (XXE in XML pretty formatter /
isXmlEqualTo); fixed in AssertJ 3.27.7. [2][3] - Jetty 9.4.52.v20230823 (via Jetty server/servlets artifacts)
- CVE-2024-9823 (Jetty
DosFilterDoS / OOM); patched in Jetty 9.4.54+. [4][3] - CVE-2024-8184 (Jetty
ThreadLimitHandler.getRemote()DoS / OOM); affected up to < 9.4.56 (so 9.4.56+ is outside the affected range). [5] - CVE-2024-13009 (gzip inflate error can lead to incorrect buffer release/data sharing); affected 9.4.0 to 9.4.56, patched in 9.4.57+. [6]
- CVE-2024-9823 (Jetty
What to do
- Prefer upgrading
system-stubs-jupiterto a newer release (latest shown is 2.1.8, dated Apr 05, 2025). [7] - Additionally (or if you must stay on 2.1.6), override dependency versions to patched releases (e.g., assertj-core 3.27.7+ and Jetty 9.4.57+ / or newer supported Jetty line). [2][6][3]
Sources: [1] [2] [3] [4] [5] [6] [7]
Upgrade all three dependencies to latest stable versions; system-stubs-jupiter 2.1.6 introduces indirect CVEs.
- PostgreSQL driver 42.7.2 is outdated; upgrade to 42.7.10 (latest, published Feb 2026). While 42.7.2 itself is not vulnerable to CVE-2024-1597 or CVE-2025-49146, the current version provides 8 patch releases of improvements.
- testcontainers-neo4j 1.20.4 is outdated; upgrade to 2.0.3 (latest modern artifact, Dec 2025) or at minimum 1.21.4 (legacy artifact).
- system-stubs-jupiter 2.1.6 introduces multiple indirect CVEs through transitive dependencies: CVE-2026-24400 in AssertJ 3.24.2 (XXE in XML formatter), CVE-2024-9823 in Jetty 9.4.52 (DoS/OOM), CVE-2024-8184 and CVE-2024-13009 in Jetty (DoS/buffer issues). Upgrade to 2.1.8 (released Apr 2025), or override AssertJ to ≥3.27.7 and Jetty to ≥9.4.57.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pom.xml` around lines 137 - 141, Update the three Maven dependencies shown by
replacing the artifact versions: bump artifactId "postgresql" from 42.7.2 to
42.7.10; upgrade "testcontainers-neo4j" to either 2.0.3 (preferred) or at
minimum 1.21.4; and update "system-stubs-jupiter" to 2.1.8 (or if you prefer to
keep 2.1.x, add explicit dependencyManagement or dependency overrides for
AssertJ >= 3.27.7 and Jetty >= 9.4.57 to mitigate the transitive CVEs). Ensure
the pom's dependency entries for artifactId postgresql, testcontainers-neo4j,
and system-stubs-jupiter are updated accordingly and run mvn dependency:tree to
verify the transitive versions were resolved as intended.
| protected static String extractJsonFromCodeBlocks(String response) { | ||
| // If no JSON found, try to find JSON in code blocks | ||
| String[] lines = response.split("\\n"); | ||
| StringBuilder jsonBuilder = new StringBuilder(); | ||
| boolean inCodeBlock = false; | ||
| boolean foundJson = false; | ||
| int braceDepth = 0; | ||
| int bracketDepth = 0; | ||
|
|
||
| for (String line : lines) { | ||
| if (line.trim().startsWith("```")) { | ||
| inCodeBlock = !inCodeBlock; | ||
| } | ||
| else if (inCodeBlock || line.trim().startsWith("{") || line.trim().startsWith("[") || foundJson) { | ||
| jsonBuilder.append(line).append("\n"); | ||
| foundJson = true; | ||
|
|
||
| // Count braces and brackets to handle nested structures | ||
| for (char c : line.toCharArray()) { | ||
| if (c == '{') braceDepth++; | ||
| else if (c == '}') braceDepth--; | ||
| else if (c == '[') bracketDepth++; | ||
| else if (c == ']') bracketDepth--; | ||
| } | ||
|
|
||
| // Break when we've closed all opened braces/brackets | ||
| if (foundJson && braceDepth == 0 && bracketDepth == 0) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return jsonBuilder.toString().trim(); | ||
| } |
There was a problem hiding this comment.
Depth-tracking approach for extractJsonFromCodeBlocks is a solid improvement — but the early-exit can miss JSON that doesn't start on the first captured line.
The brace/bracket depth tracking fixes the original truncation bug. However, once foundJson is set to true (line 586), the break condition on line 597 fires when both depths are zero. If the first captured line inside a code block contains no braces (e.g., a blank line or comment preceding the JSON), foundJson becomes true while depths remain at 0, causing an immediate break before the actual JSON is reached.
This is a narrow edge case since AI responses typically place JSON immediately after the code fence, but worth being aware of.
Proposed fix — only break after at least one brace/bracket has been opened
+ boolean hasOpenedStructure = false;
+
for (String line : lines) {
if (line.trim().startsWith("```")) {
inCodeBlock = !inCodeBlock;
}
else if (inCodeBlock || line.trim().startsWith("{") || line.trim().startsWith("[") || foundJson) {
jsonBuilder.append(line).append("\n");
foundJson = true;
// Count braces and brackets to handle nested structures
for (char c : line.toCharArray()) {
- if (c == '{') braceDepth++;
+ if (c == '{') { braceDepth++; hasOpenedStructure = true; }
else if (c == '}') braceDepth--;
- else if (c == '[') bracketDepth++;
+ else if (c == '[') { bracketDepth++; hasOpenedStructure = true; }
else if (c == ']') bracketDepth--;
}
// Break when we've closed all opened braces/brackets
- if (foundJson && braceDepth == 0 && bracketDepth == 0) {
+ if (hasOpenedStructure && braceDepth == 0 && bracketDepth == 0) {
break;
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/sa/com/cloudsolutions/antikythera/examples/AbstractAIService.java`
around lines 571 - 604, The extractJsonFromCodeBlocks method can prematurely
break if foundJson is set before any opening brace/bracket is seen; add a
boolean flag (e.g., hasOpenedStructure = false) initialized alongside
braceDepth/bracketDepth, set to true whenever you increment braceDepth or
bracketDepth (when encountering '{' or '['), and change the break condition to
require hasOpenedStructure && braceDepth == 0 && bracketDepth == 0; update
references to foundJson, braceDepth, bracketDepth and the loop in
extractJsonFromCodeBlocks accordingly so the parser only exits after at least
one opening structure was seen and subsequently closed.
| @BeforeEach | ||
| void setUp() throws IOException { | ||
| MockitoAnnotations.openMocks(this); | ||
|
|
||
| openAIService = new OpenAIService(); | ||
|
|
||
| // Create a test configuration | ||
| config = new HashMap<>(); | ||
| config.put("api_key", "test-api-key"); | ||
| config.put("timeout_seconds", 30); | ||
| config.put("track_usage", true); | ||
| config.put("cost_per_1k_tokens", 0.001); | ||
| config.put("initial_retry_count", 1); | ||
|
|
||
| // Configure the service | ||
| openAIService.configure(config); | ||
| } |
There was a problem hiding this comment.
MockitoAnnotations.openMocks(this) return value is not closed — resource leak.
openMocks returns an AutoCloseable that must be closed after tests to release mock resources. The simplest fix is to switch to @ExtendWith(MockitoExtension.class) on the class, which handles the lifecycle automatically.
Proposed fix — use MockitoExtension
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
class OpenAIServiceTest {Then remove MockitoAnnotations.openMocks(this) from setUp():
`@BeforeEach`
void setUp() throws IOException {
- MockitoAnnotations.openMocks(this);
-
openAIService = new OpenAIService();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/sa/com/cloudsolutions/antikythera/examples/OpenAIServiceTest.java`
around lines 72 - 88, The test uses MockitoAnnotations.openMocks(this) in
setUp() which returns an AutoCloseable that is never closed; replace manual mock
init by annotating the test class (OpenAIServiceTest) with
`@ExtendWith`(MockitoExtension.class) and remove the
MockitoAnnotations.openMocks(this) call from the setUp() method so the
MockitoExtension manages mock lifecycle automatically (no need to call/close
AutoCloseable).
|
User description
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests
Removed
CodeAnt-AI Description
Emit richer structural and behavioral relationships in the knowledge graph and unify AI service behavior
What Changed
Impact
✅ Clearer type and call relationships in generated graphs✅ Support for Apache AGE and Neo4j backends at runtime✅ Fewer AI API timeouts due to unified retry/timeout handling💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.