feat: kimchi agents extension — Agent tool, 8 default personas, phase-aware harness#167
feat: kimchi agents extension — Agent tool, 8 default personas, phase-aware harness#167tautvydasLiekis wants to merge 4 commits intomasterfrom
Conversation
Kimchi Code Review
Summary📊 Review Score: 62/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Tests exist for default agents, discovery priority, memory paths, and skill loading. However, the core execution engine ( 📝 Found 9 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
tautvydasLiekis
force-pushed
the
feat/kimchi-agents-extension
branch
from
1347bb8 to
f0bef39
Compare
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 62/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 5/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Tests exist for default agents, discovery priority, memory paths, and skill loading. However, the core execution engine (AgentManager, agent-runner.ts, and the Agent tool execute block in index.ts) lacks visible unit tests for concurrency, queue draining, error handling, and lifecycle management.
📝 Found 9 issue(s). See inline comments for details.
| record.completedAt = Date.now() | ||
| this.onComplete?.(record) | ||
| } | ||
| } |
There was a problem hiding this comment.
In drainQueue, the call to startAgent is wrapped in a try/catch. startAgent increments this.runningBackground immediately for background agents. If startAgent throws synchronously (for example, if the onStart callback throws), the catch block transitions the record to error and calls onComplete, but never decrements this.runningBackground. Over time this leaks concurrency slots until no new background agents can start.
💡 Suggestion: Decrement this.runningBackground inside the catch block before calling onComplete, or restructure startAgent so that the increment happens only after all synchronous setup has succeeded.
| durationMs, | ||
| tokens, | ||
| } | ||
| } |
There was a problem hiding this comment.
reloadCustomAgents uses process.cwd() and is called once at module load time. The session_start handler only clears completed records and does not re-run agent discovery. If a new session starts in a different project directory, agents from the previous project remain registered and the new project's .kimchi/agents/ directory is ignored.
💡 Suggestion: Refactor reloadCustomAgents to accept a cwd argument and invoke reloadCustomAgents(ctx.cwd) inside the session_start event handler instead of calling it eagerly at extension load time.
| @@ -0,0 +1,1685 @@ | |||
| /** | |||
There was a problem hiding this comment.
The extension entry point is a single 1,685-line factory file, vastly exceeding the original architecture goal of ~400 lines for the entire extension. It mixes tool schemas, execution logic, batching, notification formatting, UI widget wiring, event emission, and command handlers. This makes the code difficult to review, test, and maintain.
💡 Suggestion: Split index.ts into focused modules: agent-tool.ts (spawn/execute), result-tool.ts, steer-tool.ts, notifications.ts (XML formatting), batching.ts (group join logic), and commands.ts. Limit the factory file to wiring and registration.
| ? `${unconsumed.length} agent(s) finished (partial — others still running)` | ||
| : `${unconsumed.length} agent(s) finished` | ||
|
|
||
| const [first, ...rest] = unconsumed |
There was a problem hiding this comment.
The AgentManager instance is stored on globalThis via a well-known symbol Symbol.for("pi-subagents:manager"). If multiple sessions run concurrently in the same Node.js process, the second session overwrites the first session's manager. When the first session shuts down, its session_shutdown handler deletes the symbol, which can orphan the second session's running agents.
💡 Suggestion: Avoid global mutable state. Attach the manager to the per-session ExtensionContext object, or use a WeakMap keyed by the ExtensionAPI instance to keep manager instances isolated.
| @@ -0,0 +1,387 @@ | |||
| /** | |||
There was a problem hiding this comment.
The AgentManager class implements nuanced concurrency control (queue draining, max concurrent limits, background vs foreground lifecycle, abort semantics, and a cleanup timer), yet there are no visible unit tests covering these paths. The same gap exists for agent-runner.ts and the main Agent tool execute block in index.ts.
💡 Suggestion: Add unit tests for AgentManager that exercise queue overflow, synchronous failure during startAgent, abort of queued and running agents, and disposal. Test the Agent tool execute path with injected doubles for runAgent and ExtensionAPI.
| private onStart?: OnAgentStart | ||
| private onCompact?: OnAgentCompact | ||
| private maxConcurrent: number | ||
|
|
There was a problem hiding this comment.
The project architecture explicitly requires "factory functions, no classes". The new code introduces AgentManager, GroupJoinManager, and AgentWidget as classes, which deviates from the established pattern used in other kimchi extensions and makes the codebase inconsistent.
💡 Suggestion: Refactor AgentManager and GroupJoinManager into factory functions that return plain objects with closures or methods, matching the style of existing extensions like subagent.ts.
| const candidate = join(pkg.installedPath, "agents") | ||
| if (existsSync(candidate)) dirs.push(candidate) | ||
| } | ||
| return dirs |
There was a problem hiding this comment.
ℹ️
getInstalledPackageAgentDirs silently swallows every exception with a bare catch { return [] }. A misconfigured package, permissions error, or upstream API change will silently disable all package-level agent discovery without any diagnostic trace.
💡 Suggestion: Log the exception before returning the empty array, e.g. catch (err) { console.warn("Failed to discover package agents:", err); return [] }, or surface it through the extension's diagnostic channel.
| } | ||
|
|
||
| for (const [name, config] of userAgents) { | ||
| agents.set(name, config) |
There was a problem hiding this comment.
ℹ️⚡ Performance
resolveKey performs case-insensitive lookups by iterating over every key in the agents Map. This is O(n) and executes on every call to getAgentConfig, isValidType, and getConfig, which are invoked frequently during tool execution.
💡 Suggestion: Maintain a secondary Map<string, string> that maps lowercase names to canonical names whenever registerAgents is called. Lookups then become O(1) and case-insensitive resolution is trivial.
| clipboardImageExtension, | ||
| uiExtension, | ||
| subagentExtension, | ||
| agentsExtension, |
There was a problem hiding this comment.
ℹ️🏗️ Design
The diff adds both agentsExtension and an unrelated skillsSlashCommand extension. There is no plan, test, or documentation in this PR describing the purpose or behavior of skillsSlashCommand, indicating scope creep.
💡 Suggestion: Remove skillsSlashCommand from this PR and introduce it in a dedicated PR with its own tests and documentation, or add an explanation of its purpose to the current changeset.
tautvydasLiekis
changed the title
…s menu Replaces the primitive subagent tool with a full agent extension at src/extensions/agents/. The Agent tool is now the canonical delegation surface; persona definitions live as MD files (8 default personas bundled, plus user customs from .kimchi/agents/ and ~/.config/kimchi/agents/); runtime state is tracked by AgentManager with a live above-editor widget and a scrollable /agents menu (View / Edit / Disable / Eject / Create wizard / Settings). Subsystem boundaries - Agents extension owns: persona loading, Agent tool spawn, child lifecycle, widget rendering, conversation viewer, /agents menu, mid-run steering, persistent per-agent memory, group join, output JSONL. - Orchestrator (src/extensions/orchestration/) keeps its existing role: inform the parent LLM about available models + their strengths/tier/ vision so the parent can pick a model when it calls the Agent tool. The two systems are complementary: orchestrator chooses, agents execute. Model selection - Persona frontmatter carries `models: [...]` (an array, not a single field) — the LLM-picked allow-list for that persona. - At Agent tool call: caller-provided `model` wins; else persona.models[0]; else inherit parent. The orchestrator's effect lands at step 1: parent reads its system-prompt model table and passes the pick through. - MODEL_CAPABILITIES (orchestration/model-registry/) is the single source of truth; default agents derive their models from registry strengths. Other notable changes - /phase slash command for manual phase switching. - /agents menu: source indicators (• project, ◦ global, ✕ disabled), Eject (export bundled to .md), Disable/Enable (frontmatter toggle), Create wizard (Generate-with-model + Manual paths), Settings. - Permissions: drop `subagent` from BUILTIN_ALLOW_TOOL_NAMES, add lowercased `agent`, `get_subagent_result`, `steer_subagent`. - docs/agents.md: discovery, frontmatter reference, examples. - footer: counts active agents via AgentManager.
tautvydasLiekis
force-pushed
the
feat/kimchi-agents-extension
branch
from
59ce370 to
b31da6f
Compare
…rator auto-pick model
Three coordinated changes to the agents extension.
1. Folder restructure under src/extensions/agents/
25 files reorganized into manager/ personas/ resolution/ prompt/
memory/ ui/ subfolders. Old flat paths kept as re-export shims so
nothing outside agents/ has to change. ui/ already existed and stayed.
2. Plan persona — write scope under .kimchi/plans/
- plan persona gains write+edit tools (was read-only).
- persona body instructs the agent to only write under .kimchi/plans/.
- manager/agent-runner.ts sets process.env.KIMCHI_AGENT_PERSONA on
spawn so the permissions hook can scope writes per-persona.
- permissions/index.ts: when KIMCHI_AGENT_PERSONA=plan, write/edit
calls outside .kimchi/plans/ are blocked with a notify; calls
inside the dir pass through normal rules.
3. Orchestrator auto-pick fallback for Agent tool
New helper: orchestration/model-registry/recommend.ts exports
recommendModel({ strengths, needsVision?, preferTier? }) which
filters MODEL_CAPABILITIES and applies tier fallback (light →
standard → heavy / etc). Deterministic order.
resolution/invocation-config.ts new fallback chain for the model:
1. params.model (LLM picked) wins
2. agentConfig.models[0] stable persona default
3. recommendModel(agentConfig.strengths) orchestrator picks for persona
4. recommendModel([currentPhase]) orchestrator picks for phase
5. inherit parent
personas/types.ts gains optional strengths[] and preferTier fields;
default-agents.ts populates them for personas with a clear strength
profile (explore→[explore], researcher→[research], plan→[plan],
worker→[build], reviewer→[review]). general-purpose leaves them
undefined to inherit parent.
The orchestrator is unchanged in role: still informs the parent's
system prompt about model strengths so the parent picks. Auto-pick
only kicks in when the parent explicitly omits model on the Agent
call AND the persona has strengths declared. Parent always wins
when it specifies model.
…erflow and crashes
Introduced three example personas (Code Reviewer, Test Writer, Research Assistant) to demonstrate subagent configurations. Includes corresponding documentation under `docs/subagents/` and references in `README.md`.
1 participant
Kimchi Summary
What changed
Renames the subagent system to "agents" and replaces it with a full extension supporting customizable agent personas. Users can define specialized agents as Markdown files with YAML frontmatter; they are discovered from project (
.kimchi/agents/), user-global, and bundled extension paths.Key changes
src/cli.ts: ReplacessubagentExtensionwithagentsExtensionin the extension bootstrapsrc/components/footer.ts: Migrates active-agent indicator togetActiveAgentCountand renames UI label to "agent(s)"src/extensions/agents/: Adds the new extension module including:AgentManagerfor concurrency-limited background execution (default 4 concurrent) with automatic queuingagent-runner.tsfor foreground and background agent lifecyclecustom-agents.tsanddefault-agents.tsfor persona resolution/agentscommand with create/edit/disable/eject workflowsdocs/agents.md: Documents the phase model, discovery hierarchy, frontmatter schema (models,tools,thinking,memory,max_turns, etc.), and recovery behaviordocs/subagents/: Adds example personas (code-reviewer.md,test-writer.md,research-assistant.md) demonstrating read-only restrictions, multi-model selection, persistent memory, and skill preloadingsrc/extensions/agents/discovery-priority.test.ts: Validates that project-level agents in.kimchi/agents/override global agents with the same name.gitignore: Ignores*.bun-buildartifactsImpact
subagentExtensionmodule is removed; all internal references are migrated to the newagentsmodule structure and naming..kimchi/agents/(project) or~/.config/kimchi/harness/agents/(user). Project-level definitions always override user and default agents.