fix(minimax): refresh token-plan model availability - #48
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds robust MiniMax model discovery (Anthropic-first, OpenAI-fallback), defensive parsing and deduplication of discovered models, apply/refresh helpers and login behavior changes, integrates MiniMax refresh into the managed-provider flow, and expands tests and changelog. ChangesMiniMax Model Discovery and Managed Integration
sequenceDiagram
participant LoginFlow
participant MiniMaxDiscovery as _discover_minimax_models
participant AnthropicAPI as Anthropic_/models
participant OpenAIAPI as OpenAI_/models
participant ApplyModels as apply_minimax_models
participant ConfigStore
LoginFlow->>MiniMaxDiscovery: call with resolved_key
MiniMaxDiscovery->>AnthropicAPI: GET /models (X-Api-Key)
alt Anthropic succeeds
AnthropicAPI-->>MiniMaxDiscovery: models payload
else Anthropic fails or non-auth
MiniMaxDiscovery->>OpenAIAPI: GET /models (Authorization: Bearer)
OpenAIAPI-->>MiniMaxDiscovery: models payload / error
end
MiniMaxDiscovery->>ApplyModels: parsed MiniMaxModel tuple
ApplyModels->>ConfigStore: upsert/prune models, adjust default_model, persist changes
🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/auth/test_platforms.py (1)
878-882:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert MiniMax state is unchanged in persisted config as well.
This test proves in-memory MiniMax state is retained, but it doesn’t assert the saved config also keeps MiniMax unchanged.
Proposed assertion additions
assert changed is True assert len(saved) == 1 assert saved[0].models["pythinker-code/pythinker-for-coding"].max_context_size == 200_000 + assert "minimax/m2.7-highspeed" in saved[0].models + assert saved[0].models["minimax/m2.7-highspeed"].provider == "managed:minimax-anthropic" assert "minimax/m2.7-highspeed" in config.models🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/auth/test_platforms.py` around lines 878 - 882, Add an assertion to verify that the MiniMax model state in the saved configuration remains unchanged by checking that the saved config's MiniMax model settings are equal to those in the original config. Locate the assertions verifying config models in the test and add this new assertion after them to confirm MiniMax's state consistency in the persisted config.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pythinker_code/auth/minimax.py`:
- Around line 220-221: The discovery path currently treats an empty `models`
list as falsy and falls through to the secondary endpoint, which causes the
result to be collapsed to None and preserves stale aliases; update the logic so
that an empty list is considered authoritative: return the discovered `models`
(including empty list) from the `get`/discovery code path instead of falling
through, and ensure callers (e.g., the place that assigns `minimax_models`)
check `is not None` rather than truthiness before calling
`apply_minimax_models`; update the relevant variables `models`, `minimax_models`
and the call to `apply_minimax_models` (also adjust the code around the block at
the later fallback around lines 294-299) so an empty list is propagated and
handled correctly.
In `@src/pythinker_code/auth/platforms.py`:
- Around line 257-264: The current branch checks for
MINIMAX_ANTHROPIC_PROVIDER_KEY which embeds a transport suffix into the managed
provider identity; change logic so MiniMax is keyed as the canonical managed
provider (use managed_provider_key('minimax') / parse_managed_provider_key())
and stop special-casing MINIMAX_ANTHROPIC_PROVIDER_KEY. Update the condition
that references OPENCODE_GO_PROVIDER_KEYS or MINIMAX_ANTHROPIC_PROVIDER_KEY to
instead only special-case OpenCode Go via OPENCODE_GO_PROVIDER_KEYS, and ensure
any code that previously relied on MINIMAX_ANTHROPIC_PROVIDER_KEY derives the
wire-shape (Anthropic vs OpenAI) from the active model/provider metadata (using
parse_managed_provider_key() and managed_model_key()/managed_provider_key())
rather than a hard-coded provider key.
In `@tests/auth/test_minimax_auth.py`:
- Line 189: The test currently patches the private helper
_discover_minimax_models which couples the test to internal implementation;
instead, mock the external observable boundary used by login_minimax_api_key
(for example the HTTP client or session method it calls, e.g., requests.post or
the session object's post/send method) so the test drives behavior via real
public inputs and responses and asserts outcomes of login_minimax_api_key;
replace
monkeypatch.setattr("pythinker_code.auth.minimax._discover_minimax_models", ...)
with a monkeypatch of the HTTP call used by login_minimax_api_key (or the public
client wrapper it uses), return the same fake HTTP response payload, and keep
assertions against login_minimax_api_key results rather than internal helper
calls.
In `@tests/auth/test_platforms.py`:
- Around line 803-805: The tests currently patch the private method
_discover_minimax_models, which couples them tightly to MiniMax internals and
breaks abstraction. Instead, locate and patch the public method
refresh_minimax_models in the test platform mocks to mock the observable
behavior at the integration boundary. Replace all occurrences where
_discover_minimax_models is patched, including lines 803 and 860-862, to patch
refresh_minimax_models instead to follow the testing guidelines and reduce
coupling to private internals.
---
Outside diff comments:
In `@tests/auth/test_platforms.py`:
- Around line 878-882: Add an assertion to verify that the MiniMax model state
in the saved configuration remains unchanged by checking that the saved config's
MiniMax model settings are equal to those in the original config. Locate the
assertions verifying config models in the test and add this new assertion after
them to confirm MiniMax's state consistency in the persisted config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e7acb4a-cc57-44e0-87cd-733f7ab8aa8f
📒 Files selected for processing (4)
src/pythinker_code/auth/minimax.pysrc/pythinker_code/auth/platforms.pytests/auth/test_minimax_auth.pytests/auth/test_platforms.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/auth/minimax.py (1)
327-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle successful-but-empty MiniMax discovery during login to avoid pruning MiniMax and hijacking
default_model.When
_discover_minimax_models()returns(),modelsgets overwritten and_apply_minimax_config()deletes all existing MiniMax models (providerMINIMAX_ANTHROPIC_PROVIDER_KEY) and then setsconfig.default_modelto the first remaining non-MiniMax model alias (or"").login_minimax_api_keythen yields"success"even though zero MiniMax models were configured.Suggested fix
models = MINIMAX_MODELS try: - models = await _discover_minimax_models(resolved_key) + discovered = await _discover_minimax_models(resolved_key) + if discovered: + models = discovered + else: + yield OAuthEvent( + "info", + "MiniMax returned no models for this key; using the built-in model list.", + ) except aiohttp.ClientResponseError as exc: if exc.status in {401, 403}: yield OAuthEvent("error", "Invalid MiniMax API key; the key was not saved.") return yield OAuthEvent( "info", "MiniMax model listing is unavailable; using the built-in model list.", ) except (aiohttp.ClientError, TimeoutError, ValueError): yield OAuthEvent( "info", "MiniMax model listing is unavailable; using the built-in model list.", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pythinker_code/auth/minimax.py` around lines 327 - 344, The login flow currently overwrites models with the result of _discover_minimax_models(resolved_key) even when that call returns an empty sequence, which causes _apply_minimax_config(...) to prune all MiniMax models and mis-set config.default_model; change the logic so you only assign models = discovered if the returned value from _discover_minimax_models(resolved_key) is non-empty/truthy (otherwise keep the fallback MINIMAX_MODELS), and in the empty case yield the same OAuthEvent("info", "MiniMax model listing is unavailable; using the built-in model list.") path so successful-but-empty discovery does not remove configured MiniMax models or hijack default_model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/pythinker_code/auth/minimax.py`:
- Around line 327-344: The login flow currently overwrites models with the
result of _discover_minimax_models(resolved_key) even when that call returns an
empty sequence, which causes _apply_minimax_config(...) to prune all MiniMax
models and mis-set config.default_model; change the logic so you only assign
models = discovered if the returned value from
_discover_minimax_models(resolved_key) is non-empty/truthy (otherwise keep the
fallback MINIMAX_MODELS), and in the empty case yield the same
OAuthEvent("info", "MiniMax model listing is unavailable; using the built-in
model list.") path so successful-but-empty discovery does not remove configured
MiniMax models or hijack default_model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a8af7517-94a1-461b-87b0-0b559d227cd3
📒 Files selected for processing (4)
src/pythinker_code/auth/minimax.pysrc/pythinker_code/auth/platforms.pytests/auth/test_minimax_auth.pytests/auth/test_platforms.py
Member
Author
|
CodeRabbit outside-diff follow-up: fixed the valid default-model part in cc303d3 by preserving an existing non-MiniMax default when login receives an authoritative empty MiniMax catalog. Skipped the suggested built-in fallback because this PR's requirement is that a successful empty authenticated catalog is authoritative; falling back to static models would reintroduce unavailable/stale aliases. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/v1/modelsas fallbackTests
make check-pythinker-codemake test-pythinker-codeSummary by CodeRabbit