refactor: switch from api.json to models.dev/models.json#74
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes provider-specific configuration and lookup logic, simplifies the completion menu anchoring, and introduces a new FileCompleter that uses fd to suggest file and folder paths when typing @. Feedback on the changes includes a critical fix for a Python syntax error in the exception handling of FileCompleter and a recommendation to improve the robustness of the cache refresh logic when the cache file is empty or invalid.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| text=True, | ||
| timeout=2.0, | ||
| ) | ||
| except subprocess.TimeoutExpired, FileNotFoundError, OSError: |
There was a problem hiding this comment.
This line contains a syntax error in Python 3. Multiple exceptions in an except clause must be enclosed in parentheses. Otherwise, it will raise a SyntaxError at runtime.
| except subprocess.TimeoutExpired, FileNotFoundError, OSError: | |
| except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| if self._cache_path.exists(): | ||
| age = time.time() - self._cache_path.stat().st_mtime | ||
| if age < 3600: | ||
| return |
There was a problem hiding this comment.
If the cache file exists but is empty or contains invalid JSON, _load_cache() will return an empty dictionary, but refresh_cache() will still skip the refresh if the file was modified less than an hour ago. To make this more robust, we should only skip the refresh if the cache was successfully loaded with data.
if self._cache_path.exists() and self._load_cache():
age = time.time() - self._cache_path.stat().st_mtime
if age < 3600:
returnReplace the old provider-centric model catalog with the new flat models.dev format. Remove DEFAULT_PROVIDERS, get_provider(), and related provider lookup logic since the new JSON uses model_id as the top-level key instead of grouping by provider. Also add 1-hour TTL cache for refresh_cache() and short-name fallback in get_best_limit() to handle Anthropic API model IDs (e.g. deepseek-v4-flash -> deepseek/deepseek-v4-flash). Assisted-by: mini-agent:deepseek-v4-flash
5e0dadd to
659a37e
Compare
|
| Filename | Overview |
|---|---|
| src/mini_agent/cli/models.py | URL switched to models.json, get_best_limit simplified to flat lookup with short-name fallback, 1-hour TTL guard added to refresh_cache; two minor edge cases around corrupt cache bypassing re-download and ambiguous short-name collisions. |
| src/mini_agent/config.py | Cleanly removes DEFAULT_PROVIDERS, get_provider(), and related private fields; no remaining references to the removed API anywhere in the codebase. |
| docs/config.md | Removes provider field documentation and the default-provider prefix table; docs now accurately reflect the simplified config surface. |
Sequence Diagram
sequenceDiagram
participant User
participant prompt_model
participant refresh_cache
participant _load_cache
participant models_dev as models.dev/models.json
User->>prompt_model: /model command
prompt_model->>refresh_cache: refresh_cache()
alt "cache file exists AND age < 1 hour"
refresh_cache-->>prompt_model: return (skip network)
else cache missing or stale
refresh_cache->>models_dev: GET models.json
models_dev-->>refresh_cache: "flat {model_id: {limit: {...}}}"
refresh_cache->>refresh_cache: write to ~/.mini-agent/models.json
refresh_cache->>refresh_cache: "self._cache = data"
end
prompt_model->>prompt_model: fetch_models() via SDK/API
prompt_model->>_load_cache: get_best_limit(model_id, key)
alt full key match
_load_cache-->>prompt_model: return limit value
else short-name fallback
_load_cache->>_load_cache: "scan cache for key.split("/")[-1] == model_id"
_load_cache-->>prompt_model: return first match's limit value
end
Reviews (1): Last reviewed commit: "refactor: switch from api.json to models..." | Re-trigger Greptile
| if self._cache_path.exists(): | ||
| age = time.time() - self._cache_path.stat().st_mtime | ||
| if age < 3600: | ||
| return |
There was a problem hiding this comment.
TTL skips refresh even for corrupt cache files
The age check gates the entire fetch unconditionally on file existence and age. If the cache file is corrupted (invalid JSON), _load_cache() catches the JSONDecodeError and silently returns {}, but refresh_cache() will not attempt to re-download within the 1-hour window because the file's mtime is recent. A user would see stale empty limits for up to an hour with no feedback.
| if model is None: | ||
| for full_key, data in cache.items(): | ||
| if full_key.split("/")[-1] == model_id: | ||
| model = data | ||
| break |
There was a problem hiding this comment.
Short-name fallback can silently return data for the wrong provider
The loop returns the first entry whose full_key.split("/")[-1] equals model_id, and the order depends on JSON parse order. If two providers list a model with the same short name (e.g., provider-a/deepseek-v4-flash and provider-b/deepseek-v4-flash), the context/output limits from whichever entry appears first in models.json are returned, regardless of which provider is actually serving the model. Consider logging a warning or preferring a well-known provider when a collision is detected.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Description
Switch model data source from the old provider-centric
api.jsonformat to the new flatmodels.dev/models.json.Changes:
config.py: RemoveDEFAULT_PROVIDERS,get_provider(), and provider-related fields — no longer needed sincemodels.dev/models.jsonusesmodel_idas the top-level key instead of grouping by provider.models.py: Replacehttps://models.dev/api.jsonURL withhttps://models.dev/models.json; simplifyget_best_limit()to work with flat model data; add 1-hour TTL cache torefresh_cache()to avoid unnecessary network requests; add short-name fallback lookup for model IDs returned by the Anthropic API (e.g.deepseek-v4-flash→deepseek/deepseek-v4-flash).docs/config.md: Removeproviderfield documentation and related behavior description.Type of change
Test
get_best_limit()correctly resolves both full keys (deepseek/deepseek-v4-flash) and short names (deepseek-v4-flash)Assisted-by: mini-agent:deepseek-v4-flash