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 pull request introduces a comprehensive agent skills management system with CRUD operations, file-based persistence, API endpoints, and UI components. Skills are stored as SKILL.md files with YAML frontmatter metadata, discoverable via git sync, and managed through admin-only endpoints with validation and access control. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/UI
participant Handler as API Handler
participant SkillStore as SkillStore
participant FileSystem as File System
participant ConfigStore as ConfigStore
Client->>Handler: POST /settings/agent/skills (CreateSkillRequest)
Handler->>Handler: Validate input & check admin permission
Handler->>SkillStore: Create(ctx, skill)
SkillStore->>FileSystem: Create {id}/ directory
SkillStore->>FileSystem: Write SKILL.md (frontmatter + body)
SkillStore->>SkillStore: Update byID & byName indices
FileSystem-->>SkillStore: Success
SkillStore-->>Handler: Skill created
Handler->>Handler: Log audit event
Handler-->>Client: 201 Created + SkillResponse
Client->>Handler: PUT /settings/agent/enabled-skills (SetEnabledSkillsRequest)
Handler->>Handler: Validate skill IDs exist
Handler->>ConfigStore: Update EnabledSkills
ConfigStore->>FileSystem: Persist config
ConfigStore-->>Handler: Success
Handler->>Handler: Log audit event
Handler-->>Client: 200 OK + SetEnabledSkillsResponse
sequenceDiagram
participant Scanner as Git Sync Scanner
participant FileSystem as File System
participant DAGState as DAG State
participant DAGSync as DAG Sync Engine
Scanner->>FileSystem: Scan skills/ directory
Scanner->>FileSystem: Find SKILL.md files
Scanner->>FileSystem: Read file & compute hash
FileSystem-->>Scanner: File content
Scanner->>DAGState: Set Kind = DAGKindSkill
Scanner->>DAGState: Set Status = StatusUntracked
DAGState-->>Scanner: Updated state
Scanner->>DAGSync: Register skill DAG item
DAGSync->>DAGSync: Include in sync operations
DAGSync-->>Scanner: Complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/service/frontend/api/v1/sync.go (1)
398-407:⚠️ Potential issue | 🟠 Major
syncItemFilePathdoes not handleDAGKindSkill— skills will get a.yamlextension.This function only maps
DAGKindMemoryto.md; skill items (which are also.mdfiles) will fall through to the default.yamlextension. Since the PR adds skill kind support intoAPISyncItemKindjust above, this function should be updated too.🐛 Proposed fix
func syncItemFilePath(itemID string, kind gitsync.DAGKind) string { if kind == "" { kind = gitsync.KindForDAGID(itemID) } ext := ".yaml" - if kind == gitsync.DAGKindMemory { + if kind == gitsync.DAGKindMemory || kind == gitsync.DAGKindSkill { ext = ".md" } return itemID + ext }ui/src/pages/git-sync/index.tsx (1)
703-721:⚠️ Potential issue | 🟠 MajorSkill and memory items link to
/dags/...which is incorrect for non-DAG items.Line 706 links all items to
/dags/${encodeURIComponent(itemId)}, but skill items should link to/agent-skills/${skillId}and memory items shouldn't link to a DAG page at all. Clicking a skill row's name will navigate to a non-existent DAG page.🔧 Suggested approach
<div className="flex items-center gap-1.5"> - <a - href={`/dags/${encodeURIComponent(itemId)}`} - className="font-mono hover:underline" - > - {item.displayName} - </a> + {kind === 'skill' ? ( + <a + href={`/agent-skills/${encodeURIComponent(itemId)}`} + className="font-mono hover:underline" + > + {item.displayName} + </a> + ) : kind === 'memory' ? ( + <span className="font-mono">{item.displayName}</span> + ) : ( + <a + href={`/dags/${encodeURIComponent(itemId)}`} + className="font-mono hover:underline" + > + {item.displayName} + </a> + )}
🤖 Fix all issues with AI agents
In `@api/v1/api.yaml`:
- Around line 4068-4255: The OpenAPI operations getAgentSkill, updateAgentSkill,
deleteAgentSkill and setEnabledSkills are missing 400 response definitions for
invalid/unknown skill IDs; add a "400" response to each operation (GET
/settings/agent/skills/{skillId}, PATCH /settings/agent/skills/{skillId}, DELETE
/settings/agent/skills/{skillId}, and PUT /settings/agent/enabled-skills)
mirroring the existing Error schema entries (application/json -> $ref:
"#/components/schemas/Error") so generated clients can handle bad-request errors
properly.
In `@internal/agent/skill.go`:
- Around line 28-38: The Skill struct's SchemaVersion field isn't persisted
because the on-disk representation (skillFrontmatter) lacks that field; update
the persistence layer by adding a SchemaVersion int `yaml:"schema_version"
json:"schemaVersion"` to the skillFrontmatter type in the fileagent skill store
so it round-trips when reading/writing, and ensure the code that maps between
skillFrontmatter and Skill copies SchemaVersion; alternatively, if SchemaVersion
is unused, remove SchemaVersion from the Skill domain struct to keep models
consistent.
In `@internal/persis/fileagentskill/store_test.go`:
- Around line 333-349: The test "name conflict returns error" in store_test.go
currently asserts agent.ErrSkillAlreadyExists but after fixing store.Update to
return the proper sentinel for a name collision it should assert
agent.ErrSkillNameAlreadyExists instead; update the assertion in that test (the
require/assert around the call to store.Update in the t.Run block) to use
agent.ErrSkillNameAlreadyExists so it matches the corrected behavior of
store.Update.
In `@internal/persis/fileagentskill/store.go`:
- Around line 357-361: The update logic in the method that checks nameChanged
(variables: nameChanged, existing, s.byName, takenByID, taken, skill.ID)
currently returns agent.ErrSkillAlreadyExists on a name uniqueness conflict;
change the returned sentinel to agent.ErrSkillNameAlreadyExists so name
conflicts use the same error as Create for consistency.
In `@internal/service/frontend/api/v1/agent_skills.go`:
- Around line 283-291: The code assumes cfg is non-nil after calling
a.agentConfigStore.Load; add a nil-check after Load to guard against a (nil,
nil) return and return ErrFailedToLoadAgentConfig (or an appropriate error)
instead of dereferencing cfg; update the block around agentConfigStore.Load /
cfg.EnabledSkills / a.agentConfigStore.Save to handle cfg == nil before
assigning cfg.EnabledSkills and calling Save, and apply the same nil-guard
pattern to the similar load/save sequence used elsewhere (the other
agentConfigStore.Load/Save block).
- Around line 324-342: applySkillUpdates currently allows update.Knowledge to be
set to an empty string which can clear a required field; change the logic in
applySkillUpdates (working with agent.Skill and api.UpdateSkillRequest) so when
update.Knowledge != nil you trim whitespace (strings.TrimSpace) and only assign
to skill.Knowledge if the trimmed value is non-empty — otherwise ignore the
update (do not set an empty value); this preserves existing knowledge when
clients send empty/whitespace updates and prevents invalid skills.
- Around line 25-42: Rename the package-level error variables to follow the
Err... convention: change errAgentSkillStoreNotAvailable to
ErrAgentSkillStoreNotAvailable, errSkillNotFound to ErrSkillNotFound, and
errSkillAlreadyExists to ErrSkillAlreadyExists; update all references/usages
across this file and any callers to use the new names (search for the original
symbols errAgentSkillStoreNotAvailable, errSkillNotFound, errSkillAlreadyExists
and replace them), keeping the error values and types unchanged.
- Around line 54-64: The code currently ignores errors from
a.agentConfigStore.Load(ctx) in the handler that lists skills; change this to
check and handle the error instead of discarding it—log the failure (using
logger.Error with tag.Error(err)) and return an appropriate Error (e.g.,
ErrFailedToLoadAgentConfig or an Error with Code api.ErrorCodeInternalError and
HTTP 500) so callers know the config load failed; apply the same pattern in the
related methods GetAgentSkill and UpdateAgentSkill where a.agentConfigStore.Load
is called to ensure config load errors aren’t silently treated as “no enabled
skills.”
In `@ui/src/pages/agent-skills/index.tsx`:
- Around line 69-92: handleToggleEnabled currently reads the outer-scope skills
array (line using skills.filter) which can be stale during rapid toggles; change
the logic to derive the updated enabled list from a functional state updater so
you always use the latest state before calling client.PUT. Specifically, compute
newEnabled inside a setSkills(prev => { const currentEnabled = prev.filter(s =>
s.enabled).map(s => s.id); const updatedEnabled = skill.enabled ?
currentEnabled.filter(id => id !== skill.id) : [...currentEnabled, skill.id];
return prev.map(s => s.id === skill.id ? { ...s, enabled: !skill.enabled } : s);
}) and then use that updatedEnabled value for the client.PUT call (ensure the
API call uses the enabled IDs produced from the same functional update rather
than the closure-captured skills). This ensures handleToggleEnabled, setSkills
and client.PUT all operate on the same, latest enabled list and avoids race
conditions.
- Around line 157-161: The success banner JSX that renders when the `success`
variable is truthy uses `text-green-600` which lacks a dark-mode variant; update
the class list on that element (the JSX block rendering `{success}`) to include
a dark-mode text color (e.g., replace or augment `text-green-600` with a
matching dark variant such as `text-green-600 dark:text-green-400`) so the text
respects dark mode per the styling guidelines.
In `@ui/src/pages/git-sync/index.tsx`:
- Around line 611-615: The display is inconsistent: selectedCounts.memory is
always shown while selectedCounts.skill is conditional; update the JSX that
renders the summary (the span around "Selected: ...") to treat memory the same
as skill by only rendering the memory count when selectedCounts.memory > 0
(mirror the existing conditional used for selectedCounts.skill) and adjust comma
punctuation so commas are only inserted between visible segments; locate the
rendering using the selectedCounts identifier in ui/src/pages/git-sync/index.tsx
and modify the conditional expressions to ensure both memory and skill follow
the same visibility rule.
🧹 Nitpick comments (9)
internal/agent/skill.go (1)
50-68: Consider extracting a shared slug-validation helper.
ValidateSkillIDis structurally identical toValidateModelIDinmodel_config.go(same regex, same max length, same error wrapping). A sharedvalidateSlugID(id string, sentinel error) errorhelper could reduce duplication.ui/src/components/editors/MarkdownEditor.tsx (3)
24-31: Redundant theme-sync effect — the editor hasn't mounted yet when this runs.This
useEffectfires after the first render, but the Monaco editor mounts asynchronously viaonMount, soeditorRef.currentwill almost always benullhere. The initial theme is already correctly set via thethemeprop on the<MonacoEditor>component (line 76), making this effect a no-op in practice.🔧 Suggested cleanup
- useEffect(() => { - if (editorRef.current) { - const newTheme = document.documentElement.classList.contains('dark') - ? 'vs-dark' - : 'vs'; - monaco.editor.setTheme(newTheme); - } - }, []); -
60-64: F-key handler only blocks the letter 'f', not function keys (F1–F12).
e.code === 'KeyF'matches the physical 'F' key, not function keys like F1, F5, etc. (those have codes like'F1','F5'). If this is intended to prevent a parent hotkey (e.g., a global search triggered by 'f') from capturing keystrokes while the editor is focused, this works. But the comment says "F-key" which is ambiguous — consider clarifying the intent with a brief inline comment.
18-22: Remove manualdispose()call as@monaco-editor/reacthandles cleanup automatically.The
@monaco-editor/reactwrapper automatically disposes the editor instance on unmount. Callingdispose()manually can interfere with the wrapper's own cleanup logic. Remove this unless you've observed a specific memory leak that requires it.Note: The same pattern exists in
ui/src/features/dags/components/dag-editor/DAGEditor.tsxand should also be removed.ui/src/pages/agent-skills/SkillEditorPage.tsx (2)
146-148: Loading state returnsnull, blanking the page briefly.Per coding guidelines, full-page loading overlays should be avoided. Returning
nullhere is acceptable for an initial load (no stale data exists), but it causes a brief flash of empty content. Consider rendering the page shell (header, empty form skeleton) immediately and only guarding the data-dependent fields, so the user sees the layout instantly.
50-71: Fetch effect lacks an abort mechanism and runs as a fire-and-forget IIFE.If the user navigates away or
remoteNodechanges before the fetch completes, the stale response still callssetName,setDescription, etc. In React 19 this is silently ignored on unmounted components, but a stale response from a previousremoteNodecould still incorrectly overwrite state if the component remains mounted andremoteNodechanges. Consider using anAbortControlleror a stale-closure guard:🛡️ Suggested improvement with cleanup flag
useEffect(() => { if (isCreating || !skillId) return; + let cancelled = false; (async () => { const { data, error } = await client.GET('/settings/agent/skills/{skillId}', { params: { path: { skillId }, query: { remoteNode } }, }); + if (cancelled) return; if (error) { showError(error.message || 'Failed to load skill'); navigate('/agent-skills'); return; } setName(data.name); setIdField(data.id); setDescription(data.description ?? ''); setKnowledge(data.knowledge); setVersion(data.version ?? ''); setAuthor(data.author ?? ''); setTagsInput(data.tags?.join(', ') ?? ''); setIsLoading(false); })(); + + return () => { cancelled = true; }; }, [skillId, isCreating, client, remoteNode, showError, navigate]);ui/src/pages/agent-skills/index.tsx (1)
35-41: Consider using the toast system for transient success/error messages instead of inline banners.The
successanderrorstate variables persist until the next action. TheSkillEditorPagealready usesshowToastfor success feedback. Using the same pattern here would be more consistent and avoid stale messages.ui/src/pages/git-sync/index.tsx (1)
607-607: Nested ternary is getting complex — consider a lookup map.With four type values, the inline ternary chain is becoming hard to read. A simple map would be cleaner:
♻️ Suggested refactor
- {f === 'all' ? 'All' : f === 'dag' ? 'DAGs' : f === 'memory' ? 'Memory' : 'Skills'} ({typeCounts[f]}) + {({ all: 'All', dag: 'DAGs', memory: 'Memory', skill: 'Skills' })[f]} ({typeCounts[f]})api/v1/api.yaml (1)
7179-7287: Add schema-level constraints to mirror server validation.
The server enforces non-empty name/knowledge and a skill ID regex; encoding those constraints in the schema improves client-side validation and error messaging.✏️ Suggested schema constraints
CreateSkillRequest: type: object required: - name - knowledge properties: id: type: string + pattern: "^[a-z0-9]+(-[a-z0-9]+)*$" name: type: string + minLength: 1 knowledge: type: string + minLength: 1 SetEnabledSkillsRequest: type: object required: - skillIds properties: skillIds: type: array + uniqueItems: true items: type: string + pattern: "^[a-z0-9]+(-[a-z0-9]+)*$"
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1684 +/- ##
==========================================
- Coverage 69.80% 69.70% -0.11%
==========================================
Files 357 359 +2
Lines 39696 39941 +245
==========================================
+ Hits 27710 27841 +131
- Misses 9779 9856 +77
- Partials 2207 2244 +37
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Release Notes