Skip to content

test(cli): isolate model picker Ollama probes (Fixes #30604) - #30692

Closed
deepujain wants to merge 1 commit into
NousResearch:mainfrom
deepujain:test/30604-model-picker-ollama-hermetic
Closed

test(cli): isolate model picker Ollama probes (Fixes #30604)#30692
deepujain wants to merge 1 commit into
NousResearch:mainfrom
deepujain:test/30604-model-picker-ollama-hermetic

Conversation

@deepujain

Copy link
Copy Markdown
Contributor

Summary

  • Disable live OpenAI-compatible /models probing by default in the affected custom-provider picker tests.
  • Keep the explicit live-discovery test covered by letting it override the default mock with its own fake catalog.

Root cause

The failing fixtures use api_key: "ollama" with http://localhost:11434/v1. list_authenticated_providers() treats that as permission to probe /models, so a developer's local Ollama server can replace the small fixture model list with the live local catalog.

Why this layer

Runtime discovery for credentialed custom providers is intentional. These tests are checking grouping, slug recovery, and picker passthrough behavior, so the safest fix is to make the tests hermetic instead of changing production model-picker behavior.

Fixes #30604

Validation

  • env OLLAMA_API_KEY=ollama OLLAMA_BASE_URL=http://localhost:11434/v1 scripts/run_tests.sh tests/hermes_cli/test_model_switch_custom_providers.py tests/hermes_cli/test_list_picker_providers.py
    • 28 tests passed, 0 failed

Platform tested: macOS local checkout.

@deepujain
deepujain force-pushed the test/30604-model-picker-ollama-hermetic branch from 4b69711 to 76153e6 Compare May 23, 2026 01:59
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard provider/ollama Ollama / local models tool/mcp MCP client and OAuth labels May 23, 2026
@deepujain
deepujain marked this pull request as ready for review May 23, 2026 02:21
@deepujain
deepujain force-pushed the test/30604-model-picker-ollama-hermetic branch from 76153e6 to f48769f Compare June 2, 2026 22:25
@deepujain

deepujain commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@teknium1 I cannot formally request review because GitHub says this account lacks RequestReviewsByLogin permission on this repo. This PR has been refreshed on current main and is mergeable. Focused validation: scripts/run_tests.sh tests/hermes_cli/test_list_picker_providers.py tests/hermes_cli/test_model_switch_custom_providers.py passed, 29/29. Could you approve any pending workflow run and review when you have a chance?

@deepujain

Copy link
Copy Markdown
Contributor Author

@teknium1 Follow-up after seven quiet days. This PR still has no visible checks, assignee, requested reviewer, or human review. GitHub reports it as mergeable. Focused validation from the refreshed branch: scripts/run_tests.sh tests/hermes_cli/test_list_picker_providers.py tests/hermes_cli/test_model_switch_custom_providers.py passed, 29/29. GitHub still will not let me formally request review from this fork. Could you approve any pending workflow run and review when you have a chance?

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the custom-provider tests. The current-main premise is verified: tests/hermes_cli/test_model_switch_custom_providers.py:648 and tests/hermes_cli/test_list_picker_providers.py:260 use credentialed localhost Ollama fixtures without stubbing fetch_api_models, while hermes_cli/model_switch.py:2361-2378 probes and replaces their configured models with a non-empty live catalog.

The PR’s file-local autouse mock keeps those unit tests hermetic while preserving intentional discovery coverage: tests with live-discovery assertions replace hermes_cli.models.fetch_api_models with their own fake (for example tests/hermes_cli/test_model_switch_custom_providers.py:347-351). The patch is test-only and applies cleanly in concept to current main.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #67971 — your commit was cherry-picked onto current main with your authorship preserved in git log. You were the first submitter for this fix class (May 23); the narrower 3-test version (#62421) landed earlier today as #67921, and your broader autouse-fixture layer covering both test files landed on top. Thanks — and #30604 is closed by this.

@teknium1 teknium1 closed this Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have provider/ollama Ollama / local models sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users tool/mcp MCP client and OAuth type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Model picker tests leak live localhost Ollama models into custom provider fixtures

3 participants