Feat/mcp#305
Open
msslulu wants to merge 3 commits intoopentiny:developfrom
Open
Conversation
WalkthroughAdds Model Context Protocol (MCP) support: local tool annotation/registry, remote streamable HTTP MCP client/manager, tool implementations (demo and Git bundle importer), OpenAI/OpenAI-config binding for streamable tool routing, application config updates, and required dependency additions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AiChat as AI Chat Service
participant Registry as MCPToolRegistry
participant Local as DemoMcpTools
participant StreamMgr as StreamableHttpClientManager
participant Remote as Remote MCP Server
participant OpenAI as OpenAI API
User->>AiChat: send chat request (tools=true)
AiChat->>Registry: discover local tools
Registry-->>AiChat: return local tool schemas
AiChat->>StreamMgr: ensure remote tools listed
StreamMgr->>Remote: init session & listTools()
Remote-->>StreamMgr: return remote tool definitions
StreamMgr-->>AiChat: cached remote tools
AiChat->>OpenAI: send chat + tool schemas
OpenAI-->>AiChat: response with tool_calls
alt tool is local
AiChat->>Registry: getTool(toolName)
Registry-->>AiChat: MCPToolDefinition
AiChat->>Local: execute(arguments)
Local-->>AiChat: result
else tool is remote
AiChat->>StreamMgr: callTool(toolName, arguments)
StreamMgr->>Remote: JSON‑RPC tools/call
Remote-->>StreamMgr: tool result (JSON)
StreamMgr-->>AiChat: parsed result
end
AiChat->>OpenAI: send tool results (multi-turn)
OpenAI-->>AiChat: final assistant response
AiChat-->>User: deliver AI response (SSE if streaming)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (5)
app/src/main/java/com/tinyengine/it/TinyEngineApplication.java (1)
33-33: Keep@MapperScanlimited to mapper packages.
com.tinyengine.it.mcp.toolsis already covered by Spring component scanning; adding it here blurs responsibilities and can turn future interfaces in that package into accidental MyBatis mappers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/tinyengine/it/TinyEngineApplication.java` at line 33, The `@MapperScan` annotation on TinyEngineApplication currently lists "com.tinyengine.it.mcp.tools" alongside mapper packages; remove "com.tinyengine.it.mcp.tools" from the `@MapperScan` package list so only actual MyBatis mapper packages (e.g., "com.tinyengine.it.mapper" and "com.tinyengine.it.dynamic.dao") are scanned, leaving com.tinyengine.it.mcp.tools to Spring's regular component scanning to avoid accidentally treating non-mapper interfaces as MyBatis mappers.base/src/main/java/com/tinyengine/it/mcp/config/McpToolsConfig.java (1)
21-25: Use the application logger instead ofSystem.outhere.Bean creation logs should stay structured and level-controlled; stdout makes startup noise harder to manage in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/mcp/config/McpToolsConfig.java` around lines 21 - 25, Replace the System.out.println call used to log the tool count with the application's logger: add a logger instance (e.g., private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(McpToolsConfig.class)) to the McpToolsConfig class and change the print to a structured log call such as log.info("Tool count from provider: {}", provider.getToolCallbacks().length) or log.debug(...) depending on desired level; keep the rest of the MethodToolCallbackProvider builder code unchanged.base/src/main/java/com/tinyengine/it/mcp/tools/DemoMcpTools.java (1)
29-36: Hoist the static weather table out of the method.Rebuilding the same map on every tool call is unnecessary allocation. A
private static final Map<String, String>keeps the demo behavior and simplifies the method body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/mcp/tools/DemoMcpTools.java` around lines 29 - 36, The local mutable map "weatherMap" in DemoMcpTools should be hoisted to an immutable class-level constant to avoid rebuilding it on every call: add a private static final Map<String,String> (e.g., WEATHER_MAP) initialized once (using Map.ofEntries or an unmodifiable wrapper/static initializer) containing the Beijing/Shanghai/Guangzhou/Shenzhen entries, then replace the method's construction of weatherMap with a single return using WEATHER_MAP.getOrDefault(city, "未知城市: " + city + "，请尝试北京、上海、广州、深圳"); keep the class name DemoMcpTools and the getDefault fallback text identical.base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java (1)
319-365: Merge duplicated local-tool schema conversion methods.
convertLocalToolToSchemaandconvertToolToSchemaperform the same mapping; keep one to reduce drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java` around lines 319 - 365, The two identical methods convertLocalToolToSchema and convertToolToSchema are duplicated; remove one and consolidate callers to the remaining method to avoid divergence. Delete convertLocalToolToSchema (or convertToolToSchema) and ensure all internal references call the kept method (search for usages of convertLocalToolToSchema and convertToolToSchema), keeping convertRemoteToolToSchema as-is; run tests/compile to confirm no missing references and adjust any imports or calls accordingly.base/src/main/java/com/tinyengine/it/mcp/core/StreamableMcpClient.java (1)
293-294:doGetRequesttakesrequestBodybut does not use it.This parameter is dead and makes request-id/body flow harder to reason about.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/mcp/core/StreamableMcpClient.java` around lines 293 - 294, The doGetRequest method currently declares an unused parameter requestBody; remove this dead parameter to avoid confusion: update the StreamableMcpClient.doGetRequest signature to accept only the needed requestId (or no extra params if not used), remove requestBody from all calls to doGetRequest, and adjust any related javadoc/comments and imports so callers pass the correct arguments and compilation succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/resources/application-alpha.yml`:
- Around line 111-115: The openai.api-key is currently a hardcoded placeholder;
replace it with an environment-bound value so deployments fail fast if missing
by changing the openai.api-key entry to read from an environment/property
placeholder (e.g. use a Spring property-style placeholder like ${OPENAI_API_KEY}
with no default) and ensure any boot-time validation uses that property so the
app errors at startup when the key is not set; update any code reading
openai.api-key to expect the env-bound value and remove the literal
"your-api-key".
In `@base/src/main/java/com/tinyengine/it/mcp/core/MCPToolDefinition.java`:
- Around line 27-36: The current reflective arg resolution in MCPToolDefinition
sets args[i]=null for any param type other than JsonNode or String, which breaks
primitive/typed parameters; update the logic in the method handling
paramTypes/args/arguments to convert the JsonNode (arguments) into the target
parameter type using an ObjectMapper.convertValue call (e.g.,
convertValue(arguments, targetType)), and if paramTypes[i].isPrimitive() first
map the primitive to its wrapper class (or implement a small primitive->wrapper
map) so convertValue and Method.invoke receive the correct boxed type; ensure
you replace the null fallback with this conversion path for non-String/JsonNode
types.
In `@base/src/main/java/com/tinyengine/it/mcp/core/MCPToolRegistry.java`:
- Line 64: The current registration call tools.put(def.getName(), def) in
MCPToolRegistry silently overwrites existing entries; change the registration
flow to detect collisions by checking tools.containsKey(def.getName()) before
calling tools.put, and if a collision exists either throw an
IllegalStateException (including def.getName() and both implementations'
identifying info) or log an error and skip/abort the registration to avoid
replacing the original; update the code path that registers via tools.put to
perform this guard and ensure callers of register/put handle the thrown
exception or error return.
- Around line 71-76: The loop currently hardcodes every parameter as "string";
change it to inspect each parameter's runtime type via param.getType() and set
an appropriate JSON Schema type on the ObjectNode (e.g., if type == String.class
=> "string"; if primitive/boxed int/long => "integer"; if primitive/boxed
float/double => "number"; if boolean/Boolean => "boolean"; if array or
Collection => "array"; if Map or other complex types => "object"). Use the same
objectMapper.createObjectNode() and properties.set(paramName, prop) but replace
the fixed prop.put("type", "string") with logic mapping method.getParameters()
-> param.getType() -> schema type (handle boxed vs primitive and common
collection/map cases). Ensure MCPToolRegistry's parameter handling uses this
mapping so payload types match the method signature.
- Around line 35-37: The loop in MCPToolRegistry that inspects
bean.getClass().getDeclaredMethods() misses methods on proxied Spring beans;
change it to resolve the real class via AopUtils.getTargetClass(bean) and
iterate that class's declared methods, then call registerTool(bean, method) as
before; also add the necessary import for
org.springframework.aop.support.AopUtils and ensure you keep using the original
bean instance when invoking registerTool so proxy behavior is preserved.
In
`@base/src/main/java/com/tinyengine/it/mcp/core/StreamableHttpClientManager.java`:
- Line 36: Replace the System.out.println that prints the entire config in
StreamableHttpClientManager (constructor/initializer) to avoid exposing secrets;
instead use the project's logger (e.g., a SLF4J/Logger instance) and either log
only non-sensitive fields or a sanitized/masked representation of the config
(omit or redact API keys, tokens, credentials) before logging so sensitive
values are never printed to stdout or logs.
- Around line 54-55: The cache currently keys tools by def.getName() causing
collisions when multiple servers expose the same name; change the cache key to
include the source URL (e.g., compositeKey = url + "|" + def.getName() or use a
unique tool id if available) where StreamableHttpClientManager puts entries
(toolCache.put(...)), and update any retrieval logic that looks up by name to
use the same composite key or provide a lookup map from name->List/Map of
(url,def) to avoid overwrites; ensure calls that iterate or fetch from toolCache
(references to toolCache and any getTool/getToolByName methods) are adjusted to
use the composite key or the new lookup approach and keep the existing log line
intact.
- Line 35: The init() method in StreamableHttpClientManager must be annotated
with `@PostConstruct` so Spring calls it after bean creation; add the
`@PostConstruct` annotation above the init() method in the
StreamableHttpClientManager class (and import the appropriate PostConstruct
class from javax.annotation or jakarta.annotation consistent with the project),
which ensures init() runs to populate toolCache and clientCache during bean
initialization.
In `@base/src/main/java/com/tinyengine/it/mcp/core/StreamableMcpClient.java`:
- Around line 126-133: The current chain in doSendRequest uses
.then(sink.asMono()) which delays delivering the first SSE (202) until the
upstream Mono completes; change the flow to return sink.asMono() directly for
streaming responses (i.e., after triggering doSendRequest and wiring
timeout/onErrorResume) so the caller receives the first event immediately (keep
the pendingRequests.remove(requestId) behavior in the onErrorResume or move it
into sink.doFinally to clean up on terminal signals). Also remove the unused
requestBody parameter from doGetRequest (and update its callers) since GET
requests should not accept a body; update method signature and call sites
accordingly. Ensure you reference doSendRequest, sink.asMono,
.then(sink.asMono()), handleServerSentEvent/handleStreamingResponse for
streaming behavior, and doGetRequest/requestBody for the signature cleanup.
In `@base/src/main/java/com/tinyengine/it/mcp/tools/GitFileReaderService.java`:
- Around line 89-95: The code in GitFileReaderService calls parseBundle and
immediately dereferences bundleResultDtoResult.getData(), which can NPE if
parseBundle returned a failure; update the logic around parseBundle (the
Result<BundleResultDto> variable bundleResultDtoResult) to check the result
status (e.g., isSuccess()/isOk() or result.isFailure()) before calling
getData(), and on failure log the error (include
bundleResultDtoResult.getError()/message), then return/throw an appropriate
exception instead of proceeding; ensure subsequent uses of bundleResultDto,
componentList, and packageList only occur after the success check.
- Around line 146-153: The fetchBytes method is vulnerable to SSRF because it
accepts any URL; update fetchBytes to validate and reject unsafe targets before
opening the connection: ensure the scheme is only "http" or "https", resolve the
URL host to an InetAddress and reject loopback, link-local, site-local and
private/rfc1918/mapped IPv4, multicast and unspecified addresses, and disallow
hosts not matching an allowed set (e.g., raw.githubusercontent.com or your
approved domains) or a configured whitelist; if you need CIDR checks, use
InetAddress and compare against private ranges; throw IOException on validation
failure and only then proceed to create the HttpURLConnection (preserving
existing timeout and header code).
- Around line 358-372: The code increments addNum unconditionally even when
baseMapper.createComponent(component) fails; change the logic in
GitFileReaderService around createComponent/component handling so addNum is
incremented only when the insert succeeded (i.e., when Integer result equals 1).
Concretely, move the addNum++ into the if (result == 1) block (or check
Objects.equals(result, 1) to avoid NPE) and only call
createMaterialComponent/createMaterialHistoryComponent when result indicates
success.
- Around line 329-386: The bulkCreate method in GitFileReaderService performs
multiple dependent DB writes per component and must be executed atomically;
annotate the bulkCreate method with Spring's `@Transactional` (import
org.springframework.transaction.annotation.Transactional) so that failures
during createComponent/createMaterialComponent/createMaterialHistoryComponent or
updateComponentById roll back the whole transaction (optionally set
rollbackFor=Exception.class) and ensure the class is a Spring-managed bean so
the annotation is effective.
In
`@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java`:
- Around line 137-139: Replace the hard-coded SM4 key assigned to the variable
sm4Key and its direct use in SM4Utils.encryptECB(apiKey, sm4Key) with a secure
secret retrieval mechanism (e.g., environment variable, vault/secret-manager
client, or application config) and fail-fast if the secret is missing; update
AiChatV1ServiceImpl to read the key from a secure source (instead of the literal
"rOGV7EsU7thOawaUrOI+LA==") wherever sm4Key is used (including the other
occurrences around the 538-540 area), and ensure any code that calls
SM4Utils.encryptECB(apiKey, sm4Key) uses the retrieved secret and logs no secret
material.
- Around line 405-467: The infinite loop in AiChatV1ServiceImpl surrounding the
tool-call handling (while (true) using originalRequest/messagesArray and
executeLocalTool) can run forever; add a max-turn guard (e.g., int
maxToolCallTurns = 5 or configurable field) and replace the unbounded while with
a loop that increments a counter each iteration, breaking and throwing a
ServiceException (or returning an error) if the counter exceeds the limit;
ensure you update originalRequest.setMessages(messagesArray) each turn as now,
and keep existing checks (tool_calls empty → return) so normal termination still
works.
- Around line 122-126: The non-stream branch in AiChatV1ServiceImpl currently
always returns processStandardResponse, which skips tool-call orchestration;
change the branch to invoke processNonStreamWithTools(requestBuilder, request)
when tool orchestration is required (i.e., when the requestBuilder/request
indicate tools are present) instead of calling processStandardResponse, ensuring
you preserve any existing fallback to processStandardResponse only when no tools
are needed.
In `@pom.xml`:
- Around line 218-224: The netty BOM dependency (artifactId netty-bom, version
${netty.version}) is currently declared inside <dependencies> with scope import
which is invalid; move that <dependency> entry into the <dependencyManagement>
section so the BOM is imported correctly (create a <dependencyManagement>
element if missing) and remove it from <dependencies> so the BOM's versions are
applied to managed artifacts.
---
Nitpick comments:
In `@app/src/main/java/com/tinyengine/it/TinyEngineApplication.java`:
- Line 33: The `@MapperScan` annotation on TinyEngineApplication currently lists
"com.tinyengine.it.mcp.tools" alongside mapper packages; remove
"com.tinyengine.it.mcp.tools" from the `@MapperScan` package list so only actual
MyBatis mapper packages (e.g., "com.tinyengine.it.mapper" and
"com.tinyengine.it.dynamic.dao") are scanned, leaving
com.tinyengine.it.mcp.tools to Spring's regular component scanning to avoid
accidentally treating non-mapper interfaces as MyBatis mappers.
In `@base/src/main/java/com/tinyengine/it/mcp/config/McpToolsConfig.java`:
- Around line 21-25: Replace the System.out.println call used to log the tool
count with the application's logger: add a logger instance (e.g., private static
final org.slf4j.Logger log =
org.slf4j.LoggerFactory.getLogger(McpToolsConfig.class)) to the McpToolsConfig
class and change the print to a structured log call such as log.info("Tool count
from provider: {}", provider.getToolCallbacks().length) or log.debug(...)
depending on desired level; keep the rest of the MethodToolCallbackProvider
builder code unchanged.
In `@base/src/main/java/com/tinyengine/it/mcp/core/StreamableMcpClient.java`:
- Around line 293-294: The doGetRequest method currently declares an unused
parameter requestBody; remove this dead parameter to avoid confusion: update the
StreamableMcpClient.doGetRequest signature to accept only the needed requestId
(or no extra params if not used), remove requestBody from all calls to
doGetRequest, and adjust any related javadoc/comments and imports so callers
pass the correct arguments and compilation succeeds.
In `@base/src/main/java/com/tinyengine/it/mcp/tools/DemoMcpTools.java`:
- Around line 29-36: The local mutable map "weatherMap" in DemoMcpTools should
be hoisted to an immutable class-level constant to avoid rebuilding it on every
call: add a private static final Map<String,String> (e.g., WEATHER_MAP)
initialized once (using Map.ofEntries or an unmodifiable wrapper/static
initializer) containing the Beijing/Shanghai/Guangzhou/Shenzhen entries, then
replace the method's construction of weatherMap with a single return using
WEATHER_MAP.getOrDefault(city, "未知城市: " + city + "，请尝试北京、上海、广州、深圳"); keep the
class name DemoMcpTools and the getDefault fallback text identical.
In
`@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java`:
- Around line 319-365: The two identical methods convertLocalToolToSchema and
convertToolToSchema are duplicated; remove one and consolidate callers to the
remaining method to avoid divergence. Delete convertLocalToolToSchema (or
convertToolToSchema) and ensure all internal references call the kept method
(search for usages of convertLocalToolToSchema and convertToolToSchema), keeping
convertRemoteToolToSchema as-is; run tests/compile to confirm no missing
references and adjust any imports or calls accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 628db276-b02a-49ed-be51-ac1503c482e9
📒 Files selected for processing (15)
app/src/main/java/com/tinyengine/it/TinyEngineApplication.javaapp/src/main/resources/application-alpha.ymlapp/src/main/resources/sql/mysql/init_data_for_test_v1.0.0.sqlbase/src/main/java/com/tinyengine/it/config/OpenAIConfig.javabase/src/main/java/com/tinyengine/it/mcp/annotation/MCPTool.javabase/src/main/java/com/tinyengine/it/mcp/config/McpToolsConfig.javabase/src/main/java/com/tinyengine/it/mcp/core/MCPToolDefinition.javabase/src/main/java/com/tinyengine/it/mcp/core/MCPToolRegistry.javabase/src/main/java/com/tinyengine/it/mcp/core/RemoteToolDefinition.javabase/src/main/java/com/tinyengine/it/mcp/core/StreamableHttpClientManager.javabase/src/main/java/com/tinyengine/it/mcp/core/StreamableMcpClient.javabase/src/main/java/com/tinyengine/it/mcp/tools/DemoMcpTools.javabase/src/main/java/com/tinyengine/it/mcp/tools/GitFileReaderService.javabase/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.javapom.xml
Comment on lines
+111
to
+115
| openai: | ||
| api-key: your-api-key | ||
| base-url: https://api.deepseek.com/v1/chat/completions | ||
| default-model: deepseek-chat | ||
| timeout-seconds: 300 |
There was a problem hiding this comment.
Do not ship a placeholder API key in an active profile.
This makes the MCP/OpenAI path boot with an invalid credential and fail only on first use. Bind it from the environment instead so misconfiguration is caught at deploy time.
Suggested fix
openai:
- api-key: your-api-key
+ api-key: ${OPENAI_API_KEY}
base-url: https://api.deepseek.com/v1/chat/completions
default-model: deepseek-chat
timeout-seconds: 300🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/resources/application-alpha.yml` around lines 111 - 115, The
openai.api-key is currently a hardcoded placeholder; replace it with an
environment-bound value so deployments fail fast if missing by changing the
openai.api-key entry to read from an environment/property placeholder (e.g. use
a Spring property-style placeholder like ${OPENAI_API_KEY} with no default) and
ensure any boot-time validation uses that property so the app errors at startup
when the key is not set; update any code reading openai.api-key to expect the
env-bound value and remove the literal "your-api-key".
Comment on lines
+27
to
+36
| for (int i = 0; i < paramTypes.length; i++) { | ||
| Class<?> type = paramTypes[i]; | ||
| if (type == JsonNode.class) { | ||
| args[i] = arguments; | ||
| } else if (type == String.class) { | ||
| args[i] = arguments.toString(); | ||
| } else { | ||
| // 可以扩展更复杂的转换，例如使用 ObjectMapper 转换为指定类型 | ||
| args[i] = null; | ||
| } |
There was a problem hiding this comment.
Null fallback breaks typed/primitive tool parameters at runtime.
For non-String/JsonNode params, this sets args[i] = null, which will fail for primitive parameters and many typed methods when invoked reflectively.
🐛 Proposed fix
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.lang.reflect.Parameter;
...
+ private static final ObjectMapper MAPPER = new ObjectMapper();
...
public Object execute(JsonNode arguments) throws Exception {
- Class<?>[] paramTypes = method.getParameterTypes();
- Object[] args = new Object[paramTypes.length];
- for (int i = 0; i < paramTypes.length; i++) {
- Class<?> type = paramTypes[i];
- if (type == JsonNode.class) {
- args[i] = arguments;
- } else if (type == String.class) {
- args[i] = arguments.toString();
- } else {
- // 可以扩展更复杂的转换，例如使用 ObjectMapper 转换为指定类型
- args[i] = null;
- }
- }
+ Parameter[] params = method.getParameters();
+ Object[] args = new Object[params.length];
+ for (int i = 0; i < params.length; i++) {
+ Class<?> type = params[i].getType();
+ JsonNode valueNode = (arguments != null && arguments.isObject())
+ ? arguments.get(params[i].getName())
+ : arguments;
+ if (valueNode == null || valueNode.isNull()) {
+ if (type.isPrimitive()) {
+ throw new IllegalArgumentException("Missing required arg: " + params[i].getName());
+ }
+ args[i] = null;
+ } else if (type == JsonNode.class) {
+ args[i] = valueNode;
+ } else {
+ args[i] = MAPPER.convertValue(valueNode, type);
+ }
+ }
return method.invoke(bean, args);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/src/main/java/com/tinyengine/it/mcp/core/MCPToolDefinition.java` around
lines 27 - 36, The current reflective arg resolution in MCPToolDefinition sets
args[i]=null for any param type other than JsonNode or String, which breaks
primitive/typed parameters; update the logic in the method handling
paramTypes/args/arguments to convert the JsonNode (arguments) into the target
parameter type using an ObjectMapper.convertValue call (e.g.,
convertValue(arguments, targetType)), and if paramTypes[i].isPrimitive() first
map the primitive to its wrapper class (or implement a small primitive->wrapper
map) so convertValue and Method.invoke receive the correct boxed type; ensure
you replace the null fallback with this conversion path for non-String/JsonNode
types.
Comment on lines
+35
to
+37
| for (Method method : bean.getClass().getDeclaredMethods()) { | ||
| if (method.isAnnotationPresent(MCPTool.class)) { | ||
| registerTool(bean, method); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MCPToolRegistry.java" -type fRepository: opentiny/tiny-engine-backend-java
Length of output: 146
🏁 Script executed:
cat -n ./base/src/main/java/com/tinyengine/it/mcp/core/MCPToolRegistry.javaRepository: opentiny/tiny-engine-backend-java
Length of output: 3179
🏁 Script executed:
# Check if there are any other places where beans are scanned or if there's any proxy handling
rg "AopUtils|getTargetClass|getBeansWithAnnotation" --type java
# Check if beans in this context are actually proxied (look for `@Transactional`, `@Async`, etc.)
rg "@Transactional|@Async|@Cacheable" base/src/main/java/com/tinyengine/it/mcp/core/ --type java
# Check imports to see if AopUtils is already imported elsewhere
rg "import.*AopUtils" --type javaRepository: opentiny/tiny-engine-backend-java
Length of output: 266
🏁 Script executed:
# Check if there are beans with proxy-creating annotations anywhere in the project
# that might be picked up by getBeansWithAnnotation(Component.class)
rg "@Transactional|@Async|@Cacheable" base/src/main/java/com/tinyengine/it/mcp/ --type java -A 5
# Check the MCPTool annotation definition to understand its scope
find . -name "MCPTool.java" -type fRepository: opentiny/tiny-engine-backend-java
Length of output: 144
🏁 Script executed:
cat -n ./base/src/main/java/com/tinyengine/it/mcp/annotation/MCPTool.javaRepository: opentiny/tiny-engine-backend-java
Length of output: 485
🏁 Script executed:
# Check if there are any Component beans in the mcp module that might be proxied
find ./base/src/main/java/com/tinyengine/it/mcp -name "*.java" -type f -exec grep -l "@Component\|@Service\|@Repository" {} \;
# Check if spring-aop is in the dependencies
fd -e "pom.xml" -e "build.gradle" | head -5 | xargs grep -l "spring-aop" 2>/dev/null || echo "spring-aop dependency check..."Repository: opentiny/tiny-engine-backend-java
Length of output: 400
🏁 Script executed:
# Check the actual component beans to see if any have proxy-creating annotations
cat -n ./base/src/main/java/com/tinyengine/it/mcp/tools/DemoMcpTools.java
# Also check StreamableHttpClientManager
cat -n ./base/src/main/java/com/tinyengine/it/mcp/core/StreamableHttpClientManager.javaRepository: opentiny/tiny-engine-backend-java
Length of output: 6630
🏁 Script executed:
# Check pom.xml for spring-aop dependency
find . -name "pom.xml" -type f | xargs grep -A 5 -B 5 "spring-aop" | head -30
# Also check if spring-aop is listed without being explicitly found
cat pom.xml | head -100Repository: opentiny/tiny-engine-backend-java
Length of output: 4158
🏁 Script executed:
# Check if any of the Component beans in the MCP module have proxy-creating annotations
rg "@Transactional|@Async|@Cacheable|@CircuitBreaker" base/src/main/java/com/tinyengine/it/mcp/ --type java
# Check the full pom.xml to see what Spring version and dependencies are being used
grep -A 20 "spring-boot-starter-aop" pom.xmlRepository: opentiny/tiny-engine-backend-java
Length of output: 810
🏁 Script executed:
# Check all Component/Service/Repository beans in the entire project for proxy-creating annotations
rg "@Transactional|@Async|@Cacheable|@CircuitBreaker" base/src/main/java/com/tinyengine/it/ --type java | grep -v ".class"
# Check for aspect definitions or aspect components
rg "@Aspect|@EnableAspectJAutoProxy" base/src/main/java/ --type java
# Let's also verify what happens with getTargetClass on non-proxied beans
# by checking Spring documentation expectations
grep -r "getTargetClass" base/src/main/java/ || echo "No AopUtils usage found"Repository: opentiny/tiny-engine-backend-java
Length of output: 936
🏁 Script executed:
# Check GitFileReaderService to see if it has any proxy-creating annotations
cat -n ./base/src/main/java/com/tinyengine/it/mcp/tools/GitFileReaderService.java | head -40Repository: opentiny/tiny-engine-backend-java
Length of output: 1717
🏁 Script executed:
# Let's verify the Spring documentation expectation about AopUtils.getTargetClass
# Check if there's any documentation or examples in the codebase
rg "getDeclaredMethods|getTargetClass" --type java | head -20
# Let's also check what happens when we use getClass() on a proxied bean
# by looking at Spring Boot documentation
echo "Checking if this is a best practice issue or active bug..."Repository: opentiny/tiny-engine-backend-java
Length of output: 268
🏁 Script executed:
# Verify that AopUtils is available in the Spring version being used
grep -A 2 "spring-boot-starter-aop" pom.xml
# Check if org.springframework.aop.support.AopUtils is part of spring-aop
# by looking at what's imported from Spring in the project
rg "org.springframework.aop" base/src/main/java/ --type java | head -10Repository: opentiny/tiny-engine-backend-java
Length of output: 161
Use AopUtils.getTargetClass() to handle Spring proxies in method discovery.
When scanning for @MCPTool annotations, bean.getClass().getDeclaredMethods() fails for proxied beans (e.g., those with @Transactional). Since this registry scans arbitrary @Component beans, use AopUtils.getTargetClass(bean) to safely resolve the actual class regardless of proxy type.
♻️ Suggested fix
+import org.springframework.aop.support.AopUtils;
...
- for (Object bean : beans.values()) {
- for (Method method : bean.getClass().getDeclaredMethods()) {
+ for (Object bean : beans.values()) {
+ Class<?> targetClass = AopUtils.getTargetClass(bean);
+ for (Method method : targetClass.getDeclaredMethods()) {
if (method.isAnnotationPresent(MCPTool.class)) {
registerTool(bean, method);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/src/main/java/com/tinyengine/it/mcp/core/MCPToolRegistry.java` around
lines 35 - 37, The loop in MCPToolRegistry that inspects
bean.getClass().getDeclaredMethods() misses methods on proxied Spring beans;
change it to resolve the real class via AopUtils.getTargetClass(bean) and
iterate that class's declared methods, then call registerTool(bean, method) as
before; also add the necessary import for
org.springframework.aop.support.AopUtils and ensure you keep using the original
bean instance when invoking registerTool so proxy behavior is preserved.
| def.setBean(bean); | ||
| def.setMethod(method); | ||
| def.setInputSchema(generateSchema(method)); | ||
| tools.put(def.getName(), def); |
There was a problem hiding this comment.
Tool name collisions silently overwrite existing registrations.
tools.put(def.getName(), def) replaces previous entries without warning, which can route calls to the wrong implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/src/main/java/com/tinyengine/it/mcp/core/MCPToolRegistry.java` at line
64, The current registration call tools.put(def.getName(), def) in
MCPToolRegistry silently overwrites existing entries; change the registration
flow to detect collisions by checking tools.containsKey(def.getName()) before
calling tools.put, and if a collision exists either throw an
IllegalStateException (including def.getName() and both implementations'
identifying info) or log an error and skip/abort the registration to avoid
replacing the original; update the code path that registers via tools.put to
perform this guard and ensure callers of register/put handle the thrown
exception or error return.
Comment on lines
+71
to
+76
| for (Parameter param : method.getParameters()) { | ||
| String paramName = param.getName(); | ||
| ObjectNode prop = objectMapper.createObjectNode(); | ||
| // 简化：假设参数都是 String | ||
| prop.put("type", "string"); | ||
| properties.set(paramName, prop); |
There was a problem hiding this comment.
Generated JSON schema currently misrepresents parameter types.
All params are declared as "type": "string", which conflicts with methods expecting numbers/objects and leads to invalid tool-call payloads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/src/main/java/com/tinyengine/it/mcp/core/MCPToolRegistry.java` around
lines 71 - 76, The loop currently hardcodes every parameter as "string"; change
it to inspect each parameter's runtime type via param.getType() and set an
appropriate JSON Schema type on the ObjectNode (e.g., if type == String.class =>
"string"; if primitive/boxed int/long => "integer"; if primitive/boxed
float/double => "number"; if boolean/Boolean => "boolean"; if array or
Collection => "array"; if Map or other complex types => "object"). Use the same
objectMapper.createObjectNode() and properties.set(paramName, prop) but replace
the fixed prop.put("type", "string") with logic mapping method.getParameters()
-> param.getType() -> schema type (handle boxed vs primitive and common
collection/map cases). Ensure MCPToolRegistry's parameter handling uses this
mapping so payload types match the method signature.
Comment on lines
+358
to
+372
| if (queryComponent.isEmpty()) { | ||
| // 插入新记录 | ||
| Integer result = baseMapper.createComponent(component); | ||
| if (result == 1) { | ||
| MaterialComponent materialComponent = new MaterialComponent(); | ||
| materialComponent.setMaterialId(1); | ||
| materialComponent.setComponentId(component.getId()); | ||
| baseMapper.createMaterialComponent(materialComponent); | ||
| MaterialHistoryComponent materialHistoryComponent = new MaterialHistoryComponent(); | ||
| materialHistoryComponent.setComponentId(component.getId()); | ||
| materialHistoryComponent.setMaterialHistoryId(1); | ||
| baseMapper.createMaterialHistoryComponent(materialHistoryComponent); | ||
| } | ||
| addNum = addNum + 1; | ||
| } else { |
There was a problem hiding this comment.
Insert counter increments even when insert did not succeed.
addNum is increased regardless of createComponent result, producing incorrect outcome reporting.
🐛 Proposed fix
- if (queryComponent.isEmpty()) {
- // 插入新记录
- Integer result = baseMapper.createComponent(component);
- if (result == 1) {
+ if (queryComponent.isEmpty()) {
+ Integer result = baseMapper.createComponent(component);
+ if (result == 1) {
MaterialComponent materialComponent = new MaterialComponent();
materialComponent.setMaterialId(1);
materialComponent.setComponentId(component.getId());
baseMapper.createMaterialComponent(materialComponent);
MaterialHistoryComponent materialHistoryComponent = new MaterialHistoryComponent();
materialHistoryComponent.setComponentId(component.getId());
materialHistoryComponent.setMaterialHistoryId(1);
baseMapper.createMaterialHistoryComponent(materialHistoryComponent);
+ addNum = addNum + 1;
+ } else {
+ throw new RuntimeException("createComponent failed, result=" + result);
}
- addNum = addNum + 1;
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/src/main/java/com/tinyengine/it/mcp/tools/GitFileReaderService.java`
around lines 358 - 372, The code increments addNum unconditionally even when
baseMapper.createComponent(component) fails; change the logic in
GitFileReaderService around createComponent/component handling so addNum is
incremented only when the insert succeeded (i.e., when Integer result equals 1).
Concretely, move the addNum++ into the if (result == 1) block (or check
Objects.equals(result, 1) to avoid NPE) and only call
createMaterialComponent/createMaterialHistoryComponent when result indicates
success.
Comment on lines
122
to
126
| } else { | ||
| // return processNonStreamWithTools(requestBuilder, request); | ||
|
|
||
| return processStandardResponse(requestBuilder); | ||
| } |
There was a problem hiding this comment.
Non-stream requests with tools currently skip tool-call orchestration.
This branch always calls processStandardResponse, so multi-turn tool execution is bypassed unless stream mode was forcibly toggled earlier.
🐛 Suggested fix
- } else {
- // return processNonStreamWithTools(requestBuilder, request);
-
- return processStandardResponse(requestBuilder);
- }
+ } else {
+ if (request.getTools() != null) {
+ return processNonStreamWithTools(requestBuilder, request);
+ }
+ return processStandardResponse(requestBuilder);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else { | |
| // return processNonStreamWithTools(requestBuilder, request); | |
| return processStandardResponse(requestBuilder); | |
| } | |
| } else { | |
| if (request.getTools() != null) { | |
| return processNonStreamWithTools(requestBuilder, request); | |
| } | |
| return processStandardResponse(requestBuilder); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java`
around lines 122 - 126, The non-stream branch in AiChatV1ServiceImpl currently
always returns processStandardResponse, which skips tool-call orchestration;
change the branch to invoke processNonStreamWithTools(requestBuilder, request)
when tool orchestration is required (i.e., when the requestBuilder/request
indicate tools are present) instead of calling processStandardResponse, ensuring
you preserve any existing fallback to processStandardResponse only when no tools
are needed.
Comment on lines
+405
to
+467
| while (true) { | ||
| originalRequest.setMessages(messagesArray); | ||
| String requestBody = buildRequestBody(originalRequest); | ||
| String apiKey = getApiKey(originalRequest.getApiKey() != null ? originalRequest.getApiKey() : openAIConfig.getApiKey()); | ||
| String baseUrl = originalRequest.getBaseUrl() != null ? originalRequest.getBaseUrl() : openAIConfig.getBaseUrl(); | ||
| requestBuilder = HttpRequest.newBuilder() | ||
| .uri(URI.create(normalizeApiUrl(baseUrl))) | ||
| .header("Content-Type", "application/json") | ||
| .header("Authorization", "Bearer " + apiKey) | ||
| .POST(HttpRequest.BodyPublishers.ofString(requestBody)); | ||
|
|
||
| HttpRequest request = requestBuilder.build(); | ||
| HttpResponse<String> response = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); | ||
|
|
||
| if (response.statusCode() != 200) { | ||
| handleErrorResponse(response); | ||
| return null; | ||
| } | ||
|
|
||
| JsonNode root = objectMapper.readTree(response.body()); | ||
| JsonNode choices = root.get("choices"); | ||
| if (choices == null || choices.isEmpty()) { | ||
| throw new ServiceException("500", "Invalid LLM response: " + response.body()); | ||
| } | ||
| JsonNode messageJson = choices.get(0).get("message"); | ||
| log.info("LLM response message: {}", messageJson); | ||
|
|
||
| ObjectNode assistantMsg = objectMapper.createObjectNode(); | ||
| assistantMsg.put("role", messageJson.get("role").asText()); | ||
| if (messageJson.has("content") && !messageJson.get("content").isNull()) { | ||
| assistantMsg.put("content", messageJson.get("content").asText()); | ||
| } | ||
|
|
||
| JsonNode toolCallsJson = messageJson.get("tool_calls"); | ||
| if (toolCallsJson != null && toolCallsJson.isArray() && toolCallsJson.size() > 0) { | ||
| assistantMsg.set("tool_calls", toolCallsJson); | ||
| } | ||
|
|
||
| messagesArray.add(assistantMsg); | ||
|
|
||
| if (toolCallsJson == null || toolCallsJson.isEmpty()) { | ||
| return assistantMsg.has("content") ? assistantMsg.get("content").asText() : ""; | ||
| } | ||
|
|
||
| // 执行工具调用（仅本地） | ||
| for (JsonNode tc : toolCallsJson) { | ||
| String toolCallId = tc.get("id").asText(); | ||
| JsonNode function = tc.get("function"); | ||
| String toolName = function.get("name").asText(); | ||
| String arguments = function.get("arguments").asText(); | ||
|
|
||
| log.info("执行本地工具: {}，参数: {}", toolName, arguments); | ||
| String toolResult = executeLocalTool(toolName, arguments); | ||
|
|
||
| ObjectNode toolMsg = objectMapper.createObjectNode(); | ||
| toolMsg.put("role", "tool"); | ||
| toolMsg.put("tool_call_id", toolCallId); | ||
| toolMsg.put("content", toolResult); | ||
| messagesArray.add(toolMsg); | ||
| } | ||
|
|
||
| originalRequest.setMessages(messagesArray); | ||
| } |
There was a problem hiding this comment.
Unbounded tool-call loop can run indefinitely.
while (true) has no max-turn guard; repeated tool calls from the model can cause runaway latency/cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java`
around lines 405 - 467, The infinite loop in AiChatV1ServiceImpl surrounding the
tool-call handling (while (true) using originalRequest/messagesArray and
executeLocalTool) can run forever; add a max-turn guard (e.g., int
maxToolCallTurns = 5 or configurable field) and replace the unbounded while with
a loop that increments a counter each iteration, breaking and throwing a
ServiceException (or returning an error) if the counter exceeds the limit;
ensure you update originalRequest.setMessages(messagesArray) each turn as now,
and keep existing checks (tool_calls empty → return) so normal termination still
works.
Comment on lines
+218
to
+224
| <dependency> | ||
| <groupId>io.netty</groupId> | ||
| <artifactId>netty-bom</artifactId> | ||
| <version>${netty.version}</version> | ||
| <type>pom</type> | ||
| <scope>import</scope> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import xml.etree.ElementTree as ET
ns = {'m': 'http://maven.apache.org/POM/4.0.0'}
root = ET.parse('pom.xml').getroot()
deps = root.findall('./m:dependencies/m:dependency', ns)
dm_deps = root.findall('./m:dependencyManagement/m:dependencies/m:dependency', ns)
for label, items in [('dependencies', deps), ('dependencyManagement', dm_deps)]:
for dep in items:
aid = dep.findtext('m:artifactId', namespaces=ns)
if aid == 'netty-bom':
gid = dep.findtext('m:groupId', namespaces=ns)
scope = dep.findtext('m:scope', namespaces=ns)
typ = dep.findtext('m:type', namespaces=ns)
print(f'{label}: {gid}:{aid} scope={scope!r} type={typ!r}')
PYRepository: opentiny/tiny-engine-backend-java
Length of output: 136
🏁 Script executed:
# Check the structure of pom.xml around the problematic dependency and dependencyManagement section
sed -n '1,250p' pom.xml | head -100Repository: opentiny/tiny-engine-backend-java
Length of output: 4158
🏁 Script executed:
# Search for all Netty-related dependencies in pom.xml
grep -n -i "netty" pom.xmlRepository: opentiny/tiny-engine-backend-java
Length of output: 602
🏁 Script executed:
# Verify the dependencyManagement section structure and what's already there
grep -n "dependencyManagement\|</dependencyManagement>" pom.xmlRepository: opentiny/tiny-engine-backend-java
Length of output: 140
🏁 Script executed:
# Get the full context around netty dependencies and dependencyManagement section
sed -n '210,265p' pom.xml | cat -nRepository: opentiny/tiny-engine-backend-java
Length of output: 2318
🏁 Script executed:
# Check the full dependencyManagement section
sed -n '248,261p' pom.xml | cat -nRepository: opentiny/tiny-engine-backend-java
Length of output: 706
🏁 Script executed:
# Verify with a more detailed analysis of netty dependencies
python3 - <<'PY'
import xml.etree.ElementTree as ET
ns = {'m': 'http://maven.apache.org/POM/4.0.0'}
tree = ET.parse('pom.xml')
root = tree.getroot()
print("=== Netty dependencies in <dependencies> ===")
deps = root.findall('./m:dependencies/m:dependency', ns)
for dep in deps:
aid = dep.findtext('m:artifactId', namespaces=ns)
if aid and 'netty' in aid.lower():
gid = dep.findtext('m:groupId', namespaces=ns)
version = dep.findtext('m:version', namespaces=ns)
scope = dep.findtext('m:scope', namespaces=ns) or "compile (default)"
typ = dep.findtext('m:type', namespaces=ns) or "jar (default)"
print(f"{gid}:{aid}")
print(f" version: {version}")
print(f" scope: {scope}")
print(f" type: {typ}")
print()
print("\n=== Netty dependencies in <dependencyManagement> ===")
dm_deps = root.findall('./m:dependencyManagement/m:dependencies/m:dependency', ns)
for dep in dm_deps:
aid = dep.findtext('m:artifactId', namespaces=ns)
if aid and 'netty' in aid.lower():
gid = dep.findtext('m:groupId', namespaces=ns)
version = dep.findtext('m:version', namespaces=ns)
scope = dep.findtext('m:scope', namespaces=ns) or "compile (default)"
typ = dep.findtext('m:type', namespaces=ns) or "jar (default)"
print(f"{gid}:{aid}")
print(f" version: {version}")
print(f" scope: {scope}")
print(f" type: {typ}")
print()
if not any('netty' in dep.findtext('m:artifactId', namespaces=ns, default='').lower() for dep in dm_deps):
print("No Netty dependencies in dependencyManagement")
PYRepository: opentiny/tiny-engine-backend-java
Length of output: 490
Move netty-bom to <dependencyManagement>.
scope=import is only valid inside <dependencyManagement>. Placing it in <dependencies> violates Maven specification and prevents version management from the BOM from being applied. This will cause Maven model validation to fail.
Suggested fix
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-bom</artifactId>
<version>${netty.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-all</artifactId>
</dependency> <dependencyManagement>
<dependencies>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-bom</artifactId>
+ <version>${netty.version}</version>
+ <type>pom</type>
+ <scope>import</scope>
+ </dependency>
<dependency>
<groupId>com.opentiny</groupId>
<artifactId>tiny-engine-app</artifactId>
<version>${tiny-engine.version}</version>
</dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pom.xml` around lines 218 - 224, The netty BOM dependency (artifactId
netty-bom, version ${netty.version}) is currently declared inside <dependencies>
with scope import which is invalid; move that <dependency> entry into the
<dependencyManagement> section so the BOM is imported correctly (create a
<dependencyManagement> element if missing) and remove it from <dependencies> so
the BOM's versions are applied to managed artifacts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java (2)
123-126:⚠️ Potential issue | 🟠 MajorNon-stream tool requests still bypass tool orchestration.
When
stream=false, the code always returnsprocessStandardResponse, so tool-call execution is skipped even ifrequest.getTools()is present.🐛 Proposed fix
- } else { - - return processStandardResponse(requestBuilder); - } + } else { + if (request.getTools() != null) { + return processNonStreamWithTools(requestBuilder, request); + } + return processStandardResponse(requestBuilder); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java` around lines 123 - 126, The else branch currently always returns processStandardResponse(requestBuilder) when stream=false, which skips tool orchestration even if tools are provided; update the AiChatV1ServiceImpl method handling the stream flag to check the request's tools (e.g., requestBuilder.getRequest().getTools() or request.getTools()) when stream is false and, if tools are present, invoke the existing tool orchestration/execution flow (the method that executes tool calls/orchestrates tools in this class) before calling processStandardResponse(requestBuilder) so tool-call execution runs for non-stream requests as well.
404-466:⚠️ Potential issue | 🟠 MajorAdd a max-turn guard to the tool-call loop.
while (true)can run indefinitely if the model keeps emittingtool_calls, causing runaway latency/cost.🐛 Proposed fix
- while (true) { + final int maxToolCallTurns = 5; + int currentTurn = 0; + while (currentTurn++ < maxToolCallTurns) { originalRequest.setMessages(messagesArray); String requestBody = buildRequestBody(originalRequest); String apiKey = getApiKey(originalRequest.getApiKey() != null ? originalRequest.getApiKey() : openAIConfig.getApiKey()); String baseUrl = originalRequest.getBaseUrl() != null ? originalRequest.getBaseUrl() : openAIConfig.getBaseUrl(); @@ originalRequest.setMessages(messagesArray); } + throw new ServiceException("500", "Exceeded max tool-call turns: " + maxToolCallTurns);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java` around lines 404 - 466, The infinite while (true) loop in AiChatV1ServiceImpl that repeatedly processes tool_calls can run forever; add a max-turn guard (e.g., MAX_TOOL_CALL_TURNS constant) and a counter (e.g., int turn = 0) before/inside the loop, increment it each iteration and break/throw a ServiceException when exceeded to prevent runaway calls; apply the check around the loop that builds/sends requests (where originalRequest, messagesArray, buildRequestBody, httpClient.send, and executeLocalTool are used) so you stop processing tool_calls after the configured maximum and return a clear error or final content.
🧹 Nitpick comments (1)
base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java (1)
245-247: Consolidate duplicated local tool schema converters.
convertLocalToolToSchemaandconvertToolToSchemado the same thing; keeping both invites divergence.♻️ Proposed simplification
- toolsArray.add(convertToolToSchema(localDef)); + toolsArray.add(convertLocalToolToSchema(localDef)); @@ - /** - * 将本地工具定义转换为 OpenAI 函数格式 - */ - private ObjectNode convertToolToSchema(MCPToolDefinition def) { - ObjectNode toolNode = objectMapper.createObjectNode(); - toolNode.put("type", "function"); - - ObjectNode function = objectMapper.createObjectNode(); - function.put("name", def.getName()); - function.put("description", def.getDescription()); - function.set("parameters", def.getInputSchema()); - - toolNode.set("function", function); - return toolNode; - }Also applies to: 318-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java` around lines 245 - 247, There are two identical converters (convertLocalToolToSchema and convertToolToSchema) causing duplication; remove one and consolidate callers to the single remaining method (e.g., keep convertToolToSchema), update all references in AiChatV1ServiceImpl (the usage around the toolRegistry getTool branch and the other block at ~318-364) to call the retained method, delete the duplicate method implementation, and verify any helper methods it relies on remain accessible and tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java`:
- Around line 112-113: The code is printing raw model/tool outputs
(System.out.println("Final answer: " + finalAnswer) and writing raw
finalAnswer/messageJson payloads) which can leak sensitive data; update
AiChatV1ServiceImpl to stop emitting raw content to stdout or logs and instead
either remove these prints entirely or replace them with redacted/safe logging
at an appropriate level (use the project logger, e.g., logger.debug) that logs
only non-sensitive metadata (length, truncated hash, or "REDACTED") and keep the
outputStream.write behavior only for the client response (ensure any server-side
logs do not include finalAnswer or messageJson). Locate uses of
System.out.println and any writes referencing finalAnswer or messageJson and
replace with logger calls that reference sanitized helpers (e.g.,
redact(payload) or logPayloadMetadata(payload.length(), payloadHash)) or remove
them.
---
Duplicate comments:
In
`@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java`:
- Around line 123-126: The else branch currently always returns
processStandardResponse(requestBuilder) when stream=false, which skips tool
orchestration even if tools are provided; update the AiChatV1ServiceImpl method
handling the stream flag to check the request's tools (e.g.,
requestBuilder.getRequest().getTools() or request.getTools()) when stream is
false and, if tools are present, invoke the existing tool
orchestration/execution flow (the method that executes tool calls/orchestrates
tools in this class) before calling processStandardResponse(requestBuilder) so
tool-call execution runs for non-stream requests as well.
- Around line 404-466: The infinite while (true) loop in AiChatV1ServiceImpl
that repeatedly processes tool_calls can run forever; add a max-turn guard
(e.g., MAX_TOOL_CALL_TURNS constant) and a counter (e.g., int turn = 0)
before/inside the loop, increment it each iteration and break/throw a
ServiceException when exceeded to prevent runaway calls; apply the check around
the loop that builds/sends requests (where originalRequest, messagesArray,
buildRequestBody, httpClient.send, and executeLocalTool are used) so you stop
processing tool_calls after the configured maximum and return a clear error or
final content.
---
Nitpick comments:
In
`@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java`:
- Around line 245-247: There are two identical converters
(convertLocalToolToSchema and convertToolToSchema) causing duplication; remove
one and consolidate callers to the single remaining method (e.g., keep
convertToolToSchema), update all references in AiChatV1ServiceImpl (the usage
around the toolRegistry getTool branch and the other block at ~318-364) to call
the retained method, delete the duplicate method implementation, and verify any
helper methods it relies on remain accessible and tested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 177ca1ef-d74b-4d9f-986e-d26fd0fcb7e3
📒 Files selected for processing (3)
app/src/main/resources/application-alpha.ymlbase/src/main/java/com/tinyengine/it/mcp/config/McpToolsConfig.javabase/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (2)
- base/src/main/java/com/tinyengine/it/mcp/config/McpToolsConfig.java
- app/src/main/resources/application-alpha.yml
Comment on lines
+112
to
+113
| System.out.println("Final answer: " + finalAnswer); | ||
| outputStream.write(("data: " + finalAnswer + "\n\n").getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Avoid logging raw model outputs/tool payloads in production paths.
Printing finalAnswer and full messageJson can leak sensitive user/tool data into logs.
🔒 Suggested hardening
- System.out.println("Final answer: " + finalAnswer);
+ log.debug("Final answer generated for stream-with-tools path");
outputStream.write(("data: " + finalAnswer + "\n\n").getBytes(StandardCharsets.UTF_8));
@@
- log.info("LLM response message: {}", messageJson);
+ log.debug("LLM response received: role={}, hasToolCalls={}",
+ messageJson.path("role").asText(),
+ messageJson.has("tool_calls"));Also applies to: 429-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@base/src/main/java/com/tinyengine/it/service/app/impl/v1/AiChatV1ServiceImpl.java`
around lines 112 - 113, The code is printing raw model/tool outputs
(System.out.println("Final answer: " + finalAnswer) and writing raw
finalAnswer/messageJson payloads) which can leak sensitive data; update
AiChatV1ServiceImpl to stop emitting raw content to stdout or logs and instead
either remove these prints entirely or replace them with redacted/safe logging
at an appropriate level (use the project logger, e.g., logger.debug) that logs
only non-sensitive metadata (length, truncated hash, or "REDACTED") and keep the
outputStream.write behavior only for the client response (ensure any server-side
logs do not include finalAnswer or messageJson). Locate uses of
System.out.println and any writes referencing finalAnswer or messageJson and
replace with logger calls that reference sanitized helpers (e.g.,
redact(payload) or logPayloadMetadata(payload.length(), payloadHash)) or remove
them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Chores