diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-09-external-providers.md | 54 | ||||
| -rw-r--r-- | llamachat/config.py | 7 | ||||
| -rw-r--r-- | llamachat/providers.py | 27 | ||||
| -rwxr-xr-x | test_llamachat.py | 42 |
4 files changed, 125 insertions, 5 deletions
diff --git a/docs/superpowers/plans/2026-08-09-external-providers.md b/docs/superpowers/plans/2026-08-09-external-providers.md index 4a428a1..3fd1e2f 100644 --- a/docs/superpowers/plans/2026-08-09-external-providers.md +++ b/docs/superpowers/plans/2026-08-09-external-providers.md @@ -177,6 +177,12 @@ Expected: FAIL with `ModuleNotFoundError: No module named 'llamachat.providers'` Create `llamachat/providers.py`. Copy the 14-line GPL header verbatim from the top of `llamachat/config.py`, then: +> **This block is the state at the end of Task 1, not the final module.** +> Tasks 2, 3 and 4b all amend `parse()` in place. In particular, `parse()` as +> written here crashes on several malformed config shapes; Task 4b replaces +> its coercion and loop guards. Read top-down and implement in order, but do +> not copy this block as the finished function. + ```python """Provider definitions, model-id namespacing and API key resolution.""" @@ -905,6 +911,10 @@ merge before the loop is reached (`[[providers.local]]`). The bracket slip is the one that matters most: `[[providers.together]]` for `[providers.together]` is an easy miscount and reads as a list of tables. +`[[providers.local]]` is the worst of the eight, and the easiest to +under-rate. It is the only one where the app comes up looking healthy, so it +needs the loudest warning rather than the quietest. See Step 3. + Note the first group raises two different exception types, `ValueError` for a string or list and `TypeError` for a number. That is why the guard is an isinstance test rather than a try/except. @@ -921,6 +931,12 @@ asserting that every shape above leaves a working local provider, that a `None` entry is skipped, and that each skip names the provider on stderr (capture it with `contextlib.redirect_stderr`). +For `[[providers.local]]`, pin the loss and not only the survival: assert the +warning is emitted, that `base_url` falls back to the bare one, and that +`api_key`, `filter` and `ctx_size` set on that entry are discarded. Pin the +message wording both ways too, that a list case says `not [[providers.x]]` +and a scalar case does not mention brackets at all. + Add `test_unusable_config_exits()`, which runs the real entry point in a child process. It must be a subprocess: `CONFIG_PATH` is read from the environment at import time and bound into `load()`'s default argument, so @@ -985,15 +1001,35 @@ Replace the table coercion and the local merge at the top of `parse()`: and, in the merge below it, read the local entry through `isinstance` rather than truthiness, because `[[providers.local]]` is a truthy list that would -raise here before the loop could skip it: +raise here before the loop could skip it. Warn at this site too: the loop +never sees this entry, because the merge replaces it: ```python bare = values.get("base_url") - local = table.get(LOCAL) if isinstance(table.get(LOCAL), dict) else {} + raw_local = table.get(LOCAL) + if raw_local is not None and not isinstance(raw_local, dict): + # Warned about here rather than left to the loop, which never sees it: + # the merge below replaces it with a synthesized entry, so every field + # the user set on it is dropped. That makes this the case where saying + # something matters most, not least. The app comes up working, local + # answers, and an api_key or ctx_size they set is simply gone, with a + # healthy-looking window as the only feedback. + _skipped(LOCAL, _not_a_table(LOCAL, raw_local)) + local = raw_local if isinstance(raw_local, dict) else {} if bare and not local.get("base_url"): table[LOCAL] = {**local, "base_url": bare} ``` +This one is easy to talk yourself out of, since local still works afterwards. +It is the case that most needs the warning: a working app is the strongest +possible signal that nothing is wrong, so the discarded `api_key` or +`ctx_size` has nothing else to announce it. + +Note also that local only survives because `config.DEFAULTS` always supplies +`base_url` for the rebuild. Without a bare URL, `[[providers.local]]` yields +no local provider at all. Add a comment in `config.py` saying that key is +load-bearing, so nobody removes it as redundant. + In the loop, replace the `entry = entry or {}` guard, which caught falsy values but not a truthy non-dict. The isinstance test subsumes the `None` case, so nothing is lost by dropping it: @@ -1005,7 +1041,7 @@ case, so nothing is lost by dropping it: # Most likely [[providers.x]] written for [providers.x], which # TOML reads as a list of tables. Skipping keeps every other # provider working, including local. - _skipped(name, "it is not a [providers.<name>] table") + _skipped(name, _not_a_table(name, entry)) continue ``` @@ -1013,6 +1049,18 @@ Give the two existing skips the same voice, naming what was wrong: `_skipped(name, "a provider name cannot be empty or contain ':'")` and `_skipped(name, "it has no base_url")`. +The wording of the not-a-table message depends on what was written, so it +lives in its own helper. A list is the double-bracket slip and should name the +single-bracket fix; anything else was written as `x = 5`, where pointing at a +bracket the user never typed would send them to fix the wrong line: + +```python +def _not_a_table(name: str, entry) -> str: + if isinstance(entry, list): + return f"use [providers.{name}], not [[providers.{name}]]" + return f"a provider must be a [providers.{name}] table" +``` + - [ ] **Step 4: Fix `__main__.py`** Wrap the `config.load()` call in `main()`: diff --git a/llamachat/config.py b/llamachat/config.py index ac53823..8fa4933 100644 --- a/llamachat/config.py +++ b/llamachat/config.py @@ -126,6 +126,13 @@ def load(path: Path = CONFIG_PATH) -> Config: # Providers are built from the raw values so a bare base_url still # synthesizes the local entry. DEFAULTS supplies base_url when the file # names neither, which keeps a config with no network settings working. + # + # That default is load-bearing, not a convenience: it is the only reason + # a malformed [providers.local] still leaves a working local provider, + # since parse() rebuilds local from the bare URL after skipping it. + # Dropping base_url from DEFAULTS, or letting a caller reach parse() + # without one, would turn every such config into an app with no local + # provider at all. Keep the key. provider_table = providers_mod.parse(values) return Config( diff --git a/llamachat/providers.py b/llamachat/providers.py index 52a1132..c31e82e 100644 --- a/llamachat/providers.py +++ b/llamachat/providers.py @@ -59,6 +59,20 @@ def _number(raw, cast): return None +def _not_a_table(name: str, entry) -> str: + """Why this entry is not a provider table, phrased for what was written. + + A list is almost always the [[providers.x]] double-bracket slip, so the + message names the single-bracket fix directly: "is not a table" alone does + not tell the user which character to change. Any other scalar was written + as `x = 5`, where suggesting a bracket fix would point at a line the user + never wrote, so that case just describes the shape. + """ + if isinstance(entry, list): + return f"use [providers.{name}], not [[providers.{name}]]" + return f"a provider must be a [providers.{name}] table" + + def _skipped(name: str, reason: str) -> None: """Say that a provider was dropped, so the loss is not silent. @@ -98,7 +112,16 @@ def parse(values: dict) -> dict[str, Provider]: # [[providers.local]] is a truthy list, and merging into it would raise # here, before the loop below ever gets a chance to skip it. bare = values.get("base_url") - local = table.get(LOCAL) if isinstance(table.get(LOCAL), dict) else {} + raw_local = table.get(LOCAL) + if raw_local is not None and not isinstance(raw_local, dict): + # Warned about here rather than left to the loop, which never sees it: + # the merge below replaces it with a synthesized entry, so every field + # the user set on it is dropped. That makes this the case where saying + # something matters most, not least. The app comes up working, local + # answers, and an api_key or ctx_size they set is simply gone, with a + # healthy-looking window as the only feedback. + _skipped(LOCAL, _not_a_table(LOCAL, raw_local)) + local = raw_local if isinstance(raw_local, dict) else {} if bare and not local.get("base_url"): table[LOCAL] = {**local, "base_url": bare} @@ -109,7 +132,7 @@ def parse(values: dict) -> dict[str, Provider]: # Most likely [[providers.x]] written for [providers.x], which # TOML reads as a list of tables. Skipping keeps every other # provider working, including local. - _skipped(name, "it is not a [providers.<name>] table") + _skipped(name, _not_a_table(name, entry)) continue if not name or ":" in name: # The id scheme splits on the first colon, so a name containing diff --git a/test_llamachat.py b/test_llamachat.py index ed50995..29f305f 100755 --- a/test_llamachat.py +++ b/test_llamachat.py @@ -1054,6 +1054,48 @@ def test_provider_malformed_shapes(): messages = err.getvalue() assert "listy" in messages and "urlless" in messages and "a:b" in messages assert "base_url" in messages # the missing-URL case names what is missing + # A list is the double-bracket slip, so the message names the fix. Any + # other scalar was not written that way, and must not be told to change a + # bracket it never had. + assert "not [[providers.listy]]" in messages + scalar = io.StringIO() + with redirect_stderr(scalar): + providers.parse({"providers": {"n": 5}}) + assert "[[" not in scalar.getvalue(), scalar.getvalue() + + # A malformed [[providers.local]] is the loudest case that needs saying, + # not the quietest: the merge below it rebuilds local from the bare URL, + # so the app comes up working and every field the user set is discarded + # silently. The skip is reported at the merge site because the loop never + # sees this entry. + err = io.StringIO() + with redirect_stderr(err): + clobbered = providers.parse( + { + "base_url": "http://localhost:8181", + "providers": {"local": [{"api_key": "env:SOME_VAR", + "filter": ["qwen"], + "ctx_size": 32768}]}, + } + ) + assert "local" in err.getvalue() + assert "not [[providers.local]]" in err.getvalue() + # The local provider survives, which is the non-negotiable. + assert clobbered["local"].base_url == "http://localhost:8181" + # Pinning the loss rather than only the survival: these fields are gone, + # and the warning above is the only thing that tells the user so. + assert clobbered["local"].api_key == "" + assert clobbered["local"].filter == [] + assert clobbered["local"].ctx_size is None + + # Without a bare base_url there is nothing to rebuild local from, so it + # vanishes entirely. config.DEFAULTS always supplies one, which is what + # keeps the real app safe; this pins that the safety net is that default + # and not something parse() does on its own. + assert providers.parse( + {"providers": {"local": [{"base_url": "http://y.example.org"}]}} + ) == {} + assert "base_url" in config.DEFAULTS print("ok malformed provider shapes are skipped, not raised") |