fix(tests): #107 — isolate SYNTHBANSHEE_* env vars in CliRunner tests#110
Merged
Conversation
Adds an autouse pytest fixture in tests/conftest.py that strips SYNTHBANSHEE_DATA_DIR / SYNTHBANSHEE_CACHE_DIR / SYNTHBANSHEE_SCRIPT_CACHE_DIR before each test. Without this, sourcing the repo's .envrc (which points these at the developer's avdp-synth-corpus tree) causes CliRunner-backed tests to write generated clips into the real corpus — see #107 for the delivery-003 incident fingerprint. Adds two regression tests in TestSynthbansheeEnvVarIsolation: - Asserts the three env vars are unset during the test process. - Runs the full mocked generate pipeline and asserts no files leak into a "would-be leak" directory outside the CLI-specified output dirs. Fixes #107. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR addresses test isolation for Click envvar= defaults in synthbanshee/cli.py, preventing CliRunner-based unit tests from unintentionally writing generated artifacts to a developer’s corpus directory when SYNTHBANSHEE_*_DIR env vars are set (fixes #107).
Changes:
- Add an autouse pytest fixture that removes
SYNTHBANSHEE_DATA_DIR,SYNTHBANSHEE_CACHE_DIR, andSYNTHBANSHEE_SCRIPT_CACHE_DIRfromos.environduring each test. - Add two “regression” tests intended to assert the fixture’s behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/conftest.py |
Adds an autouse fixture to delete SYNTHBANSHEE_*_DIR env vars during tests to prevent Click envvar default leakage. |
tests/unit/test_cli.py |
Adds a new test class intended to regress #107 by asserting env vars are unset and that generate doesn’t leak artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
Self-review caught that the previous regression tests passed with or without the autouse strip fixture, so they were not real regression tests. This commit: 1. Moves the autouse fixture from ``tests/conftest.py`` to ``tests/unit/conftest.py`` — the integration suite (which uses the SpeakerConfig/SceneConfig Python APIs directly, not CliRunner) is out of scope per #107's "scope guard" note, and the integration tests do not need this fixture. 2. Replaces the two theatre tests with: - ``TestSynthbansheeEnvVarLeakWithoutFixture``: overrides the autouse strip fixture with a no-op for the duration of the class, sets the three env vars, and invokes ``synthbanshee generate`` without explicit dir flags. The test asserts that a WAV DOES land under the env-pointed leak dir — proving the leak vector exists without the fix. If Click's envvar semantics ever change so the leak no longer happens, this test fails loudly so the strip fixture can be retired. - ``TestSynthbansheeEnvVarFixtureStripsParentShellEnv``: uses a class-scoped autouse fixture to set the env vars BEFORE the function-scoped strip runs (pytest setup order: class fixtures before function fixtures for each test). This simulates the realistic scenario of ``.envrc`` setting the vars before pytest started. The test then asserts the strip fixture has cleared them. A second test in the same class verifies the end-to-end behavior: invoking the CLI without dir flags does not leak. 3. Verified manually by commenting out the strip fixture body and confirming both new tests fail, then restoring and confirming they pass — the contract is genuinely exercised. Function scope was kept on the strip fixture (the issue's spec said session scope) because session scope is incompatible with the per-class override pattern that ``TestSynthbansheeEnvVarLeakWithoutFixture`` uses to demonstrate the leak vector. Full unit suite: 1699/1699 passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
|
pr-agent-context report: This is a refreshed snapshot of the current PR state.
This run includes unresolved review comments on PR #110 in repository https://github.com/DataHackIL/SynthBanshee
For each unresolved review comment, recommend one of: resolve as irrelevant, accept and implement
the recommended solution, open a separate issue and resolve as out-of-scope for this PR, accept and
implement a different solution, or resolve as already treated by the code.
After I reply with my decision per item, implement the accepted actions, resolve the corresponding
PR comments, and push all of these changes in a single commit.
# Copilot Comments
## COPILOT-1
Location: tests/unit/test_cli.py
URL: https://github.com/DataHackIL/SynthBanshee/pull/110#discussion_r3224080238
Status: outdated
Root author: copilot-pull-request-reviewer
Comment:
`test_clirunner_generate_does_not_leak_outside_cli_dirs` doesn’t currently validate the #107 leak vector: it never sets any `SYNTHBANSHEE_*_DIR` env var and it passes `--output-dir/--cache-dir/--script-cache-dir` explicitly, so `leak_target` is not a path the CLI would ever choose. As written, this test will pass even if env-var isolation regresses. Consider either (a) removing `leak_target` and instead asserting that every file created by the command is under the explicitly provided dirs, or (b) using a subprocess-based approach (e.g., `pytester` running a fresh pytest process with the env vars set) to prove the autouse fixture defeats parent-process env defaults.
## COPILOT-2
Location: tests/unit/test_cli.py
URL: https://github.com/DataHackIL/SynthBanshee/pull/110#discussion_r3224080282
Status: outdated
Root author: copilot-pull-request-reviewer
Comment:
`test_env_vars_are_unset_in_test_process` will also pass if the autouse fixture is accidentally removed, as long as the CI/dev environment simply doesn’t have these vars set. If the goal is a true regression test for #107, the test needs to arrange for the vars to be present *before* the code under test runs (typically via a subprocess-based test run with a controlled environment) and then assert they’re stripped inside that run.Run metadata: |
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
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.
Problem
Running
pytest tests/unit/with the repo's.envrcsourced silently writesgenerated clip artifacts into
~/clones/avdp-synth-corpus/data/he/instead ofthe test's
tmp_path. Surfaced during the delivery-003 regen — 6 zero-contentWAVs (192 KB each) leaked into the corpus, one overwriting a legitimate clip.
Root cause: Click options for
--output-dir/--cache-dir/--script-cache-dirin
synthbanshee/cli.pydeclareenvvar=SYNTHBANSHEE_*_DIR. When aCliRunner-backed test does not override the flag, Click reads the parent-process env and
writes outside
tmp_path.Solution
Add an
autouse(function-scoped) pytest fixture intests/conftest.pythatstrips
SYNTHBANSHEE_DATA_DIR,SYNTHBANSHEE_CACHE_DIR, andSYNTHBANSHEE_SCRIPT_CACHE_DIRfromos.environfor the duration of everytest. No production behaviour change.
Changes per file
tests/conftest.py_isolate_synthbanshee_env_varsthat callsmonkeypatch.delenv(...)for each of the three varstests/unit/test_cli.pyTestSynthbansheeEnvVarIsolationclass with two regression testsTest plan
.venv/bin/pytest tests/unit/test_cli.py::TestSynthbansheeEnvVarIsolation -v— 2/2 passingSYNTHBANSHEE_DATA_DIR=/tmp/should_not_leak_hereand friends set in the parent env — still 2/2 passing (proves the fixture actually defeats the leak vector).venv/bin/pytest tests/unit/ -q— 1698/1698 passing (no regressions)ruff check tests/conftest.py tests/unit/test_cli.py— cleanmypy tests/conftest.py— cleanTier-3 ASR sanity (local)
Not applicable — this PR is tests-only and touches no audio rendering code
(
synthbanshee/tts/,synthbanshee/script/,synthbanshee/augment/, or thespeaker / scene / acoustic / project YAML configs). Per CLAUDE.md "ASR sanity
check policy", this PR is exempt from the local
qa-report --asrrun.Fixes #107.
🤖 Generated with Claude Code