Migrate Copilot Libraries API & Add Keyword Search#697
Conversation
Extend SQLite schema and implement migration
…gration Refactor migration libraries API
Add initial copilot extension model
…arch Add keyword-based library search and refactor copilot package structure
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Copilot library subsystem: DB-backed library search and loading, symbol-to-model conversion, instruction/service augmentation, exclusion handling, numerous POJO models/builders/adapters, module exports, tests, resources, and search-index schema/producer updates; plus minor .gitignore and Gradle changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Language Server Client
participant LSExt as CopilotLibraryService
participant LibMgr as CopilotLibraryManager
participant DB as LibraryDatabaseAccessor
participant SymProc as SymbolProcessor
participant InstLoader as InstructionLoader
participant Converter as LibraryModelConverter
Client->>LSExt: getLibrariesBySearch(keywords)
LSExt->>LibMgr: getLibrariesBySearch(keywords)
LibMgr->>DB: searchLibrariesByKeywords(keywords)
DB-->>LibMgr: Map(package -> description)
loop per package
LibMgr->>LibMgr: construct ModuleInfo & load semantic model
LibMgr->>SymProc: processModuleSymbols(semanticModel,...)
SymProc-->>LibMgr: clients, functions, typedefs
LibMgr->>Converter: convert symbol data -> Library models
Converter-->>LibMgr: Library object
LibMgr->>InstLoader: loadLibraryInstruction(package)
InstLoader-->>LibMgr: Optional(instruction)
LibMgr->>LibMgr: apply exclusions & augment instructions
end
LibMgr-->>LSExt: List<Library>
LSExt->>LSExt: ModelToJsonConverter.librariesToJson(...)
LSExt-->>Client: JsonArray
sequenceDiagram
participant LibMgr as CopilotLibraryManager
participant SvcLdr as ServiceLoader
participant InstLoader as InstructionLoader
participant DB as LibraryDatabaseAccessor
LibMgr->>SvcLdr: loadAllServices(libraryName)
SvcLdr-->>LibMgr: inbuilt + generic services (JsonArray)
LibMgr->>InstLoader: loadServiceInstruction(package/service)
InstLoader-->>LibMgr: Optional(instruction)
LibMgr->>DB: getPackageDescription(org, pkg)
DB-->>LibMgr: Optional(description)
LibMgr->>LibMgr: assemble final Library model
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
VellummyilumVinoth
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/PackageUtil.java (1)
77-86:⚠️ Potential issue | 🟠 MajorRemove the hardcoded
ballerina.homepath.This absolute, user-specific path will break on any other machine or CI. Prefer resolving from env/property (or fail fast with a clear error) and keep the distribution version aligned with
Ballerina.tomlto avoid mismatches.✅ Suggested fix (env/property based fallback)
- if (ballerinaHome == null || ballerinaHome.isEmpty()) { -// Path currentPath = getPath(Paths.get( -// PackageUtil.class.getProtectionDomain().getCodeSource().getLocation().getPath())); -// Path distributionPath = getParentPath(getParentPath(getParentPath(currentPath))); - System.setProperty(BALLERINA_HOME_PROPERTY, - "/Users/vinoth/.ballerina/ballerina-home/distributions/ballerina-2201.13.1"); - } + if (ballerinaHome == null || ballerinaHome.isEmpty()) { + String envHome = System.getenv("BALLERINA_HOME"); + if (envHome == null || envHome.isEmpty()) { + throw new IllegalStateException( + "ballerina.home is not set. Provide -Dballerina.home or BALLERINA_HOME"); + } + System.setProperty(BALLERINA_HOME_PROPERTY, envHome); + }flow-model-generator/modules/flow-model-index-generator/src/main/java/io/ballerina/indexgenerator/SearchDatabaseManager.java (1)
42-42:⚠️ Potential issue | 🟡 MinorLogger uses wrong class name.
The logger is initialized with
DatabaseManager.class.getName()instead ofSearchDatabaseManager.class.getName(). This will cause log entries to appear under the wrong class name, making debugging harder.📝 Proposed fix
- private static final Logger LOGGER = Logger.getLogger(DatabaseManager.class.getName()); + private static final Logger LOGGER = Logger.getLogger(SearchDatabaseManager.class.getName());flow-model-generator/modules/flow-model-generator-ls-extension/src/main/java/io/ballerina/flowmodelgenerator/extension/CopilotLibraryService.java (1)
39-44:⚠️ Potential issue | 🟡 MinorUpdate JavaDoc (it no longer does “streaming JSON processing”).
The class description still advertises streaming JSON, but implementation is now DB-backed via
CopilotLibraryManager, which can mislead future maintainers.Proposed doc tweak
/** * Service for managing Copilot library operations. - * Provides streaming JSON processing for efficient memory usage. + * Provides database-backed loading, filtering, and keyword search for Copilot libraries. * * `@since` 1.0.1 */
🤖 Fix all issues with AI agents
In
`@flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/adapters/StringPathAdapter.java`:
- Around line 35-48: The read method in StringPathAdapter currently calls
JsonReader.nextString() unconditionally, which fails when the JSON token is
null; update StringPathAdapter.read(JsonReader in) to check the incoming token
(e.g., in.peek() == JsonToken.NULL) and if null call in.nextNull() and return
null, otherwise call in.nextString() and return new StringPath(value) so read()
mirrors write()'s null handling.
In
`@flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/builder/TypeLinkBuilder.java`:
- Around line 88-127: The code in TypeLinkBuilder constructs recordNames =
recordName != null ? recordName.split("\\|") : new String[]{null}, which leads
to a NullPointerException when singleRecordName.trim() is called; change the
fallback to an empty array or guard the trim: initialize recordNames as new
String[0] when recordName is null (or in the for-loop skip nulls with if
(singleRecordName == null || singleRecordName.trim().isEmpty()) continue;) so
that TypeLink creation only runs for non-null, non-empty trimmed names.
In
`@flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java`:
- Around line 306-321: getDatabasePath currently copies the resource to a new
temp directory on every call which leaks files and wastes IO; change it to
create the temp file only once and cache the resulting JDBC URL in a static
field (e.g., a private static volatile String cachedJdbcUrl) and return that on
subsequent calls, ensure the initial creation is thread-safe (synchronize or use
double-checked locking) when resolving INDEX_FILE_NAME, mark the temp file for
deletion on JVM exit (tempFile.toFile().deleteOnExit()) and avoid recreating
tempDir/tempFile on repeated getDatabasePath invocations.
- Around line 261-297: The FTS5 query tokens built in formatForPackageFTS and
formatForNameDescriptionFTS are vulnerable to FTS syntax injection because raw
keywords are concatenated; create a helper (e.g., escapeAndQuoteFTSKeyword) that
trims the input, escapes FTS-significant characters (quotes, colons, AND/OR/NOT
tokens, and other operators) by backslash-escaping or safe-encoding, then wraps
the result in quotes, and use that helper when building each token in both
formatForPackageFTS and formatForNameDescriptionFTS so every token is safely
quoted and escaped before joining with " OR ".
In
`@flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/service/ServiceLoader.java`:
- Around line 72-138: The loadFromInbuiltTriggers method (and similarly
loadFromGenericServices) currently calls JsonParser.parseReader(),
getAsJsonObject(), and getAsString() without guarding against Gson runtime
exceptions; update ServiceLoader.loadFromInbuiltTriggers to catch
JsonSyntaxException, IllegalStateException, and UnsupportedOperationException
around parsing and JSON element access (parseReader/getAsJsonObject/getAsString)
and on any such exception return the empty JsonArray to honor the documented
contract; likewise, in loadFromGenericServices add null checks before calling
getAsString() on required fields and wrap parsing/accessors in the same
exception handling so malformed or type-mismatched JSON yields an empty result
instead of propagating exceptions.
In
`@flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/util/LibraryModelConverter.java`:
- Around line 89-127: The resource path parameter parsing in
LibraryModelConverter uses paramContent.split(":") which breaks on
module-qualified types and it also ignores the accessor returned by
parseResourcePath(); update the parsing to use paramContent.split(":", 2) to
limit to at most one split and preserve type strings, and when you call
parseResourcePath() use the returned accessor (pathParts[0]) to set the function
accessor via function.setAccessor(...) if non-empty instead of unconditionally
using functionData.name(); apply the same split(":", 2) fix to the other
resource-path parsing blocks in this class that currently call
paramContent.split(":").
- Around line 246-257: The code in LibraryModelConverter incorrectly uses
typeDefData.baseType() (which holds the constant's value from
TypeDefDataBuilder.buildFromConstant()) both as the value and as the varType
name; change the mapping so setValue(typeDefData.baseType()) remains but varType
is derived from the constant's actual type descriptor (use
constantSymbol.typeDescriptor() to obtain the type name) and set that on the
Type created for typeDef.setVarType(...); alternatively, extend TypeDefData to
carry both value and type and use that distinct type field when constructing the
Type for setVarType.
- Around line 169-179: parameterDataToModel is missing null-safety checks for
currentOrg and currentPackage before calling
extractTypeLinksFromSymbol/createLinksFromImports; add the same guards used in
fieldDataToModel and returnTypeToModel so you only call
extractTypeLinksFromSymbol(returnTypeData.typeSymbol(), currentOrg,
currentPackage) when currentOrg != null and currentPackage != null, then run
TypeLinkBuilder.filterInternalExternal on the result and set nameToUse via
extractRecordName when links are non-empty; this prevents null propagation into
createLinksFromImports and preserves correct internal/external classification.
In
`@flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/util/SymbolProcessor.java`:
- Around line 119-188: processClassSymbol is missing setting TypeDef.type for
non-client classes and may duplicate init methods: ensure you set the
TypeDef.type (e.g., from classData parentSymbolType or
classSymbol.typeDescriptor()/className) when creating TypeDef, and guard against
emitting an init twice by filtering out init/constructor methods from
classMethods (or skip any FunctionData whose functionResultKind ==
FunctionData.Kind.CLASS_INIT) before converting with functionDataToModel; update
the non-client branch where TypeDef is built and the loop over classMethods (and
mention initMethodToModel/functionDataToModel and buildChildNodes) accordingly.
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/main/java/io/ballerina/flowmodelgenerator/extension/CopilotLibraryService.java`:
- Around line 61-74: getLibrariesList is performing blocking DB I/O inside
CompletableFuture.supplyAsync without an explicit executor; replace the default
ForkJoinPool usage by introducing and using a dedicated ExecutorService (shared
across endpoints) for these blocking operations and call supplyAsync(...,
executor) in getLibrariesList (and the other endpoints), ensure the
ExecutorService has proper lifecycle management (create once, shutdown on
service stop), and if CopilotLibraryManager is thread-safe prefer reusing a
single instance (e.g., a shared field) instead of instantiating a new
CopilotLibraryManager per request to reduce DB load and contention with
LibraryDatabaseAccessor calls.
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/exclusion.json`:
- Around line 78-91: The exclusion.json mistakenly lists Slack functions under
the Google Sheets client: remove the two Slack-specific entries
"getConversationMembers" and "getConversationList" from the "clients" ->
"Client" functions array for the "ballerinax/googleapis.sheets" service so only
real Sheets functions (e.g., "getAllSpreadsheets") remain; locate the
"ballerinax/googleapis.sheets" block and delete those two function name entries
to restore correct exclusions.
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/ai/service.md`:
- Around line 8-12: The code block contains a stray token "qq" that breaks the
snippet; remove the "qq" line so the imports and listener declaration (imports
ballerina/ai, ballerina/http and listener ai:Listener chatListener = new
(listenOn = check http:getDefaultListener());) form a valid example; after
removing "qq", verify the snippet compiles and is copy/paste-ready.
In
`@model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/FunctionDataBuilder.java`:
- Around line 480-482: The linking logic currently always binds a symbol when
calling populateReturnTypeLinks(functionData, functionTypeSymbol,
returnData.returnType()), which can produce incorrect links if getReturnData()
has overridden the return type string; update the code paths (calls at
populateReturnTypeLinks invocation points and the populateReturnTypeLinks method
itself) to first compare the symbol signature
(functionTypeSymbol.typeSymbol().signature()) against
returnData.returnType().typeName (or equivalent return type string from
ReturnTypeData) and only attach the symbol to ReturnTypeData when they match; if
they differ, leave the ReturnTypeData without a symbol so LibraryModelConverter
will not generate a link for the overridden name. Ensure the same check is
applied where populateReturnTypeLinks is invoked (including the other two noted
call sites) and inside the method implementation to be safe.
🟡 Minor comments (17)
flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/copilot_library/get_libraries_list.json-2-4 (1)
2-4:⚠️ Potential issue | 🟡 MinorAvoid an empty fixture that makes the test vacuous.
Line 3 now expects zero libraries; if the test only asserts equality with this file, it no longer validates the listing behavior. Consider keeping at least one minimal stub library (or asserting non‑empty seeded DB results) so the test still exercises the listing path.
flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/ai/service.md-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorFix spelling/grammar in the instructions.
These typos reduce clarity and professionalism.
✍️ Proposed edits
-This library contains all the features relavant to AI functionalities which can be used to build AI powered applications. +This library contains all the features relevant to AI functionalities which can be used to build AI-powered applications. -- Use this Listener the ai:Listener only if a new service needs to be created. +- Use the ai:Listener only if a new service needs to be created. -- Agent must need a model provider. Use default model provider unless a specific model is requested. +- An agent must have a model provider. Use the default model provider unless a specific model is requested. -- These can be used directly to simply chat with an LLM for simple queries or can be used with agents or RAG abstractions build complex applications +- These can be used directly to chat with an LLM for simple queries, or with agents or RAG abstractions to build complex applications. -- Note its ideal to do ingestion and querying in seperately. +- Note it’s ideal to do ingestion and querying separately.Also applies to: 5-5, 30-30, 77-77, 109-109
flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/test/library.md-55-55 (1)
55-55:⚠️ Potential issue | 🟡 MinorFix function name typos.
The function names "beforeSuit" and "afterSuit" appear to be typos. They should be "beforeSuite" and "afterSuite" to match the annotation names and common naming conventions.
📝 Proposed fix
`@test`:BeforeSuite -function beforeSuit() { +function beforeSuite() { // initialize or execute pre-requisites } `@test`:AfterSuite -function afterSuit() { +function afterSuite() { // tear-down }Also applies to: 60-60
flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/test/library.md-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorFix spelling error.
The word "unneccessary" should be spelled "unnecessary" (one 'c', two 's').
📝 Proposed fix
-- Utilize type narrowing to avoid unneccessary type casting and assert only the necessary fields. +- Utilize type narrowing to avoid unnecessary type casting and assert only the necessary fields.flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/ai/library.md-119-123 (1)
119-123:⚠️ Potential issue | 🟡 MinorComplete the querying example with variable definitions.
The querying example references
userQueryandmodelProviderwithout defining them, making the code snippet incomplete and potentially confusing for readers trying to follow along.📝 Suggested fix
### Querying Retrieve relevant information and augment user queries: ```ballerina +string userQuery = "What is the capital of France?"; ai:QueryMatch[] matches = check knowledgeBase.retrieve(userQuery); ai:ChatUserMessage augmentedMsg = ai:augmentUserQuery(matches, userQuery); ai:ChatAssistantMessage response = check modelProvider->chat(augmentedMsg, []);Note: This assumes `modelProvider` was defined earlier in the same scope, or you could add: ```ballerina final ai:Wso2ModelProvider modelProvider = check ai:getDefaultModelProvider(); string userQuery = "What is the capital of France?"; ai:QueryMatch[] matches = check knowledgeBase.retrieve(userQuery); // ... rest of the codeflow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/graphql/service.md-3-5 (1)
3-5:⚠️ Potential issue | 🟡 MinorFix spelling and grammar errors.
These instructions are sent to the LLM, so correctness matters for optimal results.
- Line 3: "attatched" → "attached"
- Line 4: "explictily" → "explicitly"
- Line 5: "Figure out are the Query" → "Figure out the Query"
📝 Proposed fix
-- GraphQL Service always requires a graphql listener to be attatched to it. -- Service requires base path to be set. Specify /graphql if not specified explictily by the user query. -- Figure out are the Query and Mutation operations required based on the user query. +- GraphQL Service always requires a graphql listener to be attached to it. +- Service requires base path to be set. Specify /graphql if not specified explicitly by the user query. +- Figure out the Query and Mutation operations required based on the user query.flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/http/service.md-3-55 (1)
3-55:⚠️ Potential issue | 🟡 MinorFix spelling and add language identifiers to fenced code blocks.
Static analysis flagged spelling errors and missing fence languages, which reduces doc quality and lint compliance.
📝 Suggested edits
-- HTTP Service always requires a http listener to be attatched to it. Always declare the listener in the module level as variable and then use it in the service declaration. (eg; listener http:Listener ep = check new http:Listener(8080);) +- HTTP Service always requires a http listener to be attached to it. Always declare the listener in the module level as variable and then use it in the service declaration. (eg; listener http:Listener ep = check new http:Listener(8080);) @@ -- Path paramters must be specified in the resource function path. (eg: resource function get v1/user/[int userId]/profile()) +- Path parameters must be specified in the resource function path. (eg: resource function get v1/user/[int userId]/profile()) @@ -``` +```ballerina @@ -``` +```ballerinaflow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/GetLibrariesListFromSearchIndex.java-79-100 (1)
79-100:⚠️ Potential issue | 🟡 MinorDescription validation is incomplete - actual content is not compared.
The current logic only checks whether both descriptions are empty or both are non-empty, but doesn't verify that the actual description content matches when both are non-empty. This could miss cases where the description exists but has incorrect content.
🔧 Proposed fix to add content comparison
// Check if the actual description matches the expected description boolean isActualEmpty = actualDescription == null || actualDescription.trim().isEmpty(); boolean isExpectedEmpty = expectedDescription == null || expectedDescription.trim().isEmpty(); if (isActualEmpty != isExpectedEmpty) { if (isExpectedEmpty) { log.info("Expected empty description but got non-empty description for library at index " + i + ": '" + actualLibrary + "' - actual: '" + actualDescription + "'"); } else { log.info("Expected non-empty description but got empty description for library at index " + i + ": '" + actualLibrary + "'"); } assertFailure = true; break; + } else if (!isActualEmpty && !actualDescription.equals(expectedDescription)) { + log.info("Description mismatch at index " + i + ": expected '" + expectedDescription + + "', got '" + actualDescription + "'"); + assertFailure = true; + break; }flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java-131-207 (1)
131-207:⚠️ Potential issue | 🟡 MinorAlign result-limit documentation with actual behavior.
Javadoc says “Limited to 20 results” but the SQL usesLIMIT 9. Please update either the limit or the doc to avoid confusion.📝 Suggested doc fix (if 9 is intended)
- * Returns a map with package name (org/package_name) as key and description as value, - * ordered by relevance score (best matches first). Limited to 20 results. + * Returns a map with package name (org/package_name) as key and description as value, + * ordered by relevance score (best matches first). Limited to 9 results.flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/http/test.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorMinor grammatical error in the heading statement.
The sentence should use imperative form for instruction documentation.
📝 Proposed fix
-Generated test code following these strict guidelines: +Generate test code following these strict guidelines:flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/CopilotInstructionAugmentationTest.java-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorCopyright year inconsistency.
This file uses copyright year 2025, while other new files in this PR use 2026. Consider updating for consistency.
📝 Proposed fix
-* Copyright (c) 2025, WSO2 LLC. (http://www.wso2.com) +* Copyright (c) 2026, WSO2 LLC. (http://www.wso2.com)flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/generic-services.json-44-47 (1)
44-47:⚠️ Potential issue | 🟡 MinorInconsistent description for
ballerina/ailistener parameter.The description mentions "HTTP service listener" but this is the
ballerina/ailibrary. This appears to be a copy-paste error from the HTTP service definition.📝 Proposed fix
{ "name": "listenOn", - "description": "Listening port of the HTTP service listener", + "description": "Listening port of the AI service listener", "type": {flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/GetFilteredLibrariesFromSemanticModel.java-283-306 (1)
283-306:⚠️ Potential issue | 🟡 Minor
testExternalLinksshould assert the library identity (not just “non-empty”).Since you request only
"ballerina/http", it’d be safer to assertlibraries.size() == 1and/or the"name"is"ballerina/http"before counting links—otherwise a future API change that returns extra libraries could make this test validate the wrong entry.flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/service/ServiceLoader.java-296-305 (1)
296-305:⚠️ Potential issue | 🟡 MinorQualifier detection: don’t rely on the first qualifier only.
If the qualifiers array contains multiple entries,
"resource"may not always be at index 0. Safer: scan the array and treat it as resource if any element equals"resource".Possible fix
String methodType = "remote"; if (functionData.has("qualifiers")) { JsonArray qualifiers = functionData.getAsJsonArray("qualifiers"); if (qualifiers != null && !qualifiers.isEmpty()) { - String qualifier = qualifiers.get(0).getAsString(); - methodType = qualifier.equals("resource") ? "resource" : "remote"; + boolean isResource = false; + for (JsonElement q : qualifiers) { + if ("resource".equals(q.getAsString())) { + isResource = true; + break; + } + } + methodType = isResource ? "resource" : "remote"; } }flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/util/SymbolProcessor.java-242-270 (1)
242-270:⚠️ Potential issue | 🟡 MinorDon’t set an empty
varTypefor constants.If
varTypeNamecan’t be resolved (stays empty), settingnew Type("")will likely generate confusing output; consider only settingvarTypewhen non-blank.Guard against empty type
Type varType = new Type(varTypeName); - typeDef.setVarType(varType); + if (varTypeName != null && !varTypeName.isBlank()) { + typeDef.setVarType(varType); + }flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/GetFilteredLibrariesFromSemanticModel.java-54-57 (1)
54-57:⚠️ Potential issue | 🟡 MinorClose the config reader (avoid leaking file handles in test runs).
Files.newBufferedReader(...)should be wrapped in try-with-resources.Proposed change
public void test(Path config) throws IOException { Path configJsonPath = configDir.resolve(config); - TestConfig testConfig = gson.fromJson(Files.newBufferedReader(configJsonPath), TestConfig.class); + try (var reader = Files.newBufferedReader(configJsonPath)) { + TestConfig testConfig = gson.fromJson(reader, TestConfig.class); + }flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/builder/TypeDefDataBuilder.java-73-76 (1)
73-76:⚠️ Potential issue | 🟡 MinorRemove unused type map—it's never read after population.
The
typeMapvariable is populated byFunctionDataBuilder.allMembers()at lines 74-75 but never referenced afterward in the method. This is dead work and should be removed unless the map result was intended to be used.♻️ Suggested cleanup
- // Use FunctionDataBuilder.allMembers to extract all type information - Map<String, TypeSymbol> typeMap = new LinkedHashMap<>(); - FunctionDataBuilder.allMembers(typeMap, typeDescriptor); -
🧹 Nitpick comments (21)
flow-model-generator/modules/flow-model-generator-core/src/main/java/module-info.java (1)
59-65: Confirm these new exports are intended as public API.This widens the module’s public surface. If some packages are internal or only needed by specific modules, consider using qualified exports (
exports ... to ...) or keeping them unexported to preserve encapsulation.flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/model/Listener.java (1)
33-51: Preserve non-null invariant and avoid list aliasing.
setParameterscan acceptnull, which defeats the constructor’s non-null default and can lead to NPEs. It also aliases the caller’s list, so external mutations can leak into this model. Consider null-guarding and defensively copying.Suggested update
public void setParameters(List<Parameter> parameters) { - this.parameters = parameters; + this.parameters = (parameters == null) + ? new ArrayList<>() + : new ArrayList<>(parameters); }model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/FieldData.java (1)
28-40: Consider using Java records for immutable data carriers.This class is a simple immutable data holder with final fields and accessor methods that follow record-style naming. If the project targets Java 16+, this could be simplified to a Java record, which would auto-generate the constructor, accessors,
equals(),hashCode(), andtoString().Additionally, there's no defensive handling if
nullis passed fornameortype. If these are required fields, consider adding validation or documenting nullability expectations.♻️ Optional: Convert to Java record (if Java 16+)
public record FieldData(String name, String description, FieldType type, boolean optional) { public record FieldType(String name, TypeSymbol typeSymbol) { public FieldType(String name) { this(name, null); } } }model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/TypeDefData.java (1)
36-42: Wrap or copy thefieldslist to preserve immutability.The
fieldslist is assigned directly in the constructor and returned as-is from the accessor. This allows external callers to mutate the internal state, breaking the intended immutability of this data class.♻️ Proposed fix to ensure immutability
+import java.util.Collections; import java.util.List; // In constructor: public TypeDefData(String name, String description, TypeCategory type, List<FieldData> fields, String baseType) { this.name = name; this.description = description; this.type = type; - this.fields = fields; + this.fields = fields != null ? List.copyOf(fields) : List.of(); this.baseType = baseType; }Alternatively, if you need to preserve
nullsemantics:- this.fields = fields; + this.fields = fields != null ? Collections.unmodifiableList(new ArrayList<>(fields)) : null;Also applies to: 57-59
model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/PackageUtil.java (1)
213-219: Remove commented-out helper methods.These dead blocks make the file noisier and invite confusion. Delete them entirely, or reintroduce them as active helpers if still needed.
♻️ Cleanup suggestion
-// private static Path getPath(Path path) { -// return Objects.requireNonNull(path, "Path cannot be null"); -// } -// -// private static Path getParentPath(Path path) { -// return Objects.requireNonNull(path, "Path cannot be null").getParent(); -// }flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/test/library.md (1)
7-7: Consider adding language specifiers to code blocks.The three code blocks on lines 7, 26, and 53 are missing language specifiers. Adding
ballerinaas the language identifier will enable syntax highlighting and improve readability.♻️ Proposed improvement
-``` +```ballerina import ballerina/test;(Apply similar changes to the code blocks starting at lines 26 and 53)
Also applies to: 26-26, 53-53
flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/ai/library.md (2)
34-36: Consider adding context for thePersontype.The example shows generating structured output with a custom
Person[]type, but this type is not defined anywhere in the documentation. While the intent is clear, adding a brief note or a simple type definition would help readers understand how to define their own types for structured output.📝 Suggested enhancement
Add a brief explanation before or after the example:
// Structured output with custom types +// Note: Define record types matching your desired output structure Person[] persons = check modelProvider->generate(`Generate 10 person example records`);
106-106: Explain the "AUTO" parameter.The
VectorKnowledgeBaseconstructor uses"AUTO"as a parameter without explanation. Readers would benefit from understanding what this parameter controls and what other valid values exist.📝 Suggested enhancement
Add a comment explaining the parameter:
-final ai:VectorKnowledgeBase knowledgeBase = new (vectorStore, embeddingProvider, "AUTO"); +// "AUTO" enables automatic chunking strategy; other options: "FIXED", "SEMANTIC" +final ai:VectorKnowledgeBase knowledgeBase = new (vectorStore, embeddingProvider, "AUTO");model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/FunctionData.java (1)
47-77: Document nullability ofreturnTypeDatato prevent misuse.
returnTypeData()can return null when not populated. Consider documenting that explicitly (or switching toOptional) to reduce accidental NPEs.💡 Minimal doc-only tweak
+ /** + * `@return` return type metadata, or null when unavailable + */ public ReturnTypeData returnTypeData() { return returnTypeData; }Also applies to: 140-142
flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/util/TypeSymbolExtractor.java (1)
59-59: Consider importingArrayListfor consistency.Using fully qualified
java.util.ArrayListinline (lines 59 and 95) while otherjava.utiltypes are imported is inconsistent. Consider addingArrayListto the imports.♻️ Suggested import addition
Add to imports:
import java.util.ArrayList;Then update line 59:
- List<String> memberTypes = new java.util.ArrayList<>(); + List<String> memberTypes = new ArrayList<>();And line 95:
- List<TypeLink> allLinks = new java.util.ArrayList<>(); + List<TypeLink> allLinks = new ArrayList<>();flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/model/LibraryFunction.java (1)
41-43: Inconsistent field initialization:pathsnot initialized whileparametersis.The constructor initializes
parametersto an emptyArrayList, butpaths(also aList) remainsnull. This inconsistency could causeNullPointerExceptionifgetPaths()is called beforesetPaths().♻️ Proposed fix for consistent initialization
public LibraryFunction() { + this.paths = new ArrayList<>(); this.parameters = new ArrayList<>(); }flow-model-generator/modules/flow-model-generator-ls-extension/src/main/java/io/ballerina/flowmodelgenerator/extension/CopilotLibraryService.java (1)
65-66: Normalize/filter request inputs (mode,libNames,keywords) before querying.
modedefaults only when null (not when blank), andkeywords/libNamesaren’t filtered for null/blank entries. This can lead to odd queries (e.g., empty mode) or unexpected search behavior.Possible input normalization
+import java.util.Arrays; import java.util.List; import java.util.concurrent.CompletableFuture; @@ public CompletableFuture<GetAllLibrariesResponse> getLibrariesList(GetAllLibrariesRequest request) { return CompletableFuture.supplyAsync(() -> { try { - String mode = request.mode() != null ? request.mode() : MODE_ALL; + String mode = (request.mode() == null || request.mode().isBlank()) + ? MODE_ALL + : request.mode().trim(); CopilotLibraryManager manager = new CopilotLibraryManager(); List<Library> libraries = manager.loadLibrariesFromDatabase(mode); @@ public CompletableFuture<GetAllLibrariesResponse> getFilteredLibraries( GetSelectedLibrariesRequest request) { return CompletableFuture.supplyAsync(() -> { try { - if (request.libNames() == null || request.libNames().length == 0) { + String[] libNames = request.libNames() == null ? new String[0] + : Arrays.stream(request.libNames()) + .filter(s -> s != null && !s.isBlank()) + .map(String::trim) + .toArray(String[]::new); + if (libNames.length == 0) { return createResponse(new JsonArray()); } CopilotLibraryManager manager = new CopilotLibraryManager(); - List<Library> libraries = manager.loadFilteredLibraries(request.libNames()); + List<Library> libraries = manager.loadFilteredLibraries(libNames); @@ public CompletableFuture<GetAllLibrariesResponse> getLibrariesBySearch( GetLibrariesBySearchRequest request) { return CompletableFuture.supplyAsync(() -> { try { - if (request.keywords() == null || request.keywords().length == 0) { + String[] keywords = request.keywords() == null ? new String[0] + : Arrays.stream(request.keywords()) + .filter(s -> s != null && !s.isBlank()) + .map(String::trim) + .toArray(String[]::new); + if (keywords.length == 0) { return createResponse(new JsonArray()); } CopilotLibraryManager manager = new CopilotLibraryManager(); - List<Library> libraries = manager.getLibrariesBySearch(request.keywords()); + List<Library> libraries = manager.getLibrariesBySearch(keywords);Also applies to: 81-87, 99-105
flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/model/TypeDef.java (1)
29-37: Consider defaulting list fields to empty lists (vs null) to reduce null-branching.Right now
fields,members, andfunctionscan be null, which forces every downstream consumer to guard. If the JSON contract allows empty arrays, initializing these to empty lists can simplify conversion/serialization paths.Also applies to: 81-103
flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/CopilotLibraryExclusionTest.java (1)
92-107: Makeclientslist mutable in test fixtures to avoidUnsupportedOperationException.
lib.setClients(List.of(client))creates an unmodifiable list. If exclusion logic ever needs to remove clients (e.g., empty clients after filtering), this test will fail for the wrong reason.Safer fixture construction
private List<Library> buildLibWithClient(String libName, String clientName, String[] funcNames) { List<Library> libraries = new ArrayList<>(); Library lib = new Library(libName, ""); Client client = new Client(clientName, ""); @@ client.setFunctions(functions); - lib.setClients(List.of(client)); + lib.setClients(new ArrayList<>(List.of(client))); lib.setFunctions(new ArrayList<>()); libraries.add(lib); return libraries; }flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/GetFilteredLibrariesFromSemanticModel.java (2)
74-91: Avoid order-dependent assertions in config-driven test.The loop compares expected vs actual libraries by index, which can fail if the response order changes while the content is correct. Prefer matching by
name(e.g., build a map from actual array keyed by library name).
115-252: Make link verification helpers return “true = valid” (or rename to reflect current semantics).Right now
verifyLinkStructure(...)returnstrueon failure, andverifyLinksInFunctions(...)returnstrueon failure, whileverifyLinksInTypeDefs(...)returnsfalseon failure. This is workable, but it’s very easy to get wrong when extending the checks.One way to simplify: rename to “hasInvalid…”
- private boolean verifyLinksInFunctions(JsonArray functions) { + private boolean hasInvalidLinksInFunctions(JsonArray functions) { @@ - if (verifyLinkStructure(typeObj.getAsJsonArray("links"))) { + if (hasInvalidLinkStructure(typeObj.getAsJsonArray("links"))) { return true; } @@ - if (verifyLinkStructure(typeObj.getAsJsonArray("links"))) { + if (hasInvalidLinkStructure(typeObj.getAsJsonArray("links"))) { return true; } @@ - private boolean verifyLinkStructure(JsonArray links) { + private boolean hasInvalidLinkStructure(JsonArray links) {flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/service/ServiceLoader.java (2)
167-175: Be defensive when reading required fields fromgeneric-services.json.
service.get("type").getAsString()andservice.get("instructions").getAsString()will throw if the field is missing or null. Even if the resource is controlled, guarding makes failures easier to diagnose (and avoids crashing unrelated flows).
233-245: Optional: make listener parameter order deterministic.Iterating
properties.keySet()can yield non-deterministic ordering (depending on how the JSON is produced). If downstream consumers/tests expect stable output, consider sorting keys before emitting parameters.flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/util/LibraryModelConverter.java (1)
140-194: Optional: omitreturnwhen there’s no return type info.
function.setReturnInfo(returnInfo)is called even whenreturnInfo.typeisn’t set, which can serialize as an emptyreturnobject depending onModelToJsonConverter. If consumers treat presence ofreturnas meaningful, consider only setting it when a type is available.flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/model/Library.java (1)
37-40: Consider a no-arg constructor and/or empty-list defaults if you expect framework deserialization.If any part of the system ever needs to deserialize
Library(e.g., Gson/Jackson), the lack of a default constructor and nullable list fields can become friction. If this is strictly created programmatically, feel free to ignore.Also applies to: 66-96
flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/CopilotLibraryManager.java (1)
278-289: Cache per-library instructions outside the service loop.
InstructionLoader.loadTestInstructionandloadServiceInstructionare invoked for every service; loading them once per library reduces redundant IO/lookup.♻️ Suggested refactor
private void augmentServicesWithInstructions(List<Service> services, String libraryName) { + var testInstruction = InstructionLoader.loadTestInstruction(libraryName); + var serviceInstruction = InstructionLoader.loadServiceInstruction(libraryName); for (Service service : services) { // Add test generation instruction to all services - InstructionLoader.loadTestInstruction(libraryName) - .ifPresent(service::setTestGenerationInstruction); + testInstruction.ifPresent(service::setTestGenerationInstruction); // Add service instruction only to generic services if (TYPE_GENERIC.equals(service.getType())) { - InstructionLoader.loadServiceInstruction(libraryName) - .ifPresent(service::setInstructions); + serviceInstruction.ifPresent(service::setInstructions); } } }
...e/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/adapters/StringPathAdapter.java
Show resolved
Hide resolved
...core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/builder/TypeLinkBuilder.java
Show resolved
Hide resolved
...main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java
Show resolved
Hide resolved
...main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java
Outdated
Show resolved
Hide resolved
...r-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/service/ServiceLoader.java
Show resolved
Hide resolved
...or-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/util/SymbolProcessor.java
Show resolved
Hide resolved
...extension/src/main/java/io/ballerina/flowmodelgenerator/extension/CopilotLibraryService.java
Show resolved
Hide resolved
...enerator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/exclusion.json
Show resolved
Hide resolved
...model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/ai/service.md
Show resolved
Hide resolved
...generator-commons/src/main/java/io/ballerina/modelgenerator/commons/FunctionDataBuilder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
flow-model-generator/modules/flow-model-index-generator/src/main/java/io/ballerina/indexgenerator/SearchListGenerator.java (1)
58-65:⚠️ Potential issue | 🟡 MinorPotential
UnsupportedOperationExceptionrisk withMap.of().
Map.of()returns an unmodifiable map, and while the map's values (the lists) come fromgetPackageList()which usesCollectors.toList(), the contract ofCollectors.toList()does not guarantee mutability. CallingremoveIf()on these lists may fail on some JDK implementations.Consider using a mutable map and explicitly mutable lists for safety.
🛡️ Proposed fix for explicit mutability
- Map<String, List<PackageMetadataInfo>> packagesMap = - Map.of("ballerina", ballerinaPackages, "ballerinax", ballerinaxPackages, "xlibb", xlibbPackages); + Map<String, List<PackageMetadataInfo>> packagesMap = new HashMap<>(); + packagesMap.put("ballerina", new ArrayList<>(ballerinaPackages)); + packagesMap.put("ballerinax", new ArrayList<>(ballerinaxPackages)); + packagesMap.put("xlibb", new ArrayList<>(xlibbPackages));And add the imports:
import java.util.ArrayList; import java.util.HashMap;flow-model-generator/modules/flow-model-index-generator/src/main/java/io/ballerina/indexgenerator/SearchIndexGenerator.java (1)
94-95:⚠️ Potential issue | 🟡 MinorComment does not match the actual timeout value.
The comment says "5-minute timeout" but the code uses
3, TimeUnit.MINUTES.📝 Proposed fix
- // Apply 5-minute timeout - future.get(3, TimeUnit.MINUTES); + // Apply 3-minute timeout + future.get(3, TimeUnit.MINUTES);flow-model-generator/modules/flow-model-index-generator/src/main/java/io/ballerina/indexgenerator/SearchDatabaseManager.java (1)
42-42:⚠️ Potential issue | 🟡 MinorLogger uses wrong class name.
The logger is initialized with
DatabaseManager.class.getName()instead ofSearchDatabaseManager.class.getName(). This will cause log messages to be attributed to the wrong class.📝 Proposed fix
- private static final Logger LOGGER = Logger.getLogger(DatabaseManager.class.getName()); + private static final Logger LOGGER = Logger.getLogger(SearchDatabaseManager.class.getName());
🤖 Fix all issues with AI agents
In
`@flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/builder/TypeLinkBuilder.java`:
- Around line 87-123: The code in TypeLinkBuilder constructs recordNames from
recordName without guarding for null/blank, causing singleRecordName.trim() to
NPE; before splitting or iterating use a guard that returns/continues when
recordName is null or blank (e.g., if (recordName == null ||
recordName.trim().isEmpty()) return/continue), or trim first and then split into
recordNames; ensure any downstream use of recordNames (and variables like
trimmedName) only runs when recordName produced non-empty tokens; keep the
existing logic for importStatements, isPredefinedLangLib,
currentOrg/currentPackage and the per-type loop intact.
In
`@flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java`:
- Around line 306-321: getDatabasePath currently creates a new temp directory
and copies INDEX_FILE_NAME on every call, leaking disk space; modify
getDatabasePath to cache the constructed JDBC path in a static field (e.g., a
private static volatile String cachedJdbcUrl) and return it on subsequent calls
to avoid repeated copies, perform the copy only if the cached path is null (use
synchronization or double-checked locking to be thread-safe), and ensure the
created tempFile/tempDir are scheduled for cleanup by calling deleteOnExit on
files and registering a JVM shutdown hook to recursively delete the temp
directory (or otherwise track and remove temp copies) so long‑running processes
don't accumulate temp files; reference getDatabasePath, INDEX_FILE_NAME,
tempDir, and tempFile when making these changes.
- Around line 261-298: formatForPackageFTS and formatForNameDescriptionFTS
currently inject raw keywords into FTS5 MATCH clauses; add a helper (e.g.,
escapeFTS5Term(String term)) that doubles any embedded double-quote characters
(replace " with "") and then wraps the resulting term in double-quotes to force
literal matching, and call this helper when building tokens (replace
keyword.trim() with escapeFTS5Term(keyword.trim())); keep the existing column
filters ("{package_name description keywords}:" and "{name description}:") and
OR-joining logic unchanged.
In
`@flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/service/ServiceLoader.java`:
- Around line 81-135: The JSON parsing call in ServiceLoader (the
try-with-resources block using ServiceLoader.class.getResourceAsStream and
JsonParser.parseReader) currently only catches IOException, so
JsonSyntaxException and JsonIOException can propagate; update the catch to also
handle com.google.gson.JsonSyntaxException and com.google.gson.JsonIOException
(or catch Exception from Gson parsing) so malformed JSON or Gson I/O errors are
handled consistently and return the empty services list; no logic changes to
buildListenerFromTriggerData or transformServiceMethod are needed—just extend
the catch clause/imports around that parsing block.
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/exclusion.json`:
- Around line 86-91: Remove the two Slack-specific exclusion entries
"getConversationMembers" and "getConversationList" from the exclusions list so
they are not present for the googleapis.sheets client; ensure only the
legitimate "getAllSpreadsheets" exclusion remains in the exclusion list for the
sheets connector (edit the JSON entries that currently list
getConversationMembers and getConversationList and delete those objects).
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/http/test.md`:
- Line 25: The template examples for resource invocations omit the required
check keyword; update the POST/PUT/DELETE/PATCH template patterns (the lines
that read like "Resource invocation -> {Type} {variableName} =
clientEp->/books.post({payload});" and the analogous .put/.delete/.patch lines)
to prepend check before the client call (i.e., use "check clientEp->...") so the
generated examples match the stated guidance and propagate errors correctly;
ensure you modify the templates used for POST, PUT, DELETE and PATCH
invocations.
In
`@model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/PackageUtil.java`:
- Around line 79-86: The code in PackageUtil currently hardcodes a user-specific
ballerina.home via System.setProperty when ballerinaHome (from
System.getProperty(BALLERINA_HOME_PROPERTY)) is empty; remove the hardcoded path
and implement a portable fallback: first check the system property
(System.getProperty(BALLERINA_HOME_PROPERTY)), then environment variable
(System.getenv("BALLERINA_HOME")), then try to compute the distribution path
(reuse the commented logic that derives a distribution from
PackageUtil.class.getProtectionDomain().getCodeSource().getLocation().getPath()),
and if none are available, throw an informative exception (fail fast) rather
than setting a user-specific path; reference the variables/methods
ballerinaHome, BALLERINA_HOME_PROPERTY, System.getProperty, System.getenv, and
the commented getPath/getParentPath logic to locate where to change the
behavior.
🟡 Minor comments (26)
model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/TypeDefData.java-85-91 (1)
85-91:⚠️ Potential issue | 🟡 MinorGuard against null in
fromString.
equalsIgnoreCasewill throw ifvalueis null, which can leak NPEs to callers.🔧 Proposed fix
public static TypeCategory fromString(String value) { + if (value == null) { + return OTHER; + } for (TypeCategory category : TypeCategory.values()) { if (category.value.equalsIgnoreCase(value)) { return category; } } return OTHER; }.gitignore-49-49 (1)
49-49:⚠️ Potential issue | 🟡 MinorClarify the relevance and categorization of
.claude.The addition of
.claudeto the ignore list raises a few questions:
Relevance to PR: This change appears unrelated to the PR's objectives (Copilot Libraries API migration and keyword search). Is this a necessary project-wide ignore rule, or was it added incidentally?
Incorrect categorization:
.claudeis placed under "macOS metadata files," but it's not a standard macOS system artifact like.DS_Store. If it's related to Claude AI tooling or development artifacts, it should be categorized appropriately (e.g., under "IDE specific files" or a new "Development tools" section).Lack of context: The PR description doesn't explain why this ignore rule is needed.
Consider either:
- Removing this entry if it's a personal development artifact that doesn't affect the project
- Moving it to a more appropriate section with a descriptive comment if it's a legitimate project-wide ignore rule
flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/test/library.md-7-15 (1)
7-15:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks.
The Markdown linter flags these blocks for missing language tags, which also reduces readability in rendered docs.
✍️ Suggested fix
-``` +```ballerina import ballerina/test; // Test function `@test`:Config {} function testFunction() { test:assertTrue(true, msg = "Failed!"); }@@
-+ballerina
import ballerina/io;
import ballerina/test;
@@@@ -``` +```ballerina `@test`:BeforeSuite function beforeSuit() { // initialize or execute pre-requisites } @@Also applies to: 26-50, 53-63
flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/test/library.md-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorFix spelling: “unneccessary”.
Typo in user-facing documentation.
✍️ Suggested fix
-- Utilize type narrowing to avoid unneccessary type casting and assert only the necessary fields. +- Utilize type narrowing to avoid unnecessary type casting and assert only the necessary fields.flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/test/library.md-55-60 (1)
55-60:⚠️ Potential issue | 🟡 MinorExample function names likely misspelled (“Suit” vs “Suite”).
The example suggests
beforeSuit/afterSuit, which can confuse users who copy-paste.✍️ Suggested fix
-@test:BeforeSuite -function beforeSuit() { +@test:BeforeSuite +function beforeSuite() { // initialize or execute pre-requisites } @@ -@test:AfterSuite -function afterSuit() { +@test:AfterSuite +function afterSuite() { // tear-down }flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/adapters/StringPathAdapter.java-44-48 (1)
44-48:⚠️ Potential issue | 🟡 MinorMissing null handling in
readmethod causes asymmetry withwrite.The
writemethod correctly handles null values, butreaddoes not. When the JSON containsnull,in.nextString()will throw anIllegalStateExceptionbecause it expects a STRING token.🐛 Proposed fix to handle null values
+import com.google.gson.stream.JsonToken;`@Override` public StringPath read(JsonReader in) throws IOException { + if (in.peek() == JsonToken.NULL) { + in.nextNull(); + return null; + } String value = in.nextString(); return new StringPath(value); }flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/README.md-24-24 (1)
24-24:⚠️ Potential issue | 🟡 MinorFix subject-verb agreement.
The phrase "fixed services which has a json" contains a grammatical error. Since "services" is plural, the verb should be "have" rather than "has".
📝 Grammar fix
-Instructions specific to writing services using the library. For fixed services which has a json, its automatically covered but for the generic triggers, you have to give instructions here. +Instructions specific to writing services using the library. For fixed services which have a JSON definition, it's automatically covered, but for generic triggers, you must provide instructions here.flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/README.md-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorCorrect the grammar: "information" is uncountable.
The phrase "all these information" is grammatically incorrect. Since "information" is an uncountable noun, it should be "all this information".
📝 Grammar fix
-- Keep in mind that all these information will be sent to the LLM if the library was selected for the usecase. +- Keep in mind that all this information will be sent to the LLM if the library is selected for the use case.flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/README.md-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorClarify the "BI Copilot" abbreviation.
The abbreviation "BI Copilot" is unclear and may confuse contributors. Consider using the full term (e.g., "Ballerina Integration Copilot" or simply "Copilot") for better clarity.
📝 Suggested clarification
-This directory contains custom instructions to enhance the performance specific library usages of BI Copilot. +This directory contains custom instructions to enhance the performance of specific library usages in Copilot.flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/http/test.md-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorAdd missing comma in import list.
The list of imports should have proper comma separation.
📝 Proposed fix
-- Start with necessary imports, including `ballerina/http`, `ballerina/test` any other imports that is required. +- Start with necessary imports, including `ballerina/http`, `ballerina/test`, and any other imports that are required.flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/http/test.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorFix grammar: use imperative mood for instructions.
The sentence uses past tense ("Generated") instead of imperative mood. Instructions should be commands.
📝 Proposed fix
-Generated test code following these strict guidelines: +Generate test code following these strict guidelines:flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java-131-139 (1)
131-139:⚠️ Potential issue | 🟡 MinorDoc mismatch: limit is 9, not 20.
Lines 136–139 claim “Limited to 20 results,” but the SQL uses
LIMIT 9(Line 207). Please align the Javadoc with the actual limit or update the query limit.Also applies to: 206-208
flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java-70-78 (1)
70-78:⚠️ Potential issue | 🟡 MinorFilter out NULL orgs to avoid “null/package” keys.
Line 70 only filters
package_name IS NOT NULL. Iforgis NULL, the map key becomesnull/<package>. Suggest addingAND org IS NOT NULLor guarding inpopulatePackageMap.✅ Minimal SQL tweak
- sqlBuilder.append("SELECT DISTINCT org, package_name, description FROM Package WHERE package_name IS NOT NULL"); + sqlBuilder.append("SELECT DISTINCT org, package_name, description FROM Package WHERE package_name IS NOT NULL AND org IS NOT NULL");flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/service/ServiceLoader.java-296-304 (1)
296-304:⚠️ Potential issue | 🟡 MinorDetermine
resourcevsremotefrom any qualifier, not just the first.Qualifier order isn’t guaranteed; scanning the array prevents misclassification.
💡 Suggested change
- if (functionData.has("qualifiers")) { - JsonArray qualifiers = functionData.getAsJsonArray("qualifiers"); - if (qualifiers != null && !qualifiers.isEmpty()) { - String qualifier = qualifiers.get(0).getAsString(); - methodType = qualifier.equals("resource") ? "resource" : "remote"; - } - } + if (functionData.has("qualifiers")) { + JsonArray qualifiers = functionData.getAsJsonArray("qualifiers"); + if (qualifiers != null) { + for (JsonElement q : qualifiers) { + if ("resource".equals(q.getAsString())) { + methodType = "resource"; + break; + } + } + } + }flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/service/ServiceLoader.java-167-175 (1)
167-175:⚠️ Potential issue | 🟡 MinorAdd defensive checks for consistency.
While the current generic-services.json always provides
"type"and"instructions"fields, the code should defensively check for their presence before callinggetAsString(). This maintains consistency with the existinghas()check for"listener"(lines 177-179) and improves robustness against malformed or future JSON entries.💡 Suggested change
- JsonObject serviceObj = new JsonObject(); - serviceObj.addProperty("type", service.get("type").getAsString()); - serviceObj.addProperty("instructions", service.get("instructions").getAsString()); + if (!service.has("type") || service.get("type").isJsonNull()) { + continue; + } + JsonObject serviceObj = new JsonObject(); + serviceObj.addProperty("type", service.get("type").getAsString()); + if (service.has("instructions") && !service.get("instructions").isJsonNull()) { + serviceObj.addProperty("instructions", service.get("instructions").getAsString()); + } else { + serviceObj.addProperty("instructions", ""); + }flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/service/ServiceLoader.java-278-281 (1)
278-281:⚠️ Potential issue | 🟡 MinorAdd type check before calling
getAsString()on placeholder field.Gson's
getAsString()throwsUnsupportedOperationExceptionif the element is not aJsonPrimitive. Guard withisJsonPrimitive()before conversion:Suggested change
- if (property.has("placeholder") && !property.get("placeholder").isJsonNull()) { - paramObj.addProperty("default", property.get("placeholder").getAsString()); - } + if (property.has("placeholder") + && !property.get("placeholder").isJsonNull() + && property.get("placeholder").isJsonPrimitive()) { + paramObj.addProperty("default", property.get("placeholder").getAsString()); + }flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/http/service.md-3-9 (1)
3-9:⚠️ Potential issue | 🟡 MinorFix spelling and add language tags to code fences.
Addresses static-analysis warnings and improves doc quality.
✏️ Suggested fixes
-- HTTP Service always requires a http listener to be attatched to it. Always declare the listener in the module level as variable and then use it in the service declaration. (eg; listener http:Listener ep = check new http:Listener(8080);) +- HTTP Service always requires an http listener to be attached to it. Always declare the listener at the module level as a variable and then use it in the service declaration. (eg; listener http:Listener ep = check new http:Listener(8080);) ... -- Path paramters must be specified in the resource function path. (eg: resource function get v1/user/[int userId]/profile()) +- Path parameters must be specified in the resource function path. (eg: resource function get v1/user/[int userId]/profile()) ... -``` +```ballerina import ballerina/http; ... -``` +``` ... -``` +```ballerina import ballerina/http; ... -``` +```Also applies to: 11-12, 53-54
flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/generic-services.json-38-49 (1)
38-49:⚠️ Potential issue | 🟡 MinorFix the AI listener description (currently references HTTP).
This looks like copy/paste and may mislead users.
✏️ Suggested fix
- "description": "Listening port of the HTTP service listener", + "description": "Listening port of the AI service listener",flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/CopilotLibraryFilterTest.java-56-58 (1)
56-58:⚠️ Potential issue | 🟡 MinorUpdate the stale comment to remove “mode”.
The constructor no longer takes a mode, so the comment is misleading.
✏️ Suggested fix
- // Create request with library names to filter and mode + // Create request with library names to filterflow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/ai/service.md-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorFix spelling and grammar issues.
Static analysis flagged spelling and grammar issues in the document header.
📝 Suggested fix
# Overall library instructions -This library contains all the features relavant to AI functionalities which can be used to build AI powered applications. +This library contains all the features relevant to AI functionalities which can be used to build AI-powered applications.flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/ai/service.md-8-22 (1)
8-22:⚠️ Potential issue | 🟡 MinorFix stray characters and add language specifier to code block.
Line 11 contains stray
🔧 Suggested fix
-``` +```ballerina import ballerina/ai; import ballerina/http; -qq + listener ai:Listener chatListener = new (listenOn = check http:getDefaultListener());flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/CopilotLibraryManager.java-93-151 (1)
93-151:⚠️ Potential issue | 🟡 MinorAdd null/empty guards for
libraryNamesand entries.
IflibraryNamesis null or contains null/empty entries, the loop will throw and fail the request. A small guard prevents avoidable 500s.🔧 Suggested fix
public List<Library> loadFilteredLibraries(String[] libraryNames) { List<Library> libraries = new ArrayList<>(); + if (libraryNames == null || libraryNames.length == 0) { + return libraries; + } for (String libraryName : libraryNames) { + if (libraryName == null || libraryName.isEmpty()) { + continue; + } // Parse library name "org/package_name" String[] parts = libraryName.split("/"); if (parts.length != 2) { continue; // Skip invalid format }flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/CopilotLibraryManager.java-239-251 (1)
239-251:⚠️ Potential issue | 🟡 MinorDon’t silently drop exclusion loading failures.
Returningnullon IO errors makes exclusions silently disappear. Consider returning an empty list for “resource missing” but failing fast (or logging) on read/parse errors.🔧 Suggested fix
private List<ExclusionEntry> loadExclusions() { try (InputStream inputStream = CopilotLibraryManager.class.getResourceAsStream(EXCLUSION_JSON_PATH)) { if (inputStream == null) { - return null; + return List.of(); } try (InputStreamReader reader = new InputStreamReader(inputStream, StandardCharsets.UTF_8)) { Type listType = new TypeToken<List<ExclusionEntry>>() { }.getType(); return GSON.fromJson(reader, listType); } } catch (IOException e) { - return null; + throw new RuntimeException("Failed to load exclusions from " + EXCLUSION_JSON_PATH, e); } }flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/CopilotLibraryManager.java-159-174 (1)
159-174:⚠️ Potential issue | 🟡 MinorShort‑circuit on empty
keywordsto avoid unnecessary DB calls.
This avoids a DB hit (and potential NPEs) when the request contains no search terms.🔧 Suggested fix
public List<Library> getLibrariesBySearch(String[] keywords) { List<Library> libraries = new ArrayList<>(); + if (keywords == null || keywords.length == 0) { + return libraries; + } try { Map<String, String> packageToDescriptionMap = LibraryDatabaseAccessor.searchLibrariesByKeywords(keywords);flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/graphql/service.md-1-13 (1)
1-13:⚠️ Potential issue | 🟡 MinorFix typos and grammar for clarity.
✍️ Suggested edits
-- GraphQL Service always requires a graphql listener to be attatched to it. -- Service requires base path to be set. Specify /graphql if not specified explictily by the user query. -- Figure out are the Query and Mutation operations required based on the user query. +- A GraphQL service always requires a GraphQL listener to be attached to it. +- Service requires a base path to be set. Specify /graphql if not specified explicitly by the user query. +- Determine whether Query and Mutation operations are required based on the user query.flow-model-generator/modules/flow-model-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/builder/TypeDefDataBuilder.java-73-75 (1)
73-75:⚠️ Potential issue | 🟡 MinorRemove unused
typeMapvariable.The
typeMapvariable is created and populated viaFunctionDataBuilder.allMembers()on lines 73-75 but is never referenced afterward. The method uses type-specific extraction methods instead (e.g.,extractRecordFields,extractEnumMembers,extractUnionMembers), which do not depend on the populated map.♻️ Proposed fix
- // Use FunctionDataBuilder.allMembers to extract all type information - Map<String, TypeSymbol> typeMap = new LinkedHashMap<>(); - FunctionDataBuilder.allMembers(typeMap, typeDescriptor); - // Determine type category
...core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/builder/TypeLinkBuilder.java
Show resolved
Hide resolved
...main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java
Show resolved
Hide resolved
...main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java
Outdated
Show resolved
Hide resolved
...r-core/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/service/ServiceLoader.java
Show resolved
Hide resolved
...enerator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/exclusion.json
Show resolved
Hide resolved
...-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/http/test.md
Show resolved
Hide resolved
model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/PackageUtil.java
Show resolved
Hide resolved
model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/PackageUtil.java
Outdated
Show resolved
Hide resolved
model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/PackageUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/graphql/service.md`:
- Around line 3-4: Update the two user-facing instruction strings to fix
spelling typos: change "GraphQL Service always requires a graphql listener to be
attatched to it." to use "attached" and change "Service requires base path to be
set. Specify /graphql if not specified explictily by the user query." to use
"explicitly"; ensure the corrected lines (the exact strings above) are updated
in the file so the user-facing docs show "attached" and "explicitly".
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/http/test.md`:
- Around line 20-22: The three example lines containing Ballerina resource
signatures and invocations (e.g., "resource function get books/[string isbn]()",
"clientEp->/books/[{isbn}]()", and "Book book = check
clientEp->/books/[12345678]()") are being parsed as empty Markdown links; fix by
formatting each example as inline code (wrap each line in backticks) or place
them in a fenced code block so the square-bracket URI syntax is not interpreted
as links and MD042 lint errors are eliminated.
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/README.md`:
- Line 11: Update the README sentence "Execute ./gradlew clean pack -x check -x
test and you should see jar inside build/ folder." to include the missing
article "a" so it reads "you should see a jar inside the build/ folder" (edit
the README.md entry for the build step).
- Line 34: Replace the incorrect phrase "all these information" in the README
text with the grammatically correct "all this information"; locate the exact
string "all these information" in the README.md content and update it to "all
this information" to use the singular form with the uncountable noun
"information".
- Line 24: Fix the grammar and capitalization in the README sentence: change
"fixed services which has a json, its automatically covered but for the generic
triggers, you have to give instructions here." to a grammatically correct
sentence that uses "have" for subject-verb agreement, capitalizes "JSON", fixes
the possessive/contracted "it's"/"its" misuse, and smooths punctuation—e.g.,
make it read like "Instructions specific to writing services using the library.
For fixed services which have a JSON, it’s automatically covered; for generic
triggers you must provide instructions here." Update the README.md text
accordingly.
- Line 2: Update the README.md sentence that currently reads "This directory
contains custom instructions to enhance the performance specific library usages
of BI Copilot." to include the missing preposition; change it to either "This
directory contains custom instructions to enhance the performance of specific
library usages for BI Copilot." or "This directory contains custom instructions
to enhance performance for specific library usages of BI Copilot." to correct
the grammar and restore clarity.
...-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/graphql/service.md
Show resolved
Hide resolved
...-model-generator-ls-extension/src/main/resources/copilot/instructions/ballerina/http/test.md
Show resolved
Hide resolved
.../modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/README.md
Show resolved
Hide resolved
.../modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/README.md
Show resolved
Hide resolved
.../modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/README.md
Show resolved
Hide resolved
.../modules/flow-model-generator-ls-extension/src/main/resources/copilot/instructions/README.md
Show resolved
Hide resolved
Fix failing LS extension & Artifacts tests
There was a problem hiding this comment.
🧹 Nitpick comments (3)
flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/GetFilteredLibrariesFromSemanticModel.java (3)
56-56: Close the reader and pin UTF‑8 for JSON parsing.
Files.newBufferedReader(...)isn’t closed and uses platform default charset, which can be flaky across environments. Use try‑with‑resources + UTF‑8.♻️ Proposed fix
+import java.nio.charset.StandardCharsets; ... - TestConfig testConfig = gson.fromJson(Files.newBufferedReader(configJsonPath), TestConfig.class); + TestConfig testConfig; + try (var reader = Files.newBufferedReader(configJsonPath, StandardCharsets.UTF_8)) { + testConfig = gson.fromJson(reader, TestConfig.class); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/GetFilteredLibrariesFromSemanticModel.java` at line 56, The JSON reader created for parsing TestConfig via gson.fromJson(Files.newBufferedReader(configJsonPath), TestConfig.class) is not closed and uses the platform default charset; wrap the reader creation in a try-with-resources block and open it with UTF-8 (e.g., Files.newBufferedReader(configJsonPath, StandardCharsets.UTF_8)) so the Reader is automatically closed and parsing uses a fixed charset when constructing the TestConfig from gson.
69-90: Avoid order‑sensitive library assertions unless ordering is guaranteed.The test compares actual vs expected by index. If the API returns libraries in a different order, this becomes flaky. Consider matching by
nameinstead.♻️ Sketch of a safer matching approach
- for (int i = 0; i < actualLibraries.size(); i++) { - JsonObject actualLibrary = actualLibraries.get(i).getAsJsonObject(); - Library expectedLibrary = testConfig.expectedLibraries().get(i); + Map<String, Library> expectedByName = testConfig.expectedLibraries().stream() + .collect(Collectors.toMap(Library::name, l -> l)); + for (int i = 0; i < actualLibraries.size(); i++) { + JsonObject actualLibrary = actualLibraries.get(i).getAsJsonObject(); + String actualLibraryName = actualLibrary.get("name").getAsString(); + Library expectedLibrary = expectedByName.get(actualLibraryName); + if (expectedLibrary == null) { + log.info("Unexpected library '" + actualLibraryName + "'"); + assertFailure = true; + break; + } ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/GetFilteredLibrariesFromSemanticModel.java` around lines 69 - 90, The test is order‑sensitive: instead of comparing actualLibraries and testConfig.expectedLibraries() by index, build a lookup (e.g., Map<String,Library>) from expectedLibraries() keyed by expectedLibrary.name() and then iterate actualLibraries (JsonObjects), for each get "name" and lookup the expected Library by name to assert presence and structure (and fail if missing); remove index-based comparisons and breaks so each actual library is validated by name, and optionally assert no unexpected libraries remain by checking map emptiness or verifying all expected names were seen.
128-145: Align “verify” method semantics (true = valid) to avoid inverted logic.
verifyLinksInTypeDefsreturns true on success, butverifyLinksInFunctionsreturns false on success. This inconsistency is easy to misuse and obscures intent.♻️ Proposed alignment
- if (client.has("functions")) { - if (verifyLinksInFunctions(client.getAsJsonArray("functions"))) { - return false; - } - } + if (client.has("functions")) { + if (!verifyLinksInFunctions(client.getAsJsonArray("functions"))) { + return false; + } + } ... - if (library.has("functions")) { - if (verifyLinksInFunctions(library.getAsJsonArray("functions"))) { - return false; - } - } + if (library.has("functions")) { + if (!verifyLinksInFunctions(library.getAsJsonArray("functions"))) { + return false; + } + } ... - private boolean verifyLinksInFunctions(JsonArray functions) { + private boolean verifyLinksInFunctions(JsonArray functions) { for (JsonElement funcElement : functions) { ... - if (verifyLinkStructure(typeObj.getAsJsonArray("links"))) { - return true; - } + if (verifyLinkStructure(typeObj.getAsJsonArray("links"))) { + return false; + } ... - if (verifyLinkStructure(typeObj.getAsJsonArray("links"))) { - return true; - } + if (verifyLinkStructure(typeObj.getAsJsonArray("links"))) { + return false; + } ... } - return false; + return true; }Also applies to: 179-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/GetFilteredLibrariesFromSemanticModel.java` around lines 128 - 145, The logic is inverted: verifyLinksInTypeDefs returns true on success but verifyLinksInFunctions returns false on success; fix by making verifyLinksInFunctions' return semantics align with verifyLinksInTypeDefs (return true on success, false on failure) and update all call sites that check its result (e.g., the blocks that inspect "clients" and "functions" in the library and the other affected section around lines 179-213) so they use the new semantics (remove the inverted checks or negate where appropriate). Ensure you update the verifyLinksInFunctions implementation and any conditional usages that currently treat true as failure to instead treat true as success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/java/io/ballerina/flowmodelgenerator/extension/GetFilteredLibrariesFromSemanticModel.java`:
- Line 56: The JSON reader created for parsing TestConfig via
gson.fromJson(Files.newBufferedReader(configJsonPath), TestConfig.class) is not
closed and uses the platform default charset; wrap the reader creation in a
try-with-resources block and open it with UTF-8 (e.g.,
Files.newBufferedReader(configJsonPath, StandardCharsets.UTF_8)) so the Reader
is automatically closed and parsing uses a fixed charset when constructing the
TestConfig from gson.
- Around line 69-90: The test is order‑sensitive: instead of comparing
actualLibraries and testConfig.expectedLibraries() by index, build a lookup
(e.g., Map<String,Library>) from expectedLibraries() keyed by
expectedLibrary.name() and then iterate actualLibraries (JsonObjects), for each
get "name" and lookup the expected Library by name to assert presence and
structure (and fail if missing); remove index-based comparisons and breaks so
each actual library is validated by name, and optionally assert no unexpected
libraries remain by checking map emptiness or verifying all expected names were
seen.
- Around line 128-145: The logic is inverted: verifyLinksInTypeDefs returns true
on success but verifyLinksInFunctions returns false on success; fix by making
verifyLinksInFunctions' return semantics align with verifyLinksInTypeDefs
(return true on success, false on failure) and update all call sites that check
its result (e.g., the blocks that inspect "clients" and "functions" in the
library and the other affected section around lines 179-213) so they use the new
semantics (remove the inverted checks or negate where appropriate). Ensure you
update the verifyLinksInFunctions implementation and any conditional usages that
currently treat true as failure to instead treat true as success.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/agents_manager/config/get_tool_nodes.json (2)
666-676:⚠️ Potential issue | 🟡 MinorSame version mismatch for
timepackage.The icon URL references version
2.8.0(ballerina_time_2.8.0.png) but thecodedata.versionfield remains"2.7.0". This affects bothutcFromString(lines 666/674) andutcNow(lines 682/690).Suggested fix for time package entries
"codedata": { "node": "FUNCTION_CALL", "org": "ballerina", "module": "time", "packageName": "time", "symbol": "utcFromString", - "version": "2.7.0" + "version": "2.8.0" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/agents_manager/config/get_tool_nodes.json` around lines 666 - 676, The JSON entries for the time package have mismatched versions: the icon filenames reference 2.8.0 but the codedata.version is 2.7.0 for the functions utcFromString and utcNow; update the codedata.version value for the nodes with symbol "utcFromString" and "utcNow" to "2.8.0" so the "codedata.version" matches the icon (ballerina_time_2.8.0.png) and the package/module metadata remains consistent.
593-602:⚠️ Potential issue | 🟡 MinorVersion mismatch between icon URL and codedata version.
The icon URL references version
2.16.1(ballerina_log_2.16.1.png) but thecodedata.versionfield at line 601 remains"2.14.0". The same inconsistency appears in all fourlogfunctions (lines 593/601, 609/617, 625/633, 641/649).If the icon version should reflect the package version, consider updating the version fields to match:
Suggested fix for log package entries
"codedata": { "node": "FUNCTION_CALL", "org": "ballerina", "module": "log", "packageName": "log", "symbol": "printDebug", - "version": "2.14.0" + "version": "2.16.1" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/agents_manager/config/get_tool_nodes.json` around lines 593 - 602, The icon URLs for the Ballerina log functions (symbols "printDebug", "printError", "printInfo", "printWarn") reference version 2.16.1 but each codedata.version is "2.14.0"; update the codedata.version fields for these entries to "2.16.1" so the package version in codedata matches the icon URL (verify the four log function objects where codedata.node == "FUNCTION_CALL" and codedata.module == "log").flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/search/config/functions/custom_default2.json (1)
598-627:⚠️ Potential issue | 🟡 MinorAlign icon asset versions with codedata versions.
Icon URLs were updated but codedata versions still point to older releases. If these are meant to be in sync, bump the versions as well.
🔧 Suggested update to align codedata versions
- "version": "2.7.0" + "version": "2.8.0" ... - "version": "2.7.0" + "version": "2.8.0" ... - "version": "2.14.0" + "version": "2.16.1" ... - "version": "2.14.0" + "version": "2.16.1" ... - "version": "2.14.0" + "version": "2.16.1" ... - "version": "2.14.0" + "version": "2.16.1"Also applies to: 652-712
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/search/config/functions/custom_default2.json` around lines 598 - 627, The icon URLs were updated to 2.8.0 but the codedata "version" fields remain at 2.7.0; update the "version" value in each affected codedata block (e.g., entries with "symbol": "utcFromString" and "symbol": "utcNow" and the other entries referenced in the same section) to match the icon asset version (change "2.7.0" to "2.8.0") so codedata and icon assets are in sync; ensure you update every codedata.version in the 650–720 region similarly.flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/search/config/functions/custom_default1.json (1)
599-627:⚠️ Potential issue | 🟡 MinorAlign icon asset versions with codedata versions.
The icon assets are bumped to 2.8.0/2.16.1 while the codedata versions still say 2.7.0/2.14.0. If these versions should match, update the codedata fields; otherwise keep icons on the older asset versions to avoid misleading metadata.
🔧 Suggested update to align codedata versions
- "version": "2.7.0" + "version": "2.8.0" ... - "version": "2.7.0" + "version": "2.8.0" ... - "version": "2.14.0" + "version": "2.16.1" ... - "version": "2.14.0" + "version": "2.16.1" ... - "version": "2.14.0" + "version": "2.16.1" ... - "version": "2.14.0" + "version": "2.16.1"Also applies to: 652-712
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/search/config/functions/custom_default1.json` around lines 599 - 627, The metadata icon versions (e.g., time icon URLs showing 2.8.0) are inconsistent with the codedata.version fields (2.7.0); pick one source of truth and make them match — either update the codedata.version values for the affected function symbols (e.g., "utcFromString", "utcNow" and the other functions in the same block) to the icon asset version (2.8.0) or change the icon URLs to the codedata.version (2.7.0); ensure you update every occurrence in this JSON section (the "codedata"."version" fields) or every icon URL so all entries are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/search/config/functions/custom_default.json`:
- Around line 313-551: The JSON contains duplicate entries for the time
functions utcFromString and utcNow which causes repeated search results; remove
the duplicated objects that have "codedata.symbol": "utcFromString" and
"codedata.symbol": "utcNow" (the second occurrences) so each symbol appears only
once in the array, ensuring you delete the entire duplicate objects including
their "metadata", "codedata" and "enabled" fields.
---
Outside diff comments:
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/agents_manager/config/get_tool_nodes.json`:
- Around line 666-676: The JSON entries for the time package have mismatched
versions: the icon filenames reference 2.8.0 but the codedata.version is 2.7.0
for the functions utcFromString and utcNow; update the codedata.version value
for the nodes with symbol "utcFromString" and "utcNow" to "2.8.0" so the
"codedata.version" matches the icon (ballerina_time_2.8.0.png) and the
package/module metadata remains consistent.
- Around line 593-602: The icon URLs for the Ballerina log functions (symbols
"printDebug", "printError", "printInfo", "printWarn") reference version 2.16.1
but each codedata.version is "2.14.0"; update the codedata.version fields for
these entries to "2.16.1" so the package version in codedata matches the icon
URL (verify the four log function objects where codedata.node == "FUNCTION_CALL"
and codedata.module == "log").
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/search/config/functions/custom_default1.json`:
- Around line 599-627: The metadata icon versions (e.g., time icon URLs showing
2.8.0) are inconsistent with the codedata.version fields (2.7.0); pick one
source of truth and make them match — either update the codedata.version values
for the affected function symbols (e.g., "utcFromString", "utcNow" and the other
functions in the same block) to the icon asset version (2.8.0) or change the
icon URLs to the codedata.version (2.7.0); ensure you update every occurrence in
this JSON section (the "codedata"."version" fields) or every icon URL so all
entries are consistent.
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/search/config/functions/custom_default2.json`:
- Around line 598-627: The icon URLs were updated to 2.8.0 but the codedata
"version" fields remain at 2.7.0; update the "version" value in each affected
codedata block (e.g., entries with "symbol": "utcFromString" and "symbol":
"utcNow" and the other entries referenced in the same section) to match the icon
asset version (change "2.7.0" to "2.8.0") so codedata and icon assets are in
sync; ensure you update every codedata.version in the 650–720 region similarly.
...-model-generator-ls-extension/src/test/resources/search/config/functions/custom_default.json
Show resolved
Hide resolved
Fix test resources
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/search/config/types/default.json`:
- Around line 509-512: User-facing strings in the "metadata" object for the type
labeled "BaseToolKit" contain typos; update the "description" value to correct
spelling and grammar (e.g., "Allows implementing custom toolkits by extending
this type. Toolkits can help define new types of tools so that agents can
understand them.") and also fix the same typos in the other occurrence of the
"metadata" block for "BaseToolKit" elsewhere in the file so both descriptions
match and are grammatically correct.
---
Duplicate comments:
In
`@flow-model-generator/modules/flow-model-generator-ls-extension/src/test/resources/search/config/functions/custom_default.json`:
- Around line 627-667: Ensure the duplicate entries for the time module are
fully removed by verifying there is only one item object for each symbol
"utcFromString" and "utcNow" within the items array for the "time" metadata
block; if any extra objects remain, delete the duplicates and make sure the
remaining entries have correct metadata (label, description, icon) and codedata
(node, org, module, packageName, symbol, version) and are enabled:true so the
module contains exactly one canonical definition per function.
...odules/flow-model-generator-ls-extension/src/test/resources/search/config/types/default.json
Outdated
Show resolved
Hide resolved
|
Copilot stuff LGTM @dulajdilshan / @nipunayf Lets merge this if test changes are okay? |
nipunayf
left a comment
nipunayf
left a comment
There was a problem hiding this comment.
Minor comments on the documented versions.
...e/src/main/java/io/ballerina/flowmodelgenerator/core/copilot/adapters/StringPathAdapter.java
Outdated
Show resolved
Hide resolved
model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/TypeDefData.java
Outdated
Show resolved
Hide resolved
model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/TypeDefData.java
Outdated
Show resolved
Hide resolved
nipunayf
left a comment
nipunayf
left a comment
There was a problem hiding this comment.
Only concerned with the file copy scenario; the rest can be ignored.
...del-generator-core/src/main/java/io/ballerina/flowmodelgenerator/core/InstructionLoader.java
Outdated
Show resolved
Hide resolved
...extension/src/main/java/io/ballerina/flowmodelgenerator/extension/CopilotLibraryService.java
Show resolved
Hide resolved
...main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java
Outdated
Show resolved
Hide resolved
| sqlBuilder.append(" AND org = 'ballerina'"); | ||
| } else if (MODE_HEALTHCARE.equalsIgnoreCase(mode)) { | ||
| // Load packages starting with 'health' and ballerina/http | ||
| sqlBuilder.append(" AND (package_name LIKE 'health%' OR (org = 'ballerina' AND package_name = 'http'))"); |
There was a problem hiding this comment.
OR operation can be expensive for this use case; I would rather cache the http result, and combine it.
There was a problem hiding this comment.
Caching can be bit expensive in healthcare case as this is executed less.
Will evaluate and address this in a seperate PR
...main/java/io/ballerina/flowmodelgenerator/core/copilot/database/LibraryDatabaseAccessor.java
Outdated
Show resolved
Hide resolved
xlight05
force-pushed
the
copilot-library-unify
branch
from
66b419c to
b0ccf86
Compare
3 participants
Description
This resolves wso2/product-ballerina-integrator#1909, wso2/product-ballerina-integrator#2384
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary
This PR migrates the Copilot libraries integration from a file-based JSON approach to a database-backed search index and adds keyword search. It establishes end-to-end infrastructure for indexing, querying, transforming, and enriching library metadata and service definitions for Copilot usage.
Key Changes
Database-backed search
Keyword search endpoint
Centralized Copilot library management
Instruction and service augmentation
Model and conversion utilities
API and request changes
Index generation and tooling
Documentation and instructional content
Tests
Outcomes / Intent
Notable Surface Changes