test(conviction): stop reloaded conviction_flow env overrides leaking across tests

[stranske]

Problem

tests/test_crowded_contrarian.py reloads etl.conviction_flow while CONVICTION_FLOW_CRON / CONVICTION_FLOW_TIMEZONE are monkeypatched, and never restores the module. Because the module resolves its deployment schedule from the environment at import time:

CONVICTION_FLOW_NIGHTLY_CRON = os.getenv("CONVICTION_FLOW_CRON", "0 2 * * *")
CONVICTION_FLOW_TIMEZONE = os.getenv("CONVICTION_FLOW_TIMEZONE", os.getenv("TZ", "UTC"))
conviction_flow_deployment = conviction_flow.to_deployment(..., schedule=Cron(...))

the overridden schedule stays on the shared module object for the rest of the pytest session. Any later test that reads the module-level deployment then sees 15 3 * * * / America/New_York.

Reproduced on current main (872569b):

$ pytest -o addopts= tests/test_crowded_contrarian.py tests/test_conviction_flow.py::test_conviction_deployment_nightly_utc_schedule
FAILED tests/test_conviction_flow.py::test_conviction_deployment_nightly_utc_schedule
E       AssertionError: assert '15 3 * * *' == '0 2 * * *'

CI has been green only by accident of collection order: pytest collects files alphabetically, so test_conviction_flow.py runs before test_crowded_contrarian.py. Any reordering, sharding, -p randomly, or -k/path selection that puts the crowding tests first turns this into a confusing red that looks like a scheduling bug rather than test pollution.

Fix

A reload_conviction_module fixture owns the reload and restores the pristine module on teardown by dropping the env overrides and reloading again. Both deployment tests now go through it. The teardown deliberately does not force TZ, so the restored timezone matches whatever the module saw at original import.

A new regression test, test_conviction_module_schedule_restored_after_env_override_reload, runs right after the override test and asserts the shared module is back to the nightly default — this is the gate that fails if the restore is ever dropped again.

Test gate

• Previously failing order now passes: pytest -o addopts= tests/test_crowded_contrarian.py tests/test_manager_similarity_flow.py tests/test_conviction_flow.py tests/test_alert_engine.py = 38 passed (was 1 failed / 36 passed on main).
• Narrow repro passes: pytest -o addopts= tests/test_crowded_contrarian.py tests/test_conviction_flow.py::test_conviction_deployment_nightly_utc_schedule = 16 passed.
• CI's alphabetical order still passes: pytest -o addopts= tests/test_alert_engine.py tests/test_conviction_flow.py tests/test_crowded_contrarian.py tests/test_manager_similarity_flow.py = 38 passed.
• Deliberate-break demonstration: with the teardown importlib.reload removed, the narrow repro fails on both the new gate and the original victim test —
  FAILED tests/test_crowded_contrarian.py::test_conviction_module_schedule_restored_after_env_override_reload and
  FAILED tests/test_conviction_flow.py::test_conviction_deployment_nightly_utc_schedule (2 failed, 14 passed). The break was reverted and the suite re-run green.
• ruff check clean; black --check clean; git diff --check clean.

Scope

Tests only — one file, +32/-4. No production module, migration, or schema change. Found by the closer lane while validating the rebase of #1498 onto main after #1497 merged; kept out of that PR because it is unrelated to the short-interest feature.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 40 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1a553a8e-d9a0-4cbe-8a7a-7774b96c3c68

📥 Commits

Reviewing files that changed from the base of the PR and between 872569b and 73964b0.

📒 Files selected for processing (1)

• tests/test_crowded_contrarian.py

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Provider Comparison Report

Provider Summary

Provider	Model	Verdict	Confidence	Summary
openai	gpt-5.4	PASS	95%	The merged change correctly addresses the reported test-state leak by introducing a fixture that reloads etl.conviction_flow under patched environment variables and then restores the module state...
anthropic	claude-opus-4-6	PASS	95%	The PR correctly addresses the test isolation problem where importlib.reload of conviction_flow under patched environment variables was leaking state to subsequent tests. The solution introduce...

📋 Full Provider Details (click to expand)

openai

• Model: gpt-5.4
• Verdict: PASS
• Confidence: 95%
• Scores:
  • Correctness: 9.0/10
  • Completeness: 9.0/10
  • Quality: 9.0/10
  • Testing: 9.0/10
  • Risks: 9.0/10
• Summary: The merged change correctly addresses the reported test-state leak by introducing a fixture that reloads etl.conviction_flow under patched environment variables and then restores the module state during teardown. The updated tests now exercise both default and overridden deployment schedule behavior through the fixture, and the added guard test explicitly verifies that overridden cron/timezone values do not persist into later tests. This is a focused, readable, low-risk test-only fix that matches the stated scope and acceptance intent. While the acceptance items reference specific pytest command outcomes and formatting/lint checks that cannot be proven from diff alone, the code changes are consistent with those goals and adequately cover the regression being fixed.

anthropic

• Model: claude-opus-4-6
• Verdict: PASS
• Confidence: 95%
• Scores:
  • Correctness: 9.0/10
  • Completeness: 9.0/10
  • Quality: 9.0/10
  • Testing: 9.0/10
  • Risks: 9.0/10
• Summary: The PR correctly addresses the test isolation problem where importlib.reload of conviction_flow under patched environment variables was leaking state to subsequent tests. The solution introduces a reload_conviction_module pytest fixture that performs the reload and, on teardown, clears the env vars and reloads the module again to restore defaults. Both affected tests now use this fixture. A new guard test (test_conviction_module_schedule_restored_after_env_override_reload) explicitly verifies that the module state is restored after the override tests run. The code is clean, well-documented with a clear docstring, and the approach is idiomatic pytest. The acceptance criteria regarding lint/format cleanliness and test pass counts are code-level concerns that the diff satisfies structurally. The only minor note is the conviction_deployment vs conviction_flow_deployment attribute name difference in the guard test, which appears intentional given the stated passing test results.
• Concerns:
  • Minor inconsistency: the guard test references conviction_module.conviction_deployment while the other tests reference conviction_module.conviction_flow_deployment. This could be intentional (different attribute) or a typo — if it's a typo, the guard test would fail at runtime, but since the acceptance criteria state all 38 tests pass, it's presumably a valid distinct attribute.
  • The fixture teardown relies on monkeypatch from the enclosing scope to delenv before reloading. Since monkeypatch undo happens after fixture teardown in pytest's teardown order, this should work correctly, but it's a subtle ordering dependency worth noting.

Agreement

• Verdict: PASS (all providers)
• Correctness: scores within 1 point (avg 9.0/10, range 9.0-9.0)
• Completeness: scores within 1 point (avg 9.0/10, range 9.0-9.0)
• Quality: scores within 1 point (avg 9.0/10, range 9.0-9.0)
• Testing: scores within 1 point (avg 9.0/10, range 9.0-9.0)
• Risks: scores within 1 point (avg 9.0/10, range 9.0-9.0)

Disagreement

No major disagreements detected.

Unique Insights

• openai: The merged change correctly addresses the reported test-state leak by introducing a fixture that reloads etl.conviction_flow under patched environment variables and then restores the module state during teardown. The updated tests now exercise both default and overridden deployment schedule beh...
• anthropic: Minor inconsistency: the guard test references conviction_module.conviction_deployment while the other tests reference conviction_module.conviction_flow_deployment. This could be intentional (different attribute) or a typo — if it's a typo, the guard test would fail at runtime, but since the acceptance criteria state all 38 tests pass, it's presumably a valid distinct attribute.; The fixture teardown relies on monkeypatch from the enclosing scope to delenv before reloading. Since monkeypatch undo happens after fixture teardown in pytest's teardown order, this should work correctly, but it's a subtle ordering dependency worth noting.

🔍 LangSmith Traces

• openai
• anthropic

[github-actions]

Workflow state fingerprint for Agents Verifier. Do not edit.