feat(agents): named agent definitions with per-agent tool restrictions#90
Merged
Conversation
#84) Add reusable subagent presets discovered from markdown files and advertised to the LLM through the subagent tool. - new internal/agents package: discovery from .agents/agents/, .kit/agents/, and ~/.config/kit/agents/ with project-over-user precedence; YAML frontmatter (description, model, tools, temperature, timeout, hidden, disabled); body becomes the system prompt - built-in agents: general (full tool set) and explore (read-only) - subagent tool: new optional agent parameter; tool description now lists discovered agents and their tool access at registration time - per-agent tool allowlists map tools: frontmatter to the subagent's tool set; restricted agents also skip child MCP loading so servers cannot bypass the allowlist; recursion guard preserved - SDK: SubagentConfig.Agent/.Temperature, Kit.GetAgents/GetAgent, LoadAgentDefinitions, Options.NoAgents (KIT_NO_AGENTS / no-agents) - docs: README, www (subagents, sdk, configuration, index), pkg/kit README, kit-sdk skill Fixes #84
|
Connected to Huly®: KIT-91 |
📝 WalkthroughWalkthroughAdds named agent presets discovered from markdown files, advertises them through subagent tooling, resolves them into subagent defaults and tool allowlists, wires the data through Kit creation and spawning, and documents discovery and disable controls. ChangesNamed Agents Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LLM
participant SubagentTool
participant Kit
participant NamedAgents
LLM->>SubagentTool: subagent(task, agent="explore")
SubagentTool->>Kit: SubagentSpawnRequest
Kit->>NamedAgents: resolveAgentDefinition(cfg)
NamedAgents-->>Kit: preset values and allowlist
Kit->>Kit: Subagent(cfg)
Kit-->>SubagentTool: SubagentSpawnResult
SubagentTool-->>LLM: result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (2)
internal/agents/agents.go (1)
162-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAggregated errors lose wrapping (
%w).Both
LoadFromDirandLoadAgentscollect per-file/per-scope errors as strings and rejoin them withfmt.Errorf("...: %s", strings.Join(errs, "; ")), which discards the error chain. As per coding guidelines,**/*.go: "Always check errors and wrap them withfmt.Errorf("context: %w", err)". Consider collectingerrorvalues and usingerrors.Joinso callers can stillerrors.Is/errors.Asinto individual causes.♻️ Suggested refactor
- var loaded []*Agent - var errs []string + var loaded []*Agent + var errs []error for _, entry := range entries { if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".md") { continue } a, err := Load(filepath.Join(dir, entry.Name())) if err != nil { - errs = append(errs, err.Error()) + errs = append(errs, err) continue } loaded = append(loaded, a) } if len(errs) > 0 { - return loaded, fmt.Errorf("some agents failed to load: %s", strings.Join(errs, "; ")) + return loaded, fmt.Errorf("some agents failed to load: %w", errors.Join(errs...)) }Also applies to: 234-238
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agents/agents.go` around lines 162 - 166, The aggregated-load path in LoadFromDir and LoadAgents is flattening underlying errors into strings, so callers lose error chaining and cannot use errors.Is/errors.As. Update the error collection in these loading functions to keep error values instead of strings, then return a wrapped aggregated error using errors.Join or fmt.Errorf with %w where appropriate, preserving the individual causes while still adding context.Source: Coding guidelines
pkg/kit/kit.go (1)
1571-1577: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider structured logging instead of
fmt.Fprintf(os.Stderr, ...)for the agent-load warning.As per coding guidelines,
**/*.go: "Usegithub.com/charmbracelet/logfor structured logging." This new warning path writes directly toos.Stderrviafmt.Fprintfrather than going through the project's logger (e.g.log.Warn("failed to load some agent definitions", "err", agErr)), which would also integrate with any configured log level/format. If similar warnings elsewhere in this file already usefmt.Fprintf(os.Stderr, ...)for consistency, this may be intentional — worth confirming which convention is authoritative here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/kit/kit.go` around lines 1571 - 1577, The agent-load warning in the `LoadAgentDefinitions` path currently writes directly to stderr with `fmt.Fprintf`, bypassing the project logger. Replace that warning in `kit.go` with structured logging via `github.com/charmbracelet/log` (for example, use the existing logger path around `LoadAgentDefinitions` and log the `agErr` as a field) so it respects configured log level and format; keep the same `NoAgents`/`Quiet` gating and update any related warning site in this block consistently.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/kit/agents.go`:
- Around line 51-62: `Kit.GetAgents` currently returns a shallow copy of
`m.namedAgents`, so the “snapshot” guarantee in the doc only applies to the
slice container, not the underlying `AgentDefinition` values. Update
`GetAgents()` to return a deep copy of each `*AgentDefinition` (including nested
mutable fields like `Agent`/`Tools` as needed) so callers cannot mutate shared
Kit state through returned pointers, or else adjust the comment to explicitly
document pointer sharing. Keep the behavior for `Options.NoAgents` and preserve
the existing nil/empty semantics.
- Around line 100-116: `resolveAgentDefinition()` currently treats
`SubagentConfig.Tools` as an allowlist by intersecting it with the named agent’s
`Tools`, which conflicts with the documented override behavior. Update the logic
in `Kit.resolveAgentDefinition` and/or `applyAgentDefinition` so explicit
`cfg.Tools` truly replaces the agent’s tool set, or revise the
`SubagentConfig`/`Tools` documentation to clearly state it is restricted by the
agent allowlist. Add a test covering the explicit-`Tools` case to lock in the
intended behavior.
In `@pkg/kit/kit.go`:
- Around line 1571-1577: The agent-discovery guard in the `LoadAgentDefinitions`
path already checks `v.GetBool("no-agents")`, but the matching CLI flag is not
wired up. Add a `--no-agents` flag binding in the root command setup (the
`cmd/root.go` registration that populates viper) and ensure it maps to the same
`no-agents` key used in `kit.go`, so the existing `opts.NoAgents` and
`v.GetBool("no-agents")` check can actually disable agent loading from the CLI.
In `@www/pages/sdk/options.md`:
- Around line 74-78: Update the `NoAgents` example comment in the options docs
so it reflects the full discovery scope handled by the `NoAgents` setting.
Expand the inline note beside `NoAgents` to mention that it disables named agent
discovery from `.agents/agents/`, `.kit/agents`, `~/.config/kit/agents`, and
built-ins, keeping the example consistent with the runtime contract.
---
Nitpick comments:
In `@internal/agents/agents.go`:
- Around line 162-166: The aggregated-load path in LoadFromDir and LoadAgents is
flattening underlying errors into strings, so callers lose error chaining and
cannot use errors.Is/errors.As. Update the error collection in these loading
functions to keep error values instead of strings, then return a wrapped
aggregated error using errors.Join or fmt.Errorf with %w where appropriate,
preserving the individual causes while still adding context.
In `@pkg/kit/kit.go`:
- Around line 1571-1577: The agent-load warning in the `LoadAgentDefinitions`
path currently writes directly to stderr with `fmt.Fprintf`, bypassing the
project logger. Replace that warning in `kit.go` with structured logging via
`github.com/charmbracelet/log` (for example, use the existing logger path around
`LoadAgentDefinitions` and log the `agErr` as a field) so it respects configured
log level and format; keep the same `NoAgents`/`Quiet` gating and update any
related warning site in this block consistently.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c1a23f6-2c66-4827-9965-1fe937fce4da
📒 Files selected for processing (20)
README.mdinternal/agent/agent.gointernal/agent/factory.gointernal/agents/agents.gointernal/agents/agents_test.gointernal/core/subagent.gointernal/core/subagent_named_test.gointernal/core/subagent_test.gointernal/core/tools.gointernal/kitsetup/setup.gopkg/kit/README.mdpkg/kit/agents.gopkg/kit/agents_test.gopkg/kit/kit.goskills/kit-sdk/SKILL.mdwww/pages/advanced/subagents.mdwww/pages/configuration.mdwww/pages/index.mdwww/pages/sdk/options.mdwww/pages/sdk/overview.md
…tools contract (#84) Address review findings on the named agents PR: - GetAgents/GetAgent now return deep copies (Tools slice, Temperature pointer) so callers cannot mutate shared Kit state through the returned definitions - add the missing --no-agents CLI flag bound to the no-agents viper key, following the --no-skills pattern; document it in README, cli/flags, subagents page, and the kit-sdk skill - document that SubagentConfig.Tools is intersected with a named agent's allowlist (narrows, never widens) rather than overriding it; this is deliberate since the agent-loop spawner always passes inherited tools and an override would bypass the allowlist - expand the NoAgents example comment in sdk/options.md to cover the full discovery scope - tests: TestKitGetAgents_DeepCopy, TestResolveAgentDefinition_ExplicitToolsIntersectAllowlist
The Agent field doc already stated the intersection semantics, but the Tools field doc itself still read as a plain override. Add a cross-reference so both godoc entries describe the same contract.
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.
Description
Adds named agents: reusable subagent presets discovered from markdown files, advertised to the LLM through the
subagenttool, with per-agent tool allowlists.Previously every subagent was an ad-hoc prompt — callers re-specified system prompts, models, and tool sets on every spawn, and the LLM had no way to know specialist agents existed. Now a definition file like
.agents/agents/code-reviewer.md(frontmatter:description,model,tools,temperature,timeout,hidden,disabled; body = system prompt) becomes a preset the LLM can invoke by name:and the SDK can use directly:
Three connected pieces, matching the issue's proposal:
internal/agents/): definitions load from<project>/.agents/agents/,<project>/.kit/agents/, and~/.config/kit/agents/(XDG-aware), with project-over-user-over-builtin precedence. Two built-ins ship with Kit:general(full subagent tool set) andexplore(read-onlyread, grep, find, ls).disabled: trueremoves an agent and anything it shadows;hidden: truekeeps it resolvable but unadvertised.internal/core/subagent.go): the subagent tool description is generated at registration time, listing each non-hidden agent with its description and tool access. A new optionalagentparameter resolves presets; explicitmodel/system_prompt/timeout_secondsarguments still override.tools:frontmatter list restricts the subagent to exactly those tools. Restricted agents also suppress child MCP server loading so MCP tools cannot bypass the allowlist. The existing recursion guard (nosubagenttool in children) is preserved.Fixes #84
Type of Change
Checklist
go test -race ./...,golangci-lint runclean)Additional Information
New files
internal/agents/agents.go(+ tests) — definition type, frontmatter parsing, discovery, precedence merge, built-inspkg/kit/agents.go(+ tests) — SDK surface:AgentDefinition,LoadAgentDefinitions(),Kit.GetAgents()/GetAgent(), preset resolution, allowlist filteringinternal/core/subagent_named_test.go— dynamic description +agentarg pass-through testsModified
internal/core/subagent.go—agentparam, dynamic description,SubagentSpawnFuncrefactored to a struct-basedSubagentSpawnRequestinternal/core/tools.go,internal/agent/{agent,factory}.go,internal/kitsetup/setup.go—NamedAgentsthreading viaWithNamedAgentsToolOptionpkg/kit/kit.go—SubagentConfig.Agent/.Temperature,Options.NoAgents, discovery wiring inNew(), resolution inSubagent()README.md,pkg/kit/README.md,www/pages/(subagents, sdk/overview, sdk/options, configuration, index),skills/kit-sdk/SKILL.md— also fixes a pre-existing doc bug (Task:→Promptin the www SDK example)Backward compatibility
subagenttool calls andKit.Subagent()call sites work unchanged; without any definition files only the two built-ins are advertisedno-agentsconfig key,KIT_NO_AGENTS=true, orOptions.NoAgentscore.SubagentSpawnFuncnow takesSubagentSpawnRequest(not part of the public SDK)Verified interactively (tmux + live Anthropic model): agent advertisement in tool description, preset system prompt application, allowlist enforcement (write/bash correctly unavailable to a read-only agent), unknown-agent error with available-agent listing, and built-in
exploreend-to-end.Summary by CodeRabbit
agentpreset to inherit prompt/model/tools/timeout/temperature defaults while explicit fields override and tool access only narrows.NoAgentsand--no-agentsto disable discovery.