internal/persis/fileagentmodel/store_test.go (1)

329-346: LGTM — correct error assertions and good new coverage.

ErrModelNameAlreadyExists is the right sentinel now. The "empty name returns error" sub-test is properly parallelised and covers the new validation path. The string-match assertion (assert.Contains) is acceptable since no sentinel error exists for the "name required" case; optionally, defining ErrModelNameRequired would let you use assert.ErrorIs consistently with the rest of the tests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/persis/fileagentmodel/store_test.go` around lines 329 - 346, Add a
sentinel error ErrModelNameRequired and return it from the model-name validation
path used by store.Update so callers can check the exact error; then update the
"empty name returns error" sub-test in store_test.go to assert.ErrorIs(t, err,
agent.ErrModelNameRequired) instead of assert.Contains. Locate the validation
logic invoked by the Update method (the code that currently produces the "model
name is required" string) and replace the string-only error with the new
ErrModelNameRequired value so the test and other callers can use error identity
checks.

ui/src/features/agent/components/messages/SubAgentChips.tsx (1)

30-30: Consider narrowing the useMemo dependency to toolCall.function.arguments.

The current dependency [toolCall] triggers recomputation on any object reference change, even if the arguments string hasn't changed. Since parseDelegateTasks only reads toolCall.function.arguments, using that as the dependency is more precise.

♻️ Suggested change

-  const tasks = useMemo(() => parseDelegateTasks(toolCall), [toolCall]);
+  const tasks = useMemo(() => parseDelegateTasks(toolCall), [toolCall.function.arguments]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/src/features/agent/components/messages/SubAgentChips.tsx` at line 30, The
useMemo call in SubAgentChips (const tasks = useMemo(() =>
parseDelegateTasks(toolCall), [toolCall])) is too broad and causes unnecessary
recomputation; change the dependency array to depend specifically on
toolCall.function.arguments (e.g., [toolCall.function.arguments]) so
parseDelegateTasks only reruns when the arguments string actually changes,
keeping the same parseDelegateTasks and useMemo usage.

internal/core/spec/step.go (1)

1636-1640: Duplicated regex acknowledged — consider a shared internal package if this pattern spreads further.

The comment on Line 1637 explains the duplication is to avoid an import cycle (spec → agent → spec). This is pragmatic for now, but if more validation constants end up duplicated, consider extracting them into a small leaf package (e.g., internal/core/slugid) that both spec and agent can import.

🤖 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 1636 - 1640, The comment notes a
duplicated regexp; to prevent future duplication extract the shared regexp and
constants into a small leaf package (e.g., internal/core/slugid) and update both
packages to import it: move validSkillIDRegexp and maxSkillIDLength (and the
original agent.validSlugRegexp) into the new package, expose them with clear
names (e.g., SlugRegexp, MaxSlugLength), update references in spec
(validSkillIDRegexp) and agent to use the new package, and run/update tests to
ensure no import cycles or behavior changes.

internal/agent/skill.go (1)

96-118: LoadSkillSummaries issues N individual GetByID calls — acceptable for small lists, but document the expectation.

For typical per-agent skill lists (single digits), this is fine. If this ever needs to handle larger sets, a batch-fetch method on SkillStore would avoid the N+1 pattern.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agent/skill.go` around lines 96 - 118, LoadSkillSummaries currently
issues N individual SkillStore.GetByID calls (N+1 pattern) which is fine for
small per-agent lists but should be documented and made easy to change; update
the comment and/or function documentation for LoadSkillSummaries to state it
performs one GetByID per enabled ID and is intended for small lists
(single-digit), and add a TODO suggesting adding a batch method on SkillStore
(e.g., GetByIDs or ListByIDs) to replace the loop if larger sets are needed;
reference LoadSkillSummaries and SkillStore.GetByID in the note so future
maintainers know where to modify.

ui/src/features/agent/components/messages/AssistantMessage.tsx (2)

25-26: Hardcoded 'delegate' tool name — extract to a constant.

If the tool name is ever renamed, both filter lines silently stop working. A shared constant (e.g., in constants.ts or an agentTools.ts) would make the coupling explicit.

♻️ Proposed change

In constants.ts:

+export const DELEGATE_TOOL_NAME = 'delegate';

In AssistantMessage.tsx:

+import { DELEGATE_TOOL_NAME } from '../../constants';
 ...
-const delegateCalls = toolCalls?.filter((tc) => tc.function.name === 'delegate') ?? [];
-const otherCalls = toolCalls?.filter((tc) => tc.function.name !== 'delegate') ?? [];
+const delegateCalls = toolCalls?.filter((tc) => tc.function.name === DELEGATE_TOOL_NAME) ?? [];
+const otherCalls = toolCalls?.filter((tc) => tc.function.name !== DELEGATE_TOOL_NAME) ?? [];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/src/features/agent/components/messages/AssistantMessage.tsx` around lines
25 - 26, The two filters in AssistantMessage.tsx use the hardcoded tool name
'delegate' (seen in delegateCalls and otherCalls) — extract that string into a
shared constant (e.g., DELEGATE_TOOL_NAME) placed in a constants file
(constants.ts or agentTools.ts) and import it into AssistantMessage.tsx; update
both filters to compare tc.function.name against that constant so renaming the
tool only requires changing the constant.

---

15-17: content: string should be typed as content?: string.

Message.content is string | undefined (optional in the interface). Typing this prop as required forces callers to write content={message.content ?? ''}. Since the render already guards with {content && ...}, marking it optional is cleaner and avoids the mismatch.

♻️ Proposed change

-  content: string;
+  content?: string;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/src/features/agent/components/messages/AssistantMessage.tsx` around lines
15 - 17, The AssistantMessage component currently types the destructured prop as
content: string which conflicts with Message.content being string | undefined;
update the prop typing in the AssistantMessage props signature to content?:
string (make it optional) so callers don't need to pass a fallback and the
component's existing guard {content && ...} remains correct; locate the prop
definition in the AssistantMessage function/component and change the content
type to optional to match Message.content.

internal/core/spec/step_test.go (1)

3563-3644: Add boundary test cases for the max-length constraint.

slug.go enforces maxSlugLength = 128. The current suite covers regex/format paths but has no case for an ID at exactly 128 characters (valid) or 129+ characters (should error). This is the primary gap against the length validation branch.

➕ Suggested boundary cases to add

+ {
+     name:    "exactly max length is valid",
+     skills:  []string{strings.Repeat("a", 128)},
+     wantErr: false,
+ },
+ {
+     name:    "exceeds max length is invalid",
+     skills:  []string{strings.Repeat("a", 129)},
+     wantErr: true,
+     errMsg:  "invalid skill ID",
+ },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/spec/step_test.go` around lines 3563 - 3644, Add two boundary
test cases to TestValidateAgent_SkillIDs that exercise the maxSlugLength from
slug.go: one skill ID exactly 128 characters (should be valid) and one 129+
characters (should error with "invalid skill ID"); create the long strings in
the test (e.g., repeat a safe character like 'a' to length 128 and 129), set
them as step.Agent.Skills, call validateAgent(step), and assert no error for the
128-char case and an error containing "invalid skill ID" for the 129+ case.

ui/src/features/agent/components/messages/ToolResultMessage.tsx (1)

7-10: truncateContent duplicates the pattern of truncateTask in constants.ts.

Both functions perform the same substring + ellipsis truncation. Consider generalizing: extract a shared truncate(text: string, maxLength: number): string utility in constants.ts, and have truncateTask call it with TASK_TRUNCATE_LENGTH. This avoids drift between the two implementations (slice vs substring, > vs <=).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/src/features/agent/components/messages/ToolResultMessage.tsx` around lines
7 - 10, Create a single shared truncate utility in constants.ts (export function
truncate(text: string, maxLength: number): string) and have both truncateContent
in ToolResultMessage.tsx and truncateTask in constants.ts call that shared
truncate with the appropriate max length (e.g., TASK_TRUNCATE_LENGTH) instead of
duplicating substring logic; update imports/exports accordingly and remove the
duplicate implementation so both use the exact same behavior.

ui/src/features/agent/hooks/useDelegateManager.ts (1)

47-60: handleDelegateSnapshots increments zIndex for every snapshot, even for existing delegates.

Each call to handleDelegateSnapshots bumps zIndexCounterRef for every delegate in the snapshot array (Line 52), regardless of whether the delegate already exists. If snapshots arrive periodically via SSE, this causes unbounded z-index inflation. Consider preserving the existing zIndex for known delegates:

♻️ Preserve existing zIndex

       for (const snap of snapshots) {
+        const existing = prev[snap.id];
         next[snap.id] = {
-          info: { id: snap.id, task: snap.task, status: snap.status, zIndex: ++zIndexCounterRef.current, positionIndex: 0 },
-          messages: prev[snap.id]?.messages || [],
-          isOpen: prev[snap.id]?.isOpen || false,
+          info: {
+            id: snap.id,
+            task: snap.task,
+            status: snap.status,
+            zIndex: existing?.info.zIndex ?? ++zIndexCounterRef.current,
+            positionIndex: 0,
+          },
+          messages: existing?.messages || [],
+          isOpen: existing?.isOpen || false,
         };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/src/features/agent/hooks/useDelegateManager.ts` around lines 47 - 60,
handleDelegateSnapshots currently increments zIndexCounterRef for every
snapshot, causing z-index inflation for existing delegates; modify the update
inside setDelegateStore so that for each snap you reuse
prev[snap.id]?.info.zIndex when present and only assign
++zIndexCounterRef.current for new delegates (or when info.zIndex is undefined),
update delegateStoreRef.current and return next as before; reference the
handleDelegateSnapshots function, zIndexCounterRef, delegateStoreRef, and
setDelegateStore to locate and change the logic that sets info.zIndex.

ui/src/features/agent/components/AgentChatModal.tsx (1)

88-95: Add setError to the dependency array for consistency with other callbacks.

setError is created from useState (line 148 in useAgentChat.ts), making it stable across renders, so the missing dependency is technically benign. However, setError is included in the dependency arrays of handleCancel (line 109) and handleSelectSession (line 121) elsewhere in the same file. Add it to the effect's dependency array at line 95 for consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/src/features/agent/components/AgentChatModal.tsx` around lines 88 - 95,
The effect that calls findLatestSession and selectSession is missing setError in
its dependency array; update the useEffect whose dependencies are [isOpen,
sessions, sessionId, selectSession] to also include setError so it matches other
callbacks (see setError, findLatestSession, selectSession) and keeps dependency
lists consistent across handleCancel and handleSelectSession.

internal/persis/fileagentskill/store_test.go (1)

645-738: Consider adding a zero-results Search test.

The four sub-cases cover positive paths well, but there's no test asserting that Search returns an empty result set for a non-matching query or tag. As per coding guidelines, tests should cover failure paths. Adding one case (e.g., Query: "nomatch") would close this gap.

🧪 Suggested additional sub-case

+	t.Run("no results when query matches nothing", func(t *testing.T) {
+		t.Parallel()
+		store, _ := setupTestStore(t)
+		createSkill(t, store, newTestSkill("k8s", "Kubernetes"))
+
+		result, err := store.Search(ctx, agent.SearchSkillsOptions{
+			Paginator: exec.DefaultPaginator(),
+			Query:     "nomatch",
+		})
+		require.NoError(t, err)
+		assert.Empty(t, result.Items)
+		assert.Equal(t, 0, result.TotalCount)
+	})

Based on learnings: "Co-locate Go tests as *_test.go files; favour table-driven cases and cover failure paths."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/persis/fileagentskill/store_test.go` around lines 645 - 738, Add a
zero-results subtest to TestStore_Search that verifies Search returns an empty
result set for non-matching queries or tags: inside TestStore_Search add a
t.Run("zero-results", ...) which calls setupTestStore, creates one or two test
skills via createSkill/newTestSkill, then calls store.Search(ctx,
agent.SearchSkillsOptions{Paginator: exec.DefaultPaginator(), Query: "nomatch"})
and assert require.NoError(err) and require.Len(result.Items, 0) and
assert.Equal(t, 0, result.TotalCount); you can also add a second check using
Tags: []string{"no-such-tag"} to confirm tag-based zero results.

internal/persis/filesession/store.go (1)

530-530: result should be initialised to an empty slice, not nil.

ListSessions (line 303) uses make([]*agent.Session, 0, len(sessIDs)). ListSubSessions leaves result as a nil slice when there are no matches, which is an inconsistency callers may need to guard against. A pre-allocated slice also avoids repeated reallocations when children are appended.

-	var result []*agent.Session
+	result := make([]*agent.Session, 0)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/persis/filesession/store.go` at line 530, ListSubSessions currently
declares "var result []*agent.Session" leaving it nil when empty; change it to
initialize an empty preallocated slice (e.g. result := make([]*agent.Session, 0,
len(children))) so callers get a non-nil slice and appends avoid reallocations.
Mirror the pattern used in ListSessions and use the "children" length for
capacity when available; keep the variable name "result" and the function name
"ListSubSessions".

internal/persis/fileagentskill/examples.go (1)

17-20: Hardcoded IDs could drift from embedded examples.

ExampleSkillIDs() is a manually maintained list. If a new skill is added under examples/, this list must be updated in lockstep. The tests catch this, but you could derive IDs from the embedded FS at init time to eliminate the drift risk.

💡 Derive IDs from embedded FS

-func ExampleSkillIDs() []string {
-	return []string{"dagu-ai-workflows", "dagu-containers", "dagu-server-worker"}
-}
+var exampleSkillIDs []string
+
+func init() {
+	entries, err := fs.ReadDir(exampleSkillsFS, "examples")
+	if err != nil {
+		return
+	}
+	for _, e := range entries {
+		if e.IsDir() {
+			exampleSkillIDs = append(exampleSkillIDs, e.Name())
+		}
+	}
+}
+
+func ExampleSkillIDs() []string {
+	return append([]string(nil), exampleSkillIDs...)
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/persis/fileagentskill/examples.go` around lines 17 - 20, Replace the
hardcoded list in ExampleSkillIDs() with a computed slice populated at package
init by scanning the embedded examples filesystem (the embedded FS variable and
the examples/ directory), deriving each skill ID from the directory or manifest
found (e.g., directory names or a skill metadata file) and storing them in a
package-level variable; update ExampleSkillIDs() to return that variable so new
example skills added to the embedded FS are automatically included and tests no
longer drift.

internal/agent/system_prompt.go (1)

52-61: Consider an options struct as the parameter list grows.

GenerateSystemPrompt now takes 6 parameters. If more context is added in the future, consider grouping them into a SystemPromptInput struct for readability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agent/system_prompt.go` around lines 52 - 61, GenerateSystemPrompt
takes many parameters; introduce a new SystemPromptInput struct that groups
EnvironmentInfo, CurrentDAG, MemoryContent, auth.Role (or precomputed User via
buildUserCapabilities), AvailableSkills and SkillCount, then change the
GenerateSystemPrompt signature to accept a single SystemPromptInput (or a
pointer) and adapt the function body to populate systemPromptData from that
struct; update all call sites to construct SystemPromptInput before calling
GenerateSystemPrompt and ensure references to systemPromptData,
buildUserCapabilities (if you prefer to pass Role and compute User inside the
function) and AvailableSkills/SkillCount are adjusted accordingly.

internal/agent/search_skills_tool_test.go (1)

16-143: Move testSkillStore to internal/test and consolidate the 19 TestSearchSkills_* functions into a table-driven test.

Two guideline violations confirmed in this file:

1. Duplicated mock across multiple files: testSkillStore (with testHasAllTags and testMatchesSkill helpers) is currently duplicated across three test files: search_skills_tool_test.go, tool_registry_test.go, and delegate_test.go. Per guidelines, this reusable fixture should live in internal/test to eliminate duplication.

2. Non-table-driven tests: The 19 standalone TestSearchSkills_* functions follow an identical structure. They map cleanly to a table-driven test with fields like name, query, tags, allowedSkills, wantContains, and wantNotContains.

Both align with the coding guidelines: "Use shared fixtures from internal/test instead of duplicating mocks" and "favour table-driven cases and cover failure paths".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agent/search_skills_tool_test.go` around lines 16 - 143, Move the
reusable test fixture (testSkillStore plus helper functions testHasAllTags and
testMatchesSkill) into a shared package under internal/test and update any
imports in files that used the duplicate copies (search_skills_tool_test.go,
tool_registry_test.go, delegate_test.go) to reference the new internal/test
package; then replace the 19 standalone TestSearchSkills_* functions in
search_skills_tool_test.go with a single table-driven test that iterates cases
(fields: name, query, tags, allowedSkills, wantContains, wantNotContains) and
asserts results using the shared testSkillStore and its Search method
(referencing Search on testSkillStore and the existing
SearchSkillsOptions/Paginator usage). Ensure helper removal from the original
file and fix any test references to testHasAllTags/testMatchesSkill to use the
centralized helpers or exported equivalents.

api/v1/api.gen.go (1)

438-447: Generated code looks consistent; consider ID naming in the OpenAPI spec.

This file is auto-generated (api.gen.go), so issues should be addressed at the source (likely api/v1/api.yaml or generator config). One minor note: Id on line 441 should conventionally be ID in Go (per golint/idiomatic Go). If your generator supports a naming override or if the spec field can be aliased, consider renaming it so the generated Go symbol becomes ID while the JSON tag remains "id".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@api/v1/api.gen.go` around lines 438 - 447, The generated struct
AgentDelegateSnapshot uses a non-idiomatic field name `Id`; update the source
OpenAPI spec or generator config so the property name maps to `ID` while keeping
the JSON tag `"id"` (e.g., change the property name/alias for the `id` field in
the OpenAPI definition that produces AgentDelegateSnapshot) or enable a naming
override in the generator so the generated struct field becomes `ID` with
`json:"id"`, leaving the rest of the generated code untouched.

internal/agent/delegate_test.go (1)

605-612: Repeated event-filtering pattern could be extracted into a helper.

The pattern for extracting DelegateEvents from registry.events is duplicated across 4+ tests (Lines 607–612, 644–649, 683–688, etc.). A small helper would reduce boilerplate:

func delegateEvents(registry *mockSubSessionRegistry) []DelegateEvent {
    var events []DelegateEvent
    for _, ev := range registry.events {
        if ev.DelegateEvent != nil {
            events = append(events, *ev.DelegateEvent)
        }
    }
    return events
}

Also applies to: 644-649, 683-688

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agent/delegate_test.go` around lines 605 - 612, The test file
repeats the same loop to extract DelegateEvent values from registry.events; add
a helper function (e.g., delegateEvents(registry *mockSubSessionRegistry)
[]DelegateEvent) that iterates registry.events, appends non-nil ev.DelegateEvent
to a slice and returns it, then replace the duplicated loops in tests (the
blocks that reference registry.mu.Lock()/Unlock() and build events from
ev.DelegateEvent) with calls to this helper (ensuring any required locking is
done either inside the helper or kept around the call to preserve concurrency
safety).

internal/cmd/context.go (1)

425-464: Early returns prevent creating independent stores.

If fileagentmodel.New fails (Line 440), the method returns before creating MemoryStore and SkillStore, even though these stores don't depend on the model store. This means a model-store initialization failure unnecessarily disables memory and skill features for CLI commands.

Consider continuing past model store failure:

♻️ Suggested change

 	ams, err := fileagentmodel.New(filepath.Join(c.Config.Paths.DataDir, "agent", "models"))
 	if err != nil {
 		logger.Warn(c, "Failed to create agent model store", tag.Error(err))
-		return result
+	} else {
+		result.ModelStore = ams
 	}
-	result.ModelStore = ams
 
 	ms, err := filememory.New(c.Config.Paths.DAGsDir)
 	if err != nil {
 		logger.Warn(c, "Failed to create agent memory store", tag.Error(err))
-		return result
-	}
-	result.MemoryStore = ms
+	} else {
+		result.MemoryStore = ms
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/cmd/context.go` around lines 425 - 464, The agentStores function
currently returns immediately when fileagentmodel.New fails, preventing creation
of independent stores; change the fileagentmodel.New error branch so it logs the
failure (keep logger.Warn with tag.Error(err)) but does not return—leave
result.ModelStore nil and continue to initialize filememory.New and
fileagentskill.New (so MemoryStore and SkillStore get created even if ModelStore
initialization failed); keep the early return behavior for failures that truly
prevent further initialization (e.g., fileagentconfig.New) and ensure result
fields are set only when their constructors succeed.

ui/src/features/agent/hooks/useSSEConnection.ts (1)

96-106: No feedback when SSE retries are exhausted.

When retryCountRef.current >= MAX_SSE_RETRIES and the EventSource is CLOSED, the handler silently gives up. Consider adding an onConnectionLost callback to SSECallbacks so the consumer can show a "connection lost" indicator, or at minimum log the failure.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/src/features/agent/hooks/useSSEConnection.ts` around lines 96 - 106, The
SSE onerror handler currently stops retrying silently when retryCountRef.current
>= MAX_SSE_RETRIES; add a way to notify consumers by extending SSECallbacks with
an onConnectionLost callback and invoke it when eventSource.readyState ===
EventSource.CLOSED and retryCountRef.current >= MAX_SSE_RETRIES (or at least log
an error). Specifically, update the SSECallbacks interface to include
onConnectionLost?: (sessionId?: string) => void, wire the callback into the hook
that creates eventSource (alongside existing setSseRetryTrigger and
eventSourceRef checks), and call props.sseCallbacks.onConnectionLost(sessionId)
(or processLogger.error) in the else branch where retries are exhausted so the
UI can show a "connection lost" indicator.

internal/service/frontend/api/v1/agent_skills_test.go (1)

105-135: mockSkillStore.Search ignores Query, Tags, and AllowedIDs filters.

The mock only handles pagination but ignores all filtering fields from SearchSkillsOptions. If ListAgentSkills (or future tests) passes query/tag/allowed-ID filters, the mock will return unfiltered results, potentially masking bugs.

Consider adding at minimum AllowedIDs filtering since the production handler likely uses it:

♻️ Suggested filtering addition

 func (m *mockSkillStore) Search(_ context.Context, opts agent.SearchSkillsOptions) (*exec.PaginatedResult[agent.SkillMetadata], error) {
 	var items []agent.SkillMetadata
 	for _, s := range m.skills {
+		if opts.AllowedIDs != nil {
+			if _, ok := opts.AllowedIDs[s.ID]; !ok {
+				continue
+			}
+		}
 		items = append(items, agent.SkillMetadata{

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/frontend/api/v1/agent_skills_test.go` around lines 105 -
135, mockSkillStore.Search currently only paginates; update it to apply
SearchSkillsOptions filters before pagination: filter m.skills by AllowedIDs (if
opts.AllowedIDs non-empty include only skills whose ID is in that set), by Tags
(if opts.Tags non-empty include only skills that contain all requested tags),
and by Query (if opts.Query non-empty perform a case-insensitive substring match
against skill.Name and skill.Description). After filtering, sort and then use
the existing paginator logic (pg := opts.Paginator / exec.DefaultPaginator(),
Offset/Limit calculation) and return exec.NewPaginatedResult on the filtered
slice; reference mockSkillStore.Search, SearchSkillsOptions, AllowedIDs, Tags,
Query, and exec.NewPaginatedResult.

internal/service/frontend/server.go (1)

508-530: Extract autoEnableExampleSkills to a shared package to avoid duplication.

This function (internal/service/frontend/server.go, lines 508–530) is duplicated identically in internal/cmd/context.go. Both load config, deduplicate example skill IDs, and save—this logic should be shared rather than duplicated. Extract to internal/agent or internal/persis/fileagentskill to prevent drift.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/frontend/server.go` around lines 508 - 530, Move the
duplicated autoEnableExampleSkills logic into a single shared package (e.g.,
internal/agent or internal/persist/fileagentskill) and replace both copies with
calls to the new function; specifically extract the implementation that uses
agent.ConfigStore (calls configStore.Load, reads/updates cfg.EnabledSkills using
fileagentskill.ExampleSkillIDs, then configStore.Save) into a new exported
helper (e.g., EnableExampleSkills(ctx, store)), update callers in
internal/service/frontend/server.go and internal/cmd/context.go to call that
helper, and remove the duplicated function definitions so only the shared
implementation remains.

ui/src/features/agent/hooks/useAgentChat.ts (1)

261-279: dm in dependency arrays defeats useCallback memoization.

useDelegateManager() returns a new object literal each render, so dm has a new identity every cycle. Listing dm in useCallback deps (lines 278, 290, 307) means selectSession, handleClearSession, and reopenDelegate are re-created on every render. This isn't a correctness bug but negates memoization.

Destructure the stable callbacks you actually reference instead:

Proposed approach

- const dm = useDelegateManager();
+ const dm = useDelegateManager();
+ const {
+   restoreDelegates, resetDelegates, openDelegate,
+   hasDelegateMessages, setDelegateMessagesForId,
+ } = dm;

Then use those individual (stable) references in the dependency arrays:

- [client, remoteNode, setSessionId, setMessages, setSessionState, dm]
+ [client, remoteNode, setSessionId, setMessages, setSessionState, restoreDelegates]

- [clearSession, dm]
+ [clearSession, resetDelegates]

- [client, remoteNode, dm]
+ [client, remoteNode, hasDelegateMessages, setDelegateMessagesForId, openDelegate]

🤖 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 261 - 279, The
issue is that listing the whole dm object from useDelegateManager in dependency
arrays defeats useCallback memoization because dm is re-created each render; fix
by destructuring the specific stable methods you call from dm (e.g., const {
restoreDelegates, clearDelegates, reopenDelegate } = dm) and then use those
method references in the dependency arrays of selectSession, handleClearSession,
and reopenDelegate instead of dm; update each useCallback dependency list to
include only the stable functions (and other true deps) and remove dm.

internal/agent/session.go (1)

440-473: Consider guarding sm.enabledSkills / sm.skillStore access.

createLoop reads sm.enabledSkills and sm.skillStore outside the lock (the mutex is released in ensureLoop before calling createLoop). These fields are set once in NewSessionManager and never mutated, so this is safe today. If that invariant ever changes, this becomes a data race. A brief comment documenting immutability would help future readers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agent/session.go` around lines 440 - 473, The createLoop function
reads sm.enabledSkills and sm.skillStore without holding the session mutex
(ensureLoop releases the lock before calling createLoop), so add a short
explicit comment near the top of createLoop (or on the SessionManager fields)
stating that enabledSkills and skillStore are immutable after NewSessionManager
(or otherwise never mutated) to document the invariant; alternatively, if you
prefer stronger safety, capture copies of sm.enabledSkills and sm.skillStore
into local variables while holding the lock in ensureLoop before calling
createLoop (identify the fields by name: sm.enabledSkills, sm.skillStore and the
functions createLoop and ensureLoop).

internal/agent/api.go (1)

440-444: Direct field assignment for registry breaks encapsulation but is justified.

mgr.registry = &sessionRegistry{sessions: &a.sessions, parent: mgr} sets the unexported field post-construction to resolve the circular dependency (registry needs mgr, mgr config accepts registry). This pattern is used in both CreateSession and reactivateSession. It's safe since the field isn't read until AcceptUserMessage, but a brief inline comment explaining why it's set post-construction would help.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agent/api.go` around lines 440 - 444, Add a brief inline comment
explaining why mgr.registry is set directly after NewSessionManager — note that
the direct assignment mgr.registry = &sessionRegistry{sessions: &a.sessions,
parent: mgr} is used to break a circular dependency between sessionRegistry and
session manager and is safe because the registry field isn't accessed until
AcceptUserMessage; add this comment next to the assignment in both CreateSession
and reactivateSession so future readers understand the post-construction
assignment intent.