diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-09-external-providers.md | 66 | ||||
| -rw-r--r-- | llamachat/providers.py | 24 | ||||
| -rwxr-xr-x | test_llamachat.py | 19 |
3 files changed, 105 insertions, 4 deletions
diff --git a/docs/superpowers/plans/2026-08-09-external-providers.md b/docs/superpowers/plans/2026-08-09-external-providers.md index 0c8fc19..72db023 100644 --- a/docs/superpowers/plans/2026-08-09-external-providers.md +++ b/docs/superpowers/plans/2026-08-09-external-providers.md @@ -332,6 +332,8 @@ def test_model_ids_and_filtering(): assert providers.split("weird:name", table) == ("local", "weird:name") # Only the first colon splits. assert providers.split("together:a:b", table) == ("together", "a:b") + # Task 9 addresses a provider itself with an empty model name. + assert providers.split("together:", table) == ("together", "") # Filtering is case-insensitive substring, any match wins. listed = [ @@ -353,11 +355,41 @@ def test_model_ids_and_filtering(): # A filter matching nothing yields nothing, it does not fall back to all. table["together"].filter = ["zzz"] assert providers.apply_filter(table["together"], listed) == [] + # An empty needle is a typo rather than a request to hide everything, so + # it means no filter. The opposite of the "zzz" case above, deliberately. + table["together"].filter = [""] + assert providers.apply_filter(table["together"], listed) == listed print("ok model ids and filtering") ``` Register it after `test_provider_parsing()` in the `__main__` block. +Task 2 also closes three malformed-config shapes found reviewing Task 1, by +adding to `test_provider_parsing()`: + +```python + # Any other non-list shape is wrapped too, rather than iterated: a number + # would raise, and a dict would silently degrade into its keys. + odd = providers.parse( + {"providers": {"n": {"base_url": "http://x.example.org", "filter": 5}, + "d": {"base_url": "http://y.example.org", + "filter": {"a": 1}}}} + ) + assert odd["n"].filter == ["5"] + # The invariant is that the dict was wrapped whole, not iterated into its + # keys. Asserting that rather than its repr, which is not ours to pin. + assert odd["d"].filter != ["a"] and len(odd["d"].filter) == 1 + + # A colon in a provider name would make every id built from it ambiguous, + # so such a provider is skipped rather than silently routed to local. + colonic = providers.parse( + {"providers": {"local": {"base_url": "http://x.example.org"}, + "a:b": {"base_url": "http://y.example.org"}, + "": {"base_url": "http://z.example.org"}}} + ) + assert set(colonic) == {"local"} +``` + - [ ] **Step 2: Run the suite to verify it fails** Run: `./test_llamachat.py` @@ -365,7 +397,26 @@ Expected: FAIL with `AttributeError: module 'llamachat.providers' has no attribu - [ ] **Step 3: Write the implementation** -Append to `llamachat/providers.py`: +In `parse()`, broaden the filter guard so any non-list shape is wrapped as one +needle rather than iterated, and skip a provider whose name is unusable: + +```python + name = str(name) + if not name or ":" in name: + # The id scheme splits on the first colon, so a name containing + # one builds ids that split back to something else entirely, and + # an empty name builds ":model". Both route to local under a + # nonsense name, silently. Skipping is the only honest option. + continue +``` + +```python + needles = entry.get("filter") or [] + if not isinstance(needles, (list, tuple)): + needles = [needles] +``` + +Then append to `llamachat/providers.py`: ```python def qualify(provider: str, model: str) -> str: @@ -381,6 +432,14 @@ def split(model_id: str, table: dict[str, Provider]) -> tuple[str, str]: A prefix that is not a configured provider is treated as part of the model name, which keeps a bare local model containing a colon working. """ + # ponytail: this is not injective, and cannot be while local ids stay + # bare. A local model actually named "together:x" is indistinguishable + # from together's model "x", so it resolves to local only until the user + # configures a provider called "together", at which point the stored + # session id rebinds to the cloud model and starts billing without a + # word. Unlikely, since local names are usually filenames, and the bare + # local id is the feature's premise. The upgrade path is to qualify local + # as "local:<model>" too and migrate the sessions.model column. prefix, sep, rest = model_id.partition(":") if sep and prefix in table and prefix != LOCAL: return prefix, rest @@ -393,8 +452,13 @@ def apply_filter(provider: Provider, listed: list[str]) -> list[str]: Case-insensitive, because provider ids capitalise inconsistently: "qwen" has to match "Qwen/Qwen2.5-72B-Instruct-Turbo". """ + # Every path returns a fresh list on purpose: callers hold onto the + # result, and handing back "listed" itself would let them mutate the + # caller's own list. Not a redundant copy, do not simplify it away. if provider.is_local or not provider.filter: return list(listed) + # An empty needle is a typo, not a request to hide every model, so a + # filter of only empty strings means no filter. needles = [f.lower() for f in provider.filter if f] if not needles: return list(listed) diff --git a/llamachat/providers.py b/llamachat/providers.py index 5874b32..0d2d9f7 100644 --- a/llamachat/providers.py +++ b/llamachat/providers.py @@ -76,6 +76,13 @@ def parse(values: dict) -> dict[str, Provider]: out: dict[str, Provider] = {} for name, entry in table.items(): entry = entry or {} + name = str(name) + if not name or ":" in name: + # The id scheme splits on the first colon, so a name containing + # one builds ids that split back to something else entirely, and + # an empty name builds ":model". Both route to local under a + # nonsense name, silently. Skipping is the only honest option. + continue base_url = str(entry.get("base_url") or "").rstrip("/") if not base_url: # ponytail: a provider with no URL is misconfigured, not a @@ -91,8 +98,8 @@ def parse(values: dict) -> dict[str, Provider]: if not isinstance(needles, (list, tuple)): needles = [needles] vision = entry.get("vision") - out[str(name)] = Provider( - name=str(name), + out[name] = Provider( + name=name, base_url=base_url, api_key=str(entry.get("api_key") or ""), filter=[str(f) for f in needles], @@ -117,6 +124,14 @@ def split(model_id: str, table: dict[str, Provider]) -> tuple[str, str]: A prefix that is not a configured provider is treated as part of the model name, which keeps a bare local model containing a colon working. """ + # ponytail: this is not injective, and cannot be while local ids stay + # bare. A local model actually named "together:x" is indistinguishable + # from together's model "x", so it resolves to local only until the user + # configures a provider called "together", at which point the stored + # session id rebinds to the cloud model and starts billing without a + # word. Unlikely, since local names are usually filenames, and the bare + # local id is the feature's premise. The upgrade path is to qualify local + # as "local:<model>" too and migrate the sessions.model column. prefix, sep, rest = model_id.partition(":") if sep and prefix in table and prefix != LOCAL: return prefix, rest @@ -129,8 +144,13 @@ def apply_filter(provider: Provider, listed: list[str]) -> list[str]: Case-insensitive, because provider ids capitalise inconsistently: "qwen" has to match "Qwen/Qwen2.5-72B-Instruct-Turbo". """ + # Every path returns a fresh list on purpose: callers hold onto the + # result, and handing back "listed" itself would let them mutate the + # caller's own list. Not a redundant copy, do not simplify it away. if provider.is_local or not provider.filter: return list(listed) + # An empty needle is a typo, not a request to hide every model, so a + # filter of only empty strings means no filter. needles = [f.lower() for f in provider.filter if f] if not needles: return list(listed) diff --git a/test_llamachat.py b/test_llamachat.py index 5f6bad6..8b722ae 100755 --- a/test_llamachat.py +++ b/test_llamachat.py @@ -987,7 +987,18 @@ def test_provider_parsing(): "filter": {"a": 1}}}} ) assert odd["n"].filter == ["5"] - assert odd["d"].filter == ["{'a': 1}"] + # The invariant is that the dict was wrapped whole, not iterated into its + # keys. Asserting that rather than its repr, which is not ours to pin. + assert odd["d"].filter != ["a"] and len(odd["d"].filter) == 1 + + # A colon in a provider name would make every id built from it ambiguous, + # so such a provider is skipped rather than silently routed to local. + colonic = providers.parse( + {"providers": {"local": {"base_url": "http://x.example.org"}, + "a:b": {"base_url": "http://y.example.org"}, + "": {"base_url": "http://z.example.org"}}} + ) + assert set(colonic) == {"local"} print("ok provider config parsing") @@ -1023,6 +1034,8 @@ def test_model_ids_and_filtering(): assert providers.split("weird:name", table) == ("local", "weird:name") # Only the first colon splits. assert providers.split("together:a:b", table) == ("together", "a:b") + # Task 9 addresses a provider itself with an empty model name. + assert providers.split("together:", table) == ("together", "") # Filtering is case-insensitive substring, any match wins. listed = [ @@ -1044,6 +1057,10 @@ def test_model_ids_and_filtering(): # A filter matching nothing yields nothing, it does not fall back to all. table["together"].filter = ["zzz"] assert providers.apply_filter(table["together"], listed) == [] + # An empty needle is a typo rather than a request to hide everything, so + # it means no filter. The opposite of the "zzz" case above, deliberately. + table["together"].filter = [""] + assert providers.apply_filter(table["together"], listed) == listed print("ok model ids and filtering") |