Skip to content

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

Merged
dgageot merged 4 commits intodocker:mainfrom
dgageot:rag-toolset
Mar 24, 2026
Merged

Refactor RAG from agent-level config to standard toolset type#2210
dgageot merged 4 commits 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

@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
docker-agent[bot]

This comment was marked as outdated.

@dgageot dgageot marked this pull request as draft March 23, 2026 08:12
dgageot added 4 commits March 23, 2026 22:17
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
The PR docker#2210 moved RAG from agent-level config to standard toolset type
(tools.Startable) but removed the event forwarding that showed indexing
progress in the TUI sidebar.

This restores event forwarding by:
- Adding an EventCallback to RAGTool that forwards rag.Manager events
  during Start() initialization
- Having StartBackgroundRAGInit discover RAG tools from agent toolsets
  and wire up the event callback before initialization happens
- Converting RAG manager events (indexing started/progress/completed,
  usage, errors) back to runtime events for the TUI

Assisted-By: docker-agent
Signed-off-by: David Gageot <david.gageot@docker.com>
- Remove RAGInitializer interface and StartBackgroundRAGInit indirection.
  RAG event callbacks are now wired in configureToolsetHandlers alongside
  other handler setup, using the same pattern as Elicitable/OAuthCapable.

- Remove deprecated NewManagers wrapper (no callers after toolset refactor).

- Clean up RAGTool: unexport internal types (QueryRAGArgs, QueryResult),
  inline sortResults, remove verbose debug logging from Tools(), simplify
  handleQueryRAG.

Assisted-By: docker-agent
@dgageot dgageot marked this pull request as ready for review March 24, 2026 10:13
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

Found 3 issues requiring attention: 1 high-severity potential nil pointer dereference and 2 medium-severity logic issues in the RAG toolset refactoring.

return nil, errors.New("query cannot be empty")
}

results, err := t.manager.Query(ctx, args.Query)
Copy link

Choose a reason for hiding this comment

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

🔴 HIGH SEVERITY: Nil pointer dereference

The handleQueryRAG method calls t.manager.Query(ctx, args.Query) without checking if t.manager is nil. While the normal construction path via createRAGTool ensures the manager is non-nil, the Start() and Stop() methods both include defensive nil checks (if t.manager == nil { return nil }), suggesting that a nil manager is considered a valid state.

If a RAGTool is somehow constructed with a nil manager (e.g., through a test fixture or future code path), invoking the query_rag tool will panic.

Recommendation: Add a nil check at the start of handleQueryRAG:

func (t *RAGTool) handleQueryRAG(ctx context.Context, args queryRAGArgs) (*tools.ToolCallResult, error) {
    if t.manager == nil {
        return nil, fmt.Errorf("RAG manager not initialized")
    }
    // ... rest of implementation
}

// ragEventForwarder returns a callback that converts RAG manager events to runtime events.
func ragEventForwarder(ragName string, r *LocalRuntime, sendEvent func(Event)) builtin.RAGEventCallback {
return func(ragEvent ragtypes.Event) {
agentName := r.CurrentAgentName()
Copy link

Choose a reason for hiding this comment

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

⚠️ MEDIUM SEVERITY: RAG events may be misattributed after agent handoff

The ragEventForwarder closure captures the runtime and reads r.CurrentAgentName() at the time each event is processed (line 14), not when the RAG tool is initialized. If an agent starts a long-running RAG indexing operation and then hands off to another agent, subsequent indexing events will be attributed to the new agent rather than the agent that initiated the operation.

This could confuse users in the TUI, as RAG indexing progress would appear under the wrong agent's context.

Recommendation: Capture the agent name at the time the forwarder is created:

func (r *LocalRuntime) ragEventForwarder(sendEvent func(event.Event)) func(rag.Event) {
    agentName := r.CurrentAgentName()  // Capture once at creation time
    return func(e rag.Event) {
        sendEvent(event.Event{
            Type:      event.TypeRAG,
            AgentName: agentName,  // Use captured name
            RAG:       &e,
        })
    }
}

func() { events <- Authorization(tools.ElicitationActionAccept, a.Name()) },
r.managedOAuth,
)

Copy link

Choose a reason for hiding this comment

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

⚠️ MEDIUM SEVERITY: RAG initialization timing unclear

The old code in pkg/app/app.go (removed lines 119-131) called StartBackgroundRAGInit() early in the application lifecycle to begin indexing before the agent loop started. The new design relies on the tools.Startable interface and RAGTool.Start() method, but the diff doesn't show where Start() is called.

If Start() is only invoked lazily (e.g., when the tool is first used), RAG indexing will be delayed compared to the previous behavior, potentially degrading user experience.

Recommendation: Verify that Start() is called early in the toolset initialization path (e.g., in configureToolsetHandlers or during agent setup) to maintain the previous early-indexing behavior. If not already implemented, add an explicit call to start RAG tools during agent initialization.

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

I <3 when we remove things from the runtime

@dgageot dgageot merged commit 00cb2d5 into docker:main Mar 24, 2026
8 checks passed
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