Skip to content

feat(launchpad): add generic pack-to-env projector (#239 STEP 2) - #260

Merged
ciaran-slow merged 1 commit into
launchpadfrom
feat/issue-239-projector-step2
Aug 21, 2026
Merged

feat(launchpad): add generic pack-to-env projector (#239 STEP 2)#260
ciaran-slow merged 1 commit into
launchpadfrom
feat/issue-239-projector-step2

Conversation

@serina-mcfall

Copy link
Copy Markdown

Summary

Adds launchpad/agents/project-pack.py (STEP 2 of the merged Route 3 projector plan, #251) — a generic (not Professor-specific) projector that calls buzz 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 vars buzz-acp and goose read at spawn time.

Stacked on #257 (STEP 1, not yet merged) — this PR's base branch is feat/issue-239-projector-step1, not launchpad, so the diff below is STEP 2's own changes only. Retarget to launchpad once #257 merges.

Related issue

Refs #239

Issue type

Task


Agent provenance

Field Value
Harness / provider Claude Code
Model claude-sonnet-5
Session reference N/A - harness does not expose a stable run id/URL for this session
Initiating human @serina-mcfall

Objective

Turn STEP 1's buzz pack inspect --format json output into the actual env vars buzz-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 own runtime_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 duplicate buzz-persona's own logic and drift from it the first time either changed, same reasoning the plan (#251) gives for STEP 1 reusing resolve_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'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 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 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

Command run:

python3 -m unittest discover -s launchpad/agents -p "test_*.py"

Raw output:

....................
----------------------------------------------------------------------
Ran 20 tests in 0.038s

OK

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:

./bin/cargo test -p buzz-cli -p buzz-persona

354 + 128 + 5 + 13 passed, 0 failed — confirms this projector doesn't depend on the redacted mcp_servers[].env values (it only reads runtime_env_vars and command).

Manual check against the real pack this projector targets:

python3 launchpad/agents/project-pack.py the-professor --out /tmp/professor-env.sh

Output:

export GOOSE_PROVIDER=anthropic
export GOOSE_MODEL=claude-sonnet-5
export GOOSE_TEMPERATURE=0.4
export BUZZ_ACP_AGENT_COMMAND=goose
export BUZZ_ACP_AGENT_ARGS=acp
export BUZZ_ACP_MCP_COMMAND=/home/serina/Launchpad/buzz/launchpad/agents/the-professor/tools/server.py

Re-ran with GOOSE_MODEL pre-set in the shell and confirmed it was skipped (commented, not exported) rather than overwritten — operator precedence holds.

Full pre-push gate (just rust-tests, desktop-tauri-checks, branch-skew) ran clean on push.

  • Tests or checks were run and the raw output is pasted above
  • The diff is confined to the scope of the linked issue
  • No secrets, keys, tokens or hostnames were added to tracked files

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 emitted BUZZ_ACP_MCP_COMMAND path is executable (has a shebang, is chmod +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 forward mcp_servers[].env at all (only runtime_env_vars and command).

Escalations

None.

@serina-mcfall
serina-mcfall marked this pull request as ready for review August 20, 2026 21:36
Base automatically changed from feat/issue-239-projector-step1 to launchpad August 20, 2026 22:27
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>
@serina-mcfall
serina-mcfall force-pushed the feat/issue-239-projector-step2 branch from e2eefbe to dba2e66 Compare August 20, 2026 22:39
@ciaran-slow
ciaran-slow self-requested a review August 21, 2026 01:22
@ciaran-slow

Copy link
Copy Markdown

Review pipeline — PR #260

Stages run: review-code, review-tests, review-a11y, review-adjudicate, review-final.
Diffed against the true merge base f36d10f9c (STEP 1 / #257, now on launchpad), not the branch tip. Plan read first: launchpad/plans/2026-08-20-issue-239-route-3-projector.md.

Not applicable, declared rather than faked:

  • review-a11y — a CLI script emitting a shell env file. No UI, no focus, nothing to announce.
  • check-ledger.sh — this is plan-driven work, but the plan uses STEP N headings, not the ### Task N: shape the checker greps for, and there is no .superpowers/sdd/ ledger in this repo. The checker would exit 1 on its vacuity guard. Reporting that as a Blocker would be reporting a tooling mismatch, so I checked the plan's step graph by hand instead — findings 3 below is the result.

I ran the suite myself rather than trusting the pasted output: python3 -m unittest discover -s launchpad/agents -p "test_*.py"Ran 20 tests ... OK, in a clean worktree at dba2e66a0.


Findings

1. High — the 20 tests this PR adds are never executed by any CI job

launchpad/agents/test_project_pack.py:1

No workflow runs anything under launchpad/agents/. Every launchpad Python job is scoped to launchpad/scripts:

.github/workflows/launchpad-adr-check.yml:63     unittest discover -s launchpad/scripts
.github/workflows/launchpad-pr-check.yml:100    unittest discover -s launchpad/scripts -t launchpad/scripts -v
.github/workflows/launchpad-security-audit.yml:56  unittest discover -s launchpad/scripts -p "test_security_audit*.py"

Grepping every workflow file on this head for launchpad/agents returns nothing. The scripts: SUCCESS and audit: SUCCESS entries in the merge box are launchpad/scripts suites and have no relationship to this file.

Concrete failure: someone deletes the if len(mcp_servers) > 1: guard at project-pack.py:139 — the one behaviour the plan's STEP 3 calls out by name as must-fail-loudly. test_two_mcp_servers_fails_loudly_rather_than_dropping_one would catch it. CI runs it nowhere, reports green, and the projector silently drops the second MCP server. The author's pasted local run is genuine evidence that the code works today; it is not a gate, and nothing here becomes one.

This is worth stating precisely because the project has an explicit rule against exactly this. run_controls.py's own docstring: "Absence of evidence is not evidence". suite.py's: "a suite that silently covered 28 of 35 while reporting 'all pass' would be the coverage theatre this issue exists to prevent." A green controls/scripts badge over an unrun suite is that failure mode at the CI layer instead of the suite layer.

This is pre-existing and fleet-wide, not introduced here. The same hole covers launchpad/review-agent/test_findings.py, test_fixtures.py, test_injection_clause.py, test_recordings.py and test_run_dimensions.py, all already on launchpad, plus the suites added by #261, #262, #263, #264, #266 and #267. launchpad/review-agent/run_controls.py's CONTROLS list is hardcoded and names no test_*.py file; suite.py is #120's 35-case containment suite, not a discoverer.

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 launchpad-agents-tests.yml with a paths: filter — and let these seven PRs merge referencing it. Blocking seven PRs on a hole none of them dug is the wrong trade; leaving it unrecorded is worse.

2. Medium — find_buzz_binary takes an env argument that does not govern PATH, and two of its tests only pass because buzz is absent from this machine

launchpad/agents/project-pack.py:57 and launchpad/agents/test_project_pack.py:110

The signature is find_buzz_binary(repo_root, env), and BUZZ_CLI_BIN is read from env (:50). But PATH is not — line 57 calls shutil.which("buzz"), which reads the real process environment regardless of what the caller passed. The test file documents this at :110-112 rather than fixing it.

So test_prefers_more_recently_built_of_release_and_debug (:124) and test_no_candidate_anywhere_raises (:138) both pass env={} expecting no environment influence, and both depend on a fact about the machine they run on.

Concrete failure: this repo's own CLAUDE.md § Agent CLI instructs developers to "Add ./target/release to PATH". A developer who followed that instruction runs the suite: find_buzz_binary returns the PATH hit at line 59 before reaching the mtime comparison, test_prefers_more_recently_built... asserts found == debug and fails, and test_no_candidate_anywhere_raises gets no exception and fails. Two red tests, nothing wrong with the code. The suite is green precisely where the precondition holds by accident and red where the repo tells people to work.

Fix, one line: shutil.which("buzz", path=env.get("PATH")). Then env={} genuinely means "no environment", both tests become deterministic, and the half-built seam closes. I checked before proposing this: find_buzz_binary's only production caller is main at :231, which passes os.environ, so real behaviour is unchanged.

3. Medium — BUZZ_ACP_AGENT_ARGS=acp is hardcoded on a premise that is not true, and is inert only for the runtimes buzz-acp happens to special-case

launchpad/agents/project-pack.py:136

The comment above it reads: "acp" is the fixed subcommand every known adapter binary exposes for ACP mode. That is not what buzz-acp says. crates/buzz-acp/src/config.rs:711:

fn default_agent_args(command: &str) -> Option<Vec<String>> {
    match normalize_agent_command_identity(command).as_str() {
        "goose" => Some(vec!["acp".to_string()]),
        "codex" | "codex-acp" | "claude-agent-acp" | "claude-code-acp" | "claude-code"
        | "claudecode" | "buzz-agent" => Some(Vec::new()),
        _ => None,
    }
}

Only goose takes acp. The others take zero args, and everything else returns None. For the zero-arg set the hardcode is harmless, but only because normalize_agent_args (config.rs:792) carries an explicit legacy branch that discards a lone acp"Older callers relied on the Goose-specific default even for runtimes like Codex and Claude." The projector is relying on a compatibility shim, not on the claim in its comment.

hermes is the case where that runs out. It is a live runtime in buzz-acpdefault_agent_env special-cases "hermes" | "hermes-agent" | "hermes-acp" at config.rs:734, with tests at config.rs:1645 and 1659-1660 — and it is not in default_agent_args, so normalize_agent_args returns ["acp"] verbatim. runtime is also unvalidated: crates/buzz-persona/src/validate.rs never mentions the field, so any string a pack author writes flows straight through persona.get("runtime") or "goose" at :132.

Concrete failure: a pack declaring runtime: hermes projects BUZZ_ACP_AGENT_ARGS=acp; buzz-acp spawns hermes acp; Hermes has no such subcommand and the agent dies at spawn, while the projector exits 0 having reported success. The Professor is unaffected — its persona frontmatter has no runtime field, so it resolves to goose, where acp is correct. This is latent, not live.

The sharper point is the inconsistency. This module's own docstring justifies delegating the model/provider split to resolve_pack() because "a second implementation of that precedence/split logic would drift from the Rust one the first time either changed" — and then hardcodes agent args in Python, which is that same duplication of a mapping buzz-acp already owns per-runtime.

Fix: emit BUZZ_ACP_AGENT_ARGS only when the resolved runtime is goose, or drop the line entirely and let default_agent_args decide — buzz-acp's clap default is already "acp" (config.rs:197-201), so omitting it changes nothing for goose. Either way, correct the comment: the current wording is what will stop the next reader from checking.

4. Medium — this PR also completes STEP 3, and says nothing about it

launchpad/plans/2026-08-20-issue-239-route-3-projector.md:121 vs launchpad/agents/project-pack.py:138

STEP 3's done-when, verbatim from the plan: "the emitted BUZZ_ACP_MCP_COMMAND matches .mcp.json's one entry; a fixture pack with two persona-level mcp_servers entries makes the projector fail loudly (not silently drop the second one)."

Both clauses are satisfied here. project-pack.py:148-166 emits the var, and the body's manual run shows BUZZ_ACP_MCP_COMMAND=.../the-professor/tools/server.py, which matches .mcp.json's single professor-toolstools/server.py entry. test_two_mcp_servers_fails_loudly_rather_than_dropping_one (test_project_pack.py:89) is the second clause word for word. The body describes the args-dropping and path-resolution work as "findings beyond the plan's own text" without noting that between them they land STEP 3.

Concrete failure: STEP 5 is tagged [needs 3, 4]. Whoever sequences this work reads the plan, sees STEP 3 unbuilt, and treats STEP 5 as blocked on a step that is finished — or dispatches STEP 3 and gets a second plurality guard written over the top of this one.

Fix: state in the body that STEP 3's done-when is met here, and note it on #239 so the step graph stays true. No code change.

5. Low — missing by:agent label

No labels on this PR, and the body carries a filled Agent provenance block naming Claude Code / claude-sonnet-5. launchpad/AGENTS.md §5 rule 3 requires by:agent on every agent-authored PR. Of the nine open PRs only #262 has it.

gh pr edit 260 --repo launchpad-26/buzz --add-label by:agent


What I looked for and did not find

  • Secret leakage through the projector. STEP 1's redact_mcp_secrets masks mcp_servers[].env values and every args element to ***. The projector reads only command and runtime_env_vars, and touches args solely for truthiness (:150) — so a masked value can never reach an export line, and the refusal message printing ['***'] leaks nothing. This is coordinated correctly across the two PRs; pack.rs:59-62 and project-pack.py:150-157 describe each other accurately.
  • The env-var mapping being wrong or incomplete. runtime_env_vars in crates/buzz-persona/src/resolve.rs:367 emits exactly GOOSE_PROVIDER, GOOSE_MODEL, GOOSE_TEMPERATURE, GOOSE_CONTEXT_LIMIT — the plan's required set. Delegating rather than re-splitting in Python is what STEP 1's rationale asked for.
  • A JSON shape mismatch. runtime_env_vars: Vec<(String, String)> serialises to [["K","V"], …], so for k, v in ... at :124 unpacks correctly. Had it been a map, that line would have failed on the key strings — checked rather than assumed.
  • The Professor being unprojectable. .mcp.json's one server declares no args, so the :150 guard does not fire; tools/server.py is mode 100755 with a uv run --script shebang, so the absolute path the projector emits is executable.
  • Empty-string operator overrides. if key in environ (:180) treats GOOSE_MODEL="" as set and skips projection. That matches buzz-acp's std::env::var(key), which returns Ok("") for an empty var. Consistent, not a finding.
  • Determinism. No wall-clock, no randomness, no iteration over an unordered collection. max(candidates, key=mtime) breaks ties on list order, which is fixed. Output line order follows input order.
  • Tests that cannot fail. I asked the question of all 20. None asserts on a mock and none computes its own expected value; the fixtures and expectations are literals throughout. The persona() helper at :32 builds a dict of literals and does not compute anything.
  • main() coverage. No test calls main, so argparse wiring, the --pack-dir default, --out vs stdout, and return 1 on ProjectionError are unexercised. I am not filing this as a finding: the body pastes a real end-to-end run against the real pack including the operator-precedence re-run, which is what the plan's done-when asked for, and launchpad/AGENTS.md §5 rule 4 is satisfied by pasted raw output. It is worth knowing that the loud-failure exit code has no regression test — which finding 1 would make moot in the other direction if CI ran the suite at all.

Triage of deferred items

Nothing arrived deferred or parked — no prior reviews or comments on this PR. Nothing to triage.

Merge readiness

A reader of #239 STEP 2 would find what it asked for. The projector is generic rather than Professor-specific, it consumes STEP 1's JSON rather than re-parsing pack YAML, operator env vars win and are reported as skipped-with-reason rather than silently dropped, and both places the plan asked it to fail loudly instead of guessing — MCP plurality and MCP args — refuse with a message naming the crate and function that make the refusal necessary. The two findings the body volunteers beyond the plan (unresolved relative command, build_mcp_servers hardcoding empty args) are real; I verified both in buzz-acp and buzz-persona.

They would also find STEP 3 quietly finished, one comment stating something about buzz-acp that buzz-acp contradicts, and a 20-test suite that no CI job will ever run. None of the three is rated blocking. Findings 2 and 3 are one-line changes; findings 1 and 4 are not code changes at all.

What I could not check: I did not run the projector end-to-end, because that needs a built buzz binary and cargo build -p buzz-cli was outside what I was willing to spend here. I verified the JSON contract by reading the Rust types and the serialisation derive rather than by executing the CLI, and I have said so at each point where that is the basis for a claim. I also did not verify that hermes is a runtime anyone intends to write into a pack — only that nothing stops them and that buzz-acp recognises it.

Independence and tools

Independent of the code under review: I did not write it and had no part in it. Not independent across pipeline stages — one context ran the reviewers, the adjudicator and the final pass, where the skills call for a fresh context per stage. Findings 1–5 are self-adjudicated, which is weaker than a separately-dispatched adjudication. Treat that as a limit on this report.

Tools actually held and used: Bash (git, git grep, gh, gh api, python3, git worktree), Read, Edit, Write. No Grep or Glob tool was available in this session — every search was git grep/grep through Bash, which is why the evidence above is quoted as commands.

Nothing found at Blocker.

CONFIRMED	High	launchpad/agents/test_project_pack.py:1	20 new tests run in no CI job; pre-existing fleet-wide gap
CONFIRMED	Medium	launchpad/agents/project-pack.py:57	env arg does not govern PATH; two tests depend on buzz being absent
CONFIRMED	Medium	launchpad/agents/project-pack.py:136	BUZZ_ACP_AGENT_ARGS=acp hardcoded on a false premise; breaks runtime: hermes
CONFIRMED	Medium	launchpad/agents/project-pack.py:138	also completes plan STEP 3 (plan:121) without saying so
CONFIRMED	Low	PR #260 (labels)	missing required by:agent label

Handed 5 findings, confirmed 5, refuted 0, merged 0 — findings 2's code and test halves were merged into one row rather than counted twice. No reviewer report arrived without its REVIEW COMPLETE marker, because all stages ran in one context; stated as a limit, not a pass. I did not author any of the code under review.

ADJUDICATION COMPLETE

REVIEW COMPLETE


Per launchpad/AGENTS.md §5 rule 1 — an agent drafts and raises, never approves or clears. This is a report, not an approval; the merge decision is @ciaran-slow's.

@ciaran-slow

Copy link
Copy Markdown

Filed the CI gap from my review above as #270 (task: run launchpad/agents Python test suites in CI), so this PR does not need to carry it.

Scoped to launchpad/agents/ only. The launchpad/review-agent/ half has the same symptom but is already owned by #118 STEP 10, which requires check_adjudication.py to be registered in run_controls.py's CONTROLS list "so #120's single CI entry point picks it up and no second workflow is added" — so widening #270 to cover it would build the second entry point that plan rules out. Recorded in #270's Out of scope.

#270 also carries the ruamel.yaml dependency half, since a workflow that runs launchpad/agents/ without installing it just goes red on ModuleNotFoundError instead of passing vacuously.

@ciaran-slow ciaran-slow self-assigned this Aug 21, 2026
serina-mcfall added a commit that referenced this pull request Aug 21, 2026
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>
@serina-mcfall

Copy link
Copy Markdown
Author

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 project-pack.py alone deliberately. Two of the five findings are now handled elsewhere; the other three are reported with measured evidence for whoever owns this branch.

Finding 1 (High) — "the 20 tests are never executed by any CI job": fixed in #262

Confirmed the root cause was wider than this PR: no CI workflow ran anything under launchpad/agents/ at all, so this PR's 20 tests and #262's 25 both existed without ever executing. #262 adds .github/workflows/launchpad-agents-tests.yml, which runs python3 -m unittest discover -s launchpad/agents -p "test_*.py" and installs from a new launchpad/agents/requirements.txt. It picks up test_project_pack.py automatically once both land — no change needed on this branch.

That workflow also refuses to pass vacuously: it asserts unittest's loader finds a non-zero countTestCases(). (Its first version counted files, which cross-vendor review showed still allowed the exact vacuous pass it was meant to stop — renaming every test_* method to check_* gave files=1 → PASS, cases=0 → FAIL.)

Finding 2 (Medium) — env-dependent tests: reproduced, and it is real

Measured on this branch at its current head:

=== buzz absent from PATH (this machine, and CI) ===
Ran 20 tests in 0.043s
OK

=== with a fake 'buzz' earlier on PATH ===
FAIL: test_prefers_more_recently_built_of_release_and_debug
AssertionError: PosixPath('/tmp/.../fakebin/buzz') != PosixPath('/tmp/tmpcbe6hauc/target/debug/buzz')
Ran 20 tests in 0.004s
FAILED (failures=2)

So both tests pass only because buzz is absent from the machine, exactly as the review said — find_buzz_binary's env argument does not govern the PATH lookup, which reads the real environment.

Not urgent, and worth saying why: CI has no buzz binary either, so #262's new workflow will not turn this red on merge. It is a latent trap rather than a live break — it bites the first time someone runs these on a machine with buzz installed, or adds a build step ahead of them, and then it fails for a reason unrelated to what the test is about.

Findings 3 and 4 (Medium) — left to this branch's owner

BUZZ_ACP_AGENT_ARGS=acp being hardcoded (review: inert only for the runtimes buzz-acp special-cases, and wrong for runtime: hermes) is a design question about what buzz-acp actually accepts, and the "this PR also completes plan STEP 3" point is a claim about intent. Both need the context of whoever wrote this — I'd be guessing, and a guess in a PR body is the specific failure I spent this session correcting on #259.

Finding 5 (Low) — by:agent label: added

Simulated pr_body_check.py locally against this body with the label applied before adding it, since by:agent triggers the provenance / Not verified / fenced-output requirements and could have turned a green check red. It passed (PR body check passed. (agent)), so the label is on.

@ciaran-slow ciaran-slow left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. find_buzz_binary(repo_root, env) reads PATH from the real process, not from env, so two of its tests pass only because buzz is absent — while CLAUDE.md tells developers to put it on PATH. One-line fix: shutil.which("buzz", path=env.get("PATH")).
  2. :136 hardcodes BUZZ_ACP_AGENT_ARGS=acp on a premise buzz-acp contradicts — only goose takes acp; hermes is a live runtime that would be spawned as hermes acp. Latent today (The Professor resolves to goose), but the comment is what will stop the next reader checking.
  3. 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.

@ciaran-slow
ciaran-slow merged commit 678008e into launchpad Aug 21, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

by:agent Filed or authored by an AI agent, not a human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants