Skip to content

fix(tests): #107 — isolate SYNTHBANSHEE_* env vars in CliRunner tests#110

Merged
shaypal5 merged 2 commits into
mainfrom
fix/tests-isolate-synthbanshee-env-vars
May 12, 2026
Merged

fix(tests): #107 — isolate SYNTHBANSHEE_* env vars in CliRunner tests#110
shaypal5 merged 2 commits into
mainfrom
fix/tests-isolate-synthbanshee-env-vars

Conversation

@shaypal5

Copy link
Copy Markdown
Member

Problem

Running pytest tests/unit/ with the repo's .envrc sourced silently writes
generated clip artifacts into ~/clones/avdp-synth-corpus/data/he/ instead of
the test's tmp_path. Surfaced during the delivery-003 regen — 6 zero-content
WAVs (192 KB each) leaked into the corpus, one overwriting a legitimate clip.

Root cause: Click options for --output-dir / --cache-dir / --script-cache-dir
in synthbanshee/cli.py declare envvar=SYNTHBANSHEE_*_DIR. When a CliRunner-
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 in tests/conftest.py that
strips SYNTHBANSHEE_DATA_DIR, SYNTHBANSHEE_CACHE_DIR, and
SYNTHBANSHEE_SCRIPT_CACHE_DIR from os.environ for the duration of every
test. No production behaviour change.

Changes per file

File Change
tests/conftest.py New autouse fixture _isolate_synthbanshee_env_vars that calls monkeypatch.delenv(...) for each of the three vars
tests/unit/test_cli.py New TestSynthbansheeEnvVarIsolation class with two regression tests

Test plan

  • .venv/bin/pytest tests/unit/test_cli.py::TestSynthbansheeEnvVarIsolation -v — 2/2 passing
  • Same tests rerun with SYNTHBANSHEE_DATA_DIR=/tmp/should_not_leak_here and 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 — clean
  • mypy tests/conftest.py — clean
  • Pre-commit hooks (ruff, ruff-format, mypy) — clean at commit time

Tier-3 ASR sanity (local)

Not applicable — this PR is tests-only and touches no audio rendering code
(synthbanshee/tts/, synthbanshee/script/, synthbanshee/augment/, or the
speaker / scene / acoustic / project YAML configs). Per CLAUDE.md "ASR sanity
check policy", this PR is exempt from the local qa-report --asr run.

Fixes #107.

🤖 Generated with Claude Code

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>
Copilot AI review requested due to automatic review settings May 12, 2026 06:05
@shaypal5 shaypal5 added the type: test New or improved tests label May 12, 2026
@github-actions

This comment has been minimized.

Copilot AI 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.

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, and SYNTHBANSHEE_SCRIPT_CACHE_DIR from os.environ during 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.

Comment thread tests/unit/test_cli.py Outdated
Comment thread tests/unit/test_cli.py Outdated
@github-actions

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>
@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown

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:

Tool ref: v4
Tool version: 4.0.21
Trigger: schedule
Workflow run: 25736919518 attempt 1
Comment timestamp: 2026-05-12T13:16:20.398958+00:00
PR head commit: 90a0ec4de82badb444d627ec9e05ae755e9f2f9e

@shaypal5 shaypal5 merged commit 2388fbb into main May 12, 2026
6 checks passed
@shaypal5 shaypal5 deleted the fix/tests-isolate-synthbanshee-env-vars branch May 12, 2026 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: test New or improved tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(tests): pytest leaks generated clips into corpus when SYNTHBANSHEE_DATA_DIR is set in env

2 participants