fix: reject non-finite prices and escape a model named DEFAULT

float() accepts "nan" and "inf", so a hand-edited models.ini could feed
either into the cost arithmetic and render "$nan". Rejected once in _get
rather than at each downstream call site.

A model whose id is literally DEFAULT wrote a [DEFAULT] section, which a
stock configparser reader treats as defaults inherited by every other
model. Since the file is documented for hand-editing, that reintroduces
the contamination the renamed default_section exists to prevent, so the
id is escaped on write and mapped back on read.

Also drops the marker branch in save(), leaving mark_skipped() as the
sole writer of the "marker present iff no real keys" invariant, and
prunes two test blocks that could not fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

1 files changed, 177 insertions, 22 deletions

diff --git a/docs/superpowers/plans/2026-08-09-external-providers.md b/docs/superpowers/plans/2026-08-09-external-providers.md
index 3fd1e2f..88e050c 100644
--- a/docs/superpowers/plans/2026-08-09-external-providers.md
+++ b/docs/superpowers/plans/2026-08-09-external-providers.md
@@ -1175,15 +1175,84 @@ def test_models_store():
         # A model id with a colon must survive being an ini section name.
         again.save("together:org/name:v2", models.ModelInfo(ctx_size=4096))
         assert models.ModelStore(path).get("together:org/name:v2").ctx_size == 4096
+
+        # False must survive as False, not degrade to None: "no vision" is a
+        # real answer that shadows a provider default, unlike "unknown".
+        # Pins the lowercase wire format too, which Task 15 documents for
+        # hand-editing. (Collapsing the bool branch in save() would still
+        # pass, since _get_bool lowercases: this guards the meaning, not
+        # that one branch.)
+        again.save("together:novision", models.ModelInfo(vision=False))
+        assert models.ModelStore(path).get("together:novision").vision is False
+        assert "vision = false" in path.read_text(encoding="utf-8")
+
+        # Cancelling a dialog over a model we already know must not erase it,
+        # nor mark it skipped: the marker means "no real keys", so a section
+        # holding both would be a state no reader is written to expect.
+        again.mark_skipped("together:Qwen/Qwen2.5")
+        assert models.ModelStore(path).get("together:Qwen/Qwen2.5").ctx_size == 32768
+        assert models.SKIPPED not in path.read_text(encoding="utf-8").split(
+            "[together:Qwen/Qwen2.5]"
+        )[1].split("[")[0]
+
+        # A model literally named DEFAULT must not write a [DEFAULT] section:
+        # a stock-configparser reader, which Task 15 invites by documenting
+        # this file, would read it as inherited defaults for every model.
+        again.save("DEFAULT", models.ModelInfo(ctx_size=2048))
+        assert "[DEFAULT]" not in path.read_text(encoding="utf-8")
+        escaped = models.ModelStore(path)
+        assert escaped.get("DEFAULT").ctx_size == 2048
+        assert escaped.was_offered("DEFAULT") is True
+        # The escape must not swallow a neighbouring id.
+        assert escaped.was_offered("DEFAULTS") is False
+
+        # A hand-edited file must degrade, not raise: "32k" is not an int.
+        # The [DEFAULT] value is deliberately a *valid* int, so this catches
+        # the leak itself rather than an unparseable value hiding it.
+        path.write_text(
+            "[DEFAULT]\nctx_size = 999\n\n"
+            "[together:junk]\nctx_size = 32k\nprice_in = free\n\n"
+            "[together:empty]\n",
+            encoding="utf-8",
+        )
+        edited = models.ModelStore(path)
+        assert edited.get("together:junk") is None
+        assert edited.was_offered("together:junk") is True
+        # Would be ctx_size 999, inherited from [DEFAULT], if the store used
+        # configparser's real default section.
+        assert edited.get("together:empty") is None
+
+        # nan and inf parse cleanly through float(), so they would reach the
+        # cost arithmetic and render as "$nan". Unknown is the honest answer.
+        path.write_text(
+            "[together:nan]\nprice_in = nan\nprice_out = inf\n\n"
+            "[together:neg]\nprice_in = -inf\nctx_size = 8192\n",
+            encoding="utf-8",
+        )
+        weird = models.ModelStore(path)
+        assert weird.get("together:nan") is None
+        # A bad price must not take the good ctx_size down with it.
+        assert weird.get("together:neg").price_in is None
+        assert weird.get("together:neg").ctx_size == 8192
+
+        # A file that is not ini at all reads as empty rather than raising.
+        path.write_text("this is not an ini file\n", encoding="utf-8")
+        assert models.ModelStore(path).was_offered("together:junk") is False
+
+        # Nor may a binary file take the app down at startup: that raises
+        # UnicodeDecodeError, which is not a configparser.Error.
+        path.write_bytes(b"\xff\xfe\x00not utf-8 at all\x00")
+        assert models.ModelStore(path).was_offered("together:junk") is False
     print("ok  models.ini storage")
 ```
 
+
 Register it after `test_config_providers()`.
 
 - [ ] **Step 2: Run the suite to verify it fails**
 
 Run: `./test_llamachat.py`
-Expected: FAIL with `ModuleNotFoundError: No module named 'llamachat.models'`
+Expected: FAIL with `ImportError: cannot import name 'models' from 'llamachat'`
 
 - [ ] **Step 3: Write the implementation**
 
@@ -1193,16 +1262,34 @@ Create `llamachat/models.py` with the GPL header copied from `config.py`, then:
 """Per-model metadata: what presets.ini cannot answer for a cloud model."""
 
 import configparser
+import math
+import os
 from dataclasses import dataclass
 from pathlib import Path
 
+# configparser reads a literal [DEFAULT] as inherited defaults for every
+# other section. A model genuinely named DEFAULT is stored under this name
+# instead, so the file stays safe for the stock-configparser readers that
+# Task 15 invites by documenting it for hand-editing.
+#
+# ponytail: only the exact id "DEFAULT" collides. A file already containing
+# a hand-written [DEFAULT] is handled by _empty() below, not here, and no
+# other [DEFAULT]-alike is defended against. Provider ids arrive as
+# "provider:model", so a bare DEFAULT can only be a local model.
+_DEFAULT_ID = "DEFAULT"
+_DEFAULT_ESCAPED = "\x00llamachat-model-DEFAULT"
+
 # Recorded when the dialog is cancelled, so a model tried once never nags.
-SKIPPED = "configured"
+# Only the section's presence is read, never this value; the key exists to
+# give an otherwise empty section something readable in the file. It means
+# "no real keys here", and mark_skipped() is its only writer.
+SKIPPED = "offered"
 
 
 @dataclass
 class ModelInfo:
     """What a model can tell us. Every field may be unknown."""
+
     ctx_size: int | None = None
     vision: bool | None = None
     price_in: float | None = None
@@ -1220,9 +1307,18 @@ def _get(section, key, cast):
     if not raw:
         return None
     try:
-        return cast(raw)
+        value = cast(raw)
     except ValueError:
+        # A hand-edited "32k" or "3.7" for an int degrades to unknown rather
+        # than raising: the file is user-editable, and one bad line must not
+        # take down every other model's metadata with it.
         return None
+    # float() accepts "nan" and "inf", which would survive every later check
+    # and surface as a "$nan" cost. Caught here rather than at each cost call
+    # site, so the arithmetic downstream can assume a real number.
+    if cast is float and not math.isfinite(value):
+        return None
+    return value
 
 
 def _get_bool(section, key):
@@ -1242,23 +1338,51 @@ class ModelStore:
     """
 
     def __init__(self, path: Path):
-        self.path = path
+        self.path = Path(path)
         # Model ids contain colons and slashes, so no key/value delimiter
-        # may be inferred from a section name. Sections are safe as-is.
-        self.parser = configparser.ConfigParser(interpolation=None)
-        if path.exists():
+        # may be inferred from a section name. Sections are safe as-is:
+        # brackets, colons, equals and percent signs all round-trip, and
+        # interpolation is off so a "%" in an id or value is literal.
+        self.parser = self._empty()
+        if self.path.exists():
             try:
-                self.parser.read(path, encoding="utf-8")
-            except configparser.Error:
+                self.parser.read(self.path, encoding="utf-8")
+            except (configparser.Error, OSError, UnicodeDecodeError):
                 # ponytail: a corrupt file reads as empty; the dialog can
-                # rewrite it. Failing to start over metadata is worse.
-                self.parser = configparser.ConfigParser(interpolation=None)
+                # rewrite it. Failing to start over metadata is worse. The
+                # cost is that the next save silently discards whatever was
+                # readable, which is acceptable for a cache of prices.
+                self.parser = self._empty()
+
+    @staticmethod
+    def _empty():
+        # default_section is renamed away from "DEFAULT" so that a hand-added
+        # [DEFAULT] in the file is read as an ordinary section rather than as
+        # defaults inherited by every other one. Without this, one such block
+        # silently gives every model its ctx_size and prices, including
+        # models with no entry at all. Reserving a name containing a NUL,
+        # which no model id can hold, keeps every section independent.
+        return configparser.ConfigParser(
+            interpolation=None, default_section="\x00llamachat-default"
+        )
+
+    @staticmethod
+    def _key(model_id: str) -> str:
+        """The section name for a model id. Only 'DEFAULT' is rewritten."""
+        return _DEFAULT_ESCAPED if model_id == _DEFAULT_ID else model_id
 
     def get(self, model_id: str) -> ModelInfo | None:
-        """Stored metadata, or None when there is none worth having."""
-        if not self.parser.has_section(model_id):
+        """Stored metadata, or None when there is none worth having.
+
+        None and was_offered() True together mean "asked, learned nothing":
+        either the dialog was cancelled or every field was left blank. The
+        caller wants exactly that distinction, so it is not a trap: get()
+        answers "what do we know", was_offered() answers "should we ask".
+        """
+        name = self._key(model_id)
+        if not self.parser.has_section(name):
             return None
-        section = self.parser[model_id]
+        section = self.parser[name]
         info = ModelInfo(
             ctx_size=_get(section, "ctx_size", int),
             vision=_get_bool(section, "vision"),
@@ -1269,7 +1393,7 @@ class ModelStore:
 
     def was_offered(self, model_id: str) -> bool:
         """Whether the dialog has already been shown for this model."""
-        return self.parser.has_section(model_id)
+        return self.parser.has_section(self._key(model_id))
 
     def save(self, model_id: str, info: ModelInfo) -> None:
         section = self._section(model_id)
@@ -1282,30 +1406,61 @@ class ModelStore:
             if value is None:
                 section.pop(key, None)
             elif isinstance(value, bool):
+                # Before the numeric branch: bool is a subclass of int, so
+                # str(False) would otherwise write "False" for vision.
                 section[key] = "true" if value else "false"
             else:
                 section[key] = str(value)
+        # The marker means "no real keys", and mark_skipped() is its only
+        # writer. Dropping it here keeps that invariant with one enforcer
+        # instead of two that have to agree. A save with every field blank
+        # leaves an empty section, which configparser round-trips fine.
         section.pop(SKIPPED, None)
         self._write()
 
     def mark_skipped(self, model_id: str) -> None:
-        """Record a cancelled dialog: offered, declined, do not ask again."""
+        """Record a cancelled dialog: offered, declined, do not ask again.
+
+        Never clears fields already stored. Cancelling a dialog opened over
+        a model we know about must not delete what we know.
+        """
         section = self._section(model_id)
         if not any(k != SKIPPED for k in section):
-            section[SKIPPED] = "false"
+            section[SKIPPED] = "true"
         self._write()
 
     def _section(self, model_id: str):
-        if not self.parser.has_section(model_id):
-            self.parser.add_section(model_id)
-        return self.parser[model_id]
+        name = self._key(model_id)
+        if not self.parser.has_section(name):
+            self.parser.add_section(name)
+        return self.parser[name]
 
     def _write(self) -> None:
+        """Rewrite the whole file from the in-memory parser.
+
+        ponytail: last writer wins. Each store holds a full parser, so two
+        live instances would drop each other's sections rather than merge.
+        Not defended against because ipc.py enforces a single instance and
+        the window holds exactly one store, making a second writer a case
+        that cannot arise. The upgrade path, if the app ever grows a second
+        writer, is to reread before each save and merge.
+        """
         self.path.parent.mkdir(parents=True, exist_ok=True)
-        with open(self.path, "w", encoding="utf-8") as fh:
-            self.parser.write(fh)
+        # Written to a sibling and renamed: os.replace is atomic within a
+        # directory, so a crash mid-write leaves the old file intact instead
+        # of a truncated one. Cheap here, and the alternative is losing every
+        # recorded price to one bad moment.
+        tmp = self.path.with_name(self.path.name + f".{os.getpid()}.tmp")
+        try:
+            with open(tmp, "w", encoding="utf-8") as fh:
+                self.parser.write(fh)
+            os.replace(tmp, self.path)
+        except OSError:
+            tmp.unlink(missing_ok=True)
+            raise
 ```
 
+
 - [ ] **Step 4: Run the suite to verify it passes**
 
 Run: `./test_llamachat.py`