Feat/tool alias support#1
Conversation
feat(agentic_model): - format print - support agentic chat template - support to compose agentic odel&agentic tools node - support agentic tool node - support agentic message concat
cloudwego#882) feat(adk): add agentmd middleware for auto-injecting Agents.md into model input Change-Id: I34add4f925a23c6d6821925c482a21f6cddfddd4
…wego#847) * fix(adk): preserve multimodal content fields in rewriteMessage When rewriteMessage rewrites a ChatModelAgent's output as a User message for history context, it now carries over MultiContent, UserInputMultiContent, and converts AssistantGenMultiContent to UserInputMultiContent (text/image/ audio/video parts). Reasoning parts are dropped as they have no user input equivalent. Change-Id: If645454287b3fb5da0634b71b2e2eed7a3692c08 * fix(adk): use composite literal for slice copy in rewriteMessage Change-Id: Ib433159ec54876d295e6c43544f4cae2f3b1789e * fix(adk): preserve nil slice semantics in rewriteMessage Change-Id: I90080a89c96ab34f0620c2cef011ab6f7e270973
…ling in ConcatMessages (cloudwego#886) ConcatMessages merges TokenUsage fields (PromptTokens, CompletionTokens, TotalTokens, PromptTokenDetails.CachedTokens) but omits CompletionTokensDetails.ReasoningTokens, causing reasoning token counts to be lost when concatenating streamed messages. Made-with: Cursor
- Add NameAliases and ParamAliases fields to schema.ToolInfo - Route tool calls by alias name to real tool transparently - Remap aliased param keys in JSON args before tool invocation - Real key takes precedence when both real and alias keys present - Detect conflicts between aliases and existing tool/param names - Reject dot-path notation in alias keys (reserved for future use) - Use sonic for JSON marshal/unmarshal in alias remapping - Add stream path test for alias tool name preservation - Fix typo "failed to executed" -> "failed to execute" - Rename allToolNames -> realToolNames for clarity - Add tool name context to param schema parse errors - Add WithNameAliases and WithParamAliases options to tool utils Infer* functions, so aliases can be set when creating tools via goStruct2ToolInfo - Add NameAliases and ParamAliases fields to filesystem ToolConfig - Add ExecuteToolConfig to both Config and MiddlewareConfig for consistent execute tool configuration across old and new APIs - Refactor execute tool into toolSpec-based flow with conditional append when Shell/StreamingShell is available - Pass alias options through createToolFromSpec to all tool constructors - Add tests for alias propagation on filesystem and execute tools
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughIntroduces agentic message support and tool aliasing throughout the system while adding checkpoint format migration for legacy state versions. Adds new agentic-specific model, template, and tools components alongside extended schema types for OpenAI, Claude, and Gemini providers. Refactors state field exports and checkpoint serialization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
codeant-ai
Bot
added
the
size:XXL
Sequence DiagramThis PR adds alias support for tool names and tool parameters, and wires it through middleware configuration into tool execution. It enables models to call tools with alternate names while still executing the canonical tool with normalized arguments. sequenceDiagram
participant App
participant Middleware
participant ToolsNode
participant Model
participant Tool
App->>Middleware: Configure tool name and parameter aliases
Middleware->>ToolsNode: Register tools with alias metadata
Model->>ToolsNode: Invoke tool using alias name and alias params
ToolsNode->>ToolsNode: Resolve canonical tool and remap params
ToolsNode->>Tool: Execute canonical tool with normalized args
ToolsNode-->>Model: Return tool result using original called name
Generated by CodeAnt AI |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the framework's capabilities by introducing robust tool alias support and a new agentic message format for multi-modal interactions. It also ensures backward compatibility for checkpoints and integrates a new middleware for dynamic content injection, alongside extending the callback system for better observability of agentic components. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Nitpicks 🔍
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant and valuable features, including support for agentic message formats, tool aliasing, and backward compatibility for checkpoints. The changes are extensive but well-structured, with comprehensive test coverage that inspires confidence. The new agentsmd middleware is also a great addition for dynamic instruction loading. I've found one minor issue in the concatenation logic for agentic message extensions, but overall, this is an excellent contribution that greatly enhances the framework's capabilities.
| if extensions.IsValid() && !extensions.IsZero() { | ||
| ret.Extension, err = internal.ConcatSliceValue(extensions) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ret.Extension = extensions.Interface() | ||
| } |
There was a problem hiding this comment.
There appears to be a logic error in how the concatenated extensions are assigned. The result of internal.ConcatSliceValue(extensions) is assigned to ret.Extension, but then immediately overwritten by extensions.Interface(), which is the original slice of extensions, not the concatenated one. This will lead to incorrect data when streaming assistant-generated text with custom extensions.
The implementation in concatAgenticResponseMeta seems correct and could be used as a reference.
| if extensions.IsValid() && !extensions.IsZero() { | |
| ret.Extension, err = internal.ConcatSliceValue(extensions) | |
| if err != nil { | |
| return nil, err | |
| } | |
| ret.Extension = extensions.Interface() | |
| } | |
| if extensions.IsValid() && !extensions.IsZero() { | |
| concatenated, err := internal.ConcatSliceValue(extensions) | |
| if err != nil { | |
| return nil, err | |
| } | |
| ret.Extension = concatenated.Interface() | |
| } |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| // Try to get cached content from RunLocalValue. | ||
| if cached, found, err := adk.GetRunLocalValue(ctx, agentsMDCacheKey); err == nil && found { | ||
| if s, ok := cached.(string); ok { | ||
| content = s | ||
| } | ||
| } | ||
|
|
||
| if content == "" { | ||
| var err error | ||
| content, err = m.loader.load(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("[agentsmd]: failed to load agent files: %w", err) | ||
| } | ||
| // Cache the loaded content for subsequent model calls in this Run(). | ||
| if content != "" { | ||
| _ = adk.SetRunLocalValue(ctx, agentsMDCacheKey, content) | ||
| } |
There was a problem hiding this comment.
Suggestion: Empty Agents.md content is never cached, so every model call in the same run reloads files again and re-triggers warnings. This creates repeated I/O and inconsistent per-run behavior when files are missing or empty. Track cache hits separately from content value and cache empty-string results too. [logic error]
Severity Level: Major ⚠️
- ⚠️ Repeated file reads during multi-step agent runs.
- ⚠️ Duplicate missing-file warnings flood run logs.
- ⚠️ Empty Agents.md still incurs per-call overhead.| // Try to get cached content from RunLocalValue. | |
| if cached, found, err := adk.GetRunLocalValue(ctx, agentsMDCacheKey); err == nil && found { | |
| if s, ok := cached.(string); ok { | |
| content = s | |
| } | |
| } | |
| if content == "" { | |
| var err error | |
| content, err = m.loader.load(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("[agentsmd]: failed to load agent files: %w", err) | |
| } | |
| // Cache the loaded content for subsequent model calls in this Run(). | |
| if content != "" { | |
| _ = adk.SetRunLocalValue(ctx, agentsMDCacheKey, content) | |
| } | |
| cacheHit := false | |
| // Try to get cached content from RunLocalValue. | |
| if cached, found, err := adk.GetRunLocalValue(ctx, agentsMDCacheKey); err == nil && found { | |
| if s, ok := cached.(string); ok { | |
| content = s | |
| cacheHit = true | |
| } | |
| } | |
| if !cacheHit { | |
| var err error | |
| content, err = m.loader.load(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("[agentsmd]: failed to load agent files: %w", err) | |
| } | |
| // Cache the loaded content (including empty) for subsequent model calls in this Run(). | |
| _ = adk.SetRunLocalValue(ctx, agentsMDCacheKey, content) |
Steps of Reproduction ✅
1. Create/run a `ChatModelAgent` with `agentsmd.New(...)` middleware and at least one tool
so one Run can perform multiple model calls (multi-call behavior is verified in
`adk/handler_test.go:1584-1623`, `RunLocalValuePersistsAcrossModelCalls`).
2. Start execution via `agent.Run(...)` (`adk/chatmodel.go:952`) with `AgentsMDFiles`
pointing to missing/empty files; loader returns empty content
(`adk/middlewares/agentsmd/loader.go:130,174-176`) and can emit warnings for missing files
(`loader.go:160-162`).
3. During each model invocation, the wrapper chain calls middleware `WrapModel` again
(`adk/wrappers.go:48-53`, `96-101`), which reaches `prependAgentMD`.
4. In `prependAgentMD`, empty content is never cached because `SetRunLocalValue` is
guarded by `if content != ""` (`adk/middlewares/agentsmd/agentsmd.go:137-139`), so
subsequent model calls in the same Run re-execute `m.loader.load(ctx)`
(`agentsmd.go:130-136`), repeating file I/O and warning callbacks.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** adk/middlewares/agentsmd/agentsmd.go
**Line:** 122:139
**Comment:**
*Logic Error: Empty Agents.md content is never cached, so every model call in the same run reloads files again and re-triggers warnings. This creates repeated I/O and inconsistent per-run behavior when files are missing or empty. Track cache hits separately from content value and cache empty-string results too.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| // A post-match filter further requires the path to contain "/" or end with | ||
| // an allowed extension (see allowedImportExts), so bare words like @someone | ||
| // and email-like patterns like @example.com are ignored. | ||
| var importRegex = regexp.MustCompile(`@([a-zA-Z0-9_.~/][a-zA-Z0-9_.~/\-]*)`) |
There was a problem hiding this comment.
Suggestion: The import regex greedily includes trailing dots, so text like @sub/rules.md. is parsed as sub/rules.md. and the import fails with a false missing-file warning. Tighten the regex so matched paths cannot end with .. [logic error]
Severity Level: Major ⚠️
- ⚠️ Sentence-final imports resolve to wrong filenames.
- ❌ Imported instruction sections silently missing.
- ⚠️ Extra false "file not found" load warnings.| var importRegex = regexp.MustCompile(`@([a-zA-Z0-9_.~/][a-zA-Z0-9_.~/\-]*)`) | |
| var importRegex = regexp.MustCompile(`@([a-zA-Z0-9_.~/](?:[a-zA-Z0-9_.~/\-]*[a-zA-Z0-9_~/\-])?)`) |
Steps of Reproduction ✅
1. Middleware call path is real and hot: `Generate/Stream` in
`adk/middlewares/agentsmd/agentsmd.go:98-112` -> `prependAgentMD` (`:120-143`) ->
`loader.load()` (`:132`) -> `collectImports()` (`loader.go:179`, `:197-233`).
2. Put a host file content like `"... @sub/rules.md."` (sentence-ending punctuation),
which is consistent with existing prose-style import tests in
`adk/middlewares/agentsmd/agentsmd_test.go:445` and `:516` (imports embedded in
sentences).
3. Regex at `loader.go:39` captures trailing `.` because `.` is allowed in the repeated
class; `collectImports` then treats it as import (`:201-208`, slash check passes).
4. Loader attempts `Read` on joined path ending with dot (`:217-225`), missing file is
downgraded to warning at `loadFile` (`:160-163`), and intended imported file is skipped
from formatted output.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** adk/middlewares/agentsmd/loader.go
**Line:** 39:39
**Comment:**
*Logic Error: The import regex greedily includes trailing dots, so text like `@sub/rules.md.` is parsed as `sub/rules.md.` and the import fails with a false missing-file warning. Tighten the regex so matched paths cannot end with `.`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| visited[filePath] = true | ||
| defer delete(visited, filePath) | ||
|
|
||
| fileContent, err := l.backend.Read(ctx, &ReadRequest{FilePath: filePath, Offset: 1}) |
There was a problem hiding this comment.
Suggestion: The file read call does not set Limit, so backends that apply a default line cap (2000 in the in-memory backend) will silently truncate large instruction files. This breaks the middleware contract that each file is loaded in full. [logic error]
Severity Level: Critical 🚨
- ⚠️ Agents.md instructions truncated above 2000 lines.
- ❌ Model receives incomplete guidance in Generate/Stream.
- ⚠️ Violates "file always loaded in full" contract.| fileContent, err := l.backend.Read(ctx, &ReadRequest{FilePath: filePath, Offset: 1}) | |
| fileContent, err := l.backend.Read(ctx, &ReadRequest{FilePath: filePath, Offset: 1, Limit: int(^uint(0) >> 1)}) |
Steps of Reproduction ✅
1. Configure this middleware via `agentsmd.New()` in
`adk/middlewares/agentsmd/agentsmd.go:65-73`, then trigger any model call (`Generate` at
`:98-104` or `Stream` at `:106-112`), which always enters `prependAgentMD` (`:120-143`)
and calls `m.loader.load(ctx)` at `:132`.
2. In `adk/middlewares/agentsmd/loader.go:158`, `loadFile()` reads with
`ReadRequest{FilePath, Offset:1}` but no `Limit`; backend contract in
`adk/filesystem/backend.go:73-76` states non-positive/omitted limit defaults to 2000
lines.
3. When backend is `InMemoryBackend` (`adk/filesystem/backend_inmemory.go:136-155`),
`limit` becomes 2000 if unset (`:153-155`), so files over 2000 lines are truncated
silently.
4. Loader continues formatting truncated content (`loader.go:186-188`, `:265-298`), so
injected system reminder content is incomplete without an error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** adk/middlewares/agentsmd/loader.go
**Line:** 158:158
**Comment:**
*Logic Error: The file read call does not set `Limit`, so backends that apply a default line cap (2000 in the in-memory backend) will silently truncate large instruction files. This breaks the middleware contract that each file is loaded in full.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| } | ||
|
|
||
| func (c *ExecuteToolConfig) toToolConfig() *ToolConfig { | ||
| if c == nil { | ||
| return nil | ||
| } | ||
| return &ToolConfig{ | ||
| Name: c.Name, | ||
| Desc: c.Desc, | ||
| NameAliases: c.NameAliases, | ||
| ParamAliases: c.ParamAliases, |
There was a problem hiding this comment.
Suggestion: ExecuteToolConfig only carries name/description/alias fields, and toToolConfig() drops Disable and CustomTool. This makes execute impossible to disable or override via config, unlike other tools, which can unintentionally expose command execution when Shell is set. Add parity fields and copy them through conversion so execute follows the same control contract as other tools. [security]
Severity Level: Major ⚠️
- ❌ Execute tool always registers when shell is configured.
- ⚠️ Cannot provide safer custom execute implementation.
- ⚠️ Tool-level disable policy cannot include execute.| } | |
| func (c *ExecuteToolConfig) toToolConfig() *ToolConfig { | |
| if c == nil { | |
| return nil | |
| } | |
| return &ToolConfig{ | |
| Name: c.Name, | |
| Desc: c.Desc, | |
| NameAliases: c.NameAliases, | |
| ParamAliases: c.ParamAliases, | |
| // CustomTool provides a custom implementation for this tool. | |
| // optional | |
| CustomTool tool.BaseTool | |
| // Disable disables this tool. | |
| // optional, false by default | |
| Disable bool | |
| } | |
| func (c *ExecuteToolConfig) toToolConfig() *ToolConfig { | |
| if c == nil { | |
| return nil | |
| } | |
| return &ToolConfig{ | |
| Name: c.Name, | |
| Desc: c.Desc, | |
| NameAliases: c.NameAliases, | |
| ParamAliases: c.ParamAliases, | |
| CustomTool: c.CustomTool, | |
| Disable: c.Disable, |
Steps of Reproduction ✅
1. Configure middleware through exported entrypoint `New` in
`adk/middlewares/filesystem/filesystem.go:386-404` with `Shell` set (or `StreamingShell`)
so shell capability is enabled.
2. `getFilesystemTools` in `adk/middlewares/filesystem/filesystem.go:437-525` always
appends execute tool spec when shell exists (`:503-513`), then routes creation through
`createToolFromSpec`.
3. `createToolFromSpec` only suppresses a tool when `mergedConfig.Disable` is true
(`adk/middlewares/filesystem/filesystem.go:534-536`), but `ExecuteToolConfig.toToolConfig`
(`:102-111`) never provides `Disable` or `CustomTool`.
4. As a result, execute cannot be disabled/overridden via config and remains registered;
this behavior is consistent with tests showing execute appears whenever shell is present
(`adk/middlewares/filesystem/filesystem_test.go:1626-1655`) while "disable all tools" only
covers non-execute configs (`:1568-1580`).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** adk/middlewares/filesystem/filesystem.go
**Line:** 100:110
**Comment:**
*Security: `ExecuteToolConfig` only carries name/description/alias fields, and `toToolConfig()` drops `Disable` and `CustomTool`. This makes `execute` impossible to disable or override via config, unlike other tools, which can unintentionally expose command execution when `Shell` is set. Add parity fields and copy them through conversion so execute follows the same control contract as other tools.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| } | ||
|
|
||
| func (t *interruptingSubAgentTool) InvokableRun(ctx context.Context, argumentsInJSON string, _ ...tool.Option) (string, error) { | ||
| _, _, _ = tool.GetInterruptState[string](ctx) |
There was a problem hiding this comment.
Suggestion: The tool implementation ignores the GetInterruptState result, so this compatibility test can still pass even when resume state is not restored correctly. Make the tool fail/re-interrupt when it is executed without resumed interrupt state, so the test actually validates checkpoint resume behavior. [logic error]
Severity Level: Critical 🚨
- ⚠️ Resume-compat test misses interrupt-state restoration regressions.
- ❌ Backward-compat checkpoint bugs can slip into releases.| _, _, _ = tool.GetInterruptState[string](ctx) | |
| wasInterrupted, _, _ := tool.GetInterruptState[string](ctx) | |
| if !wasInterrupted { | |
| return "", tool.StatefulInterrupt(ctx, "expected resumed state", argumentsInJSON) | |
| } |
Steps of Reproduction ✅
1. Check fixture generation flow at
`adk/prebuilt/deep/testdata/_gen/generate_test.go:72-77`: the original `interruptTool`
interrupts on first run and only returns `"resumed"` when `wasInterrupted` is true.
2. Run this PR test flow `TestDeepAgentCheckpointCompat_V0_8_Resume` ->
`runDeepAgentCheckpointCompat`
(`adk/prebuilt/deep/checkpoint_compat_resume_test.go:170-201`, `:89-168`) which calls
`runner.Resume(...)` at `:146`.
3. During resume execution, the test tool `interruptingSubAgentTool.InvokableRun`
(`:73-76`) ignores `GetInterruptState` and always returns success.
4. If resume-state restoration regresses in checkpoint loading (`adk/runner.go:149-152`
path), this tool still reports success, so assertions at
`checkpoint_compat_resume_test.go:166-167` can pass without actually validating resumed
interrupt state.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** adk/prebuilt/deep/checkpoint_compat_resume_test.go
**Line:** 74:74
**Comment:**
*Logic Error: The tool implementation ignores the `GetInterruptState` result, so this compatibility test can still pass even when resume state is not restored correctly. Make the tool fail/re-interrupt when it is executed without resumed interrupt state, so the test actually validates checkpoint resume behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| sb.WriteString(fmt.Sprintf(" name: %s\n", m.Name)) | ||
| sb.WriteString(fmt.Sprintf(" result: %s\n", m.Result)) | ||
| if m.Error != nil { | ||
| sb.WriteString(fmt.Sprintf(" error: [%d] %s\n", *m.Error.Code, m.Error.Message)) |
There was a problem hiding this comment.
Suggestion: The string formatter dereferences Error.Code without checking for nil, but Code is optional and can be absent. Guard the pointer before dereferencing to avoid panics in logging/printing paths. [null pointer]
Severity Level: Critical 🚨
- ❌ String rendering can panic on optional error code.
- ⚠️ Debug/logging paths crash MCP error inspection.| sb.WriteString(fmt.Sprintf(" error: [%d] %s\n", *m.Error.Code, m.Error.Message)) | |
| if m.Error.Code != nil { | |
| sb.WriteString(fmt.Sprintf(" error: [%d] %s\n", *m.Error.Code, m.Error.Message)) | |
| } else { | |
| sb.WriteString(fmt.Sprintf(" error: %s\n", m.Error.Message)) | |
| } |
Steps of Reproduction ✅
1. Follow the same string rendering path used by tests
(`schema/agentic_message_test.go:1380` calls `msg.String()`).
2. Construct a message with `MCPToolResult.Error` set but `Error.Code == nil` (valid by
schema because `Code` is optional pointer at `schema/agentic_message.go:367`).
3. `msg.String()` dispatches to `MCPToolResult.String()`
(`schema/agentic_message.go:1976+`).
4. At `schema/agentic_message.go:1982`, `*m.Error.Code` is dereferenced unguarded, causing
panic during logging/debug rendering.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** schema/agentic_message.go
**Line:** 1982:1982
**Comment:**
*Null Pointer: The string formatter dereferences `Error.Code` without checking for nil, but `Code` is optional and can be absent. Guard the pointer before dereferencing to avoid panics in logging/printing paths.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| continue | ||
| } | ||
| if ret.Refusal == nil { | ||
| ret.Refusal = ext.Refusal |
There was a problem hiding this comment.
Suggestion: Assigning ret.Refusal = ext.Refusal aliases the input object, and later concatenation mutates the original chunk's refusal text. This creates unintended side effects and can corrupt caller-visible state. Copy the refusal value when initializing the result. [logic error]
Severity Level: Major ⚠️
- ⚠️ Caller-owned chunk data mutated during message concatenation.
- ⚠️ Reusing source chunks yields unexpected refusal text.| ret.Refusal = ext.Refusal | |
| ret.Refusal = &OutputRefusal{Reason: ext.Refusal.Reason} |
Steps of Reproduction ✅
1. Use `schema.ConcatAgenticMessages` (`/workspace/eino/schema/agentic_message.go:768`)
with streaming assistant text chunks containing OpenAI refusals; this is the real merge
path.
2. `concatAssistantGenTexts` collects OpenAI extensions
(`/workspace/eino/schema/agentic_message.go:1269-1275`) and calls
`openai.ConcatAssistantGenTextExtensions` (`:1297`).
3. In `openai.ConcatAssistantGenTextExtensions`, first refusal pointer is reused
(`ret.Refusal = ext.Refusal` at `/workspace/eino/schema/openai/extension.go:161`), then
mutated (`ret.Refusal.Reason += ...` at `:163`).
4. Observe caller-owned first chunk's `Refusal.Reason` is modified as a side effect after
concatenation, causing state corruption if input chunks are reused/logged later.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** schema/openai/extension.go
**Line:** 161:161
**Comment:**
*Logic Error: Assigning `ret.Refusal = ext.Refusal` aliases the input object, and later concatenation mutates the original chunk's refusal text. This creates unintended side effects and can corrupt caller-visible state. Copy the refusal value when initializing the result.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| case components.ComponentOfPrompt: | ||
| return c.promptHandler.OnStart(ctx, info, prompt.ConvCallbackInput(input)) | ||
| case components.ComponentOfAgenticPrompt: | ||
| return c.agenticPromptHandler.OnStart(ctx, info, prompt.ConvCallbackInput(input)) |
There was a problem hiding this comment.
Suggestion: The agentic prompt OnStart path uses prompt.ConvCallbackInput, which does not handle *prompt.AgenticCallbackInput and returns nil for normal agentic prompt events. This makes callback handlers receive empty input unexpectedly. Use the agentic converter instead. [logic error]
Severity Level: Critical 🚨
- ❌ Agentic prompt OnStart receives nil instead of real input.
- ⚠️ Callback logic may panic on input dereference.| return c.agenticPromptHandler.OnStart(ctx, info, prompt.ConvCallbackInput(input)) | |
| return c.agenticPromptHandler.OnStart(ctx, info, prompt.ConvAgenticCallbackInput(input)) |
Steps of Reproduction ✅
1. Build a handler with `AgenticPrompt` and invoke agentic template `Format`
(`components/prompt/agentic_chat_template.go:46-51`), which sends
`&prompt.AgenticCallbackInput{...}` to `callbacks.OnStart`.
2. Dispatcher path (`callbacks/aspect_inject.go:74-77` ->
`internal/callbacks/inject.go:107-112`) reaches `handlerTemplate.OnStart` for
`components.ComponentOfAgenticPrompt` (`utils/callbacks/template.go:185-186`).
3. Current code calls `prompt.ConvCallbackInput(input)` at line 186.
4. `prompt.ConvCallbackInput` (`components/prompt/callback_extra.go:45-55`) only handles
`*prompt.CallbackInput` or `map[string]any`; it does not handle
`*prompt.AgenticCallbackInput`, so it returns `nil`, and the callback receives empty
input.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** utils/callbacks/template.go
**Line:** 186:186
**Comment:**
*Logic Error: The agentic prompt `OnStart` path uses `prompt.ConvCallbackInput`, which does not handle `*prompt.AgenticCallbackInput` and returns `nil` for normal agentic prompt events. This makes callback handlers receive empty input unexpectedly. Use the agentic converter instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| case components.ComponentOfPrompt: | ||
| return c.promptHandler.OnEnd(ctx, info, prompt.ConvCallbackOutput(output)) | ||
| case components.ComponentOfAgenticPrompt: | ||
| return c.agenticPromptHandler.OnEnd(ctx, info, prompt.ConvCallbackOutput(output)) |
There was a problem hiding this comment.
Suggestion: The agentic prompt OnEnd path uses prompt.ConvCallbackOutput, which does not handle *prompt.AgenticCallbackOutput or []*schema.AgenticMessage, so agentic prompt completion data is converted to nil. Switch to the agentic output converter. [logic error]
Severity Level: Critical 🚨
- ❌ Agentic prompt OnEnd loses generated agentic messages.
- ⚠️ Observability callbacks miss completion output data.| return c.agenticPromptHandler.OnEnd(ctx, info, prompt.ConvCallbackOutput(output)) | |
| return c.agenticPromptHandler.OnEnd(ctx, info, prompt.ConvAgenticCallbackOutput(output)) |
Steps of Reproduction ✅
1. Run `(*DefaultAgenticChatTemplate).Format`
(`components/prompt/agentic_chat_template.go:68-71`), which emits `callbacks.OnEnd` with
`&prompt.AgenticCallbackOutput{Result: []*schema.AgenticMessage...}`.
2. Through callback dispatch (`callbacks/aspect_inject.go:86-89` ->
`internal/callbacks/inject.go:117-122`), template handler `OnEnd` handles
`components.ComponentOfAgenticPrompt` (`utils/callbacks/template.go:224-225`).
3. Code converts output using `prompt.ConvCallbackOutput(output)` at line 225.
4. `prompt.ConvCallbackOutput` (`components/prompt/callback_extra.go:59-69`) only supports
`*prompt.CallbackOutput` or `[]*schema.Message`; it does not support
`*prompt.AgenticCallbackOutput` / `[]*schema.AgenticMessage`, so conversion returns `nil`
and end payload is dropped.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** utils/callbacks/template.go
**Line:** 225:225
**Comment:**
*Logic Error: The agentic prompt `OnEnd` path uses `prompt.ConvCallbackOutput`, which does not handle `*prompt.AgenticCallbackOutput` or `[]*schema.AgenticMessage`, so agentic prompt completion data is converted to `nil`. Switch to the agentic output converter.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| OnStart func(ctx context.Context, runInfo *callbacks.RunInfo, input *prompt.CallbackInput) context.Context | ||
| // OnEnd is the callback function for the end of the agentic prompt. | ||
| OnEnd func(ctx context.Context, runInfo *callbacks.RunInfo, output *prompt.CallbackOutput) context.Context |
There was a problem hiding this comment.
Suggestion: The agentic prompt handler is declared with non-agentic prompt types, so it cannot receive the actual *prompt.AgenticCallbackInput and *prompt.AgenticCallbackOutput emitted by agentic prompt components. This breaks the callback contract and causes handlers to lose typed data. Change the handler field types to the agentic prompt callback types. [type error]
Severity Level: Major ⚠️
- ⚠️ Agentic prompt callbacks cannot expose agentic typed payloads.
- ⚠️ Handler authors lose templates/result type safety.| OnStart func(ctx context.Context, runInfo *callbacks.RunInfo, input *prompt.CallbackInput) context.Context | |
| // OnEnd is the callback function for the end of the agentic prompt. | |
| OnEnd func(ctx context.Context, runInfo *callbacks.RunInfo, output *prompt.CallbackOutput) context.Context | |
| OnStart func(ctx context.Context, runInfo *callbacks.RunInfo, input *prompt.AgenticCallbackInput) context.Context | |
| // OnEnd is the callback function for the end of the agentic prompt. | |
| OnEnd func(ctx context.Context, runInfo *callbacks.RunInfo, output *prompt.AgenticCallbackOutput) context.Context |
Steps of Reproduction ✅
1. Register an agentic prompt callback using
`template.NewHandlerHelper().AgenticPrompt(...)` in `utils/callbacks/template.go:134-137`,
where handler function currently accepts `*prompt.CallbackInput` /
`*prompt.CallbackOutput` (`utils/callbacks/template.go:666,669`).
2. Execute an agentic prompt through `(*DefaultAgenticChatTemplate).Format`
(`components/prompt/agentic_chat_template.go:46-73`), which emits `callbacks.OnStart(...,
&AgenticCallbackInput{...})` at line 48 and `callbacks.OnEnd(...,
&AgenticCallbackOutput{...})` at line 68.
3. Callback dispatch path (`callbacks/aspect_inject.go:74-87` ->
`internal/callbacks/inject.go:107-122`) invokes template handler `OnStart/OnEnd` for
`components.ComponentOfAgenticPrompt` (`utils/callbacks/template.go:185-186,224-225`).
4. Because `AgenticPromptCallbackHandler` is typed to non-agentic prompt structs,
agentic-specific fields (`Templates []schema.AgenticMessagesTemplate`, `Result
[]*schema.AgenticMessage` defined in `components/prompt/agentic_callback_extra.go:29,37`)
are not representable in handler signatures; agentic callback contract is mismatched and
typed payload is lost.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** utils/callbacks/template.go
**Line:** 666:668
**Comment:**
*Type Error: The agentic prompt handler is declared with non-agentic prompt types, so it cannot receive the actual `*prompt.AgenticCallbackInput` and `*prompt.AgenticCallbackOutput` emitted by agentic prompt components. This breaks the callback contract and causes handlers to lose typed data. Change the handler field types to the agentic prompt callback types.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
schema/openai/extension_test.go (1)
73-193: Consider adding a test for empty input error case.The implementation returns an error when
chunksis empty, but there's no test case verifying this behavior.💡 Suggested test case
t.Run("empty input - returns error", func(t *testing.T) { _, err := ConcatAssistantGenTextExtensions([]*AssistantGenTextExtension{}) assert.Error(t, err) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schema/openai/extension_test.go` around lines 73 - 193, Add a unit test that verifies ConcatAssistantGenTextExtensions returns an error when given an empty slice; the missing case is that the implementation errors on empty chunks, so add a new subtest (e.g., t.Run("empty input - returns error", ...)) that calls ConcatAssistantGenTextExtensions([]*AssistantGenTextExtension{}) and asserts an error is returned to cover this behavior.compose/checkpoint.go (1)
246-268: Potential nil pointer dereference in recursive call.The
migrateCheckpointfunction doesn't check ifcpis nil before accessingcp.Stateandcp.SubGraphs. While the initial call fromMigrateCheckpointStatepasses a non-nil checkpoint, the recursive iteration overSubGraphscould theoretically contain nil values if the map has nil entries.🛡️ Suggested defensive check
// migrateCheckpoint recursively applies migrate to cp.State and all SubGraphs. func migrateCheckpoint(cp *checkpoint, migrate func(state any) (any, bool, error)) (bool, error) { + if cp == nil { + return false, nil + } anyChanged := false if cp.State != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compose/checkpoint.go` around lines 246 - 268, migrateCheckpoint currently dereferences cp without checking for nil, risking a panic when a SubGraphs entry is nil; at the start of migrateCheckpoint add a defensive nil check (if cp == nil return false, nil) so recursive calls and iterations over cp.SubGraphs safely handle nil entries, and when iterating SubGraphs skip nil subgraphs before calling migrateCheckpoint to avoid the recursive nil dereference; reference migrateCheckpoint, cp.State and cp.SubGraphs so the guard is placed at the very top of that function and the loop skips nil sub values.adk/react.go (1)
194-196: Consider removing redundantsetReturnDirectlyToolCallIDcall.
ReturnDirectlyToolCallIDis already assigned at line 188, andHasReturnDirectlyis assigned at line 187. The call tosetReturnDirectlyToolCallIDis redundant unless you're intentionally normalizing potentially inconsistent legacy data whereHasReturnDirectlymight not matchReturnDirectlyToolCallID.If this is intentional defensive coding, a brief comment would clarify the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adk/react.go` around lines 194 - 196, The call to setReturnDirectlyToolCallID is redundant because ReturnDirectlyToolCallID is already assigned (and HasReturnDirectly set) earlier; remove the conditional block that invokes setReturnDirectlyToolCallID(sc.ReturnDirectlyToolCallID) to avoid duplicate work, or if this is defensive normalization for legacy inconsistent state, retain the call but add a one-line comment above it explaining that it ensures legacy consistency when HasReturnDirectly may not match ReturnDirectlyToolCallID; refer to setReturnDirectlyToolCallID, ReturnDirectlyToolCallID, and HasReturnDirectly when making the change.components/model/agentic_callback_extra_test.go (1)
27-35: Consider adding a negative test case forConvAgenticCallbackOutput.The test validates that
ConvAgenticCallbackInput("asd")returnsnilfor an invalid string input, but there's no corresponding assertion forConvAgenticCallbackOutput("asd"). Addingassert.Nil(t, ConvAgenticCallbackOutput("asd"))would ensure symmetric coverage for both conversion helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/model/agentic_callback_extra_test.go` around lines 27 - 35, Add a negative test for ConvAgenticCallbackOutput to mirror the existing negative test for ConvAgenticCallbackInput: update TestConvAgenticModel to include an assertion that ConvAgenticCallbackOutput("asd") returns nil so both conversion helpers have symmetric invalid-input coverage; locate TestConvAgenticModel and add assert.Nil(t, ConvAgenticCallbackOutput("asd")) alongside the existing assertions for ConvAgenticCallbackInput and ConvAgenticCallbackOutput.components/tool/utils/invokable_func.go (1)
108-121: Consider reusing the computed options to avoid redundant processing.
getToolOptions(opts...)is called here at line 109, and thengoStruct2ParamsOneOf[T](opts...)is called at line 110, which internally callsgetToolOptions(opts...)again at line 124. This results in processing the options twice.While this is functionally correct (options processing is idempotent), you could optimize by passing the already-computed
optionstogoStruct2ParamsOneOfor extracting the schema reflection logic to avoid the redundant call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tool/utils/invokable_func.go` around lines 108 - 121, The code redundantly recomputes options by calling getToolOptions(opts...) in goStruct2ToolInfo and again inside goStruct2ParamsOneOf; modify goStruct2ParamsOneOf to accept a precomputed options struct (e.g., add a parameter like optsStruct or toolOptions) or provide an internal helper that takes options so goStruct2ToolInfo can call getToolOptions once and pass the resulting options variable into goStruct2ParamsOneOf, then update all call sites accordingly (references: goStruct2ToolInfo, goStruct2ParamsOneOf, getToolOptions, and the local options variable).components/prompt/callback_extra_test.go (1)
27-45: Missing negative test case forConvCallbackOutput.The test validates
ConvCallbackInput("asd")returnsnilfor invalid input, but there's no corresponding assertion forConvCallbackOutput("asd"). Consider adding this assertion to maintain symmetric coverage:assert.Nil(t, ConvCallbackOutput("asd"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/prompt/callback_extra_test.go` around lines 27 - 45, Add a symmetric negative test for ConvCallbackOutput in the TestConvPrompt test: within the TestConvPrompt function add an assertion that ConvCallbackOutput("asd") returns nil (e.g., assert.Nil(t, ConvCallbackOutput("asd"))) so invalid input is covered the same way as ConvCallbackInput; update only the TestConvPrompt test to include this extra assertion referencing ConvCallbackOutput.components/prompt/agentic_callback_extra_test.go (1)
27-45: Consider adding negative test forConvAgenticCallbackOutput.The test covers valid inputs for
ConvAgenticCallbackOutputbut doesn't test the rejection of invalid types (like the"asd"test forConvAgenticCallbackInput). Consider adding:assert.Nil(t, ConvAgenticCallbackOutput("invalid"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/prompt/agentic_callback_extra_test.go` around lines 27 - 45, Add a negative test for ConvAgenticCallbackOutput similar to the ConvAgenticCallbackInput negative case: call ConvAgenticCallbackOutput with an invalid type (e.g., the string "invalid") and assert the result is nil (use assert.Nil(t, ConvAgenticCallbackOutput("invalid"))), placing it alongside the existing positive assertions for ConvAgenticCallbackOutput in TestConvAgenticPrompt.compose/tool_node.go (1)
422-442: Dot-path restriction is forward-looking but error message could be clearer.The restriction on
.in alias keys is good for future nested alias support. Consider clarifying in the error message that this applies to both canonical keys and their aliases.📝 Suggested error message improvement
for canonicalKey, aliases := range paramAliases { if strings.Contains(canonicalKey, ".") { - return nil, fmt.Errorf("param alias key '%s' must not contain '.', dot-path notation is reserved for future nested alias support", canonicalKey) + return nil, fmt.Errorf("canonical param key '%s' must not contain '.'; dot-path notation is reserved for future nested alias support", canonicalKey) } for _, alias := range aliases { if strings.Contains(alias, ".") { - return nil, fmt.Errorf("param alias key '%s' must not contain '.', dot-path notation is reserved for future nested alias support", alias) + return nil, fmt.Errorf("param alias '%s' must not contain '.'; dot-path notation is reserved for future nested alias support", alias) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compose/tool_node.go` around lines 422 - 442, The error messages in buildParamInverseMap are ambiguous: update the two fmt.Errorf calls that check for '.' to explicitly state which identifier is invalid (one must reference the canonical key variable canonicalKey and the other must reference the alias variable alias) and clarify that the dot-path restriction applies to both canonical keys and aliases (e.g. "parameter key '%s' must not contain '.'; dot-path notation is reserved for future nested alias support"), leaving the rest of the conflict checks (existingParams and inverse collision) unchanged; ensure you reference canonicalKey and alias in their respective messages so the logged value matches the failing input.schema/agentic_message_test.go (2)
172-204: Misleading test name: tests AssistantGenText, not UserInputText.The test is named
"concat user input text"but actually testsContentBlockTypeAssistantGenTextwithAssistantGenTextblocks. Consider renaming to"concat assistant gen text - additional case"or similar for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schema/agentic_message_test.go` around lines 172 - 204, Rename the test case to accurately reflect what it's verifying: update the t.Run string from "concat user input text" to something like "concat assistant gen text - additional case" (or similar) to match the structures being tested (AgenticMessage with ContentBlockTypeAssistantGenText and AssistantGenText) in the ConcatAgenticMessages test; ensure the descriptive name references AssistantGenText/ContentBlockTypeAssistantGenText so it isn't misleading.
349-381: Duplicate test case.This test
"concat assistant gen image"duplicates the test at lines 206-241 which has the same name and tests the same functionality. Consider removing this duplicate or consolidating the test cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schema/agentic_message_test.go` around lines 349 - 381, Remove the duplicate test named "concat assistant gen image" that repeats the same scenario already covered earlier; locate the second occurrence of the test in agentic_message_test.go (the t.Run block that constructs two AgenticMessage instances and calls ConcatAgenticMessages) and delete that entire t.Run block so only the original test remains, ensuring no other tests rely on this duplicate.utils/callbacks/template.go (1)
745-751: Parameter type should becallbacks.CallbackOutputfor semantic consistency.The function converts output but takes
callbacks.CallbackInputas parameter type. While both are type aliases toanyand functionally equivalent, usingCallbackOutputwould be semantically clearer.♻️ Suggested fix
-func convAgenticToolsNodeCallbackOutput(src callbacks.CallbackInput) []*schema.AgenticMessage { +func convAgenticToolsNodeCallbackOutput(src callbacks.CallbackOutput) []*schema.AgenticMessage {Note: The existing
convToolsNodeCallbackOutputat line 638 has the same issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/callbacks/template.go` around lines 745 - 751, Change the parameter type of convAgenticToolsNodeCallbackOutput from callbacks.CallbackInput to callbacks.CallbackOutput (and do the same for convToolsNodeCallbackOutput) so the signature semantically represents an output converter; update the function declarations to accept callbacks.CallbackOutput and leave the body unchanged (pattern match on the value as []*schema.AgenticMessage), ensuring imports/uses still compile.schema/agentic_message.go (1)
2036-2041: UTF-8 truncation may split multi-byte characters.
truncateStringuses byte length (len(s)) and byte slicing (s[:maxLen]), which can split UTF-8 multi-byte characters, producing invalid strings. For a debugging/logging utility this may be acceptable, but worth noting.♻️ Optional: Rune-safe truncation
func truncateString(s string, maxLen int) string { - if len(s) <= maxLen { + runes := []rune(s) + if len(runes) <= maxLen { return s } - return s[:maxLen] + "..." + return string(runes[:maxLen]) + "..." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schema/agentic_message.go` around lines 2036 - 2041, The truncateString function currently measures and slices by bytes which can split UTF-8 multi-byte characters; change it to operate on runes: convert s to []rune, check rune length against maxLen, return the original string if short, otherwise build a string from the first maxLen runes and append "..." (also handle maxLen <= 0 gracefully). Update references to truncateString accordingly so all callers get rune-safe truncation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adk/flow.go`:
- Around line 191-233: The conversion loop over AssistantGenMultiContent
performs shallow copies which can cause aliasing (e.g., assigning Extra:
part.Extra and embedding part.Image.MessagePartCommon), so deep-clone maps and
nested media fields when appending to rewritten.UserInputMultiContent: create a
fresh Extra map and copy entries, and construct new MessagePartCommon values
with newly allocated string fields for URL and Base64Data (and a deep-copied
Extra) for Image/Audio/Video cases; update the conversion logic in the block
handling schema.ChatMessagePartTypeText/ImageURL/AudioURL/VideoURL (where
schema.MessageInputPart and schema.MessageInputImage/Audio/Video and
MessagePartCommon are built) to perform these deep copies and add regression
tests that mutate Extra and nested URL/Base64Data after conversion to ensure no
original is mutated.
In `@adk/middlewares/agentsmd/agentsmd.go`:
- Around line 179-181: The error string mistakenly references
AllAgentMDDocsMaxBytes instead of the actual field name AllAgentsMDMaxBytes;
update the fmt.Errorf call in the validation branch that checks
c.AllAgentsMDMaxBytes to use the correct identifier (AllAgentsMDMaxBytes) in the
error message so it matches the field name (function/method containing this
check: the validation logic around c.AllAgentsMDMaxBytes in agentsmd.go).
In `@components/model/option.go`:
- Around line 129-137: WithAgenticToolChoice currently stores the caller-owned
*schema.AgenticToolChoice pointer directly, breaking the Options immutability
contract because AgenticToolChoice contains nested mutable pointers/slices
(e.g., Allowed and Forced); fix by deep-cloning the incoming value inside
WithAgenticToolChoice (or accept it by value and copy) before returning the
Option so opts.AgenticToolChoice is set to an independent copy in the apply
closure; ensure you copy nested slices and any pointer fields (Allowed, Forced
and their slice members) to avoid sharing, and apply the same pattern to
WithToolChoice for its AllowedToolNames slice.
In `@components/prompt/agentic_chat_template.go`:
- Around line 46-74: In Format (DefaultAgenticChatTemplate.Format) the inner
call "msgs, err := template.Format(...)" shadows the outer named return err so
the deferred callbacks.OnError never sees template.Format errors; change the
redeclaration to assignment ("msgs, err = template.Format(...)") or use a
distinct variable name for the returned error, ensure you return immediately on
error so the outer err is set and the deferred OnError is invoked, and keep
references to result and t.templates unchanged.
In `@schema/agentic_message.go`:
- Around line 1288-1294: The code calls internal.ConcatSliceValue(extensions)
and assigns its result to ret.Extension but then immediately overwrites
ret.Extension with extensions.Interface() (the original slice), undoing the
concatenation; update the block in the IsValid/IsZero branch so that
ret.Extension keeps the concatenated value returned by
internal.ConcatSliceValue(extensions) (and still returns the error on failure)
instead of being overwritten by extensions.Interface(); remove the redundant
assignment to extensions.Interface() so ret.Extension contains the concat
result.
- Around line 1981-1984: The code dereferences m.Error.Code without checking for
nil, which can panic if Error is present but Code is nil; update the block in
the String()/formatting method that builds the error line to first test whether
m.Error.Code != nil and only dereference when non-nil (otherwise print a
placeholder like "nil" or omit the numeric code), i.e., guard the access to
m.Error.Code before using *m.Error.Code so the method safely handles a nil
pointer.
In `@schema/openai/extension.go`:
- Around line 156-165: The loop over chunks can mutate an input chunk because
when ret.Refusal is first set to ext.Refusal you later append to
ret.Refusal.Reason which alters the original ext.Refusal; to fix it, create a
defensive copy when assigning ret.Refusal (e.g., allocate a new Refusal value
and copy fields from ext.Refusal) and then append subsequent ext.Refusal.Reason
strings to that new copy instead of assigning pointers directly; update the loop
handling in the function that iterates chunks so it uses the new copied instance
(referencing ret.Refusal, ext.Refusal, and chunks) to avoid mutating input
objects.
---
Nitpick comments:
In `@adk/react.go`:
- Around line 194-196: The call to setReturnDirectlyToolCallID is redundant
because ReturnDirectlyToolCallID is already assigned (and HasReturnDirectly set)
earlier; remove the conditional block that invokes
setReturnDirectlyToolCallID(sc.ReturnDirectlyToolCallID) to avoid duplicate
work, or if this is defensive normalization for legacy inconsistent state,
retain the call but add a one-line comment above it explaining that it ensures
legacy consistency when HasReturnDirectly may not match
ReturnDirectlyToolCallID; refer to setReturnDirectlyToolCallID,
ReturnDirectlyToolCallID, and HasReturnDirectly when making the change.
In `@components/model/agentic_callback_extra_test.go`:
- Around line 27-35: Add a negative test for ConvAgenticCallbackOutput to mirror
the existing negative test for ConvAgenticCallbackInput: update
TestConvAgenticModel to include an assertion that
ConvAgenticCallbackOutput("asd") returns nil so both conversion helpers have
symmetric invalid-input coverage; locate TestConvAgenticModel and add
assert.Nil(t, ConvAgenticCallbackOutput("asd")) alongside the existing
assertions for ConvAgenticCallbackInput and ConvAgenticCallbackOutput.
In `@components/prompt/agentic_callback_extra_test.go`:
- Around line 27-45: Add a negative test for ConvAgenticCallbackOutput similar
to the ConvAgenticCallbackInput negative case: call ConvAgenticCallbackOutput
with an invalid type (e.g., the string "invalid") and assert the result is nil
(use assert.Nil(t, ConvAgenticCallbackOutput("invalid"))), placing it alongside
the existing positive assertions for ConvAgenticCallbackOutput in
TestConvAgenticPrompt.
In `@components/prompt/callback_extra_test.go`:
- Around line 27-45: Add a symmetric negative test for ConvCallbackOutput in the
TestConvPrompt test: within the TestConvPrompt function add an assertion that
ConvCallbackOutput("asd") returns nil (e.g., assert.Nil(t,
ConvCallbackOutput("asd"))) so invalid input is covered the same way as
ConvCallbackInput; update only the TestConvPrompt test to include this extra
assertion referencing ConvCallbackOutput.
In `@components/tool/utils/invokable_func.go`:
- Around line 108-121: The code redundantly recomputes options by calling
getToolOptions(opts...) in goStruct2ToolInfo and again inside
goStruct2ParamsOneOf; modify goStruct2ParamsOneOf to accept a precomputed
options struct (e.g., add a parameter like optsStruct or toolOptions) or provide
an internal helper that takes options so goStruct2ToolInfo can call
getToolOptions once and pass the resulting options variable into
goStruct2ParamsOneOf, then update all call sites accordingly (references:
goStruct2ToolInfo, goStruct2ParamsOneOf, getToolOptions, and the local options
variable).
In `@compose/checkpoint.go`:
- Around line 246-268: migrateCheckpoint currently dereferences cp without
checking for nil, risking a panic when a SubGraphs entry is nil; at the start of
migrateCheckpoint add a defensive nil check (if cp == nil return false, nil) so
recursive calls and iterations over cp.SubGraphs safely handle nil entries, and
when iterating SubGraphs skip nil subgraphs before calling migrateCheckpoint to
avoid the recursive nil dereference; reference migrateCheckpoint, cp.State and
cp.SubGraphs so the guard is placed at the very top of that function and the
loop skips nil sub values.
In `@compose/tool_node.go`:
- Around line 422-442: The error messages in buildParamInverseMap are ambiguous:
update the two fmt.Errorf calls that check for '.' to explicitly state which
identifier is invalid (one must reference the canonical key variable
canonicalKey and the other must reference the alias variable alias) and clarify
that the dot-path restriction applies to both canonical keys and aliases (e.g.
"parameter key '%s' must not contain '.'; dot-path notation is reserved for
future nested alias support"), leaving the rest of the conflict checks
(existingParams and inverse collision) unchanged; ensure you reference
canonicalKey and alias in their respective messages so the logged value matches
the failing input.
In `@schema/agentic_message_test.go`:
- Around line 172-204: Rename the test case to accurately reflect what it's
verifying: update the t.Run string from "concat user input text" to something
like "concat assistant gen text - additional case" (or similar) to match the
structures being tested (AgenticMessage with ContentBlockTypeAssistantGenText
and AssistantGenText) in the ConcatAgenticMessages test; ensure the descriptive
name references AssistantGenText/ContentBlockTypeAssistantGenText so it isn't
misleading.
- Around line 349-381: Remove the duplicate test named "concat assistant gen
image" that repeats the same scenario already covered earlier; locate the second
occurrence of the test in agentic_message_test.go (the t.Run block that
constructs two AgenticMessage instances and calls ConcatAgenticMessages) and
delete that entire t.Run block so only the original test remains, ensuring no
other tests rely on this duplicate.
In `@schema/agentic_message.go`:
- Around line 2036-2041: The truncateString function currently measures and
slices by bytes which can split UTF-8 multi-byte characters; change it to
operate on runes: convert s to []rune, check rune length against maxLen, return
the original string if short, otherwise build a string from the first maxLen
runes and append "..." (also handle maxLen <= 0 gracefully). Update references
to truncateString accordingly so all callers get rune-safe truncation.
In `@schema/openai/extension_test.go`:
- Around line 73-193: Add a unit test that verifies
ConcatAssistantGenTextExtensions returns an error when given an empty slice; the
missing case is that the implementation errors on empty chunks, so add a new
subtest (e.g., t.Run("empty input - returns error", ...)) that calls
ConcatAssistantGenTextExtensions([]*AssistantGenTextExtension{}) and asserts an
error is returned to cover this behavior.
In `@utils/callbacks/template.go`:
- Around line 745-751: Change the parameter type of
convAgenticToolsNodeCallbackOutput from callbacks.CallbackInput to
callbacks.CallbackOutput (and do the same for convToolsNodeCallbackOutput) so
the signature semantically represents an output converter; update the function
declarations to accept callbacks.CallbackOutput and leave the body unchanged
(pattern match on the value as []*schema.AgenticMessage), ensuring imports/uses
still compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 610cfbae-0a30-46f1-96b8-972f1ad636f4
⛔ Files ignored due to path filters (6)
adk/prebuilt/deep/testdata/_gen/generate_test.gois excluded by!**/_gen/**adk/prebuilt/deep/testdata/checkpoint_data_v0.7.37.binis excluded by!**/*.binadk/prebuilt/deep/testdata/checkpoint_data_v0.8.2.binis excluded by!**/*.binadk/prebuilt/deep/testdata/checkpoint_data_v0.8.3.binis excluded by!**/*.binadk/prebuilt/deep/testdata/checkpoint_data_v0.8.4.binis excluded by!**/*.bingo.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
adk/chatmodel.goadk/chatmodel_test.goadk/flow.goadk/flow_test.goadk/handler.goadk/interrupt.goadk/interrupt_test.goadk/middlewares/agentsmd/agentsmd.goadk/middlewares/agentsmd/agentsmd_test.goadk/middlewares/agentsmd/loader.goadk/middlewares/filesystem/filesystem.goadk/middlewares/filesystem/filesystem_test.goadk/middlewares/summarization/summarization.goadk/prebuilt/deep/checkpoint_compat_resume_test.goadk/react.goadk/react_test.gocomponents/model/agentic_callback_extra.gocomponents/model/agentic_callback_extra_test.gocomponents/model/interface.gocomponents/model/option.gocomponents/model/option_test.gocomponents/prompt/agentic_callback_extra.gocomponents/prompt/agentic_callback_extra_test.gocomponents/prompt/agentic_chat_template.gocomponents/prompt/agentic_chat_template_test.gocomponents/prompt/callback_extra_test.gocomponents/prompt/interface.gocomponents/tool/utils/create_options.gocomponents/tool/utils/invokable_func.gocomponents/types.gocompose/agentic_tools_node.gocompose/agentic_tools_node_test.gocompose/chain.gocompose/chain_branch.gocompose/chain_parallel.gocompose/checkpoint.gocompose/checkpoint_migrate_test.gocompose/component_to_graph_node.gocompose/graph.gocompose/tool_node.gocompose/tool_node_test.gocompose/types.gocompose/workflow.gogo.modinternal/concat.goschema/agentic_message.goschema/agentic_message_test.goschema/claude/consts.goschema/claude/extension.goschema/claude/extension_test.goschema/gemini/extension.goschema/gemini/extension_test.goschema/message.goschema/message_test.goschema/openai/consts.goschema/openai/extension.goschema/openai/extension_test.goschema/tool.goutils/callbacks/template.goutils/callbacks/template_test.go
| if msg.MultiContent != nil { | ||
| rewritten.MultiContent = append([]schema.ChatMessagePart{}, msg.MultiContent...) | ||
| } | ||
| if msg.UserInputMultiContent != nil { | ||
| rewritten.UserInputMultiContent = append([]schema.MessageInputPart{}, msg.UserInputMultiContent...) | ||
| } | ||
|
|
||
| // Convert AssistantGenMultiContent to UserInputMultiContent, since the role changes to User. | ||
| // Reasoning parts have no user input equivalent and are dropped. | ||
| for _, part := range msg.AssistantGenMultiContent { | ||
| switch part.Type { | ||
| case schema.ChatMessagePartTypeText: | ||
| rewritten.UserInputMultiContent = append(rewritten.UserInputMultiContent, schema.MessageInputPart{ | ||
| Type: part.Type, | ||
| Text: part.Text, | ||
| Extra: part.Extra, | ||
| }) | ||
| case schema.ChatMessagePartTypeImageURL: | ||
| if part.Image != nil { | ||
| rewritten.UserInputMultiContent = append(rewritten.UserInputMultiContent, schema.MessageInputPart{ | ||
| Type: part.Type, | ||
| Image: &schema.MessageInputImage{MessagePartCommon: part.Image.MessagePartCommon}, | ||
| Extra: part.Extra, | ||
| }) | ||
| } | ||
| case schema.ChatMessagePartTypeAudioURL: | ||
| if part.Audio != nil { | ||
| rewritten.UserInputMultiContent = append(rewritten.UserInputMultiContent, schema.MessageInputPart{ | ||
| Type: part.Type, | ||
| Audio: &schema.MessageInputAudio{MessagePartCommon: part.Audio.MessagePartCommon}, | ||
| Extra: part.Extra, | ||
| }) | ||
| } | ||
| case schema.ChatMessagePartTypeVideoURL: | ||
| if part.Video != nil { | ||
| rewritten.UserInputMultiContent = append(rewritten.UserInputMultiContent, schema.MessageInputPart{ | ||
| Type: part.Type, | ||
| Video: &schema.MessageInputVideo{MessagePartCommon: part.Video.MessagePartCommon}, | ||
| Extra: part.Extra, | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "rewriteMessage copy sites:"
rg -n -C3 'rewritten\.MultiContent|rewritten\.UserInputMultiContent|MessagePartCommon: part\.(Image|Audio|Video)\.MessagePartCommon|Extra: part\.Extra' adk/flow.go
echo
echo "Copied types with nested mutable fields:"
rg -n -C5 --glob '*.go' 'type ChatMessagePart struct|type MessageInputPart struct|type MessagePartCommon struct' schema
echo
echo "Current rewriteMessage mutation coverage:"
rg -n -C3 'TestRewriteMessage|mutated' adk/flow_test.goRepository: coderabbit-test/eino
Length of output: 5075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete TestRewriteMessage test
echo "=== Full TestRewriteMessage test ==="
sed -n '34,/^func /p' adk/flow_test.go | head -n -1
echo
echo "=== MessageInputPart and ChatMessagePart Extra field ==="
rg -n -A20 'type MessageInputPart struct' schema/message.go | grep -E '(Extra|map)'
echo
echo "=== MessagePartCommon complete definition ==="
sed -n '160,210p' schema/message.goRepository: coderabbit-test/eino
Length of output: 4942
Deep-clone nested media structs and Extra maps to prevent aliasing mutations.
The code performs shallow copies: Extra: part.Extra directly assigns the map reference (not a copy), and MessagePartCommon: part.Image.MessagePartCommon copies the struct by value but retains pointers to URL and Base64Data strings. Downstream mutations to the rewritten message's Extra fields or nested URL pointers will mutate the source message stored in session state. Deep-clone Extra maps and nested media struct fields (URL, Base64Data, Extra within MessagePartCommon), and add regression test cases that mutate Extra maps and pointer-referenced strings in converted parts (not just top-level Text).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@adk/flow.go` around lines 191 - 233, The conversion loop over
AssistantGenMultiContent performs shallow copies which can cause aliasing (e.g.,
assigning Extra: part.Extra and embedding part.Image.MessagePartCommon), so
deep-clone maps and nested media fields when appending to
rewritten.UserInputMultiContent: create a fresh Extra map and copy entries, and
construct new MessagePartCommon values with newly allocated string fields for
URL and Base64Data (and a deep-copied Extra) for Image/Audio/Video cases; update
the conversion logic in the block handling
schema.ChatMessagePartTypeText/ImageURL/AudioURL/VideoURL (where
schema.MessageInputPart and schema.MessageInputImage/Audio/Video and
MessagePartCommon are built) to perform these deep copies and add regression
tests that mutate Extra and nested URL/Base64Data after conversion to ensure no
original is mutated.
| if c.AllAgentsMDMaxBytes < 0 { | ||
| return fmt.Errorf("[agentsmd]: AllAgentMDDocsMaxBytes must be non-negative") | ||
| } |
There was a problem hiding this comment.
Fix typo in error message.
The error message references AllAgentMDDocsMaxBytes but the actual field name is AllAgentsMDMaxBytes.
📝 Proposed fix
if c.AllAgentsMDMaxBytes < 0 {
- return fmt.Errorf("[agentsmd]: AllAgentMDDocsMaxBytes must be non-negative")
+ return fmt.Errorf("[agentsmd]: AllAgentsMDMaxBytes must be non-negative")
}📝 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.
| if c.AllAgentsMDMaxBytes < 0 { | |
| return fmt.Errorf("[agentsmd]: AllAgentMDDocsMaxBytes must be non-negative") | |
| } | |
| if c.AllAgentsMDMaxBytes < 0 { | |
| return fmt.Errorf("[agentsmd]: AllAgentsMDMaxBytes must be non-negative") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@adk/middlewares/agentsmd/agentsmd.go` around lines 179 - 181, The error
string mistakenly references AllAgentMDDocsMaxBytes instead of the actual field
name AllAgentsMDMaxBytes; update the fmt.Errorf call in the validation branch
that checks c.AllAgentsMDMaxBytes to use the correct identifier
(AllAgentsMDMaxBytes) in the error message so it matches the field name
(function/method containing this check: the validation logic around
c.AllAgentsMDMaxBytes in agentsmd.go).
| // WithAgenticToolChoice is the option to set tool choice for the agentic model. | ||
| // Only available for AgenticModel. | ||
| func WithAgenticToolChoice(toolChoice *schema.AgenticToolChoice) Option { | ||
| return Option{ | ||
| apply: func(opts *Options) { | ||
| opts.AgenticToolChoice = toolChoice | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tool choice option constructors:"
rg -n -C3 'func WithToolChoice|func WithAgenticToolChoice' components/model/option.go
echo
echo "AgenticToolChoice definition:"
rg -n -C5 --glob '*.go' 'type AgenticToolChoice struct' schemaRepository: coderabbit-test/eino
Length of output: 1390
🏁 Script executed:
# Get full AgenticToolChoice struct definition
rg -n -A 10 'type AgenticToolChoice struct' schema/
# Check how WithToolChoice stores the pointer (understand the closure)
rg -n -B 2 -A 8 'func WithToolChoice' components/model/option.go
# Find other option constructors to understand pattern
rg -n 'func With.*Option \{' components/model/option.go | head -20Repository: coderabbit-test/eino
Length of output: 1490
🏁 Script executed:
# Get full definitions of nested struct types
rg -n -A 8 'type AgenticAllowedToolChoice struct|type AgenticForcedToolChoice struct' schema/
# Check if WithToolChoice's AllowedToolNames slice is actually copied or just referenced
rg -n -B 5 -A 5 'AllowedToolNames' components/model/option.go | head -30
# Look for usage of WithAgenticToolChoice to see if it's reused or mutated
rg -n 'WithAgenticToolChoice' --glob '*.go' | head -20Repository: coderabbit-test/eino
Length of output: 2104
🏁 Script executed:
# Check for Clone methods or copy patterns in schema types
rg -n 'func.*Clone|func.*Copy' schema/ --glob '*.go'
# Look for immutability documentation in option.go or comments
rg -n -B 5 'immutable|composable|clone|copy' components/model/option.go
# Check the test file to see how WithAgenticToolChoice is used
rg -n -B 3 -A 5 'WithAgenticToolChoice' components/model/option_test.goRepository: coderabbit-test/eino
Length of output: 1209
WithAgenticToolChoice should not retain the caller-owned pointer.
The documented immutability contract states "Options are immutable and composable." However, this function stores the original pointer to schema.AgenticToolChoice without cloning. Since the struct contains nested mutable pointers (Allowed and Forced fields with slice members), reusing or mutating the same struct after building the option will affect later calls and can race under concurrent reuse.
Clone the value on entry, or take it by value, to preserve immutability. Note: WithToolChoice has a similar issue with its AllowedToolNames slice parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/model/option.go` around lines 129 - 137, WithAgenticToolChoice
currently stores the caller-owned *schema.AgenticToolChoice pointer directly,
breaking the Options immutability contract because AgenticToolChoice contains
nested mutable pointers/slices (e.g., Allowed and Forced); fix by deep-cloning
the incoming value inside WithAgenticToolChoice (or accept it by value and copy)
before returning the Option so opts.AgenticToolChoice is set to an independent
copy in the apply closure; ensure you copy nested slices and any pointer fields
(Allowed, Forced and their slice members) to avoid sharing, and apply the same
pattern to WithToolChoice for its AllowedToolNames slice.
| func (t *DefaultAgenticChatTemplate) Format(ctx context.Context, vs map[string]any, opts ...Option) (result []*schema.AgenticMessage, err error) { | ||
| ctx = callbacks.EnsureRunInfo(ctx, t.GetType(), components.ComponentOfAgenticPrompt) | ||
| ctx = callbacks.OnStart(ctx, &AgenticCallbackInput{ | ||
| Variables: vs, | ||
| Templates: t.templates, | ||
| }) | ||
| defer func() { | ||
| if err != nil { | ||
| _ = callbacks.OnError(ctx, err) | ||
| } | ||
| }() | ||
|
|
||
| result = make([]*schema.AgenticMessage, 0, len(t.templates)) | ||
| for _, template := range t.templates { | ||
| msgs, err := template.Format(ctx, vs, t.formatType) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| result = append(result, msgs...) | ||
| } | ||
|
|
||
| _ = callbacks.OnEnd(ctx, &AgenticCallbackOutput{ | ||
| Result: result, | ||
| Templates: t.templates, | ||
| }) | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Bug: Variable shadowing prevents error callback from triggering.
On line 60, err is redeclared with := inside the loop, creating a new variable that shadows the outer err declared on line 46. This means the deferred error callback (lines 52-56) will never see errors from template.Format() because it checks the outer err which remains nil.
🐛 Proposed fix
func (t *DefaultAgenticChatTemplate) Format(ctx context.Context, vs map[string]any, opts ...Option) (result []*schema.AgenticMessage, err error) {
ctx = callbacks.EnsureRunInfo(ctx, t.GetType(), components.ComponentOfAgenticPrompt)
ctx = callbacks.OnStart(ctx, &AgenticCallbackInput{
Variables: vs,
Templates: t.templates,
})
defer func() {
if err != nil {
_ = callbacks.OnError(ctx, err)
}
}()
result = make([]*schema.AgenticMessage, 0, len(t.templates))
for _, template := range t.templates {
- msgs, err := template.Format(ctx, vs, t.formatType)
+ var msgs []*schema.AgenticMessage
+ msgs, err = template.Format(ctx, vs, t.formatType)
if err != nil {
return nil, err
}
result = append(result, msgs...)
}📝 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.
| func (t *DefaultAgenticChatTemplate) Format(ctx context.Context, vs map[string]any, opts ...Option) (result []*schema.AgenticMessage, err error) { | |
| ctx = callbacks.EnsureRunInfo(ctx, t.GetType(), components.ComponentOfAgenticPrompt) | |
| ctx = callbacks.OnStart(ctx, &AgenticCallbackInput{ | |
| Variables: vs, | |
| Templates: t.templates, | |
| }) | |
| defer func() { | |
| if err != nil { | |
| _ = callbacks.OnError(ctx, err) | |
| } | |
| }() | |
| result = make([]*schema.AgenticMessage, 0, len(t.templates)) | |
| for _, template := range t.templates { | |
| msgs, err := template.Format(ctx, vs, t.formatType) | |
| if err != nil { | |
| return nil, err | |
| } | |
| result = append(result, msgs...) | |
| } | |
| _ = callbacks.OnEnd(ctx, &AgenticCallbackOutput{ | |
| Result: result, | |
| Templates: t.templates, | |
| }) | |
| return result, nil | |
| } | |
| func (t *DefaultAgenticChatTemplate) Format(ctx context.Context, vs map[string]any, opts ...Option) (result []*schema.AgenticMessage, err error) { | |
| ctx = callbacks.EnsureRunInfo(ctx, t.GetType(), components.ComponentOfAgenticPrompt) | |
| ctx = callbacks.OnStart(ctx, &AgenticCallbackInput{ | |
| Variables: vs, | |
| Templates: t.templates, | |
| }) | |
| defer func() { | |
| if err != nil { | |
| _ = callbacks.OnError(ctx, err) | |
| } | |
| }() | |
| result = make([]*schema.AgenticMessage, 0, len(t.templates)) | |
| for _, template := range t.templates { | |
| var msgs []*schema.AgenticMessage | |
| msgs, err = template.Format(ctx, vs, t.formatType) | |
| if err != nil { | |
| return nil, err | |
| } | |
| result = append(result, msgs...) | |
| } | |
| _ = callbacks.OnEnd(ctx, &AgenticCallbackOutput{ | |
| Result: result, | |
| Templates: t.templates, | |
| }) | |
| return result, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/prompt/agentic_chat_template.go` around lines 46 - 74, In Format
(DefaultAgenticChatTemplate.Format) the inner call "msgs, err :=
template.Format(...)" shadows the outer named return err so the deferred
callbacks.OnError never sees template.Format errors; change the redeclaration to
assignment ("msgs, err = template.Format(...)") or use a distinct variable name
for the returned error, ensure you return immediately on error so the outer err
is set and the deferred OnError is invoked, and keep references to result and
t.templates unchanged.
| if extensions.IsValid() && !extensions.IsZero() { | ||
| ret.Extension, err = internal.ConcatSliceValue(extensions) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ret.Extension = extensions.Interface() | ||
| } |
There was a problem hiding this comment.
Bug: Concatenated extension result is immediately overwritten.
internal.ConcatSliceValue(extensions) returns the concatenated value, but on line 1293 it's overwritten with extensions.Interface() (the original slice, not the concatenated result). This defeats the purpose of the concatenation.
🐛 Proposed fix
if extensions.IsValid() && !extensions.IsZero() {
- ret.Extension, err = internal.ConcatSliceValue(extensions)
+ extension, err := internal.ConcatSliceValue(extensions)
if err != nil {
return nil, err
}
- ret.Extension = extensions.Interface()
+ ret.Extension = extension.Interface()
}📝 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.
| if extensions.IsValid() && !extensions.IsZero() { | |
| ret.Extension, err = internal.ConcatSliceValue(extensions) | |
| if err != nil { | |
| return nil, err | |
| } | |
| ret.Extension = extensions.Interface() | |
| } | |
| if extensions.IsValid() && !extensions.IsZero() { | |
| extension, err := internal.ConcatSliceValue(extensions) | |
| if err != nil { | |
| return nil, err | |
| } | |
| ret.Extension = extension.Interface() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schema/agentic_message.go` around lines 1288 - 1294, The code calls
internal.ConcatSliceValue(extensions) and assigns its result to ret.Extension
but then immediately overwrites ret.Extension with extensions.Interface() (the
original slice), undoing the concatenation; update the block in the
IsValid/IsZero branch so that ret.Extension keeps the concatenated value
returned by internal.ConcatSliceValue(extensions) (and still returns the error
on failure) instead of being overwritten by extensions.Interface(); remove the
redundant assignment to extensions.Interface() so ret.Extension contains the
concat result.
| if m.Error != nil { | ||
| sb.WriteString(fmt.Sprintf(" error: [%d] %s\n", *m.Error.Code, m.Error.Message)) | ||
| } | ||
| return sb.String() |
There was a problem hiding this comment.
Potential nil pointer dereference when Error.Code is nil.
m.Error.Code is a *int64 pointer. If the error exists but Code is nil, dereferencing with *m.Error.Code will panic.
🛡️ Proposed fix
if m.Error != nil {
- sb.WriteString(fmt.Sprintf(" error: [%d] %s\n", *m.Error.Code, m.Error.Message))
+ if m.Error.Code != nil {
+ sb.WriteString(fmt.Sprintf(" error: [%d] %s\n", *m.Error.Code, m.Error.Message))
+ } else {
+ sb.WriteString(fmt.Sprintf(" error: %s\n", m.Error.Message))
+ }
}📝 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.
| if m.Error != nil { | |
| sb.WriteString(fmt.Sprintf(" error: [%d] %s\n", *m.Error.Code, m.Error.Message)) | |
| } | |
| return sb.String() | |
| if m.Error != nil { | |
| if m.Error.Code != nil { | |
| sb.WriteString(fmt.Sprintf(" error: [%d] %s\n", *m.Error.Code, m.Error.Message)) | |
| } else { | |
| sb.WriteString(fmt.Sprintf(" error: %s\n", m.Error.Message)) | |
| } | |
| } | |
| return sb.String() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schema/agentic_message.go` around lines 1981 - 1984, The code dereferences
m.Error.Code without checking for nil, which can panic if Error is present but
Code is nil; update the block in the String()/formatting method that builds the
error line to first test whether m.Error.Code != nil and only dereference when
non-nil (otherwise print a placeholder like "nil" or omit the numeric code),
i.e., guard the access to m.Error.Code before using *m.Error.Code so the method
safely handles a nil pointer.
| for _, ext := range chunks { | ||
| if ext.Refusal == nil { | ||
| continue | ||
| } | ||
| if ret.Refusal == nil { | ||
| ret.Refusal = ext.Refusal | ||
| } else { | ||
| ret.Refusal.Reason += ext.Refusal.Reason | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential mutation of input object when concatenating Refusal.
When the first non-nil Refusal is encountered, you assign the pointer directly to ret.Refusal. Subsequent iterations then mutate this object via ret.Refusal.Reason += ext.Refusal.Reason, which modifies the original input chunk's Refusal.Reason in place.
🛡️ Proposed fix to avoid mutating input
for _, ext := range chunks {
if ext.Refusal == nil {
continue
}
if ret.Refusal == nil {
- ret.Refusal = ext.Refusal
+ ret.Refusal = &OutputRefusal{Reason: ext.Refusal.Reason}
} else {
ret.Refusal.Reason += ext.Refusal.Reason
}
}📝 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.
| for _, ext := range chunks { | |
| if ext.Refusal == nil { | |
| continue | |
| } | |
| if ret.Refusal == nil { | |
| ret.Refusal = ext.Refusal | |
| } else { | |
| ret.Refusal.Reason += ext.Refusal.Reason | |
| } | |
| } | |
| for _, ext := range chunks { | |
| if ext.Refusal == nil { | |
| continue | |
| } | |
| if ret.Refusal == nil { | |
| ret.Refusal = &OutputRefusal{Reason: ext.Refusal.Reason} | |
| } else { | |
| ret.Refusal.Reason += ext.Refusal.Reason | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schema/openai/extension.go` around lines 156 - 165, The loop over chunks can
mutate an input chunk because when ret.Refusal is first set to ext.Refusal you
later append to ret.Refusal.Reason which alters the original ext.Refusal; to fix
it, create a defensive copy when assigning ret.Refusal (e.g., allocate a new
Refusal value and copy fields from ext.Refusal) and then append subsequent
ext.Refusal.Reason strings to that new copy instead of assigning pointers
directly; update the loop handling in the function that iterates chunks so it
uses the new copied instance (referencing ret.Refusal, ext.Refusal, and chunks)
to avoid mutating input objects.
8 participants
User description
Summary by CodeRabbit
Release Notes
New Features
@importresolution and byte budgeting.Tests
CodeAnt-AI Description
Support agentic models, tool name/parameter aliases, and resume older checkpoints; add Agents.md injection middleware
What Changed
Impact
✅ Clearer resume errors for legacy checkpoints (automatic migration and retries)✅ Tools callable by alias (models can use alternate tool names)✅ Tools accept common shorthand params (LLM can use alias keys like "q" -> "query")💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.