feat(launchpad): add generic pack-to-env projector (#239 STEP 2) - #260
Conversation
Adds launchpad/agents/project-pack.py: a generic (not Professor-specific) projector that calls `buzz pack inspect --format json <dir>` (STEP 1) and emits a shell-sourceable env file mapping a persona's fully-resolved config onto the env vars buzz-acp and goose read at spawn time -- PERSONA_PACK_SPEC.md Section 10 and buzz-acp's own CLI env-var names. Reuses resolve_pack()'s own runtime_env_vars (model/provider split, temperature) rather than re-deriving that precedence logic a second time in Python, per the plan's own reasoning for why a second parser would drift. Two findings beyond the plan's own text, both handled by failing loudly rather than silently: - buzz-acp's build_mcp_servers() hardcodes args: vec![] regardless of what a pack declares -- a persona whose MCP server needs args would have them silently dropped by BUZZ_ACP_MCP_COMMAND. The projector now refuses to project such a persona instead of emitting a command that would run with the wrong arguments. - Neither buzz-persona nor buzz-acp resolve an MCP server's `command` path against the pack directory -- it's used exactly as authored. A relative command (The Professor's own "tools/server.py") only works if buzz-acp's cwd happens to be the pack directory, which nothing guarantees. The projector now resolves a relative command against the pack directory before emitting it. Operator-env-var precedence (a var already set before the script runs is left untouched, not overwritten) is applied in the projector itself, since buzz-acp's own std::env::var(key) check happens after this script's export lines are already in its environment and cannot tell the two apart. Verification: python3 -m unittest discover -s launchpad/agents -p "test_*.py" -- 20 passed Manually ran against launchpad/agents/the-professor's real pack: emitted values match `buzz pack inspect --format json`'s output exactly (GOOSE_ PROVIDER/MODEL/TEMPERATURE, resolved absolute MCP command path); re-ran with GOOSE_MODEL pre-set in the shell and confirmed it was skipped (commented, not exported) rather than overwritten. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
e2eefbe to
dba2e66
Compare
Review pipeline — PR #260Stages run: Not applicable, declared rather than faked:
I ran the suite myself rather than trusting the pasted output: Findings1. High — the 20 tests this PR adds are never executed by any CI job
No workflow runs anything under Grepping every workflow file on this head for Concrete failure: someone deletes the This is worth stating precisely because the project has an explicit rule against exactly this. This is pre-existing and fleet-wide, not introduced here. The same hole covers Recommendation, and I would not block this PR on it: file one issue to wire both directories into CI — either widen the discover roots or add a 2. Medium —
|
|
Filed the CI gap from my review above as #270 ( Scoped to #270 also carries the |
A second independent review-code pass on PR #262 found five more findings (1 High, 3 Medium, 1 Low), all confirmed. Fixes: - High: ruamel.yaml was recorded nowhere and installed by nothing, so the module raised ImportError on any machine without it and no CI job could have caught that -- because NO CI job ran launchpad/agents tests at all. The same High was found independently on PR #260 (its 20 tests also never executed). Adds launchpad/agents/requirements.txt (the dependency, with why ruamel and not PyYAML) and .github/workflows/launchpad-agents-tests.yml, which installs it and runs the suite. The workflow fails if it discovers zero test files, since `unittest discover` exits 0 on an empty suite and a vacuous pass is exactly the gap being closed. - Medium: comment preservation -- the guarantee this module exists for -- was only asserted across read_config -> write_config_atomic, which skips merge_developer_extension entirely. Since the merge copies the mapping, a copy that dropped ruamel's comment attachments would have lost every comment on the real path while the test still passed. Verified the real path is in fact correct (comments do survive), so this was a coverage gap rather than a live bug -- but it was proving the wrong thing. - Medium: every fixture was built by calling this module's own writer, so "before" and "after" had both been through the same serializer -- the one shape that cannot detect a serializer mangling human-authored YAML. Adds OPERATOR_AUTHORED_CONFIG as raw text (top-of-file comment, inline comment, comment nested two levels deep, deliberately quoted scalar, inline comment inside `extensions`) and four controls through the real entry point, including one asserting the developer block is the ONLY line added. - Medium: nothing in the code said that enabling goose's `developer` extension grants shell and filesystem access, or that the plan's OPEN item 2 leaves the live/unattended decision explicitly unsettled. Now stated in enable_developer_extension's own docstring, quoting the plan. - Low: --help dumped the whole 40-line docstring. Now first line only, matching project-pack.py's own `__doc__.splitlines()[0]`. Mutation-checked the new coverage: reverting the dumper to PyYAML makes the suite fail (1 failure, 8 errors) rather than pass. 29 tests, all green. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
|
Cross-checking this PR's review findings from a separate session. No code changed here — this branch is someone else's active work and it already needed one force-push today, so I've left Finding 1 (High) — "the 20 tests are never executed by any CI job": fixed in #262Confirmed the root cause was wider than this PR: no CI workflow ran anything under That workflow also refuses to pass vacuously: it asserts Finding 2 (Medium) — env-dependent tests: reproduced, and it is realMeasured on this branch at its current head: So both tests pass only because Not urgent, and worth saying why: CI has no Findings 3 and 4 (Medium) — left to this branch's owner
Finding 5 (Low) —
|
ciaran-slow
left a comment
There was a problem hiding this comment.
Comment review recording the pipeline result. Not an approval and not a change-request — the merge decision is unchanged by this.
Reviewed via the full pipeline — detail in my comment on this PR. I ran the suite myself in a clean worktree at dba2e66a0: Ran 20 tests ... OK.
No blocking findings in this diff. STEP 2's done-when is met: the projector is generic rather than Professor-specific, consumes STEP 1's JSON instead of re-parsing pack YAML, operator env vars win and are reported as skipped-with-reason, and both places the plan asked it to fail loudly — MCP plurality and MCP args — refuse with a message naming the crate and function that make the refusal necessary. I verified the JSON contract against the Rust types: runtime_env_vars: Vec<(String, String)> serialises as [["K","V"], …], so the unpack at :124 is correct.
The High I raised — that these 20 tests run in no CI job — is a pre-existing, repo-wide gap, not something this PR dug, and it is now tracked as #270. Not holding this PR for it.
Three Medium items for the author, none blocking:
find_buzz_binary(repo_root, env)reads PATH from the real process, not fromenv, so two of its tests pass only becausebuzzis absent — whileCLAUDE.mdtells developers to put it on PATH. One-line fix:shutil.which("buzz", path=env.get("PATH")).:136hardcodesBUZZ_ACP_AGENT_ARGS=acpon a premisebuzz-acpcontradicts — onlygoosetakesacp;hermesis a live runtime that would be spawned ashermes acp. Latent today (The Professor resolves togoose), but the comment is what will stop the next reader checking.- This PR also completes plan STEP 3's done-when without saying so, which leaves STEP 5 looking blocked on a finished step. Worth a line in the body and a note on #239.
Summary
Adds
launchpad/agents/project-pack.py(STEP 2 of the merged Route 3 projector plan, #251) — a generic (not Professor-specific) projector that callsbuzz pack inspect --format json <dir>(STEP 1, #257) and emits a shell-sourceable env file mapping a persona's fully-resolved config onto the env varsbuzz-acpand goose read at spawn time.Stacked on #257 (STEP 1, not yet merged) — this PR's base branch is
feat/issue-239-projector-step1, notlaunchpad, so the diff below is STEP 2's own changes only. Retarget tolaunchpadonce #257 merges.Related issue
Refs #239
Issue type
Task
Agent provenance
Objective
Turn STEP 1's
buzz pack inspect --format jsonoutput into the actual env varsbuzz-acp+ goose read at spawn time (PERSONA_PACK_SPEC.md§10,buzz-acp's own CLI env-var names), so a persona pack becomes a runnable agent process with no other human step.Impacted components
launchpad/agents/project-pack.py
launchpad/agents/test_project_pack.py
Approach and rejected alternatives
Reuses
resolve_pack()'s ownruntime_env_vars(model/provider split, temperature) via STEP 1's JSON output rather than re-deriving that precedence logic a second time in Python — a second implementation would duplicatebuzz-persona's own logic and drift from it the first time either changed, same reasoning the plan (#251) gives for STEP 1 reusingresolve_pack()instead of a second YAML parser.Two findings beyond the plan's own text, both handled by failing loudly rather than silently:
buzz-acp'sbuild_mcp_servers()hardcodesargs: vec![]regardless of what a pack declares — a persona whose MCP server needs args would have them silently dropped byBUZZ_ACP_MCP_COMMAND. The projector refuses to project such a persona instead of emitting a command that would run with the wrong arguments.buzz-personanorbuzz-acpresolve an MCP server'scommandpath against the pack directory — it's used exactly as authored. A relative command (The Professor's owntools/server.py) only works ifbuzz-acp's cwd happens to be the pack directory, which nothing guarantees. The projector resolves a relative command against the pack directory before emitting it.Operator-env-var precedence (a var already set before the script runs is left untouched, not overwritten) is applied in the projector itself, since
buzz-acp's ownstd::env::var(key)check happens after this script's export lines are already in its environment and cannot tell the two apart.Verification
Command run:
Raw output:
Also re-ran the full Rust suite for the crates STEP 1's redaction fix (#257) touched, since this PR is stacked on top of it:
354 + 128 + 5 + 13 passed, 0 failed — confirms this projector doesn't depend on the redacted
mcp_servers[].envvalues (it only readsruntime_env_varsandcommand).Manual check against the real pack this projector targets:
Output:
Re-ran with
GOOSE_MODELpre-set in the shell and confirmed it was skipped (commented, not exported) rather than overwritten — operator precedence holds.Full pre-push gate (
justrust-tests, desktop-tauri-checks, branch-skew) ran clean on push.Not verified
This PR emits the env file only — it does not launch
buzz-acp/goose with it (that's STEP 5 in the plan, "no other human step"). Whether the emittedBUZZ_ACP_MCP_COMMANDpath is executable (has a shebang, ischmod +x) was not checked here; the plan's STEP 7 (live end-to-end proof) is where that gets exercised for real.Security implications
None identified. This is a read-only, local-filesystem-only script that shells out to the already-reviewed
buzz pack inspect --format json(env values redacted there per #257) and writes a plain-text env file to a path the caller chooses — it doesn't read or forwardmcp_servers[].envat all (onlyruntime_env_varsandcommand).Escalations
None.