feat(buzz-acp): wire persona-pack MCP servers and skills into spawn - #7359
Open
mraad wants to merge 6 commits into
Open
feat(buzz-acp): wire persona-pack MCP servers and skills into spawn#7359mraad wants to merge 6 commits into
mraad wants to merge 6 commits into
Conversation
Persona packs resolved MCP servers and scoped skills, but nothing consumed them: `resolve_pack` was only reachable from `buzz pack validate/show`, and `ResolvedPersona.skills` was marked "reserved for future use, not yet wired". Every agent behind one harness host therefore saw the same tools. Add `--persona-pack <DIR>` / `--persona <NAME>` (mutually required) so the harness resolves a pack at startup and applies it: - MCP: the persona's merged servers (pack `.mcp.json` + frontmatter `mcp_servers:`) are passed to every `session/new` alongside the Buzz-managed server. The Buzz server keeps its name, so a colliding pack entry is dropped rather than shadowing the agent's relay credentials. - Skills: `pack::resolve_skills` scoping (claimed by a persona -> that persona only; unclaimed -> all) is materialized into `<workdir>/.agents/skills/<name>` and linked into the per-runtime discovery directories. An existing skill is never overwritten, but is still linked, so an operator-pinned version survives and stays reachable. Add `--workdir <DIR>`, applied before anything reads the working directory, so the ACP session cwd, the agent subprocess, and skill discovery agree. That is what makes one agent's capabilities unshared with another's. Pack failures are terminal: an unloadable pack, an unknown persona, or a declared-but-missing skill directory aborts startup rather than running an agent with silently fewer capabilities than its persona declares. Desktop spawn plumbing is untouched; it stays detached per the team migration in `migration/detach.rs`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017VkcJb4EkTx6pps1hPW9Xy Signed-off-by: mraad <mraad23@gmail.com>
🔐 Codex Security Review
|
A pack could only declare stdio MCP servers, and the restriction failed
silently: `McpServerConfig` required `command` at the frontmatter layer, and
`parse_mcp_server_config` returned `None` for anything without one, so a
url-backed entry was dropped during the merge. The agent then started with
fewer tools than its persona declared and nothing said so.
Model the transport explicitly. `ResolvedMcpServer` now carries
`McpTransport::Stdio { command, args, env }` or `McpTransport::Http { url,
headers }`, chosen by which field the entry declares. `ResolvedMcpServer` is
the pack-side type; `acp::McpServer` gains the matching wire variants.
The stdio wire form deliberately carries **no** `type` field. Adapters route
stdio on its absence (`else if (!("type" in server))`), so emitting
`"type":"stdio"` would match neither branch and the server would never start —
a silent tool loss of exactly the kind this commit removes. HTTP serializes as
`{"type":"http", name, url, headers}`.
Failures are now terminal and named:
- an entry with neither `command` nor `url` fails resolution,
- `type: "sse"` is rejected during resolution rather than at session start,
where the runtime would refuse it with a message that names no pack entry.
`mcp_servers_with_git_origin` injects `BUZZ_GIT_ORIGIN_*` into stdio servers
only; an HTTP server runs elsewhere and has no subprocess environment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017VkcJb4EkTx6pps1hPW9Xy
Signed-off-by: mraad <mraad23@gmail.com>
…-spawn Signed-off-by: mraad <mraad23@gmail.com>
…-spawn Signed-off-by: mraad <mraad23@gmail.com>
`returns_when_escaped_descendant_retains_pipe` flaked with
`ParseIntError { kind: Empty }` when read under a loaded lane.
The 300ms probe timeout was not a tuning knob, it was the setup's race
budget: the backgrounded descendant has to fork, exec perl, load POSIX,
`setsid()` out of the leader's process group, and record its PID before
the deadline fires the `killpg`. Lose that race and the descendant dies
inside the group, leaving the PID file empty, and the test asserts
against a descendant that never escaped.
Measured spawn->PID-recorded latency on a 16-core machine:
idle med 30ms p95 33ms max 47ms 0/40 >300ms
2x concurrent suite med 48ms p95 115ms max 177ms 0/40 >300ms
4x suite + 24 spinners med 68ms p95 233ms max 498ms 1/40 >300ms
Raise the timeout to 3s (~6x the observed max) and the outer watchdog to
15s, and record those numbers at the call site so the budget is not
trimmed back toward the escape latency later. CI runners have fewer
cores than the machine these were measured on.
Also replace the bare `.expect("descendant PID must be numeric")` with a
message that names the setup race, so a future recurrence reports why the
run proved nothing instead of surfacing an opaque parse error.
The test now costs its full 3s timeout by construction (the leader loops
forever to force the timeout path); that is the price of that path being
deterministic.
No production change. Verified: 12/12 under the load that reproduced the
flake; still falsifiable (deleting the production `stop.store(true)` fails
it with "output_with_timeout did not return within 15s"); full tauri lib
suite 3165 passed / 0 failed, fmt and clippy clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GecRn4WhoM35gz9fRY7Y1A
Signed-off-by: mraad <mraad23@gmail.com>
Add a second rule to Review-Proven Test Standards: a wall-clock timeout that gates test setup is a race budget, not a tuning knob. Size it against measured worst-case latency on a loaded machine rather than the idle case, record the measurement at the call site so it is not trimmed back toward the latency it exists to clear, and make a lost setup race say so rather than surfacing as an opaque parse error. Generalizes the flake fixed in a41fcb3, where the escaped-writer probe in bounded_command.rs budgeted 300ms for a descendant whose spawn-to-ready latency measured ~30ms idle but p95 233ms / max 498ms under a loaded lane. Also correct the section intro, which read "this is the test-quality rule" in the singular and attributed the section wholesale to the 25-PR review mining. This rule came from a fixed flake, not a review thread, so the intro now separates the mined rule from later additions instead of extending that provenance to them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GecRn4WhoM35gz9fRY7Y1A Signed-off-by: mraad <mraad23@gmail.com>
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
Persona packs already resolved per-persona MCP servers and scoped skills, but
nothing consumed the result:
resolve_packwas reachable only frombuzz pack validate/show, andResolvedPersona.skillscarried the comment"reserved for future use, not yet wired". Every agent behind a harness host
therefore saw the same tools, which is the opposite of
VISION_AGENT.md's "ten agents in parallel behind Buzz, each with their own MCPconfiguration".
This wires the pack into
buzz-acpspawn.--persona-pack <DIR>/--persona <NAME>(mutually required;BUZZ_ACP_PERSONA_PACK/BUZZ_ACP_PERSONA_NAME) resolve a pack at startup:.mcp.json+ frontmattermcp_servers:) are passed to everysession/newalongside the Buzz-managedserver. The Buzz server keeps its name, so a colliding pack entry is dropped
rather than shadowing the agent's relay credentials.
pack::resolve_skillsscoping (claimed by a persona → thatpersona only; unclaimed → all) is materialized into
<workdir>/.agents/skills/<name>and linked into the per-runtime discoverydirectories (
.claude/skills,.goose/skills,.codex/skills). An existingskill is never overwritten but is still linked, so an operator-pinned version
survives and stays discoverable.
--workdir <DIR>(BUZZ_ACP_WORKDIR) is applied before anything reads theworking directory, so the ACP session cwd, the agent subprocess, and skill
discovery agree. This is the part that makes one agent's capabilities actually
unshared with another's — agents sharing a directory share their skills.
Pack failures are terminal: an unloadable pack, an unknown persona, or a
declared-but-missing skill directory aborts startup rather than running an agent
with silently fewer capabilities than its persona declares (AGENTS.md rule 1).
Desktop spawn plumbing is untouched — it stays detached per the team migration
in
desktop/src-tauri/src/migration/detach.rs.Related issue
None found — searched open issues/PRs for persona-pack MCP and skill wiring.
Testing
cargo test -p buzz-acp(926 pass),cargo test -p buzz-persona --lib(127 pass),
cargo fmt --all --check,cargo clippy -p buzz-acp -p buzz-persona --all-targets -D warnings, andjust ci.Live run of the built binary against a two-persona fixture pack —
alphaclaims the
searchskill,sharedis unclaimed:agent-alphareceivedsearch+shared;agent-betareceivedsharedonly,each under its own workdir with the three runtime directories symlinked at its
own
.agents/skills. (Both runs then exited on a deliberately nonexistent agentcommand — no ACP binary was needed to exercise this path.)
MCP is verified on the wire rather than only in config: a new
acp.rstestdrives a scripted fake agent and asserts the serialized
session/newrequestcarries
mcpServers[0]with its command, args, and env.Guards were mutation-checked — removing the MCP name-collision check, the
missing-skill error, or the skill no-clobber check each fails exactly its own
test, so none of them is decorative (TESTING.md "Review-Proven Test Standards").
Out of scope, called out explicitly: desktop spawn plumbing, persona hooks
(still parsed-but-unwired), persona
runtime_env_vars, and${VAR}interpolation in MCP env.
🤖 Generated with Claude Code
https://claude.ai/code/session_017VkcJb4EkTx6pps1hPW9Xy