Skip to content

Refactor RAG from agent-level config to standard toolset type#2210

Open
dgageot wants to merge 1 commit intodocker:mainfrom
dgageot:rag-toolset
Open

Refactor RAG from agent-level config to standard toolset type#2210
dgageot wants to merge 1 commit intodocker:mainfrom
dgageot:rag-toolset

Conversation

@dgageot
Copy link
Member

@dgageot dgageot commented Mar 21, 2026

Replace the special-cased RAG configuration (AgentConfig.RAG []string referencing top-level Config.RAG map) with a standard toolset type (type: rag) that follows the same patterns as MCP and other toolsets.

Key changes:

  • Add 'type: rag' to toolset registry with ref support for shared definitions
  • Introduce RAGToolset wrapper (mirrors MCPToolset) for top-level rag section
  • Add RAGConfig field to Toolset for inline/resolved RAG configuration
  • Add resolveRAGDefinitions() mirroring resolveMCPDefinitions()
  • Extract rag.NewManager() for per-toolset manager creation
  • Implement tools.Startable on RAGTool for lazy init and file watching
  • Remove RAG special-casing from Team, LocalRuntime, and teamloader
  • Add v6→v7 migration for old rag agent field to toolset entries
  • Update schema, docs, and all example YAML files

Assisted-By: docker-agent

Replace the special-cased RAG configuration (AgentConfig.RAG []string referencing
top-level Config.RAG map) with a standard toolset type (type: rag) that follows
the same patterns as MCP and other toolsets.

Key changes:
- Add 'type: rag' to toolset registry with ref support for shared definitions
- Introduce RAGToolset wrapper (mirrors MCPToolset) for top-level rag section
- Add RAGConfig field to Toolset for inline/resolved RAG configuration
- Add resolveRAGDefinitions() mirroring resolveMCPDefinitions()
- Extract rag.NewManager() for per-toolset manager creation
- Implement tools.Startable on RAGTool for lazy init and file watching
- Remove RAG special-casing from Team, LocalRuntime, and teamloader
- Add v6→v7 migration for old rag agent field to toolset entries
- Update schema, docs, and all example YAML files

Assisted-By: docker-agent
@dgageot dgageot marked this pull request as ready for review March 22, 2026 11:30
@dgageot dgageot requested a review from a team as a code owner March 22, 2026 11:31
Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟡 NEEDS ATTENTION

This PR refactors RAG configuration from agent-level to a standard toolset type. The refactoring is well-structured and follows the established patterns for toolsets (similar to MCP).

Issues Found:

Critical (1):

  • Resource leak potential in RAG file watcher goroutine

Notable (1):

  • Shallow copy causing shared mutable state in RAG configs

Both issues should be addressed before merging to prevent runtime issues and unexpected behavior when using shared RAG definitions.

}
// Start file watcher in background
go func() {
if err := t.manager.StartFileWatcher(ctx); err != nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 CRITICAL: Potential resource leak in RAG file watcher

The file watcher is started in a fire-and-forget goroutine without checking if the context is already canceled. This can lead to:

  1. Spurious error logs if context is canceled during startup
  2. Potential race conditions between StartFileWatcher initialization and Close() calls

Recommendation: Check context before starting the watcher:

go func() {
    if ctx.Err() != nil {
        return // Context already canceled
    }
    if err := t.manager.StartFileWatcher(ctx); err != nil && ctx.Err() == nil {
        slog.Error("Failed to start RAG file watcher", "tool", t.toolName, "error", err)
    }
}()

ts.Ref = ""

if ts.RAGConfig == nil {
ts.RAGConfig = def.RAGConfig
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 ISSUE: Shallow copy causes shared mutable state

When ts.RAGConfig == nil, the code assigns ts.RAGConfig = def.RAGConfig directly. This creates a shallow copy where both the toolset and the definition share the same pointer. Mutations to the toolset's RAGConfig will affect the definition, potentially breaking other toolsets that reference the same RAG definition.

Recommendation: Deep-copy the RAGConfig before assigning:

if ts.RAGConfig == nil {
    // Deep copy to avoid shared mutable state
    copied := *def.RAGConfig
    ts.RAGConfig = &copied
    // Note: May need to deep-copy nested slices/maps as well
}

}
// InitializeRAG is a no-op. RAG initialization is now handled
// per-toolset via the tools.Startable interface.
func (r *LocalRuntime) InitializeRAG(_ context.Context, _ chan Event) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed any more?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants