fix: remove third-party skill installation from embedded AI agent#1988
Merged
fix: remove third-party skill installation from embedded AI agent#1988
Conversation
Remove the capability to install, manage, and use third-party skills in the embedded AI agent. This simplifies the agent by removing the skill store, skill tools (use_skill, search_skills), skill management API endpoints, skill picker UI, and skill references from distributed snapshots. Built-in Dagu reference knowledge (SeedReferences) and the `dagu ai install` command are preserved.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR comprehensively removes all skill-related functionality from the agent system, including skill domain types, management tools, frontend UI, API endpoints, filesystem persistence, and skill wiring throughout agent initialization and execution flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/agent/api.go (1)
1158-1160:⚠️ Potential issue | 🟡 MinorUpdate the
GenerateAssistantMessagedoc comment.The comment still says the session's “enabled skills” are reused, but that capability is gone from this path now.
Also applies to: 1196-1197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/api.go` around lines 1158 - 1160, The GenerateAssistantMessage doc comment incorrectly states that the session's "enabled skills" are reused; update the comment for GenerateAssistantMessage to remove or rephrase that part so it only mentions the session's model, soul, and DAG scope are reused (and not enabled skills), and make the same change in the other doc comment occurrence referenced around the second block (lines ~1196-1197) to keep both comments consistent.internal/agent/snapshot.go (1)
20-27:⚠️ Potential issue | 🟠 MajorBump the snapshot wire version for this schema change.
Snapshot.SkillsandSnapshotStores.SkillStorewere removed, but the payload still advertisesSnapshotVersion = 1. Older workers will accept this snapshot instead of failing fast during a rolling upgrade, even though their runtime may still expect skill metadata/tool wiring.Suggested fix
- SnapshotVersion = 1 + SnapshotVersion = 2- if snapshot.Version != SnapshotVersion { - return nil, fmt.Errorf("%w: got %d want %d", ErrUnsupportedSnapshotWire, snapshot.Version, SnapshotVersion) - } + switch snapshot.Version { + case 1, SnapshotVersion: + // Keep v1 readable if backward compatibility is required. + default: + return nil, fmt.Errorf("%w: got %d want %d", ErrUnsupportedSnapshotWire, snapshot.Version, SnapshotVersion) + }Also applies to: 36-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/snapshot.go` around lines 20 - 27, The snapshot wire version constant SnapshotVersion must be incremented to reflect the schema change that removed Snapshot.Skills and SnapshotStores.SkillStore so older workers reject incompatible snapshots; update the SnapshotVersion constant (e.g., from 1 to 2) wherever it is defined (and any other occurrences referenced in the file) so the new snapshots advertise the new version and trigger fail-fast behavior in older runtimes.
🧹 Nitpick comments (3)
internal/persis/fileagentskill/examples.go (1)
16-42: Keep regression coverage aroundSeedReferences.This is now the only remaining entrypoint in
internal/persis/fileagentskill, andinternal/service/frontend/server.gostill depends on it during startup. Since the asset source moved tobundledskills.Assetsand the old package test file was removed in this PR, I’d keep a small test here that verifies at least one bundled reference is extracted successfully.🤖 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 16 - 42, Add a small regression test that calls SeedReferences with a temporary directory and asserts it returns a non-empty path and that at least one expected bundled asset from bundledskills.Assets (under builtinKnowledgeEmbedDir) was written; create a temp dir via os.MkdirTemp, call internal/persis/fileagentskill.SeedReferences, then verify a known file name (or that the directory is non-empty) exists at the returned path and clean up the temp dir. Use the package test (same package) to reference SeedReferences, assetsFS/builtinKnowledgeEmbedDir or a known bundled file name to locate an expected output and fail the test if extraction did not occur.internal/agent/tool_registry_test.go (1)
28-43: Assert the removed skill tools are absent, not just omitted fromexpected.This still passes if
use_skillorsearch_skillsremain registered, because the test only checks that the current subset exists. Since this PR is removing those tools, please add explicit negative checks or assert the exact set.🧪 Tighten the assertion
regs := RegisteredTools() names := make(map[string]bool, len(regs)) for _, reg := range regs { names[reg.Name] = true } for _, name := range expected { assert.True(t, names[name], "expected tool %q to be registered", name) } + assert.False(t, names["use_skill"], "use_skill should no longer be registered") + assert.False(t, names["search_skills"], "search_skills should no longer be registered") + assert.Len(t, names, len(expected), "no extra tools should remain registered")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/tool_registry_test.go` around lines 28 - 43, The test only verifies that certain tools exist but doesn't assert removed tools are absent, so update the test around RegisteredTools() to either assert the exact set of registered tool names or add negative checks for "use_skill" and "search_skills"; specifically, after building the names map from regs (result of RegisteredTools()), add assertions that names["use_skill"] and names["search_skills"] are false (or replace the current expected slice check with an equality/assert-equal against the full expected set) so the test fails if those tools remain registered.internal/runtime/builtin/agentstep/executor.go (1)
288-333: Consider adding validation or logging for unknown tool names intools.enabled.Lines 321-332 silently ignore any tool name not present in
allTools. This mirrors the intentional silent dropping inValidateToolPolicy()(agent/policy.go) for backward compatibility with removed tools. However, unlike the policy validator, there's no logging or explicit documentation here. A step withtools.enabled: ["search_skills"](removed tool) will silently execute without it, which may mask configuration errors. Consider adding a warning log when unknown tool names are encountered, similar to how other config issues are logged elsewhere in this function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runtime/builtin/agentstep/executor.go` around lines 288 - 333, buildTools currently ignores unknown names in stepCfg.Tools.Enabled (iterating names and skipping missing entries from allTools) without any warning; update buildTools to log a warning when a requested tool name is not found in allTools (e.g., inside the loop over stepCfg.Tools.Enabled), referencing stepCfg.Tools.Enabled and allTools so callers see that a configured tool was dropped, and keep the existing behavior of always including "output" (use agent.GetToolByName to check presence). Ensure the log provides the step context and the unknown tool name, consistent with ValidateToolPolicy's intent but reporting the misconfiguration at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/agent/api.go`:
- Around line 1158-1160: The GenerateAssistantMessage doc comment incorrectly
states that the session's "enabled skills" are reused; update the comment for
GenerateAssistantMessage to remove or rephrase that part so it only mentions the
session's model, soul, and DAG scope are reused (and not enabled skills), and
make the same change in the other doc comment occurrence referenced around the
second block (lines ~1196-1197) to keep both comments consistent.
In `@internal/agent/snapshot.go`:
- Around line 20-27: The snapshot wire version constant SnapshotVersion must be
incremented to reflect the schema change that removed Snapshot.Skills and
SnapshotStores.SkillStore so older workers reject incompatible snapshots; update
the SnapshotVersion constant (e.g., from 1 to 2) wherever it is defined (and any
other occurrences referenced in the file) so the new snapshots advertise the new
version and trigger fail-fast behavior in older runtimes.
---
Nitpick comments:
In `@internal/agent/tool_registry_test.go`:
- Around line 28-43: The test only verifies that certain tools exist but doesn't
assert removed tools are absent, so update the test around RegisteredTools() to
either assert the exact set of registered tool names or add negative checks for
"use_skill" and "search_skills"; specifically, after building the names map from
regs (result of RegisteredTools()), add assertions that names["use_skill"] and
names["search_skills"] are false (or replace the current expected slice check
with an equality/assert-equal against the full expected set) so the test fails
if those tools remain registered.
In `@internal/persis/fileagentskill/examples.go`:
- Around line 16-42: Add a small regression test that calls SeedReferences with
a temporary directory and asserts it returns a non-empty path and that at least
one expected bundled asset from bundledskills.Assets (under
builtinKnowledgeEmbedDir) was written; create a temp dir via os.MkdirTemp, call
internal/persis/fileagentskill.SeedReferences, then verify a known file name (or
that the directory is non-empty) exists at the returned path and clean up the
temp dir. Use the package test (same package) to reference SeedReferences,
assetsFS/builtinKnowledgeEmbedDir or a known bundled file name to locate an
expected output and fail the test if extraction did not occur.
In `@internal/runtime/builtin/agentstep/executor.go`:
- Around line 288-333: buildTools currently ignores unknown names in
stepCfg.Tools.Enabled (iterating names and skipping missing entries from
allTools) without any warning; update buildTools to log a warning when a
requested tool name is not found in allTools (e.g., inside the loop over
stepCfg.Tools.Enabled), referencing stepCfg.Tools.Enabled and allTools so
callers see that a configured tool was dropped, and keep the existing behavior
of always including "output" (use agent.GetToolByName to check presence). Ensure
the log provides the step context and the unknown tool name, consistent with
ValidateToolPolicy's intent but reporting the misconfiguration at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e751f2ad-6399-4cfb-9dfc-092267823226
📒 Files selected for processing (50)
internal/agent/api.gointernal/agent/api_test.gointernal/agent/contextkeys.gointernal/agent/delegate.gointernal/agent/delegate_test.gointernal/agent/loop.gointernal/agent/model_config.gointernal/agent/search_skills_tool.gointernal/agent/search_skills_tool_test.gointernal/agent/session.gointernal/agent/skill.gointernal/agent/skill_test.gointernal/agent/skill_tool.gointernal/agent/snapshot.gointernal/agent/snapshot_stores.gointernal/agent/snapshot_stores_test.gointernal/agent/snapshot_test.gointernal/agent/system_prompt.gointernal/agent/system_prompt.txtinternal/agent/system_prompt_test.gointernal/agent/tool_registry.gointernal/agent/tool_registry_test.gointernal/agent/types.gointernal/agentsnapshot/dispatch.gointernal/cmd/ai.gointernal/cmd/ai_test.gointernal/cmd/context.gointernal/cmd/dry.gointernal/cmd/restart.gointernal/cmd/retry.gointernal/cmd/start.gointernal/persis/fileagentskill/examples.gointernal/persis/fileagentskill/examples_test.gointernal/persis/fileagentskill/store.gointernal/persis/fileagentskill/store_test.gointernal/runtime/agent/agent.gointernal/runtime/builtin/agentstep/executor.gointernal/service/frontend/api/v1/agent_skills.gointernal/service/frontend/api/v1/agent_skills_stub.gointernal/service/frontend/api/v1/agent_skills_test.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/server.gointernal/service/worker/remote_handler.gointernal/service/worker/remote_handler_test.goui/src/App.tsxui/src/features/agent/components/ChatInput.tsxui/src/features/agent/components/SkillPicker.tsxui/src/menu.tsxui/src/pages/agent-skills/SkillEditorPage.tsxui/src/pages/agent-skills/index.tsx
💤 Files with no reviewable changes (33)
- internal/cmd/start.go
- internal/agent/model_config.go
- internal/cmd/restart.go
- internal/agent/system_prompt.txt
- internal/cmd/dry.go
- internal/cmd/retry.go
- internal/agent/skill_test.go
- internal/agent/system_prompt_test.go
- internal/agent/contextkeys.go
- ui/src/menu.tsx
- ui/src/App.tsx
- internal/agent/snapshot_stores_test.go
- internal/service/frontend/api/v1/api.go
- internal/agentsnapshot/dispatch.go
- internal/agent/delegate_test.go
- internal/runtime/agent/agent.go
- internal/agent/types.go
- internal/agent/tool_registry.go
- internal/agent/snapshot_test.go
- internal/persis/fileagentskill/examples_test.go
- internal/agent/skill_tool.go
- internal/service/worker/remote_handler_test.go
- internal/agent/search_skills_tool.go
- internal/agent/loop.go
- ui/src/pages/agent-skills/index.tsx
- ui/src/pages/agent-skills/SkillEditorPage.tsx
- internal/agent/search_skills_tool_test.go
- internal/service/frontend/api/v1/agent_skills_test.go
- internal/service/frontend/api/v1/agent_skills.go
- internal/persis/fileagentskill/store.go
- internal/agent/skill.go
- internal/persis/fileagentskill/store_test.go
- ui/src/features/agent/components/SkillPicker.tsx
Remove skill path definitions and schema types from api.yaml, then regenerate api.gen.go so the StrictServerInterface no longer requires skill methods. This eliminates the temporary stub file.
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/agent/api.go (1)
1158-1160: Update the GoDoc to remove the old skill wording.The function comment still says one-shot generation reuses “enabled skills”, but that state is gone from the runtime config and prompt builder now. Please trim that sentence so the doc matches the new behavior.
Also applies to: 1196-1197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/api.go` around lines 1158 - 1160, Update the GoDoc for GenerateAssistantMessage to remove the outdated reference to “enabled skills” (the runtime no longer carries that state); change the sentence that says the one-shot generation “reuses the current session's model, soul, DAG scope, and enabled skills” to only mention model, soul, and DAG scope (or otherwise reflect current runtime/prompt-builder behavior). Also update the parallel docblock later in the file with the same fix so both comments match the new behavior.
🤖 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/runtime/builtin/agentstep/executor.go`:
- Line 137: The code currently silences unknown tool names dropped from
stepCfg.Tools.Enabled when calling buildTools; update the validation logic
(either inside buildTools or right after the call in executor.go where
buildTools(ctx, dagCtx, stepCfg, globalPolicy, stdout) is invoked) to detect any
entries in stepCfg.Tools.Enabled that are not in the supported tool set and fail
fast or at minimum emit a clear error/warning with the offending tool names;
specifically, validate against the canonical tool registry used by buildTools,
and if unknown names (including legacy use_skill/search_skills) are present,
return an error from buildTools or log an explicit process-level warning and
stop execution for that step so callers of buildTools (e.g., the executor path
handling step execution) can surface the failure instead of silently dropping
them.
---
Nitpick comments:
In `@internal/agent/api.go`:
- Around line 1158-1160: Update the GoDoc for GenerateAssistantMessage to remove
the outdated reference to “enabled skills” (the runtime no longer carries that
state); change the sentence that says the one-shot generation “reuses the
current session's model, soul, DAG scope, and enabled skills” to only mention
model, soul, and DAG scope (or otherwise reflect current runtime/prompt-builder
behavior). Also update the parallel docblock later in the file with the same fix
so both comments match the new behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e21b173e-e434-40bc-9687-e0ba2ccefb6f
📒 Files selected for processing (51)
api/v1/api.gen.goapi/v1/api.yamlinternal/agent/api.gointernal/agent/api_test.gointernal/agent/contextkeys.gointernal/agent/delegate.gointernal/agent/delegate_test.gointernal/agent/loop.gointernal/agent/model_config.gointernal/agent/search_skills_tool.gointernal/agent/search_skills_tool_test.gointernal/agent/session.gointernal/agent/skill.gointernal/agent/skill_test.gointernal/agent/skill_tool.gointernal/agent/snapshot.gointernal/agent/snapshot_stores.gointernal/agent/snapshot_stores_test.gointernal/agent/snapshot_test.gointernal/agent/system_prompt.gointernal/agent/system_prompt.txtinternal/agent/system_prompt_test.gointernal/agent/tool_registry.gointernal/agent/tool_registry_test.gointernal/agent/types.gointernal/agentsnapshot/dispatch.gointernal/cmd/ai.gointernal/cmd/ai_test.gointernal/cmd/context.gointernal/cmd/dry.gointernal/cmd/restart.gointernal/cmd/retry.gointernal/cmd/start.gointernal/persis/fileagentskill/examples.gointernal/persis/fileagentskill/examples_test.gointernal/persis/fileagentskill/store.gointernal/persis/fileagentskill/store_test.gointernal/runtime/agent/agent.gointernal/runtime/builtin/agentstep/executor.gointernal/service/frontend/api/v1/agent_skills.gointernal/service/frontend/api/v1/agent_skills_test.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/server.gointernal/service/worker/remote_handler.gointernal/service/worker/remote_handler_test.goui/src/App.tsxui/src/features/agent/components/ChatInput.tsxui/src/features/agent/components/SkillPicker.tsxui/src/menu.tsxui/src/pages/agent-skills/SkillEditorPage.tsxui/src/pages/agent-skills/index.tsx
💤 Files with no reviewable changes (35)
- internal/cmd/start.go
- internal/agent/contextkeys.go
- internal/cmd/dry.go
- internal/agent/skill_test.go
- internal/cmd/retry.go
- internal/cmd/restart.go
- internal/service/frontend/api/v1/api.go
- internal/agent/snapshot_stores_test.go
- internal/agentsnapshot/dispatch.go
- internal/agent/system_prompt_test.go
- internal/agent/types.go
- internal/agent/tool_registry.go
- ui/src/App.tsx
- internal/agent/snapshot_test.go
- internal/runtime/agent/agent.go
- internal/agent/skill_tool.go
- internal/persis/fileagentskill/examples_test.go
- internal/agent/delegate_test.go
- internal/agent/model_config.go
- internal/agent/snapshot.go
- internal/agent/snapshot_stores.go
- internal/service/worker/remote_handler_test.go
- ui/src/pages/agent-skills/SkillEditorPage.tsx
- internal/service/frontend/api/v1/agent_skills_test.go
- internal/agent/search_skills_tool_test.go
- internal/agent/search_skills_tool.go
- internal/persis/fileagentskill/store_test.go
- ui/src/features/agent/components/SkillPicker.tsx
- ui/src/pages/agent-skills/index.tsx
- api/v1/api.yaml
- ui/src/menu.tsx
- internal/agent/system_prompt.txt
- internal/agent/skill.go
- internal/service/frontend/api/v1/agent_skills.go
- internal/persis/fileagentskill/store.go
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
fileagentskill), skill tools (use_skill,search_skills), skill management REST API endpoints, skill picker UI, and skill references from distributed snapshots and system promptsSeedReferences) and thedagu ai installcommandTest plan
go build ./...passesmake lintpasses (0 issues)internal/agent,internal/cmd,internal/service/worker,internal/agentsnapshot,internal/runtime/agent,internal/runtime/builtin/agentstep)Summary by CodeRabbit
Release Notes
Removals