feat: [#916] Implement AI module with provider resolver and OpenAI driver#1421
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1421 +/- ##
==========================================
+ Coverage 65.94% 68.19% +2.25%
==========================================
Files 353 357 +4
Lines 26083 27633 +1550
==========================================
+ Hits 17201 18845 +1644
+ Misses 8141 7938 -203
- Partials 741 850 +109 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces a complete AI conversation system with provider abstraction. It implements context-aware application initialization, conversation state management with message history, a pluggable provider resolver, OpenAI provider integration, and comprehensive test coverage. Configuration structures and error definitions support flexible provider configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as Application
participant Resolver as ProviderResolver
participant Config as Config
participant Conv as Conversation
participant Provider
participant OpenAI as OpenAI API
Client->>App: NewApplication(ctx, config)
App->>Resolver: NewProviderResolver(config)
Client->>App: Agent(agentName, options...)
App->>App: Extract provider & model from options
App->>Resolver: New(providerName)
Resolver->>Config: Lookup provider config
Config-->>Resolver: ProviderConfig
Resolver->>Provider: Create provider instance
Provider-->>Resolver: Provider
Resolver-->>App: Provider
App->>Conv: NewConversation(ctx, agent, provider, model)
Conv->>Conv: Clone agent messages
Conv-->>App: Conversation
App-->>Client: Conversation
Client->>Conv: Prompt(input)
Conv->>Provider: Prompt(ctx, AgentPrompt{agent, input, model})
Provider->>OpenAI: POST /chat/completions
OpenAI-->>Provider: Response
Provider-->>Conv: Response with usage
Conv->>Conv: Append user message
Conv->>Conv: Append assistant message
Conv-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ai/option.go (1)
3-15:⚠️ Potential issue | 🟠 MajorReintroduce
WithTimeoutto avoid a public API break.
WithTimeoutappears removed whilecontractsai.OptionTimeoutstill exists. External callers usingai.WithTimeout(...)will fail to compile after this change.🧩 Compatibility fix
package ai -import contractsai "github.com/goravel/framework/contracts/ai" +import ( + "time" + + contractsai "github.com/goravel/framework/contracts/ai" +) @@ func WithModel(model string) contractsai.Option { return func(options map[string]any) { options[contractsai.OptionModel] = model } } + +func WithTimeout(timeout time.Duration) contractsai.Option { + return func(options map[string]any) { + options[contractsai.OptionTimeout] = timeout + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ai/option.go` around lines 3 - 15, You removed the public helper WithTimeout while contractsai.OptionTimeout remains, breaking callers; reintroduce a function WithTimeout(timeout time.Duration) contractsai.Option that sets options[contractsai.OptionTimeout] = timeout, add the time import if missing, and place it alongside WithProvider and WithModel so external code using ai.WithTimeout(...) continues to compile.
🧹 Nitpick comments (1)
ai/conversation_test.go (1)
165-165: Guard against empty slice before indexing.
&tt.initial[0]will panic with index out of bounds if a test case uses an emptyinitialslice. While current test cases have non-empty slices, this is fragile for future test additions.Proposed fix
assert.Equal(t, tt.expectAfter, resetMessages) - assert.NotSame(t, &tt.initial[0], &resetMessages[0]) + if len(tt.initial) > 0 && len(resetMessages) > 0 { + assert.NotSame(t, &tt.initial[0], &resetMessages[0]) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ai/conversation_test.go` at line 165, Guard against empty slices before taking element addresses: before calling assert.NotSame with &tt.initial[0] and &resetMessages[0], check that both tt.initial and resetMessages have length > 0 (e.g., if len(tt.initial) > 0 && len(resetMessages) > 0 { ... }) and only then call assert.NotSame; otherwise add an appropriate assertion for empty slices (e.g., assert.Empty on resetMessages or tt.initial) so the test won’t panic when initial is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ai/prompts/tests.md:
- Around line 57-58: The example test uses the wrong loop variable name: it
defines testCases but iterates over tests; update the range loop to iterate over
testCases (or rename the declared slice to tests) so the t.Run block receives
the correct tt values; specifically modify the for _, tt := range ... line to
use testCases to match the declared variable and ensure t.Run(tt.name, func(t
*testing.T) { ... }) uses the matching tt from testCases.
In `@ai/conversation.go`:
- Around line 28-29: The Messages() accessor currently returns the internal
backing slice allowing external mutation; change conversation.Messages() to
return a copy of the slice (e.g. create a new slice and copy/append r.messages
into it) so callers get a cloned slice rather than the original; if Message is a
pointer type also deep-clone elements (or use a per-element clone function) to
prevent callers or Agent implementations from mutating internal state and
bypassing Prompt/Reset—this mirrors the cloning done in NewConversation and
Reset.
In `@ai/openai/provider.go`:
- Around line 46-58: The code unconditionally dereferences prompt.Agent and
silently ignores unknown message roles; update the builder that constructs
messages (the block using prompt.Agent.Instructions(), prompt.Agent.Messages(),
messages, and goopenai.SystemMessage/UserMessage/AssistantMessage) to first
treat a nil prompt.Agent as empty history (skip history creation) instead of
panicking, and when iterating prompt.Agent.Messages() return an explicit error
if a message.Role is not contractsai.RoleUser or contractsai.RoleAssistant (do
not drop it silently); ensure the function returns that error to callers so
unsupported roles fail fast.
In `@ai/option_test.go`:
- Around line 100-104: The test currently uses assert.PanicsWithError but should
use assert.PanicsWithValue; update the assertion in ai/option_test.go so the
call to WithModel inside the loop is wrapped with assert.PanicsWithValue(t,
"assignment to entry in nil map", func() { for _, arg := range tt.args {
WithModel(arg)(nil) } }) instead of assert.PanicsWithError, keeping the same
panic string and the same lambda that invokes WithModel.
In `@ai/provider.go`:
- Around line 23-43: The cache-miss path in ProviderResolver.New currently calls
resolve outside the write lock which allows concurrent New(providerName) calls
to construct the same provider multiple times and potentially cache a nil value;
change the flow to serialize the miss: inside ProviderResolver.New, after the
initial read-lock miss, acquire the write lock (r.mu.Lock), re-check
r.providers[providerName] to avoid TOCTOU, then call r.resolve(providerName,
providerCfg) while holding the lock (or use a per-provider sync.Once keyed by
providerName) and ensure the returned provider is non-nil before assigning it to
r.providers[providerName]; unlock afterwards and return the created provider or
the error.
---
Outside diff comments:
In `@ai/option.go`:
- Around line 3-15: You removed the public helper WithTimeout while
contractsai.OptionTimeout remains, breaking callers; reintroduce a function
WithTimeout(timeout time.Duration) contractsai.Option that sets
options[contractsai.OptionTimeout] = timeout, add the time import if missing,
and place it alongside WithProvider and WithModel so external code using
ai.WithTimeout(...) continues to compile.
---
Nitpick comments:
In `@ai/conversation_test.go`:
- Line 165: Guard against empty slices before taking element addresses: before
calling assert.NotSame with &tt.initial[0] and &resetMessages[0], check that
both tt.initial and resetMessages have length > 0 (e.g., if len(tt.initial) > 0
&& len(resetMessages) > 0 { ... }) and only then call assert.NotSame; otherwise
add an appropriate assertion for empty slices (e.g., assert.Empty on
resetMessages or tt.initial) so the test won’t panic when initial is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64342517-016b-42d2-a30a-e7669db2b9d7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.ai/prompts/tests.mdAGENTS.mdai/application.goai/application_test.goai/conversation.goai/conversation_test.goai/openai/provider.goai/openai/provider_test.goai/openai/response.goai/option.goai/option_test.goai/provider.goai/provider_test.goai/service_provider.goai/service_provider_test.gocontracts/ai/ai.gocontracts/ai/config.gocontracts/ai/provider.goerrors/list.gogo.modmocks/ai/AI.gomocks/ai/Conversation.gomocks/ai/Provider.gomocks/ai/ProviderCreator.go
💤 Files with no reviewable changes (1)
- mocks/ai/ProviderCreator.go
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
2 participants
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.
Summary
Conversation.Promptno longer acceptscontext.Context; context is bound once viaAI.WithContext(ctx)and reused across all operationsProviderResolverlazily initialises and caches AI providers from config, withViaaccepting a liveProviderinstance or afunc() (Provider, error)factoryAgentPromptinto chat completion requests with a configurable API key, base URL, and default text model (gpt-5.4)Closes #916
Why
The AI module now has a concrete implementation behind its interfaces. Moving context out of
Conversation.Promptand intoAI.WithContext(ctx)keeps the per-prompt call clean: callers bind a context once and every subsequentPromptandAgentcall carries it automatically without repetition.The new
ProviderResolverreads from theaiconfig key, initialises each driver on first use, and caches it, so the construction cost is paid only once. TheViafield accepts either a liveProviderfor direct injection (e.g. in tests) or afunc() (Provider, error)for lazy construction, keeping the bootstrap path flexible.The OpenAI provider maps
Agent.Instructions()to the OpenAI system message,Agent.Messages()to the preceding conversation history, and the currentInputto the final user message, then callsChat.Completions.Newvia theopenai-goclient. When the provider config omits a default text model, it falls back togpt-5.4.