feat(launchpad): add --format json to buzz pack inspect (#239 STEP 1) - #257
Conversation
Adds a local --format human|json flag to `buzz pack inspect`, defaulting to human (the pre-existing pretty-print, byte-for-byte unchanged) with an opt-in json mode that serializes the full ResolvedPack -- the shape the Route 3 projector script (#239 STEP 2) will parse to configure a buzz-acp+goose runtime from a persona pack. Kept local to the subcommand rather than reusing the CLI's global --format flag: pack commands are local-only (no relay round trip), the global flag's default (json) would have silently changed pack inspect's long-documented default output, and the plan's own invocation syntax places the flag after the subcommand rather than before, where the global flag lives. Verification: cargo test -p buzz-cli -p buzz-persona -- 353 + 128 passed, 0 failed cargo clippy -p buzz-cli -p buzz-persona --all-targets -- clean Manually confirmed against launchpad/agents/the-professor: --format json emits model/temperature/mcp_servers/triggers matching the human printer's values exactly. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
benmitchell11
left a comment
There was a problem hiding this comment.
Read the full diff. The design choices are sound — reusing resolve_pack() rather than a second YAML parser (matches the plan's own reasoning), making --format local to pack inspect rather than reusing the global flag to avoid silently flipping the long-documented human default, and the tests genuinely exercise the JSON shape (including the tuple-vs-object env serialization, correctly flagged as an escalation for STEP 2's consumer).
One real gap: "Security implications: None" isn't accurate. Checked the pre-existing human-format printer directly (pack.rs, unchanged by this diff) — for mcp_servers, it only ever prints "MCP servers: {}", persona.mcp_servers.len(). It never prints command, args, or env for any MCP server, count only. But --format json serializes the full ResolvedPack, including every ResolvedMcpServer.env: Vec<(String, String)> verbatim — and per this same diff's own doc comment, those are "literals (no interpolation in this PR)," i.e. whatever a pack author wrote directly into mcp_servers[].env in their persona.md.
So this PR is the first thing in the repo's history that will print an MCP server's env values to stdout at all. That's not a privilege escalation (a local user already has filesystem read access to the same persona.md), but it is a genuine, new exposure surface in a machine-readable, easily-copy-pasted-into-a-log-or-chat format that the tool never produced before — exactly the class of thing this repo's own secrets-and-access review dimension (just merged in #248/#250/#252) exists to catch. If a future pack author puts something sensitive-looking in an MCP server's env block (a placeholder that looks like a real token, an internal URL, anything), --format json now prints it where the human format never did.
Not asking for a specific fix — that's a judgment call (redact/mask values in JSON, or just an accepted-risk note) — but the Security implications section should say this plainly instead of "None," so whoever merges this is making that call knowingly rather than by omission. Given this same repo just spent real effort teaching a review dimension to catch exactly this pattern, it seems worth holding this PR to the same bar.
Everything else — the local-flag design, the Serialize derives, the test coverage, the clippy/test verification — is solid. Happy to re-review once the Security implications section is updated.
… json (#239) benmitchell11 requested changes on #257: --format json serializes the full ResolvedMcpServer, including env values a pack author wrote as literals -- something the human printer never showed (it only prints a server count). Neither buzz-acp nor the #239 projector plan reads this field, so masking values (keeping keys) closes the new exposure for free. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
|
@benmitchell11 fixed in |
…json (#239) The prior fix (e49d188) only redacted mcp_servers[].env, but Ben's original review named command/args/env as the fields the human printer hides -- args was still serialized verbatim. A pack author can put a secret in an MCP server's CLI args (--api-key sk-...) exactly as easily as in its env block, so the exposure was only half-closed. redact_mcp_env is renamed redact_mcp_secrets and now masks each args element too (array length preserved, no key to keep since args aren't key/value). Confirmed neither buzz-acp's build_mcp_servers() (always spawns with an empty arg list) nor the #239 projector script (refuses to project a persona whose MCP server has any args) reads the real values, so nothing downstream depends on them. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
|
@benmitchell11 you were right to flag this as still open. The first fix ( Fixed in Re-review welcome. |
benmitchell11
left a comment
There was a problem hiding this comment.
Both follow-up commits checked directly, not just the write-up.
e49d1881b redacted env values only. 24ea61896 correctly caught that my original review named command/args/env together as the fields the human printer hides — args was still serialized verbatim after the first fix, and an --api-key sk-...-shaped arg is exactly as easy a mistake as a secret in env. Good catch on the second pass; the reasoning for why redacting both costs nothing downstream (buzz-acp::build_mcp_servers() always spawns with an empty arg list; the #239 projector refuses to project any persona whose MCP server has args at all) is verified against real behavior, not assumed.
Ran the new test myself rather than trusting the pasted output: redact_mcp_secrets_masks_env_values_and_args_keeps_env_keys passes. command is correctly left unredacted — it's the executable path, not secret material, and the JSON output needs it to stay useful.
The Security implications section is now honest about the two-pass process rather than presenting the final state as if it were right the first time. Approving.
Summary
Adds a local
--format human|jsonflag tobuzz pack inspect(STEP 1 of the merged Route 3 projector plan, #251).humanis the pre-existing pretty-print, unchanged;jsonemits the full resolved persona config, the shape STEP 2's projector script will parse.Related issue
Refs #239
Issue type
Task
Agent provenance
Objective
Give
buzz pack inspecta machine-readable JSON output mode, without changing its existing human-readable default, so a future projector script (#239 STEP 2) has something deterministic to parse.Impacted components
crates/buzz-cli/src/lib.rs
crates/buzz-cli/src/commands/pack.rs
crates/buzz-persona/src/resolve.rs
Approach and rejected alternatives
Added a
Serializederive toResolvedPack/ResolvedPersona/ResolvedMcpServer/ResolvedHooks/ResolvedTriggersinbuzz-persona::resolveand reusedresolve_pack()as-is, rather than writing a second JSON projection — the plan (#251) explicitly calls out that a second parser of pack YAML would duplicate precedence-resolution logic in a second place and drift from it.Made
--formata flag local to thepack inspectsubcommand (parsed after the subcommand name) rather than reusing the CLI's existing global--formatflag (parsed before the subcommand, defaultjson, used by every relay-querying command). Rejected wiring the global flag directly: its default is alreadyjson, andpack inspecthas never honored it — wiring it as-is would have silently flippedpack inspect's long-documented default output from human text to JSON for every existing caller (README.md, PERSONA_PACK_SPEC.md, the-professor-design.md all demonstrate the human default). A local flag defaulting tohumanpreserves that default byte-for-byte and only addsjsonas an explicit opt-in. This also matches the plan's own literal invocation syntax (buzz pack inspect --format json <dir>— flag after the subcommand), which the global flag's position convention (before the subcommand) would not have produced.Verification
Command run:
Raw output (tail):
Also ran (clean, no warnings):
Manual check against the real pack this projector will target:
--format jsonoutput containsmodel: "claude-sonnet-5",temperature: 0.4, onemcp_serversentry (professor-tools), andtriggersallfalse— identical to what the human printer already showed for the same pack.Full pre-push gate (
justrust-tests, desktop-tauri-checks, branch-skew) also ran clean on push.Follow-up commit 1 (
e49d1881b, env-only redaction, see Security implications below):cargo test -p buzz-cli -p buzz-persona— 354 + 128 + 5 + 13 passed, 0 failed, including the newredact_mcp_env_masks_values_keeps_keystest.cargo clippy -p buzz-cli -p buzz-persona --all-targetsclean. Full pre-push gate ran clean on push (branch-skew, rust-tests, desktop-tauri-checks).Follow-up commit 2 (
24ea61896, widened to also redactargs— see Security implications below):cargo test -p buzz-cli -p buzz-persona— same 354 + 128 + 5 + 13 passed, 0 failed, now via the renamed/extendedredact_mcp_secrets_masks_env_values_and_args_keeps_env_keys.cargo clippy -p buzz-cli -p buzz-persona --all-targetsclean,cargo fmt --checkclean. Manually ranbuzz pack inspect --format jsonagainst a test pack withargs: ["--api-key", "sk-super-secret-123"]andenv: {TOKEN: super-secret-value}— output shows"args": ["***", "***"]and"env": [["TOKEN", "***"]]. Full pre-push gate ran clean on push.Not verified
buzz pack validatewas left untouched — the plan's STEP 1 only namespack inspect, sovalidate's output format was not audited or changed. Also not verified: whether any other in-repo tooling grepspack inspect's stdout expecting the old plain-text shape by default — a repo-wide grep found only documentation references (README.md, PERSONA_PACK_SPEC.md, the-professor-design.md, this issue's own plan), none of which are executable, but a script outside what grep covers (e.g. a CI step) was not separately searched for.Security implications
Updated twice after review — second pass fixed a gap the first pass left open. The original text ("None") was wrong — @benmitchell11 caught it. The pre-existing human printer only ever prints an MCP server count; it never shows
command,args, orenv.--format jsonserializes the fullResolvedMcpServer, and bothenvvalues andargsare things a pack author may write as literals directly intomcp_servers[]in their persona.md — a secret is exactly as easy to put in an--api-key sk-...-shaped arg as in an env var.e49d1881bonly redactedenv;argswas still serialized verbatim, so the exposure was only half-closed. Caught on a second QA pass and fixed in24ea61896.pack inspect --format jsonnow redacts MCP serverenvvalues (keys kept, values become"***") and everyargselement (masked the same way; array length kept so a reader can still see how many there are — there's no "key" half to preserve for a positional arg, unlikeenv). The redaction function is nowredact_mcp_secrets(renamed fromredact_mcp_envto match its widened scope). Confirmed neitherbuzz-acp'sbuild_mcp_servers()(always spawns with an empty arg list regardless of what a pack declares) nor the #239 projector script (launchpad/agents/project-pack.py, which refuses to project any persona whose MCP server has args at all, by design) reads the realargsvalues, so redacting them costs nothing functionally — same conclusion asenv. Test renamed/extended toredact_mcp_secrets_masks_env_values_and_args_keeps_env_keys, and manually verified against a pack with both a real-lookingTOKENenv value and a--api-key sk-...-shaped arg — env key survives, env value is masked, every arg element is masked, array length is unchanged. Human format output remains byte-for-byte unchanged (still shows neither field, same as before either fix).Escalations
mcp_servers[].envserializes as an array of[key, value]pairs rather than a JSON object, since the underlying type isVec<(String, String)>— flagging this for whoever builds STEP 2's Python projector, since it needs to parse that shape rather than assume a dict, but it isn't a decision this PR needed to make (the underlying tuple-vec type is unchanged, matching every other consumer ofResolvedMcpServer). Note the values in this shape, and everyargselement, are now"***"per the redaction above; onlyenv's key names are real.