Skip to content

test(tui_gateway): assert target_model in resolve_runtime_provider call#16684

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:test/tui-gateway-make-agent-target-model-assert
Closed

test(tui_gateway): assert target_model in resolve_runtime_provider call#16684
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:test/tui-gateway-make-agent-target-model-assert

Conversation

@briandevans

@briandevans briandevans commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • _make_agent in tui_gateway/server.py:1410-1413 was updated in e9c47c704 ("fix(tui): honor launch model overrides") to pass target_model=model or None to resolve_runtime_provider. The regression test in tests/tui_gateway/test_make_agent_provider.py still pinned the old shape with assert_called_once_with(requested=None), so it fails on clean origin/main.
  • Update the assertion to match — keeps the existing intent (AIAgent is built from the resolved runtime) and the test still proves the call to resolve_runtime_provider happens once with the right kwargs.

The bug

Before e9c47c7, _make_agent called:

runtime = resolve_runtime_provider(requested=requested_provider)

After e9c47c7, it threads the configured/launch model in:

runtime = resolve_runtime_provider(
    requested=requested_provider,
    target_model=model or None,
)

The fake config used by this test (fake_cfg = {\"model\": {\"default\": \"claude-opus-4-6\", ...}}) makes model = \"claude-opus-4-6\", so the production call now includes target_model=\"claude-opus-4-6\". The strict assert_called_once_with(requested=None) raises:

AssertionError: expected call not found.
Expected: resolve_runtime_provider(requested=None)
  Actual: resolve_runtime_provider(requested=None, target_model='claude-opus-4-6')

This reproduces on ef41d3bd4 (current origin/main) under python3.11 and python3.12.

The fix

Update the assertion to:

mock_resolve.assert_called_once_with(
    requested=None, target_model=\"claude-opus-4-6\"
)

The test continues to verify the documented behavior described in its own docstring ("_make_agent forwards provider/base_url/api_key/api_mode from resolve_runtime_provider to AIAgent") via the four call_kwargs.kwargs[...] == ... assertions immediately below. Only the call-site shape was updated to reflect the new kwarg.

Test plan

  • Reproduced on clean origin/main (ef41d3bd4): 
    uv run --with pytest --with pytest-xdist python3 -m pytest tests/tui_gateway/test_make_agent_provider.py::test_make_agent_passes_resolved_provider -v
→ 1 failed (\"Expected: resolve_runtime_provider(requested=None) / Actual: ...target_model='claude-opus-4-6'\")
  • With the fix applied, full file is green: tests/tui_gateway/test_make_agent_provider.py → 6 passed.
  • No production change.

Related

  • e9c47c7 introduced the new kwarg without updating this assertion.
  • The other 5 tests in this file already account for target_model because they don't strictly pin the call signature — only this one was strict.

@briandevans @claude
test(tui_gateway): assert target_model in resolve_runtime_provider call
e7bd2ef 
_make_agent in tui_gateway/server.py:1410-1413 was updated in e9c47c7
("fix(tui): honor launch model overrides") to thread the resolved startup
model into resolve_runtime_provider:

    runtime = resolve_runtime_provider(
        requested=requested_provider,
        target_model=model or None,
    )

The regression test still asserted the pre-change call shape
(`assert_called_once_with(requested=None)`), so it failed on origin/main
once the production change landed:

    Expected: resolve_runtime_provider(requested=None)
      Actual: resolve_runtime_provider(requested=None,
                                       target_model='claude-opus-4-6')

Update the assertion to match the new shape (the fake config sets
model.default = "claude-opus-4-6", so target_model is non-None and the
call passes both kwargs). The test still verifies the original intent —
that AIAgent is constructed from the resolved provider/base_url/api_key
/api_mode — and the rest of the file already covers cases where target
model resolution differs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 27, 2026 19:21
Copilot AI reviewed Apr 27, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates a TUI gateway regression test to match the current _make_agent() call signature, ensuring the test continues to validate that runtime provider resolution is invoked with the correct kwargs after the earlier production change that threaded target_model.

Changes:

  • Update test_make_agent_passes_resolved_provider to assert resolve_runtime_provider() is called with both requested and target_model.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/tui_gateway/test_make_agent_provider.py
Comment on lines +48 to +50
mock_resolve.assert_called_once_with(
requested=None, target_model="claude-opus-4-6"
)
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/tui Terminal UI (ui-tui/ + tui_gateway/) labels Apr 27, 2026
@briandevans @claude
test(tui_gateway): clear HERMES_MODEL env vars so target_model assert…
81c2c39 
…ion is hermetic

Address Copilot review on PR NousResearch#16684: ``_resolve_model()`` consults
``HERMES_MODEL`` and ``HERMES_INFERENCE_MODEL`` *before* falling back
to the patched ``_load_cfg`` value. A developer running the suite with
either env var set would see the resolved model — and therefore the
``target_model`` kwarg passed into ``resolve_runtime_provider`` —
diverge from the ``"claude-opus-4-6"`` config default this test asserts
on.

Add ``monkeypatch.delenv(..., raising=False)`` for both env vars at the
top of the test so the assertion only depends on the patched config.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@briandevans

briandevans commented Apr 27, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Finding addressed in commit 81c2c39dd:

Finding (line 50 — target_model pinned to config default while _resolve_model() prefers env vars):
Added monkeypatch.delenv("HERMES_MODEL", raising=False) and monkeypatch.delenv("HERMES_INFERENCE_MODEL", raising=False) at the top of the test. _resolve_model() (tui_gateway/server.py:613-625) consults those two env vars before falling back to the patched _load_cfg, so a developer running the suite with either set would have observed target_model="<their env value>" instead of "claude-opus-4-6". Verified locally: with the prior code and HERMES_MODEL=fake-other-model the test failed with Actual: resolve_runtime_provider(requested=None, target_model="fake-other-model"); with the env-clearing in place the test passes regardless of the inherited environment.

All 6 tests in the file pass under HERMES_MODEL=fake-model after the fix.

This was referenced Apr 28, 2026
Closed
Closed
@briandevans

briandevans commented Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

Closing — main's tests/tui_gateway/test_make_agent_provider.py now intentionally drops the target_model="claude-opus-4-6" assertion (commit fa2bee1, landed via @kshitijk4poor's #17102 salvage). The new comment in the test explains why: due to module-level caching in tui_gateway.server, the patched _load_cfg doesn't take effect when the module was imported by an earlier test, so any concrete target_model value asserted here is non-hermetic.

This PR's monkeypatch approach only neutralises env-var overrides (HERMES_MODEL, HERMES_INFERENCE_MODEL), not the import-cache path the maintainer flagged. The current main test asserting only requested=None is the correct shape.

Thanks @kshitijk4poor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have type/test Test coverage or test infrastructure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@briandevans @alt-glitch