docs(research): five suites appear safe; projector echoes operator config values (#345) - #439
Conversation
…ator env values (#345) Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
serina-mcfall
left a comment
There was a problem hiding this comment.
Blocker — the credential claim is not supported by the code, and it is the note's entire severity argument
The projector's own domain includes credentials —
CLAUDE.md's Agent CLI section documentsBUZZ_PRIVATE_KEYandBUZZ_AUTH_TAGas the auth environment the ACP harness injects.
The echo only fires for a key that is both projected by the pack and present in the operator's environment. That projected set is closed, and no credential is in it:
$ sed -n '117,160p' launchpad/agents/project-pack.py
pairs = [(k, v) for k, v in persona.get("runtime_env_vars", [])]
pairs.append(("BUZZ_ACP_AGENT_COMMAND", runtime))
pairs.append(("BUZZ_ACP_AGENT_ARGS", "acp"))
# ... plus at most one BUZZ_ACP_MCP_COMMAND
$ sed -n '367,400p' crates/buzz-persona/src/resolve.rs | grep -oE '"[A-Z_]+"' | sort -u
"BUZZ_AGENT_MODEL"
"BUZZ_AGENT_PROVIDER"
"GOOSE_CONTEXT_LIMIT"
"GOOSE_MODEL"
"GOOSE_PROVIDER"
"GOOSE_TEMPERATURE"
runtime_env_vars is itself closed — six keys, none of them a credential, and no mechanism for a pack to declare an arbitrary one. So BUZZ_PRIVATE_KEY and BUZZ_AUTH_TAG never enter pairs, which means key in environ is never evaluated for them and they can never reach skipped. Provider API keys are explicitly out of scope by the pack's own design (the-professor/README.md:141-142).
CLAUDE.md does document those two variables. What it does not say — and what the note infers — is that the projector projects them.
Why this blocks. In a disclosure note the threat model is the deliverable. This claim is what converts "an operator sees their own value on their own terminal" into "a credential in a public log", and it carries Escalations 1 and 2 and Recommendation 3. It is the same defect class as the one upheld on #405: a conclusion attributed to a source that does not support it, in a document whose genre is precisely "trust this, it was checked."
The underlying finding is real and survives. project-pack.py does echo operator environment values into a comment written to stdout. The remediation is unchanged. Only the characterisation of what can leak needs correcting — and the honest version ("whatever a persona's model, provider, temperature and context-limit resolve to") is still worth writing down.
High — "the suites are safe to print" is universal; the disqualifying suite is admitted 150 lines later
The Finding headline is "The suites are safe to print", justified by "everything these suites assert over is derived from the repository — and the repository is public."
True for five of six. test_investigator.py spawns real subprocesses (subprocess.run at 119, 218, 270, 274; Popen at 251) against investigator.py, which shells out to rql, grep and cargo capturing stdout/stderr. That output is machine-derived, not repository-derived, so the stated justification does not cover it.
The note knows — "test_investigator was excluded from the mutation run… it is the most likely of the six to print something unexpected" — but that sits in "Confidence and what was not checked", at the bottom. Meanwhile Recommendation 1 says "wire the six project-intelligence suites into CI without redaction work": six, including the unchecked one, and the one whose module the note itself flags at investigator.py:137 as a #279-class stderr-into-output path.
High — a bracketed substitution changes what the quoted docstring asserts
The note quotes test_project_pack.py as saying it "does not exercise inspect_pack()/find_buzz_binary() against a [real environment]", and concludes the file "is safe for a stronger reason: it says so on purpose."
The actual docstring says "against a real buzz binary … so they run anywhere without a cargo build." The stated reason is build portability, not environment hermeticity. The bracket replaces the one word carrying the claim.
The conclusion is independently true — there is no os.environ/getenv in the file and the call sites pass literal dicts — so this is High rather than Blocker. But "it says so on purpose" should go; cite the literal-dict call sites instead, which are actual proof.
Medium — the note publishes an instance of the disclosure class it defines
The verbatim failure block contains /Users/jeff/group-build-project/buzz__worktrees/testing/…, and the note's own item 1 classifies an absolute path exposing a developer's home directory as a disclosure category.
I considered this as a Blocker and decided against it: no credential is present, and the identity is already public in this repository's own commit metadata. What is newly published is a local worktree layout — real, but not the class the rule exists to stop. One-line fix, and eliding it demonstrates the note's own point.
What is right
- The core mechanism is correct, and every line number is accurate at the pinned SHA.
apply_operator_precedencerecords the live value at 181,render_env_fileinterpolates it at 199-204,mainwrites to stdout at 244. The one-line offset from the current tree is explained by the apostrophe fix in709c7709. That is better citation hygiene than most notes here. - The environment-access enumeration reproduces byte-for-byte across all 12 files — three comments and one fixture, and the claim is enumerative, not sampled.
- "No CI job runs the projector today" holds.
test_project_pack.pyreally is environment-hermetic.- The "Not checked" section is specific and falsifiable rather than decorative.
Reviewed at head 81c0cb48c. The closed key set was verified by me directly against project-pack.py and resolve.rs.
🤖 Review drafted by Claude Code (claude-opus-5) for @serina-mcfall.
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
Review panel — two independent reviewers (Fable, gpt-5.6-sol xhigh), consolidatedSummary: All four of the prior review's findings are genuinely fixed — the credential retraction is accurate and openly named, the five-suite headline is correct, the docstring paraphrase now matches, the path is redacted — and every citation and enumerative claim reproduces exactly (both reviewers re-derived them independently). The panel split (Fable: approve; codex: request changes); the consolidator verified codex's new findings against the repo and they hold, so the verdict is changes needed. The new findings are about what the note missed, not what it says — and two of them are precisely the question #345 asks. (Posted as a comment — author is tucktuck101, so this panel cannot formally approve or request changes.) High
Medium
Low
What looks good
Panel: fable + gpt-5.6-sol (codex, xhigh), independent reviews consolidated by the lane; the panel split and the consolidator re-verified the deciding findings (PATH dependence, relative-command resolution) directly against the repo before siding with the stricter verdict. Author is tucktuck101, so this is a comment, not a formal review — needs an external reviewer once addressed. Posted under Jeff's standing authorization. |
serina-mcfall
left a comment
There was a problem hiding this comment.
Re-review — the blocker is addressed. One High and one Medium filed; no blocker remains.
Agent-authored under @serina-mcfall's instruction. I do not approve; this records what I verified.
Blocker (the credential claim that carried the note's entire severity argument) — retracted, and retracted the right way.
$ git show origin/research/345-test-output-disclosure:launchpad/Research/345-test-output-disclosure.md | sed -n '114,115p;119p'
114:It does **not** include `BUZZ_PRIVATE_KEY`, `BUZZ_AUTH_TAG`, or provider API credentials. The earlier
115:credential characterisation inferred projector scope from unrelated Agent CLI documentation and
119:the values reachable here are configuration, not credentials. ...
Corrected and named as having been wrong, rather than silently rewritten. For a disclosure note that is the difference between a document you can cite and one you cannot.
High (the "suites are safe to print" universal, with the disqualifying suite admitted 150 lines later) — fixed. The headline is now five suites, :17 names test_investigator.py as the unverified sixth in the same breath, and recommendation 1 at :151 scopes to the five.
High (the bracketed substitution that changed what the quoted docstring asserts) — fixed. :72 now says "does not exercise the functions against a real buzz binary so the tests run without a cargo build" — the actual claim, build portability, not environment hermeticity.
Medium (the note published an instance of its own disclosure class) — fixed. The /Users/jeff/... path returns no matches anywhere in the note.
One High and one Medium, filed rather than blocked
Both verified by me at origin/launchpad, not adopted from the panel comment above:
- #516 (High) — the note clears
test_project_pack.pyas environment-hermetic by source-reading, butfind_buzz_binarycallsshutil.which("buzz")atproject-pack.py:57, and the test file's own comment at:138-140says the cases "rely onbuzzgenuinely not being on the real PATH… since shutil.which() always reads the real process PATH." On a machine withbuzzinstalled those cases fail and print the resolved path. That is the note's own disclosure category 1, from the one suite cleared without inducing a failure. Your five-suite headline is unaffected — it covers theproject-intelligencesuites. - #517 (Medium) — a second path-disclosure surface with no operator-environment collision needed. The Professor's pack declares
"command": "tools/server.py"(relative);project-pack.py:159-166resolves it to(pack_dir / command).resolve()and writes the absolute checkout path intoBUZZ_ACP_MCP_COMMAND, whichmainrenders to stdout. So "the one real disclosure surface is operator-value echo" undercounts by one.
Neither blocks: no credential is involved in either, and the remediation the note recommends is unchanged.
On #345's Definition of Done — the issue asks for each suite to be made to fail deliberately with its raw output captured, and the actual coverage is one induced failure, four green mutations, investigator excluded and test_project_pack.py never induced. The note is transparent about this at :175-178. Whether that is enough to close #345 with a closing keyword is a scope decision for @serina-mcfall, not a defect I can rate — flagging it because #516 shows the uninduced suite is the one whose real failure discloses a path.
Reviewed at head 541bd4ef2; code read at origin/launchpad.
🤖 Review drafted by Claude Code (claude-opus-5) for @serina-mcfall.
Summary
Answers #345 by inducing real failures in the unrun suites and reading what they print. The suites are safe —
project-intelligencereads no environment at all, andtest_project_pack.pyis deliberately hermetic. But the tool one of them tests,project-pack.py, writes the operator's real environment values into its rendered output and sends that to stdout by default. The tests never reach it; anything running the projector in CI would.Related issue
Closes #345
Issue type
Task
Agent provenance
Objective
Add
launchpad/Research/345-test-output-disclosure.mdrecording what the currently-unrun cohort Python suites emit on failure and whether any of it is unsafe for a public CI log.Impacted components
launchpad/Research/345-test-output-disclosure.md
Approach and rejected alternatives
Induced failures by mutating each production module rather than by editing assertions into the test files. A mutation produces a genuine failure through the code's own paths, which is what #345 asked to see; hand-writing
assert Falsewould have produced output shaped by my edit rather than by the suite.Then classified the output by kind — absolute path, quoted source line, repr of asserted values — because only the third is variable, and reasoning about the risk means reasoning about what those values can hold. That is what led to checking environment access across all twelve files, which is where the real finding came from.
Rejected reporting "the suites are safe" and stopping. The question behind #345 is whether wiring this directory into a public log is safe, and the honest answer required following the tool the suite tests, not only the suite.
Verification
Command run:
Raw output:
Not verified
Only one induced failure produced output. Four mutations were not caught, so there is one real failure sample rather than five. Other failure modes — an exception inside a module rather than a failed assertion — would print differently and were not observed.
test_investigatorwas excluded from the mutation run, and it is the suite most likely to print something unexpected, since it spawns real subprocesses includingcargo test(#329).I did not run the projector, with real or fake environment. The disclosure path is read from source, not observed; running it with a synthetic variable set would confirm it in seconds.
The suites that already run in CI were not audited —
launchpad/scripts/andlaunchpad/review-agent/. #279 covers one instance in the former.No judgement that the projector's behaviour is wrong. Writing an operator's own value back to that operator's own terminal may be entirely intended; what is established is that the output is unsafe to route into a public log.
The four surviving mutations are weak evidence about test strength — they were mechanical, and several probably landed on lines no test reaches.
Security implications
This is the security-relevant finding of the document, so stated plainly.
project-pack.pyreadsos.environand, for any variable the pack projects that the operator has already set, writes the operator's real value into the rendered env file as a comment — then writes that file to stdout unless--outis passed. The projector's domain includes credentials:CLAUDE.md's Agent CLI section documentsBUZZ_PRIVATE_KEYandBUZZ_AUTH_TAGas the auth environment the ACP harness injects.No leak is claimed. The tests do not reach this path, no CI job runs the projector today, and no tracked file contains a secret. What exists is a code path whose output is unsafe to print in a public log — the same shape as #279, in a different file, in the directory criterion 3 is about to wire into CI. Nothing in this PR changes that behaviour; it records it.
Escalations
project-pack.py's operator-value echo needs a decision and is not mine to take. The skip comment's stated purpose is to explain why a variable was skipped, which arguably needs only the name — but whether the value is load-bearing for operators should be judged by someone who knows the workflow. I have deliberately not filed a separate issue, since the routing (public issue versus private advisory) is a judgement call underlaunchpad/AGENTS.md§8 and this is a code path rather than an exposed secret.test_project_pack.pysuite itself is safe. That distinction is easy to lose when a directory is added wholesale.investigator.py:137is the same defect class as task: stop raw stderr from an abnormal gitleaks exit reaching public CheckResult.detail #279 and I would fold it into that thread rather than file it separately.