feat(agent): multi-agent apps with per-agent skills, tools, and optional delegation - #2354
ariskemper wants to merge 6 commits into
Conversation
… apps
Support apps with one or many specialized markdown agents, each with its own
settings and its own SKILL.md, with orchestration being opt-in.
- agent-definition: parse new `skills` (true | string[]) and `delegates`
(string[]) frontmatter fields.
- discovery: recognize the colocated directory layout `agents/{id}/AGENT.md`
(+ `SKILL.md` / `skills/`) alongside the flat `agents/{id}.md` form;
nested SKILL.md and references are ignored as agents.
- agent-scoped-skill-catalog: load an agent's own colocated skills
(`{root}/SKILL.md` + `{root}/skills/**`), filtered by the `skills` selector.
- agent-delegation: opt-in `delegates` build `agent_{id}` tools that run named
specialist agents (lazy resolution, self/dupe excluded). No delegates =>
no orchestration.
- markdown adapter: thread skills/delegates/rootPath through to the runtime
agent and expose markdown metadata.
Adds focused unit tests for parsing, discovery, scoped-catalog loading, and
delegation. Plan in docs/proposals/multi-agent-skills.md.
Extend the colocated multi-agent model so each agent can own its tools the
same way it owns skills, and wire both into the running agent.
- agent-definition: new `tools` selector (true | string[]), parsed via a
shared capability-selector helper alongside `skills`.
- agent-scoped-capabilities (new): load `agents/{id}/tools/*.ts` as a
namespaced Tool record ({sanitizedAgentId}__{name}, provider-safe) and
register `agents/{id}/SKILL.md` + `skills/**` as Skill objects in the skill
registry (own skill = agent id, nested = {id}__{sub}).
- discovery handler: resolve colocated capabilities for directory agents and
pass resolved skill ids + tool record to the adapter.
- markdown adapter: resolved skill ids -> config.skills (explicit list, never
registry-wide `true` so agents don't leak each other's skills); colocated +
delegate tools merged into config.tools.
Workflows/prompts/resources/tasks stay global by design — workflows orchestrate
agents (above them); agent-level orchestration is `delegates`.
Adds unit tests for the tools loader, skill registration, namespacing, and an
end-to-end discovery assertion that colocated skills/tools land on the agent
config. Updates docs/proposals/multi-agent-skills.md.
Pass 1 — simplify: - Remove orphaned agent-scoped-skill-catalog.ts (+ test, + exports): exported but never consumed; the factory path uses agent-scoped-capabilities.ts. - Flatten createRuntimeAgentFromMarkdownDefinition from (Definition | Input) to (definition, options?), dropping the 'definition in input' discriminator. - Resolve colocated skills + tools concurrently (disjoint subtrees). Pass 2 — correctness: - Skip + report colocated tools whose namespaced name violates the provider tool-name charset / 64-char limit. - Detect and report the sanitize-collision case (e.g. 'a.b' vs 'a_b' -> 'a_b') instead of silently overwriting one agent's capabilities. - Report duplicate agent ids (flat 'x.md' + dir 'x/') instead of silent first-wins. - Colocated tools take precedence over delegate tools on key collision. - Fix skill-leak: directory agents now always resolve to their own explicit skill-id list (even empty) and never fall back to the registry-wide 'true', which would surface other agents' skills. Caught by a new regression test. - Add tests: delegate success path, factory namespaced-tool normalization, resolvedSkillIds override/empty-guard, flat back-compat, invalid SKILL.md -> recorded error (agent still registers), duplicate id, namespace collision. Pass 3 — security: - Reject '.' / '..' path segments in agent-dir, flat-id, and nested-skill-dir names (defense-in-depth path traversal) via isSafePathSegment; add a security regression test. Tests: 50 groups / 202 steps pass. Typecheck/lint clean (pre-existing react esm.sh typing error unrelated).
CI fix (the actual failure): - The 4 unit tests that load colocated tools via importModule failed in CI with 'op_read started but never completed' — importModule transpiles via esbuild, which keeps a child process alive across tests. Disable op/resource sanitizers on those tests, matching src/discovery/transpiler.test.ts. (Coverage gate also failed only because it shares the unit suite under --fail-fast.) Coverage: - Add src/discovery/file-discovery.test.ts covering the new fsAdapter branches of listDiscoveryDirectoryEntries / discoveryFileExists. - Cover the markdown metadata accessors and resolvedSkillIds override/empty-guard in agent-markdown-adapter.test.ts (function coverage 57% -> 100%). Docs: - guides/agents.md: per-agent skills & tools (directory layout), colocated namespacing, and the markdown-agent frontmatter table (skills/tools/delegates). - guides/multi-agent.md: declarative delegation with the delegates frontmatter. - api-reference is generated (deno task docs); left untouched here to avoid committing generator/formatter drift.
b384420 to
eb8fc65
Compare
The colocated-tool discovery tests transpile tool modules via importModule (esbuild), which keeps a warm child process alive across tests and trips Deno's op/resource leak sanitizers — the same reason src/discovery/transpiler.test.ts opts out. Raise SANITIZER_OPT_OUT_BASELINE 420 -> 428 to track these 4 tests (2 opt-outs each).
Review: this PR mixes several concerns, and one of them hides a real leakOverall the per-file code quality is solid — lazy delegate resolution so discovery order doesn't matter, errors reported without aborting sibling agents, sanitize-collision detection, 1. PR scope: this is three features, one fully orthogonal
(3) is the easiest to argue out: 2. Design: capability binding is coupled to file layoutThis coupling produces user-visible semantic inconsistencies:
3. Probable bug: the leak isn't actually closed (blocking)
Isolation is one-directional: directory agents can't see each other, but everyone else with Smaller note in the same vein: in Suggested path
At minimum, item 3 (the leak) should be tested/fixed before merge. |
Follow-up: a concrete path to adopt these ideas within existing conventionsFollowing up on my review above — the ideas here (colocation, per-agent capabilities, declarative delegation) are good and fit the architecture. Only the scoping mechanism conflicts with existing conventions. Here's a redesign path that keeps the feature, closes the leak, and ends up smaller. What the current conventions already give us
The only place this PR genuinely fights a convention is Proposed change: ownership lives in the registry, not in the handler/adapterInstead of the handler/adapter id-list plumbing, registration records ownership — either
This single rule fixes all three review findings at once: the What stays from this PR unchanged:
Why this is cheap right now
Suggested merge order
Happy to discuss alternatives — but the key ask is that scope becomes a property of registration, so binding ( |
Cross-platform check: three integration conflicts to settle before mergeI checked how the rest of the platform (control plane and Studio) interacts with the conventions this PR introduces. Existing projects are safe — flat TS/md agents behave identically everywhere. But three of the PR's choices conflict with established platform-wide conventions: 1.
|
|
I would not merge this as-is, although the direction is worth pursuing. The concern is not the directory-agent idea itself; it is that the current implementation makes capability ownership look scoped while still relying on global registries underneath. The main blocker is colocated skills being registered globally while I would split this before adoption:
There are also smaller correctness issues worth fixing in this PR or the split follow-ups: So my recommendation is: keep the idea, do not keep this shape. The stable version needs owner-scoped registries and enforcement at the tool boundary before it becomes a convention. |
|
Verified the three new claims from the comment above against the branch — all three hold, with file references, plus one addition: 1. 2. Delegate tool ids unsanitized — confirmed. 3. Lockfile/sanitizer churn — confirmed (new commit These findings are consistent with the rest of the thread, and the 6-step split above composes cleanly with the registry-ownership path proposed earlier: ownership metadata in the registries (step 3) makes the binding rule (step 5) expressible, and tool-boundary enforcement (step 4) is what makes it actually hold at runtime — point 1 shows config-level scoping alone is bypassable today. |
Extracted from the multi-agent staging branch (PR #2354) as standalone infrastructure: listDiscoveryDirectoryEntries lists immediate entries of a discovery directory and discoveryFileExists checks path existence, both fsAdapter-aware with Node fallback. No behavior changes to existing discovery.
Summary
Lets a Veryfront app run one or many specialized agents, each with its own settings, its own
SKILL.md, and its own tools, with opt-in orchestration. Previously markdown agents could only set persona/model/steps and could not bind project tools at all; skills were global and shared.Motivation
Teams want to ship a crew of focused agents (a researcher, a writer, a reviewer) that each carry their own knowledge (skills) and actions (tools), and to optionally place a coordinator in front of them — without hand-wiring
getAgentsAsToolsin code or leaking one agent's capabilities into another. This makes that a first-class, file-based convention.What changed
Colocated directory layout — an agent owns its capability surface:
skills: true | [..],tools: true | [..],delegates: [..].delegates:, an agent getsagent_{id}tools that run named specialists (each with their own settings/skills/tools); without it, agents are independent and selected byagentId. Flatagents/{id}.mdis unchanged (back-compat).{sanitizedAgentId}__{name}(provider-tool-name safe). A colocated agent'sskills: trueresolves to its own explicit id list, never the registry-widetrue— agents never see each other's skills.delegates:.prompts/,resources/,tasks/likewise stay global.Design notes:
docs/proposals/multi-agent-skills.md. User docs:docs/guides/agents.md(per-agent skills & tools + frontmatter table) anddocs/guides/multi-agent.md(declarative delegation).Acceptance criteria
agents/(directory and flat layouts side by side).SKILL.mdplus additionalskills/**, scoped and namespaced to that agent; loadable viaload_skill.tools/*.ts, namespaced and bound to that agent's tool set.skills:/tools:frontmatter select all (true) or a subset (list) of colocated capabilities.delegates:turns an agent into a coordinator withagent_{id}delegate tools; absent ⇒ independent agents. Self-delegation is rejected.skills: truenever surfaces another agent's skills (regression-tested).agents/{id}.md+ globalskills//tools/behave exactly as before.SKILL.md, and unsafe tool names are reported as discovery errors without aborting other agents../..path segments rejected (defense-in-depth traversal)./deep-review.Testing
/deep-review(simplify + correctness + security): fixes for tool-name validation, namespace-collision & duplicate-id reporting, tool merge precedence, a skill-leak path, and path-traversal hardening — each with a regression test.src/agent/index.ts.Review notes
/deep-review(stated transparently).deno task docs; left untouched to avoid committing generator/formatter drift (my local generator output differs in formatting from the committed, canonically-formatted pages). The feature is fully documented in the guides.RuntimeSkillDefinitioncatalog; feeding colocated skills into that path is deferred (the local/factory path is fully wired) to avoid shipping an unwired loader.Type of Change
Checklist