Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces comprehensive support for "souls" (agent personalities) across the Dagu system, enabling users to define and select custom agent identities. The feature includes API endpoints for CRUD operations on souls, file-based persistence with YAML frontmatter and Markdown content, integration with agent sessions and system prompt generation, Git sync support for soul versioning, and UI pages for managing souls alongside existing agent configuration and chat workflows. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as Agent API
participant Store as Soul Store
participant Session as Session Manager
participant Prompt as Prompt Generator
Client->>API: POST /session (with optional soulId)
API->>Store: Load soul by ID or default
Store-->>API: Soul{id, name, content}
API->>Session: Create with Soul
Session->>Prompt: Generate system prompt + soul.Content
Prompt-->>Session: Combined prompt
Session-->>API: SessionManager ready
API-->>Client: Session created
Client->>API: Chat message
API->>Session: Process with loaded soul
Session->>Prompt: Use soul content in context
Prompt-->>Session: Response generated
Session-->>API: Result
API-->>Client: Chat response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ui/src/styles/global.css (1)
222-223:⚠️ Potential issue | 🟡 MinorSame zero-contrast pattern exists for
--sidebar-primary-foregroundin dark mode.
--sidebar-primaryand--sidebar-primary-foregroundare both#8ab4f8, which would make any text rendered assidebar-primary-foregroundon asidebar-primarybackground invisible — the exact same bug this PR fixes for--primary-foreground.🛡️ Suggested fix
--sidebar-primary: `#8ab4f8`; - --sidebar-primary-foreground: `#8ab4f8`; + --sidebar-primary-foreground: `#202124`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/styles/global.css` around lines 222 - 223, In dark-mode CSS variables, --sidebar-primary and --sidebar-primary-foreground are set to the same color (`#8ab4f8`) causing zero-contrast text; update the dark-theme variable values so --sidebar-primary-foreground contrasts with --sidebar-primary (choose a lighter or darker value depending on the sidebar background) and ensure the change is made where dark-mode variables are defined (look for --sidebar-primary and --sidebar-primary-foreground in global.css/dark theme block) so text rendered with var(--sidebar-primary-foreground) is readable against var(--sidebar-primary).ui/src/features/agent/hooks/useAgentChat.ts (1)
220-239:⚠️ Potential issue | 🟡 MinorPropagate
soulIdfor existing sessions.
sendMessageacceptssoulId, but the chat payload on Line 231-238 omits it, so a caller-selected soul won’t reach the backend once a session already exists. Consider passing it through for parity with session creation (or drop the param if overrides aren’t supported).Proposed fix
body: { message, model, dagContexts: toDagContextsBody(dagContexts), safeMode: preferences.safeMode, + soulId: soulId || undefined, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/hooks/useAgentChat.ts` around lines 220 - 239, sendMessage currently accepts a soulId but does not include it in the chat API payload, so caller-selected soulIds are not propagated for existing sessions; update the POST body in sendMessage (the client.POST to '/agent/sessions/{sessionId}/chat') to include soulId (e.g., add soulId alongside message, model, dagContexts, safeMode) to mirror startSession behavior, or if soul overrides aren't supported remove the soulId parameter from sendMessage and related callers; locate sendMessage and the startSession usage to ensure consistent handling.internal/service/frontend/api/v1/agent_config.go (1)
169-181:⚠️ Potential issue | 🟡 MinorAudit logging omits
SelectedSoulIdchanges.
buildAgentConfigChangestracksenabled,defaultModelId, andtoolPolicy, but does not includeselectedSoulId. Changes to the agent's soul selection will be invisible in the audit trail.Proposed fix
func buildAgentConfigChanges(update *api.UpdateAgentConfigRequest) map[string]any { changes := make(map[string]any) if update.Enabled != nil { changes[auditFieldEnabled] = *update.Enabled } if update.DefaultModelId != nil { changes[auditFieldDefaultModelID] = *update.DefaultModelId } if update.ToolPolicy != nil { changes[auditFieldToolPolicy] = update.ToolPolicy } + if update.SelectedSoulId != nil { + changes["selected_soul_id"] = *update.SelectedSoulId + } return changes }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/agent_config.go` around lines 169 - 181, The audit function buildAgentConfigChanges currently omits changes to SelectedSoulId; update it to check update.SelectedSoulId (from UpdateAgentConfigRequest) and, when non-nil, add it to the changes map using the audit field constant (e.g. auditFieldSelectedSoulID) so selected-soul changes are recorded in the audit trail.
🧹 Nitpick comments (19)
internal/agent/soul.go (1)
28-28:yaml:"content"onContentis misleading —Contentis the Markdown body, not a frontmatter key.The file-based store parses YAML frontmatter and the Markdown body separately;
Contentis never populated from acontent:YAML key. IfSoulis ever marshaled to YAML (debug output, serialization),content: "..."would appear as a flat frontmatter field rather than the body section, breaking the intended format. Useyaml:"-"to exclude it from YAML marshaling.♻️ Proposed fix
-Content string `yaml:"content" json:"content"` +Content string `yaml:"-" json:"content"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/soul.go` at line 28, The yaml struct tag on Soul.Content is incorrect because Content holds the Markdown body (not a frontmatter key) and should be excluded from YAML marshaling; update the struct tag for the Content field in the Soul type (symbol: Content in struct Soul in internal/agent/soul.go) to use yaml:"-" and keep the json:"content" tag (e.g. yaml:"-" json:"content") so Content is not emitted as a frontmatter key when marshaling to YAML.internal/persis/fileagentsoul/examples.go (1)
17-19:ExampleSoulIDshardcodes IDs — will silently drift if more example files are added.Consider deriving IDs dynamically from
exampleSoulsFSat init time:♻️ Proposed refactor
-func ExampleSoulIDs() []string { - return []string{"default"} -} +func ExampleSoulIDs() []string { + entries, err := exampleSoulsFS.ReadDir("examples") + if err != nil { + return nil + } + var ids []string + for _, e := range entries { + if !e.IsDir() && filepath.Ext(e.Name()) == ".md" { + ids = append(ids, strings.TrimSuffix(e.Name(), ".md")) + } + } + return ids +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/fileagentsoul/examples.go` around lines 17 - 19, ExampleSoulIDs currently returns a hardcoded slice which will drift when new example files are added; instead, compute the IDs at init by reading the embedded filesystem exampleSoulsFS and deriving names (e.g., filenames without extensions) so the function returns the current set automatically. Update ExampleSoulIDs (or create an init/populated variable used by it) to iterate exampleSoulsFS (using fs.ReadDir or fs.WalkDir) to collect IDs, sort them for deterministic order, and return that slice; ensure any errors during init are handled (panic or log) so tests fail loudly.ui/src/App.tsx (1)
194-196: Routes look correct; address.then()/.catch()anti-pattern inAgentSoulsPage.The route additions mirror the
/agent-skillspattern andAdminElementwrapping is consistent. However,ui/src/pages/agent-souls/index.tsx(around line 66) fetches the default soul ID using.then()/.catch()instead ofasync/await, and the.catch(() => {})silently swallows the error:// current (index.tsx ~line 66) useEffect(() => { client.GET('/settings/agent', { params: { query: { remoteNode } } }) .then(({ data }) => { if (data) setDefaultSoulId(data.selectedSoulId); }) .catch(() => {}); }, [client, remoteNode]);Silently discarding the error means if the request fails, no soul will appear marked as "Default" and the user has no indication something went wrong.
♻️ Suggested fix
- useEffect(() => { - client.GET('/settings/agent', { params: { query: { remoteNode } } }) - .then(({ data }) => { if (data) setDefaultSoulId(data.selectedSoulId); }) - .catch(() => {}); - }, [client, remoteNode]); + useEffect(() => { + (async () => { + const { data, error } = await client.GET('/settings/agent', { + params: { query: { remoteNode } }, + }); + if (error) { + setError(error.message || 'Failed to load agent settings'); + return; + } + if (data) setDefaultSoulId(data.selectedSoulId); + })(); + }, [client, remoteNode]);As per coding guidelines: "Prefer async/await over .then() for promise handling" and "Use error handling with try/catch for async operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/App.tsx` around lines 194 - 196, In AgentSoulsPage replace the .then()/.catch() Promise chain inside the useEffect with an async function using try/catch: create an async function inside the useEffect that awaits client.GET('/settings/agent', { params: { query: { remoteNode } } }), on success call setDefaultSoulId(data.selectedSoulId), and in the catch block log or surface the error instead of swallowing it (e.g., console.error or set an error state) so failures are visible; ensure you still call the async helper immediately and include client and remoteNode in the dependency array.internal/core/step.go (1)
354-355: Consider aligning the field name withChatRequest.SoulID.
AgentStepConfig.Soul(JSON:"soul") andChatRequest.SoulID(JSON:"soul_id") both hold a soul identifier but use different Go names and JSON tags. Compare withModel string(JSON:"model"), which is consistent across both structs. The divergence is not a bug, but it adds a small cognitive overhead when reasoning about the two types together.If the YAML-facing name
soul:is intentional for the step spec, consider at least aligning the JSON tag to"soul_id"to match the REST convention used inChatRequest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/step.go` around lines 354 - 355, AgentStepConfig.Soul uses a different Go field name and JSON tag ("soul") than ChatRequest.SoulID ("soul_id"), causing cognitive overhead; update AgentStepConfig to use the same external name by changing the JSON tag on the Soul field to `json:"soul_id,omitempty"` (or rename the field to SoulID if you prefer) so AgentStepConfig.Soul / AgentStepConfig.SoulID and ChatRequest.SoulID share a consistent JSON contract.ui/src/features/agent/components/ChatInput.tsx (2)
322-336: InitialselectedSoulstate''doesn't match anySelectItemvalue.
selectedSoulstarts as'', but noSelectItemhasvalue="". This means the Radix Select will show the placeholder text rather than a selected item, which is fine visually. However, once the user explicitly picks "default" (__default__), the component switches from uncontrolled-placeholder to controlled-value state. If you want a cleaner controlled state from the start, initializeselectedSoulto'__default__'.Suggested fix
- const [selectedSoul, setSelectedSoul] = useState<string>(''); + const [selectedSoul, setSelectedSoul] = useState<string>('__default__');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/components/ChatInput.tsx` around lines 322 - 336, selectedSoul is initialized to '' which doesn't match any SelectItem value, causing the Radix Select to start in placeholder/uncontrolled state; change the initial state for selectedSoul (the state variable used by Select value and setSelectedSoul) to '__default__' so the Select is controlled from mount and matches the SelectItem value ('__default__' shown in SelectItem); update the state initializer in the ChatInput component where selectedSoul is declared and ensure any logic that treats '' is updated to expect '__default__' instead.
30-30: Consider an options object foronSendto avoid positional optional params.
onSendnow has 4 positional parameters, 3 of which are optional. This pattern tends to accrete more optional params over time. An options object would be more extensible and readable at call sites.interface SendOptions { dagContexts?: DAGContext[]; model?: string; soulId?: string; } onSend: (message: string, options?: SendOptions) => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/components/ChatInput.tsx` at line 30, The onSend callback signature on ChatInput.tsx uses four positional parameters (message, dagContexts, model, soulId) which makes optional args brittle; change it to accept an options object instead by defining a SendOptions interface (including dagContexts?: DAGContext[], model?: string, soulId?: string) and update the onSend typing to onSend: (message: string, options?: SendOptions) => void, then update all internal callers and props that call onSend to pass a single options object rather than positional args (search for usages of onSend in this component and parent components to update their call sites).internal/core/spec/step.go (1)
1664-1669: Consider a shared constant name for ID validation.The soul ID validation correctly mirrors the skill ID pattern, but reuses
maxSkillIDLengthandvalidSkillIDRegexpwhich are named specifically for skills. If the validation rules are intended to be identical for all slug-like IDs (skills, souls), a more generic name (e.g.,maxSlugLength,validSlugRegexp) would better communicate intent and avoid confusion if constraints later diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/spec/step.go` around lines 1664 - 1669, The soul ID validation reuses skill-specific constants (maxSkillIDLength, validSkillIDRegexp) which is confusing; rename or introduce shared, generic constants (e.g., maxSlugLength and validSlugRegexp) used by all slug-like IDs and update usages in the validation logic (the block referencing result.Agent.Soul) and any other places that currently reference maxSkillIDLength/validSkillIDRegexp so they consistently use the new generic names; ensure the error message text is updated if it references "skill" to use a neutral term or the appropriate entity name.internal/agent/system_prompt.go (1)
53-53: Consider a params struct to reduce parameter count.
GenerateSystemPromptnow takes 7 positional parameters, making call sites harder to read and prone to argument-ordering mistakes. ASystemPromptParamsstruct would improve clarity and make future additions safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/system_prompt.go` at line 53, Replace the long parameter list of GenerateSystemPrompt by creating a SystemPromptParams struct containing the current fields (EnvironmentInfo Env, CurrentDAG *CurrentDAG, Memory MemoryContent, Role auth.Role, AvailableSkills []SkillSummary, SkillCount int, Soul *Soul) and change GenerateSystemPrompt signature to accept a single params SystemPromptParams; update the function body to reference params.Env, params.CurrentDAG, etc., and update all callers to construct and pass SystemPromptParams (and adjust any tests/mocks) so argument ordering is no longer error-prone.ui/src/pages/agent-souls/index.tsx (1)
60-64: Preferasync/awaitover.then()per coding guidelines.This is the only place in the file using the
.then()pattern. The rest of the handlers correctly useasync/await.Proposed fix
useEffect(() => { - client.GET('/settings/agent', { params: { query: { remoteNode } } }) - .then(({ data }) => { if (data) setDefaultSoulId(data.selectedSoulId); }) - .catch(() => {}); + (async () => { + try { + const { data } = await client.GET('/settings/agent', { params: { query: { remoteNode } } }); + if (data) setDefaultSoulId(data.selectedSoulId); + } catch { + // Best-effort + } + })(); }, [client, remoteNode]);As per coding guidelines, "Prefer async/await over .then() for promise handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/agent-souls/index.tsx` around lines 60 - 64, Convert the useEffect callback to an async function and replace the .then()/.catch() chain with await and try/catch: call await client.GET('/settings/agent', { params: { query: { remoteNode } } }) inside the effect, extract data and call setDefaultSoulId(data.selectedSoulId) if present, and handle errors in a catch block (instead of an empty .catch()). Keep the same dependency array ([client, remoteNode]) and reference the existing symbols useEffect, client.GET, setDefaultSoulId, and remoteNode when locating the code to change.ui/src/pages/agent-souls/SoulEditorPage.tsx (1)
50-71: Fetch effect lacks an abort mechanism for unmount/re-render.If the component unmounts or
soulId/remoteNodechanges while the fetch is in-flight, the async IIFE will still callsetName,setContent, etc. on an unmounted component. Consider using anAbortControlleror a stale-closure guard.Example: abort signal
useEffect(() => { if (isCreating || !soulId) return; + const controller = new AbortController(); (async () => { const { data, error } = await client.GET('/settings/agent/souls/{soulId}', { params: { path: { soulId }, query: { remoteNode } }, + signal: controller.signal, }); + if (controller.signal.aborted) return; if (error) { showError(error.message || 'Failed to load soul'); navigate('/agent-souls'); return; } setName(data.name); // ... rest of state setting setIsLoading(false); })(); + + return () => controller.abort(); }, [soulId, isCreating, client, remoteNode, showError, navigate]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/agent-souls/SoulEditorPage.tsx` around lines 50 - 71, In SoulEditorPage's useEffect the async IIFE calls state setters (setName, setContent, setIdField, setDescription, setVersion, setAuthor, setTagsInput, setIsLoading) after an awaited client.GET and can update state after unmount or param change; fix by adding an AbortController or a local isMounted/stale guard: create controller/flag before the request, pass controller.signal to client.GET if supported, and in the effect cleanup call controller.abort() or set the flag false; after the await check controller.signal.aborted or the flag and return early before calling any setters or navigate to avoid updating unmounted component.internal/agent/system_prompt_test.go (1)
191-199: Consider adding a test case for non-nil soul content.The existing tests only pass
nilfor the soul parameter. There's no test verifying that when aSoulis provided, itsContentactually appears in the generated prompt. This leaves the primary new code path untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/system_prompt_test.go` around lines 191 - 199, Add a test that passes a non-nil Soul with populated Content into GenerateSystemPrompt and asserts the returned string contains that Content; specifically, create a Soul (e.g., soul := &Soul{Content: "expected content"}) and call GenerateSystemPrompt(env, nil, MemoryContent{}, auth.RoleViewer, soul, 50, nil), then use assert.Contains(t, result, "expected content") to verify the new code path handling Soul.Content is exercised.internal/persis/fileagentsoul/store_test.go (1)
117-135: Search test only covers query-based filtering; no tag-based search test.
TestStore_Searchfilters byQuery: "ops"but doesn't exercise tag-based filtering (e.g.,SearchSoulsOptions{Tags: []string{"dev"}}). If the store supports tag filtering, consider adding a sub-test for it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/fileagentsoul/store_test.go` around lines 117 - 135, TestStore_Search only asserts query-based filtering and misses a tag-based case; add a sub-test in TestStore_Search that calls store.Search with agent.SearchSoulsOptions{Tags: []string{"dev"}} (or similar) and asserts result.TotalCount == 1 and result.Items[0].ID == "dev-soul". Use the existing setup that creates two agent.Soul entries and reuse the same store variable and context; add assertions for no error and the expected item to ensure tag filtering in store.Search is exercised.ui/src/pages/git-sync/index.tsx (1)
500-500: Deeply nested ternary chains are getting unwieldy.Lines 500, 611, and 617 stack 4+ ternary operators inline. This was already present for the other types, but adding
soulpushes readability further. Consider extracting small helper functions or lookup maps.Example: extract a label map
const typeLabels: Record<TypeFilter, string> = { all: 'All', dag: 'DAGs', memory: 'Memory', skill: 'Skills', soul: 'Souls', };Then use
typeLabels[f]in the filter button andtypeLabels[typeFilter]in the empty-state message, replacing the nested ternaries.Also applies to: 611-611, 617-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/git-sync/index.tsx` at line 500, Replace the nested ternary that computes typeLabel (and the two other similar inline ternaries at usages around the other occurrences) with a single lookup map or small helper to improve readability; e.g. create a const like typeLabels: Record<TypeFilter, string> (or a function getTypeLabel(typeFilter): string) that maps 'all'|'dag'|'memory'|'skill'|'soul' to the desired display strings, then use typeLabels[typeFilter] (or getTypeLabel(typeFilter)) wherever typeLabel or the inline ternaries are used (including the spots tied to the original typeLabel computation and the occurrences referenced at ~611 and ~617) so you remove the deep nested ternary chains.internal/persis/fileagentsoul/store.go (2)
87-136:rebuildIndexusesslog.Warndirectly instead of the project logger.Lines 112–115 and 127–130 call
slog.Warndirectly. The rest of the codebase routes throughinternal/cmn/logger(e.g.,logger.Warn(ctx, ...)). SincerebuildIndexhas no context, either pass one through fromNew, or at minimum useslog.Default().Warn(...)to stay consistent with how directslogcalls are made elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/fileagentsoul/store.go` around lines 87 - 136, rebuildIndex currently calls slog.Warn directly; update it to use the project logging convention by either (a) accept and store a context or logger on Store during construction (e.g., set a field in New so Store has ctx or logger) and replace slog.Warn calls in rebuildIndex with logger.Warn(ctx, ...) (referencing Store, New, and rebuildIndex to locate the change), or (b) if you cannot pass context, call slog.Default().Warn(...) instead of slog.Warn(...) so the call matches other direct slog usages; update both warning sites (the loadSoulFromFile error log and the duplicate name warning) accordingly.
433-488:Updateloads the full soul file from disk while holding the write lock — avoidable.
loadSoulFromFileat line 458 is performed solely to getexisting.Namefor name-change detection (lines 463 and 483). Butentry.name— already fetched from the in-memory index at line 448 — holds exactly that value. The file read under the write lock adds unnecessary I/O latency and blocks all concurrent readers/writers for longer than needed.♻️ Proposed fix: replace disk read with in-memory entry.name
- existing, err := loadSoulFromFile(filePath, soul.ID) - if err != nil { - return fmt.Errorf("fileagentsoul: failed to load existing soul: %w", err) - } - - nameChanged := existing.Name != soul.Name + nameChanged := entry.name != soul.Name if nameChanged { if takenByID, taken := s.byName[soul.Name]; taken && takenByID != soul.ID { return agent.ErrSoulNameAlreadyExists } } if err := writeSoulToFile(filePath, soul); err != nil { return err } // Update cached metadata. entry.name = soul.Name entry.description = soul.Description entry.tags = soul.Tags entry.contentSize = len(soul.Content) entry.version = soul.Version entry.author = soul.Author if nameChanged { - delete(s.byName, existing.Name) + delete(s.byName, entry.name) // capture before overwriting s.byName[soul.Name] = soul.ID }Note: capture
oldName := entry.namebefore theentry.name = soul.Nameassignment if the delete and the assign are in the same block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/fileagentsoul/store.go` around lines 433 - 488, In Store.Update replace the disk read via loadSoulFromFile(filePath, soul.ID) and the use of existing.Name with the in-memory index value entry.name: compute nameChanged by comparing entry.name to soul.Name (e.g., oldName := entry.name; nameChanged := oldName != soul.Name), remove the loadSoulFromFile call and its error handling, and when updating the cache if nameChanged use oldName to delete from s.byName and then set s.byName[soul.Name] = soul.ID; keep all other updates (writeSoulToFile, entry assignments) the same and ensure oldName is captured before you assign entry.name = soul.Name.internal/service/frontend/api/v1/agent_souls.go (1)
295-304:collectSoulIDscallsList(full disk reads) when only IDs are needed.
agentSoulStore.List(ctx)reads every soul file from disk and returns full*agent.Soulobjects includingContent. Only theIDfields are used.agentSoulStore.Searchoperates entirely on the in-memory index (zero I/O) and returnsSoulMetadatawhich includes IDs — a much cheaper alternative for this purpose.♻️ Proposed refactor using Search
func (a *API) collectSoulIDs(ctx context.Context) map[string]struct{} { - souls, err := a.agentSoulStore.List(ctx) + result, err := a.agentSoulStore.Search(ctx, agent.SearchSoulsOptions{ + Paginator: exec.DefaultPaginator(), + }) if err != nil { return make(map[string]struct{}) } - ids := make(map[string]struct{}, len(souls)) - for _, s := range souls { - ids[s.ID] = struct{}{} + ids := make(map[string]struct{}, len(result.Items)) + for _, m := range result.Items { + ids[m.ID] = struct{}{} } return ids }Note: Ensure
exec.DefaultPaginator()returns enough items for all expected souls, or paginate until exhausted. If the default limit is smaller than the total soul count, ID generation may collide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/frontend/api/v1/agent_souls.go` around lines 295 - 304, collectSoulIDs currently calls agentSoulStore.List which reads full soul objects from disk; change it to use agentSoulStore.Search to fetch only metadata (SoulMetadata) from the in-memory index and build the ids map from the returned metadata IDs. In collectSoulIDs, call a Search with an empty query (or appropriate filter) and a paginator (use exec.DefaultPaginator() and, if necessary, loop pages until exhausted) to retrieve all SoulMetadata items, then populate and return the map[string]struct{} using metadata.IDs; reference agentSoulStore.Search, exec.DefaultPaginator(), and SoulMetadata when making the change.api/v1/api.yaml (3)
4741-4758: ExtractsoulIdinto a reusable parameter with basic validation.Defining a shared
SoulIdparameter (withminLength: 1) avoids empty IDs and removes duplication across GET/PATCH/DELETE.♻️ Suggested refactor (parameter reuse + validation)
/settings/agent/souls/{soulId}: get: @@ parameters: - $ref: "#/components/parameters/RemoteNode" - - name: soulId - in: path - required: true - schema: - type: string - description: "Soul ID" + - $ref: "#/components/parameters/SoulId" @@ components: parameters: + SoulId: + name: soulId + in: path + description: "Soul ID" + required: true + schema: + type: string + minLength: 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 4741 - 4758, Extract the inline path parameter "soulId" into a reusable parameter component named "SoulId" under components.parameters, add basic validation (schema type: string and minLength: 1) to prevent empty IDs, and replace the repeated inline definitions in the GET/PATCH/DELETE operations with a $ref to "#/components/parameters/SoulId"; update any references that currently use the inline "soulId" definition so they point to the new "SoulId" component.
7947-7985: Consider a lightweight list payload for souls.
ListSoulsResponsereturns full markdowncontentfor every soul, which may inflate list responses. A summary schema (orincludeContentflag) keeps list calls lean.📦 Example summary schema for list responses
SoulResponse: type: object required: - id - name @@ content: type: string description: "Markdown body content (identity definition)" + SoulSummaryResponse: + type: object + required: + - id + - name + description: "Soul summary without markdown content" + properties: + id: + type: string + name: + type: string + description: + type: string + version: + type: string + author: + type: string + tags: + type: array + items: + type: string + ListSoulsResponse: type: object @@ souls: type: array items: - $ref: "#/components/schemas/SoulResponse" + $ref: "#/components/schemas/SoulSummaryResponse"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 7947 - 7985, Create a lightweight summary schema and use it for list responses: add a new schema (e.g., SoulSummary) mirroring SoulResponse but omitting the heavy content property, then update ListSoulsResponse so its souls array items $ref "#/components/schemas/SoulSummary" instead of SoulResponse; optionally add an includeContent query parameter to the list souls operation to return full SoulResponse when true and document that behavior in the API description.
7633-7639: Clarify how clients clear the selected soul (optional).If unsetting is supported, consider allowing
nullexplicitly (and mirror the same in the response schema) to remove ambiguity.🧩 Optional schema tweak
selectedSoulId: type: string description: "ID of the soul to select" + nullable: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 7633 - 7639, The selectedSoulId field is ambiguous about how clients clear a selection; update the OpenAPI schema so selectedSoulId explicitly allows null (e.g., type: [\"string\",\"null\"]) wherever it appears in request and response schemas to indicate clients can unset the selection, and update any related response schema that returns selectedSoulId to mirror the same nullable type; look for the selectedSoulId property near the AgentToolPolicy reference to make the change consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/spec/defaults.go`:
- Around line 104-133: The agent defaults code in applyDefaults currently uses
zero-value checks on s.Agent to decide whether to apply defaults from d.Agent,
which incorrectly overrides explicit empty values (e.g. prompt: "" or soul: "")
present in the raw YAML; change the logic to consult the raw step agent map
(e.g. the parsed raw map for the step like s.Raw or rawAgent) to see if a key is
present before applying a default, and only copy each field from d.Agent into
s.Agent when that key is absent in the raw agent map; apply this presence-check
approach for all fields referenced (Model, Tools, Skills, Soul, Memory, Prompt,
MaxIterations, SafeMode) and keep the existing s.Agent initialization (s.Agent =
&agentConfig{}) behavior.
In `@internal/persis/fileagentsoul/examples.go`:
- Around line 64-75: The marker file is written and true is returned even if all
os.WriteFile calls for soul files failed; add a boolean flag (e.g., wroteAny)
initialized false, set it true inside the loop when a soul write (os.WriteFile
to destPath) succeeds, and then only write markerContent to markerPath and
return true if wroteAny is true; otherwise log the failure path(s) are already
warned and return false without creating the marker. Ensure you update the code
paths that currently call os.WriteFile for destPath, reference
markerPath/markerContent/filePermissions, and the final slog.Info to reflect
successful creation only when wroteAny is true.
In `@internal/persis/fileagentsoul/store.go`:
- Around line 143-157: The closing-delimiter detection in parseSoulFile is
fragile because using strings.Index(rest, "\n---") can match `---` inside YAML
values; instead iterate over lines in rest (e.g., using strings.Split or a line
scanner) and locate the first line that is exactly "---" (accounting for
optional CRLF) to set closingIdx and delimLen correctly; update the code that
currently uses rest, closingIdx and delimLen so it computes the byte offset of
that exact-line match (not any substring starting with "\n---"), and preserve
existing behavior when no closing delimiter is found.
In `@internal/runtime/builtin/agentstep/executor.go`:
- Around line 146-155: The code calls soulStore.GetByID(ctx, stepCfg.Soul) and
discards the returned error, which masks store failures; update the soul loading
block (variables: stepCfg, soulStore, GetByID, soul, logf, stderr) to capture
the error (e.g., soul, err := soulStore.GetByID(...)), then if err != nil log
the actual error via logf(stderr, ...) (with context like "error loading soul
%q: %v") and only treat soul==nil without err as the "not found in store" case;
ensure you preserve current warning behavior but surface real I/O/parsing
errors.
In `@internal/service/frontend/api/v1/agent_souls_test.go`:
- Around line 217-220: The test file uses invalid new("...") calls (e.g.,
new("my-custom-soul")) which won't compile; add a small helper function func
strPtr(v string) *string { return &v } near the top of the test file and replace
every new("...") occurrence (lines referencing the test structs where
Id/Name/Content are set) with strPtr("...") so string literals are converted to
*string; ensure all instances (including the ones around Id fields and any other
string pointer fields) are updated to use strPtr.
In `@internal/service/frontend/api/v1/agent_souls.go`:
- Around line 200-214: The update handler currently maps
agent.ErrSoulNameAlreadyExists to the generic errSoulAlreadyExists, which is
misleading for name-collision cases; introduce a new error value (e.g.,
errSoulNameConflict) and change the UpdateAgentSoul logic to return
errSoulNameConflict when agentSoulStore.Update returns
agent.ErrSoulNameAlreadyExists while keeping the existing mapping for
agent.ErrSoulAlreadyExists; update any references/exports as needed so clients
receive a distinct, descriptive error for name conflicts instead of the generic
"Soul already exists".
- Around line 89-136: In CreateAgentSoul, trim body.Name and body.Content before
validation, ID generation, and storing so the stored soul matches
UpdateAgentSoul behavior: call strings.TrimSpace on body.Name and body.Content
at the start (before using valueOf(body.Id), agent.UniqueID or
agent.ValidateSoulID) and use the trimmed values when constructing the
agent.Soul (ID generation via agent.UniqueID should receive the trimmed name so
collectSoulIDs/UniqueID dedupe works correctly); ensure any subsequent checks
(e.g., agent.ValidateSoulID, byName lookups) operate on the trimmed name to
avoid near-duplicate entries and lookup mismatches with applySoulUpdates.
In `@rfcs/019-soul-md.md`:
- Around line 169-173: Update the RFC markdown to reflect that per-DAG/per-step
soul overrides are implemented: remove or move the bullet "Per-DAG soul
overrides in agent step configuration" from the Out of Scope list into the In
Scope list (or delete it entirely) and add a short note referencing the
implemented agent config field (agent: { soul: "soul-id" }) to clarify that
per-step soul configuration is supported; ensure the change is applied in
rfcs/019-soul-md.md near the section containing the listed out-of-scope items
and keep wording concise.
In `@ui/src/pages/agent-settings/index.tsx`:
- Around line 539-553: The Select control uses a hardcoded fallback value
'default' which doesn't match any SelectItem and prevents deselection; remove
the fallback (use value={selectedSoulId}) and add a "None" or "Default
assistant" option as a SelectItem (e.g., value="" or value={null/empty string})
inside the SelectContent alongside mapping over souls; update onValueChange (the
handler that calls setSelectedSoulId) to convert the empty string back to
undefined (or the intended "no selection" sentinel) so users can unset the soul.
---
Outside diff comments:
In `@internal/service/frontend/api/v1/agent_config.go`:
- Around line 169-181: The audit function buildAgentConfigChanges currently
omits changes to SelectedSoulId; update it to check update.SelectedSoulId (from
UpdateAgentConfigRequest) and, when non-nil, add it to the changes map using the
audit field constant (e.g. auditFieldSelectedSoulID) so selected-soul changes
are recorded in the audit trail.
In `@ui/src/features/agent/hooks/useAgentChat.ts`:
- Around line 220-239: sendMessage currently accepts a soulId but does not
include it in the chat API payload, so caller-selected soulIds are not
propagated for existing sessions; update the POST body in sendMessage (the
client.POST to '/agent/sessions/{sessionId}/chat') to include soulId (e.g., add
soulId alongside message, model, dagContexts, safeMode) to mirror startSession
behavior, or if soul overrides aren't supported remove the soulId parameter from
sendMessage and related callers; locate sendMessage and the startSession usage
to ensure consistent handling.
In `@ui/src/styles/global.css`:
- Around line 222-223: In dark-mode CSS variables, --sidebar-primary and
--sidebar-primary-foreground are set to the same color (`#8ab4f8`) causing
zero-contrast text; update the dark-theme variable values so
--sidebar-primary-foreground contrasts with --sidebar-primary (choose a lighter
or darker value depending on the sidebar background) and ensure the change is
made where dark-mode variables are defined (look for --sidebar-primary and
--sidebar-primary-foreground in global.css/dark theme block) so text rendered
with var(--sidebar-primary-foreground) is readable against
var(--sidebar-primary).
---
Nitpick comments:
In `@api/v1/api.yaml`:
- Around line 4741-4758: Extract the inline path parameter "soulId" into a
reusable parameter component named "SoulId" under components.parameters, add
basic validation (schema type: string and minLength: 1) to prevent empty IDs,
and replace the repeated inline definitions in the GET/PATCH/DELETE operations
with a $ref to "#/components/parameters/SoulId"; update any references that
currently use the inline "soulId" definition so they point to the new "SoulId"
component.
- Around line 7947-7985: Create a lightweight summary schema and use it for list
responses: add a new schema (e.g., SoulSummary) mirroring SoulResponse but
omitting the heavy content property, then update ListSoulsResponse so its souls
array items $ref "#/components/schemas/SoulSummary" instead of SoulResponse;
optionally add an includeContent query parameter to the list souls operation to
return full SoulResponse when true and document that behavior in the API
description.
- Around line 7633-7639: The selectedSoulId field is ambiguous about how clients
clear a selection; update the OpenAPI schema so selectedSoulId explicitly allows
null (e.g., type: [\"string\",\"null\"]) wherever it appears in request and
response schemas to indicate clients can unset the selection, and update any
related response schema that returns selectedSoulId to mirror the same nullable
type; look for the selectedSoulId property near the AgentToolPolicy reference to
make the change consistently.
In `@internal/agent/soul.go`:
- Line 28: The yaml struct tag on Soul.Content is incorrect because Content
holds the Markdown body (not a frontmatter key) and should be excluded from YAML
marshaling; update the struct tag for the Content field in the Soul type
(symbol: Content in struct Soul in internal/agent/soul.go) to use yaml:"-" and
keep the json:"content" tag (e.g. yaml:"-" json:"content") so Content is not
emitted as a frontmatter key when marshaling to YAML.
In `@internal/agent/system_prompt_test.go`:
- Around line 191-199: Add a test that passes a non-nil Soul with populated
Content into GenerateSystemPrompt and asserts the returned string contains that
Content; specifically, create a Soul (e.g., soul := &Soul{Content: "expected
content"}) and call GenerateSystemPrompt(env, nil, MemoryContent{},
auth.RoleViewer, soul, 50, nil), then use assert.Contains(t, result, "expected
content") to verify the new code path handling Soul.Content is exercised.
In `@internal/agent/system_prompt.go`:
- Line 53: Replace the long parameter list of GenerateSystemPrompt by creating a
SystemPromptParams struct containing the current fields (EnvironmentInfo Env,
CurrentDAG *CurrentDAG, Memory MemoryContent, Role auth.Role, AvailableSkills
[]SkillSummary, SkillCount int, Soul *Soul) and change GenerateSystemPrompt
signature to accept a single params SystemPromptParams; update the function body
to reference params.Env, params.CurrentDAG, etc., and update all callers to
construct and pass SystemPromptParams (and adjust any tests/mocks) so argument
ordering is no longer error-prone.
In `@internal/core/spec/step.go`:
- Around line 1664-1669: The soul ID validation reuses skill-specific constants
(maxSkillIDLength, validSkillIDRegexp) which is confusing; rename or introduce
shared, generic constants (e.g., maxSlugLength and validSlugRegexp) used by all
slug-like IDs and update usages in the validation logic (the block referencing
result.Agent.Soul) and any other places that currently reference
maxSkillIDLength/validSkillIDRegexp so they consistently use the new generic
names; ensure the error message text is updated if it references "skill" to use
a neutral term or the appropriate entity name.
In `@internal/core/step.go`:
- Around line 354-355: AgentStepConfig.Soul uses a different Go field name and
JSON tag ("soul") than ChatRequest.SoulID ("soul_id"), causing cognitive
overhead; update AgentStepConfig to use the same external name by changing the
JSON tag on the Soul field to `json:"soul_id,omitempty"` (or rename the field to
SoulID if you prefer) so AgentStepConfig.Soul / AgentStepConfig.SoulID and
ChatRequest.SoulID share a consistent JSON contract.
In `@internal/persis/fileagentsoul/examples.go`:
- Around line 17-19: ExampleSoulIDs currently returns a hardcoded slice which
will drift when new example files are added; instead, compute the IDs at init by
reading the embedded filesystem exampleSoulsFS and deriving names (e.g.,
filenames without extensions) so the function returns the current set
automatically. Update ExampleSoulIDs (or create an init/populated variable used
by it) to iterate exampleSoulsFS (using fs.ReadDir or fs.WalkDir) to collect
IDs, sort them for deterministic order, and return that slice; ensure any errors
during init are handled (panic or log) so tests fail loudly.
In `@internal/persis/fileagentsoul/store_test.go`:
- Around line 117-135: TestStore_Search only asserts query-based filtering and
misses a tag-based case; add a sub-test in TestStore_Search that calls
store.Search with agent.SearchSoulsOptions{Tags: []string{"dev"}} (or similar)
and asserts result.TotalCount == 1 and result.Items[0].ID == "dev-soul". Use the
existing setup that creates two agent.Soul entries and reuse the same store
variable and context; add assertions for no error and the expected item to
ensure tag filtering in store.Search is exercised.
In `@internal/persis/fileagentsoul/store.go`:
- Around line 87-136: rebuildIndex currently calls slog.Warn directly; update it
to use the project logging convention by either (a) accept and store a context
or logger on Store during construction (e.g., set a field in New so Store has
ctx or logger) and replace slog.Warn calls in rebuildIndex with logger.Warn(ctx,
...) (referencing Store, New, and rebuildIndex to locate the change), or (b) if
you cannot pass context, call slog.Default().Warn(...) instead of slog.Warn(...)
so the call matches other direct slog usages; update both warning sites (the
loadSoulFromFile error log and the duplicate name warning) accordingly.
- Around line 433-488: In Store.Update replace the disk read via
loadSoulFromFile(filePath, soul.ID) and the use of existing.Name with the
in-memory index value entry.name: compute nameChanged by comparing entry.name to
soul.Name (e.g., oldName := entry.name; nameChanged := oldName != soul.Name),
remove the loadSoulFromFile call and its error handling, and when updating the
cache if nameChanged use oldName to delete from s.byName and then set
s.byName[soul.Name] = soul.ID; keep all other updates (writeSoulToFile, entry
assignments) the same and ensure oldName is captured before you assign
entry.name = soul.Name.
In `@internal/service/frontend/api/v1/agent_souls.go`:
- Around line 295-304: collectSoulIDs currently calls agentSoulStore.List which
reads full soul objects from disk; change it to use agentSoulStore.Search to
fetch only metadata (SoulMetadata) from the in-memory index and build the ids
map from the returned metadata IDs. In collectSoulIDs, call a Search with an
empty query (or appropriate filter) and a paginator (use exec.DefaultPaginator()
and, if necessary, loop pages until exhausted) to retrieve all SoulMetadata
items, then populate and return the map[string]struct{} using metadata.IDs;
reference agentSoulStore.Search, exec.DefaultPaginator(), and SoulMetadata when
making the change.
In `@ui/src/App.tsx`:
- Around line 194-196: In AgentSoulsPage replace the .then()/.catch() Promise
chain inside the useEffect with an async function using try/catch: create an
async function inside the useEffect that awaits client.GET('/settings/agent', {
params: { query: { remoteNode } } }), on success call
setDefaultSoulId(data.selectedSoulId), and in the catch block log or surface the
error instead of swallowing it (e.g., console.error or set an error state) so
failures are visible; ensure you still call the async helper immediately and
include client and remoteNode in the dependency array.
In `@ui/src/features/agent/components/ChatInput.tsx`:
- Around line 322-336: selectedSoul is initialized to '' which doesn't match any
SelectItem value, causing the Radix Select to start in placeholder/uncontrolled
state; change the initial state for selectedSoul (the state variable used by
Select value and setSelectedSoul) to '__default__' so the Select is controlled
from mount and matches the SelectItem value ('__default__' shown in SelectItem);
update the state initializer in the ChatInput component where selectedSoul is
declared and ensure any logic that treats '' is updated to expect '__default__'
instead.
- Line 30: The onSend callback signature on ChatInput.tsx uses four positional
parameters (message, dagContexts, model, soulId) which makes optional args
brittle; change it to accept an options object instead by defining a SendOptions
interface (including dagContexts?: DAGContext[], model?: string, soulId?:
string) and update the onSend typing to onSend: (message: string, options?:
SendOptions) => void, then update all internal callers and props that call
onSend to pass a single options object rather than positional args (search for
usages of onSend in this component and parent components to update their call
sites).
In `@ui/src/pages/agent-souls/index.tsx`:
- Around line 60-64: Convert the useEffect callback to an async function and
replace the .then()/.catch() chain with await and try/catch: call await
client.GET('/settings/agent', { params: { query: { remoteNode } } }) inside the
effect, extract data and call setDefaultSoulId(data.selectedSoulId) if present,
and handle errors in a catch block (instead of an empty .catch()). Keep the same
dependency array ([client, remoteNode]) and reference the existing symbols
useEffect, client.GET, setDefaultSoulId, and remoteNode when locating the code
to change.
In `@ui/src/pages/agent-souls/SoulEditorPage.tsx`:
- Around line 50-71: In SoulEditorPage's useEffect the async IIFE calls state
setters (setName, setContent, setIdField, setDescription, setVersion, setAuthor,
setTagsInput, setIsLoading) after an awaited client.GET and can update state
after unmount or param change; fix by adding an AbortController or a local
isMounted/stale guard: create controller/flag before the request, pass
controller.signal to client.GET if supported, and in the effect cleanup call
controller.abort() or set the flag false; after the await check
controller.signal.aborted or the flag and return early before calling any
setters or navigate to avoid updating unmounted component.
In `@ui/src/pages/git-sync/index.tsx`:
- Line 500: Replace the nested ternary that computes typeLabel (and the two
other similar inline ternaries at usages around the other occurrences) with a
single lookup map or small helper to improve readability; e.g. create a const
like typeLabels: Record<TypeFilter, string> (or a function
getTypeLabel(typeFilter): string) that maps 'all'|'dag'|'memory'|'skill'|'soul'
to the desired display strings, then use typeLabels[typeFilter] (or
getTypeLabel(typeFilter)) wherever typeLabel or the inline ternaries are used
(including the spots tied to the original typeLabel computation and the
occurrences referenced at ~611 and ~617) so you remove the deep nested ternary
chains.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1696 +/- ##
==========================================
- Coverage 70.20% 69.64% -0.56%
==========================================
Files 369 372 +3
Lines 40878 41380 +502
==========================================
+ Hits 28699 28820 +121
- Misses 9906 10108 +202
- Partials 2273 2452 +179
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit