test: add regression test for mock provider (#40)

[himanshu231204]

Summary

Adds an explicit regression test verifying that --llm-provider mock never instantiates an OpenAIProvider.

The underlying bug (commit 8fed362) was already fixed by switching _create_provider() to use the factory pattern (get_llm_provider(config)), but lacked a test that directly asserts the OpenAI client is never constructed — only checking the return type.

Changes

• ** ests/unit/test_cli/test_synth.py** — new est_mock_provider_never_instantiates_openai test that patches AsyncOpenAI and asserts it is never called when creating a mock provider
• CHANGELOG.md — documents the fix under [Unreleased] > Fixed

Test Results

• 14/14 synth CLI tests pass (including new regression test)
• Lint: all checks pass
• 1 pre-existing unrelated failure ( est_audit_error_message_includes_path — Windows path-wrapping issue)

Closes #40

[github-actions]

Here is my review.

---

Code Review: fix/mock-provider-fallback

---

Summary

Adds a regression test (test_mock_provider_never_instantiates_openai) asserting that _create_provider("mock") never calls AsyncOpenAI, plus a changelog entry documenting the underlying fix. The fix itself (switching _create_provider to the factory pattern) was committed in a previous commit.

---

Issues Found

1. [STYLE] Unused monkeypatch parameter — tests/unit/test_cli/test_synth.py:44 — LOW

monkeypatch: pytest.MonkeyPatch is declared in the test signature but never referenced. The test uses unittest.mock.patch instead. This is dead code that will trigger lint warnings and confuses readers about which patching mechanism is in use.

Fix: Remove the parameter:

def test_mock_provider_never_instantiates_openai() -> None:

---

2. [STYLE] Changelog entry describes a fix, not this PR's addition — CHANGELOG.md:13 — LOW

The entry documents a user-facing fix ("now correctly uses MockLLMProvider"), but this PR only adds a regression test — the fix was already present. Changelogs should reflect what changed in this PR. Either omit the entry (tests are rarely user-facing) or rephrase as:

- **Mock Provider Regression Test** — add regression test verifying that
  `--llm-provider mock` never falls back to `OpenAIProvider` (#40)

---

Positive Notes

• Excellent test name and docstring — immediately communicates intent and the historical bug context
• Tests both the positive (isinstance(provider, MockLLMProvider)) and negative (AsyncOpenAI never called) invariants — double-entry verification is a good pattern for regression tests
• Fully isolated — no shared fixtures, no side effects on other tests
• autospec=True on the patch provides realistic mock behavior (attribute access raises AttributeError for missing attrs)
• Follows the existing coding conventions in the file (same import style, same assertion patterns)

---

Verdict

COMMENT — Minor suggestions, no blocking issues. The test is well-written and adds meaningful regression coverage.

opencode session  |  github run

[github-actions]

Code Review: fix/mock-provider-fallback

---

Summary

Adds a regression test (test_mock_provider_never_instantiates_openai) that patches AsyncOpenAI and asserts it is never called when _create_provider("mock") runs, plus a changelog entry.

---

Issues Found

1. [STYLE] Changelog entry in wrong section — CHANGELOG.md:13 — LOW

The entry is placed under ### Fixed, but this PR adds only a test — the actual fix was already in a prior commit. This should be under ### Testing (which already exists at line 66 of the rendered file).

Change to:

### Testing

- **Mock Provider Regression Test** — add regression test verifying that ...

2. [STYLE] Changelog entry mislabels the content — CHANGELOG.md:13 — LOW

The description reads "Fixed — Mock Provider Regression Test". Tests are not fixes. Either move to ### Testing (per above) or re-label to describe the underlying bug fix if this PR included it.

---

Positive Notes

• Excellent docstring — immediately communicates the historical bug context and the invariants being tested
• Double-entry verification: asserts both the positive (isinstance(MockLLMProvider)) and negative (AsyncOpenAI.assert_not_called()) — strong regression coverage
• autospec=True ensures the mock behaves realistically (AttributeError on missing attrs)
• Fully isolated — no shared fixtures, no side effects
• Follows existing file conventions (import style, assertion patterns)

---

Verdict

COMMENT — Minor suggestions, no blocking issues. The test itself is well-written and adds meaningful regression coverage.

opencode session  |  github run

[github-actions]

Good — the patch target is correct. mock.py doesn't import from openai.py, so AsyncOpenAI is only constructed if get_llm_provider routes to the OpenAI provider.

Now I have all the information needed for a review. Let me compile it.

---

Summary

PR adds a regression test (test_mock_provider_never_instantiates_openai) that patches AsyncOpenAI and asserts it is never called when _create_provider("mock") runs, plus a changelog entry. The underlying bug fix was in a prior commit (factory pattern in _create_provider); this PR is purely test + changelog.

---

Issues Found

1. [STYLE] Changelog entry belongs under ### Testing, not ### Fixed

File: CHANGELOG.md:13
Severity: LOW

The entry reads:

### Fixed

- **Mock Provider Regression Test** — add regression test verifying that ...

A test is not a fix. The actual fix (factory pattern) was already committed. This entry should be under ### Testing consistent with Keep a Changelog conventions.

Fix:

### Testing

- **Mock Provider Regression Test** — add regression test verifying that
  `--llm-provider mock` never falls back to `OpenAIProvider` (#40)

2. [STYLE] Redundant monkeypatch parameter in new test

File: tests/unit/test_cli/test_synth.py:44
Severity: LOW

Looking at the actual code at line 44:

def test_mock_provider_never_instantiates_openai() -> None:

The test does not have a monkeypatch parameter. The previous review comments flagged this, but the current PR code does not include it. So this issue was already addressed. No action needed.

---

Positive Notes

• Excellent docstring — immediately communicates the historical bug context, the invariants being tested, and why this matters (API key / cost risk)
• Double-entry verification — asserts both the positive (isinstance(provider, MockLLMProvider)) and negative (AsyncOpenAI.assert_not_called()) — strong regression coverage
• autospec=True on the patch — provides realistic mock behavior (AttributeError on missing attrs), catch API changes
• Fully isolated — no shared fixtures, no side effects, no reliance on global state
• Follows existing conventions — same import style, same assertion patterns, same _create_provider entry point as the existing test_create_provider_uses_offline_mock_provider
• Complements existing test — the older test checks the return type + actual generation output; this one verifies the negative invariant (OpenAI never constructed), making the coverage belt-and-suspenders

---

Verdict

COMMENT — Minor suggestions, no blocking issues. The test is well-written and adds meaningful regression coverage. Only the changelog section label should be corrected.

opencode session  |  github run

[github-actions]

🎉 Congratulations @himanshu231204!

Your pull request has been successfully merged into main. 🚀

Thank you for contributing to OpenAgentHQ and helping improve the project.

We truly appreciate your contribution and hope to see you back with more amazing PRs!

Happy Open Sourcing! ❤️