fix: sharpen key resolution errors, rename KeyResolutionError

Raise KEY_TIMEOUT to 120s: a graphical pinentry plus a hardware token the
user has to find and touch makes 30s a plausible successful unlock, and
failing one converts a slow success into an error the user can only retry
against the same clock.

Point the timeout message at a pinentry that never appeared, which is the
silent failure, rather than at one the user is already looking at. Name
the binary when pass is missing instead of reporting a bare Errno 2.

Raise from None so a partial secret on the failed process stdout stays
out of printed tracebacks, and record on the cache that it is unlocked
only because every caller resolves on the GUI thread today.

Rename KeyError_ to KeyResolutionError before later tasks import it.

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

3 files changed, 257 insertions, 43 deletions

diff --git a/docs/superpowers/plans/2026-08-09-external-providers.md b/docs/superpowers/plans/2026-08-09-external-providers.md
index 72db023..d34fffc 100644
--- a/docs/superpowers/plans/2026-08-09-external-providers.md
+++ b/docs/superpowers/plans/2026-08-09-external-providers.md
@@ -188,11 +188,14 @@ from dataclasses import dataclass, field
 # prefix, so an existing session pointing at a local model still resolves.
 LOCAL = "local"
 
-# A stuck pinentry must not freeze the worker thread forever.
-KEY_TIMEOUT = 30
+# A stuck pinentry must not freeze the worker thread forever. Generous
+# because a legitimate unlock is slow: a graphical pinentry plus a hardware
+# token the user has to physically find and touch is a normal minute. This
+# bounds the pathological case, it is not a deadline for the human.
+KEY_TIMEOUT = 120
 
 
-class KeyError_(Exception):
+class KeyResolutionError(Exception):
     """An api_key that could not be resolved, phrased for the user."""
 
 
@@ -530,7 +533,7 @@ def test_key_resolution():
     try:
         resolver.resolve(missing)
         assert False, "a missing env var must raise"
-    except providers.KeyError_ as exc:
+    except providers.KeyResolutionError as exc:
         assert "gone" in str(exc)
 
     # pass: shells out. Substitute the runner rather than requiring gpg.
@@ -553,6 +556,17 @@ def test_key_resolution():
     assert cached.resolve(passed) == "line-one"
     assert len(calls) == 1
 
+    # But the cache follows the spec, not just the name. A reloaded config
+    # that points the same provider at a different entry must re-resolve,
+    # otherwise correcting a wrong entry appears to do nothing.
+    moved = providers.Provider(
+        name="together", base_url="http://x.example.org",
+        api_key="pass:api/together-corrected",
+    )
+    assert cached.resolve(moved) == "line-one"
+    assert len(calls) == 2
+    assert calls[1][0] == ["pass", "show", "api/together-corrected"]
+
     # A failing pass is reported, naming the provider.
     def boom(cmd, timeout):
         raise OSError("pass: entry not found")
@@ -560,14 +574,69 @@ def test_key_resolution():
     try:
         providers.KeyResolver(runner=boom).resolve(passed)
         assert False, "a failing pass must raise"
-    except providers.KeyError_ as exc:
+    except providers.KeyResolutionError as exc:
+        assert "together" in str(exc)
+
+    # A timeout points at the pinentry never appearing, which is the silent
+    # case, rather than at one the user can already see.
+    def slow(cmd, timeout):
+        raise subprocess.TimeoutExpired(cmd, timeout, output="partial-secret")
+
+    try:
+        providers.KeyResolver(runner=slow).resolve(passed)
+        assert False, "a pass timeout must raise"
+    except providers.KeyResolutionError as exc:
+        assert "gpg-agent" in str(exc)
+        # The partial stdout a timeout captures must never reach the message.
+        assert "partial-secret" not in str(exc)
+        # `from None` suppresses the chained traceback. It does not clear
+        # __context__, so this pins the display behavior, not unreachability.
+        assert exc.__suppress_context__ and exc.__cause__ is None
+
+    # A non-zero exit quotes gpg's stderr, which is the only useful part, and
+    # never stdout, which is where the secret would be.
+    def refused(cmd, timeout):
+        raise subprocess.CalledProcessError(
+            2, cmd, output="sk-test-not-a-real-key\n",
+            stderr="gpg: decryption failed: No secret key\n",
+        )
+
+    try:
+        providers.KeyResolver(runner=refused).resolve(passed)
+        assert False, "a non-zero pass exit must raise"
+    except providers.KeyResolutionError as exc:
+        assert "No secret key" in str(exc)
+        assert "sk-test-not-a-real-key" not in str(exc)
+
+    # An empty or absent stderr falls back to the exit status rather than
+    # reporting a blank reason.
+    for blank in ("", None):
+        def quiet(cmd, timeout, _s=blank):
+            raise subprocess.CalledProcessError(3, cmd, output="", stderr=_s)
+
+        try:
+            providers.KeyResolver(runner=quiet).resolve(passed)
+            assert False, "a non-zero pass exit must raise"
+        except providers.KeyResolutionError as exc:
+            assert "exit status 3" in str(exc)
+            assert exc.__suppress_context__ and exc.__cause__ is None
+
+    # A missing `pass` binary names the binary, not just "No such file".
+    def absent(cmd, timeout):
+        raise FileNotFoundError(2, "No such file or directory", "pass")
+
+    try:
+        providers.KeyResolver(runner=absent).resolve(passed)
+        assert False, "a missing pass binary must raise"
+    except providers.KeyResolutionError as exc:
+        assert "not installed" in str(exc)
         assert "together" in str(exc)
 
     # Empty output is a failure too: an empty key would 401 confusingly.
     try:
         providers.KeyResolver(runner=lambda cmd, timeout: "  \n").resolve(passed)
         assert False, "empty pass output must raise"
-    except providers.KeyError_:
+    except providers.KeyResolutionError:
         pass
     print("ok  api key resolution")
 ```
@@ -602,20 +671,34 @@ class KeyResolver:
 
     def __init__(self, runner=_run_pass):
         self._runner = runner
-        self._cache: dict[str, str] = {}
+        # Not locked, and that is only safe while every caller resolves on the
+        # GUI thread, where the Qt event loop serializes them. Moving a
+        # resolve onto a worker thread, or threading the model fan-out, breaks
+        # it: two threads can miss the cache together, spawn two `pass`
+        # processes and raise two pinentry prompts for one token. Whoever does
+        # that adds a threading.Lock here and holds it across the whole
+        # resolve() body, not just the dict access, since guarding only the
+        # dict leaves both threads outside the lock during the subprocess,
+        # which is the double-prompt window itself.
+        self._cache: dict[tuple[str, str], str] = {}
 
     def resolve(self, provider: Provider) -> str:
         """The bearer token for this provider, or '' when it needs none."""
         spec = provider.api_key
         if not spec:
             return ""
-        if provider.name in self._cache:
-            return self._cache[provider.name]
+        # Keyed by the spec as well as the name: the config can be reloaded
+        # onto the same resolver, and keying by name alone would keep serving
+        # the old key after the user edits the entry to fix it, so the fix
+        # would look like it did nothing until a restart.
+        cache_key = (provider.name, spec)
+        if cache_key in self._cache:
+            return self._cache[cache_key]
 
         if spec.startswith("env:"):
             value = os.environ.get(spec[4:], "")
             if not value:
-                raise KeyError_(
+                raise KeyResolutionError(
                     f"{provider.name}: environment variable {spec[4:]} is not set"
                 )
         elif spec.startswith("pass:"):
@@ -623,24 +706,53 @@ class KeyResolver:
         else:
             value = spec
 
-        self._cache[provider.name] = value
+        self._cache[cache_key] = value
         return value
 
     def _from_pass(self, provider: Provider, entry: str) -> str:
         try:
             out = self._runner(["pass", "show", entry], timeout=KEY_TIMEOUT)
+        # Every raise below is `from None`. Both TimeoutExpired and
+        # CalledProcessError carry a .stdout that can hold a partial secret,
+        # and this keeps it out of any printed traceback. Note it suppresses
+        # display only: __context__ still references the original, so the
+        # real guarantee is that no handler here puts stdout in the message.
         except subprocess.TimeoutExpired:
-            raise KeyError_(
+            # The likely cause is a pinentry that never appeared, not one
+            # sitting in front of the user: no $DISPLAY inherited, no
+            # gpg-agent, or pinentry-qt failing to open and falling back to a
+            # curses prompt on a terminal a GUI app has no stdin for.
+            raise KeyResolutionError(
                 f"{provider.name}: `pass show {entry}` timed out after "
-                f"{KEY_TIMEOUT}s. Is a pinentry waiting for input?"
-            )
+                f"{KEY_TIMEOUT}s. If no pinentry appeared, check that "
+                f"gpg-agent is running and that pinentry-qt can open a window."
+            ) from None
+        except FileNotFoundError:
+            # Errno 2 alone does not say which file is missing, and this is
+            # the common "pass was never installed" case.
+            raise KeyResolutionError(
+                f"{provider.name}: `pass` is not installed or not on PATH"
+            ) from None
+        except subprocess.CalledProcessError as exc:
+            # stderr is captured, so without this it is swallowed and the user
+            # gets an exit code where gpg told them exactly what was wrong.
+            # Only stderr is quoted, never stdout, which holds the secret.
+            detail = (exc.stderr or "").strip().splitlines()
+            reason = detail[-1] if detail else f"exit status {exc.returncode}"
+            raise KeyResolutionError(
+                f"{provider.name}: `pass show {entry}` failed: {reason}"
+            ) from None
         except Exception as exc:
-            raise KeyError_(f"{provider.name}: `pass show {entry}` failed: {exc}")
+            raise KeyResolutionError(
+                f"{provider.name}: `pass show {entry}` failed: {exc}"
+            ) from None
         # A password store entry keeps the secret on the first line and
         # metadata below it.
         first = (out or "").strip().splitlines()
         if not first or not first[0].strip():
-            raise KeyError_(f"{provider.name}: `pass show {entry}` returned nothing")
+            raise KeyResolutionError(
+                f"{provider.name}: `pass show {entry}` returned nothing"
+            )
         return first[0].strip()
 ```
 
@@ -1641,7 +1753,7 @@ class MultiClient:
         for name, provider in self.table.items():
             try:
                 available = self._listing_client(provider).models()
-            except (BackendError, providers_mod.KeyError_) as exc:
+            except (BackendError, providers_mod.KeyResolutionError) as exc:
                 problems.append(f"{name}: {exc}")
                 continue
             kept = providers_mod.apply_filter(provider, available)
@@ -2129,29 +2241,57 @@ In `_ensure_vision_model` (line 842), replace the opening check:
 Every call that currently does `self.client.<method>(model, ...)` must become a
 call on the routed client with the wire name. In `send()` (line 919) and
 `_start_stream()` (line 1040), the model passed to `StreamWorker` must be split.
-Change `_start_stream` to resolve both up front:
+
+**Resolution must happen on the worker thread, not in `_start_stream`.**
+`client_for()` resolves the API key, and for a `pass:` key that runs `pass show`,
+which blocks on a pinentry for as long as it takes the user to find and touch
+their hardware token. `_start_stream` runs on the GUI thread, before
+`worker.moveToThread(thread)` and before `thread.start()`, so resolving there
+freezes the whole window for that entire interaction, up to `KEY_TIMEOUT`
+(120s). Pass the unresolved `model` into the worker and let the worker resolve.
+
+Give `StreamWorker` the `MultiClient` and the qualified `model` it already
+takes, and do the routing at the top of `StreamWorker.run()`, inside the
+existing try that already reports failures through the `failed` signal:
 
 ```python
-    def _start_stream(self, model: str, messages: list[dict]) -> None:
+    def run(self) -> None:
         try:
-            client = self.client.client_for(model)
-        except providers_mod.KeyError_ as exc:
-            self.show_status(str(exc), error=True)
+            client = self.client.client_for(self.model)
+            wire = self.client.wire_name(self.model)
+        except providers_mod.KeyResolutionError as exc:
+            self.failed.emit(str(exc))
             return
-        wire = self.client.wire_name(model)
+        ...
 ```
 
-and pass `client` and `wire` to `StreamWorker` in place of `self.client` and
-`model`. Add the import at the top of `ui.py`:
+`failed` is already connected to a slot that calls `show_status(..., error=True)`,
+so a key error reaches the user through the path a backend error already uses,
+and it arrives as a normal queued signal on the GUI thread. Do not add a new
+signal for it. Add the import at the top of `ui.py`:
 
 ```python
 from . import providers as providers_mod
 ```
 
-Apply the same treatment in `_start_titling` (line 1133), which builds a
-`TitleWorker`: resolve `client` and `wire` the same way, and on `KeyError_` skip
-titling silently rather than showing an error, since titling is never the user's
-own turn.
+Apply the same treatment to `TitleWorker` for `_start_titling` (line 1133):
+resolve inside `TitleWorker.run()`, and on `KeyResolutionError` return without
+emitting anything, so titling is skipped silently. Titling is never the user's
+own turn, so a pinentry prompt or an error banner for it would be noise. This
+preserves the current silent-skip behavior, it just moves where it happens.
+
+**This changes the threading invariant in `providers.py`.** `KeyResolver._cache`
+is deliberately unlocked, and the comment on it says so, because at the time of
+writing every caller resolved on the GUI thread and the Qt event loop serialized
+them. Once resolution moves into `StreamWorker.run()` and `TitleWorker.run()`
+that no longer holds: `_start_titling` can overlap with a live stream, so two
+worker threads can call `resolve()` concurrently, miss the cache together, and
+spawn two `pass` processes, raising two pinentry prompts for one token. When
+executing this step, re-examine that comment and add a `threading.Lock` to
+`KeyResolver` if the overlap is real. The lock must be held across the whole
+`resolve()` body, not just the dict reads and writes: guarding only the dict
+still leaves both threads outside the lock during the subprocess call, which is
+exactly the double-prompt window. Update the `_cache` comment either way.
 
 - [ ] **Step 7: Verify the whole suite still passes**
 
diff --git a/llamachat/providers.py b/llamachat/providers.py
index 051a931..2c459e9 100644
--- a/llamachat/providers.py
+++ b/llamachat/providers.py
@@ -21,11 +21,14 @@ from dataclasses import dataclass, field
 # prefix, so an existing session pointing at a local model still resolves.
 LOCAL = "local"
 
-# A stuck pinentry must not freeze the worker thread forever.
-KEY_TIMEOUT = 30
+# A stuck pinentry must not freeze the worker thread forever. Generous
+# because a legitimate unlock is slow: a graphical pinentry plus a hardware
+# token the user has to physically find and touch is a normal minute. This
+# bounds the pathological case, it is not a deadline for the human.
+KEY_TIMEOUT = 120
 
 
-class KeyError_(Exception):
+class KeyResolutionError(Exception):
     """An api_key that could not be resolved, phrased for the user."""
 
 
@@ -175,6 +178,15 @@ class KeyResolver:
 
     def __init__(self, runner=_run_pass):
         self._runner = runner
+        # Not locked, and that is only safe while every caller resolves on the
+        # GUI thread, where the Qt event loop serializes them. Moving a
+        # resolve onto a worker thread, or threading the model fan-out, breaks
+        # it: two threads can miss the cache together, spawn two `pass`
+        # processes and raise two pinentry prompts for one token. Whoever does
+        # that adds a threading.Lock here and holds it across the whole
+        # resolve() body, not just the dict access, since guarding only the
+        # dict leaves both threads outside the lock during the subprocess,
+        # which is the double-prompt window itself.
         self._cache: dict[tuple[str, str], str] = {}
 
     def resolve(self, provider: Provider) -> str:
@@ -193,7 +205,7 @@ class KeyResolver:
         if spec.startswith("env:"):
             value = os.environ.get(spec[4:], "")
             if not value:
-                raise KeyError_(
+                raise KeyResolutionError(
                     f"{provider.name}: environment variable {spec[4:]} is not set"
                 )
         elif spec.startswith("pass:"):
@@ -207,23 +219,45 @@ class KeyResolver:
     def _from_pass(self, provider: Provider, entry: str) -> str:
         try:
             out = self._runner(["pass", "show", entry], timeout=KEY_TIMEOUT)
+        # Every raise below is `from None`. Both TimeoutExpired and
+        # CalledProcessError carry a .stdout that can hold a partial secret,
+        # and this keeps it out of any printed traceback. Note it suppresses
+        # display only: __context__ still references the original, so the
+        # real guarantee is that no handler here puts stdout in the message.
         except subprocess.TimeoutExpired:
-            raise KeyError_(
+            # The likely cause is a pinentry that never appeared, not one
+            # sitting in front of the user: no $DISPLAY inherited, no
+            # gpg-agent, or pinentry-qt failing to open and falling back to a
+            # curses prompt on a terminal a GUI app has no stdin for.
+            raise KeyResolutionError(
                 f"{provider.name}: `pass show {entry}` timed out after "
-                f"{KEY_TIMEOUT}s. Is a pinentry waiting for input?"
-            )
+                f"{KEY_TIMEOUT}s. If no pinentry appeared, check that "
+                f"gpg-agent is running and that pinentry-qt can open a window."
+            ) from None
+        except FileNotFoundError:
+            # Errno 2 alone does not say which file is missing, and this is
+            # the common "pass was never installed" case.
+            raise KeyResolutionError(
+                f"{provider.name}: `pass` is not installed or not on PATH"
+            ) from None
         except subprocess.CalledProcessError as exc:
             # stderr is captured, so without this it is swallowed and the user
             # gets an exit code where gpg told them exactly what was wrong.
             # Only stderr is quoted, never stdout, which holds the secret.
             detail = (exc.stderr or "").strip().splitlines()
             reason = detail[-1] if detail else f"exit status {exc.returncode}"
-            raise KeyError_(f"{provider.name}: `pass show {entry}` failed: {reason}")
+            raise KeyResolutionError(
+                f"{provider.name}: `pass show {entry}` failed: {reason}"
+            ) from None
         except Exception as exc:
-            raise KeyError_(f"{provider.name}: `pass show {entry}` failed: {exc}")
+            raise KeyResolutionError(
+                f"{provider.name}: `pass show {entry}` failed: {exc}"
+            ) from None
         # A password store entry keeps the secret on the first line and
         # metadata below it.
         first = (out or "").strip().splitlines()
         if not first or not first[0].strip():
-            raise KeyError_(f"{provider.name}: `pass show {entry}` returned nothing")
+            raise KeyResolutionError(
+                f"{provider.name}: `pass show {entry}` returned nothing"
+            )
         return first[0].strip()
diff --git a/test_llamachat.py b/test_llamachat.py
index 08f31c9..5ee2310 100755
--- a/test_llamachat.py
+++ b/test_llamachat.py
@@ -1098,7 +1098,7 @@ def test_key_resolution():
     try:
         resolver.resolve(missing)
         assert False, "a missing env var must raise"
-    except providers.KeyError_ as exc:
+    except providers.KeyResolutionError as exc:
         assert "gone" in str(exc)
 
     # pass: shells out. Substitute the runner rather than requiring gpg.
@@ -1139,9 +1139,25 @@ def test_key_resolution():
     try:
         providers.KeyResolver(runner=boom).resolve(passed)
         assert False, "a failing pass must raise"
-    except providers.KeyError_ as exc:
+    except providers.KeyResolutionError as exc:
         assert "together" in str(exc)
 
+    # A timeout points at the pinentry never appearing, which is the silent
+    # case, rather than at one the user can already see.
+    def slow(cmd, timeout):
+        raise subprocess.TimeoutExpired(cmd, timeout, output="partial-secret")
+
+    try:
+        providers.KeyResolver(runner=slow).resolve(passed)
+        assert False, "a pass timeout must raise"
+    except providers.KeyResolutionError as exc:
+        assert "gpg-agent" in str(exc)
+        # The partial stdout a timeout captures must never reach the message.
+        assert "partial-secret" not in str(exc)
+        # `from None` suppresses the chained traceback. It does not clear
+        # __context__, so this pins the display behavior, not unreachability.
+        assert exc.__suppress_context__ and exc.__cause__ is None
+
     # A non-zero exit quotes gpg's stderr, which is the only useful part, and
     # never stdout, which is where the secret would be.
     def refused(cmd, timeout):
@@ -1153,15 +1169,39 @@ def test_key_resolution():
     try:
         providers.KeyResolver(runner=refused).resolve(passed)
         assert False, "a non-zero pass exit must raise"
-    except providers.KeyError_ as exc:
+    except providers.KeyResolutionError as exc:
         assert "No secret key" in str(exc)
         assert "sk-test-not-a-real-key" not in str(exc)
 
+    # An empty or absent stderr falls back to the exit status rather than
+    # reporting a blank reason.
+    for blank in ("", None):
+        def quiet(cmd, timeout, _s=blank):
+            raise subprocess.CalledProcessError(3, cmd, output="", stderr=_s)
+
+        try:
+            providers.KeyResolver(runner=quiet).resolve(passed)
+            assert False, "a non-zero pass exit must raise"
+        except providers.KeyResolutionError as exc:
+            assert "exit status 3" in str(exc)
+            assert exc.__suppress_context__ and exc.__cause__ is None
+
+    # A missing `pass` binary names the binary, not just "No such file".
+    def absent(cmd, timeout):
+        raise FileNotFoundError(2, "No such file or directory", "pass")
+
+    try:
+        providers.KeyResolver(runner=absent).resolve(passed)
+        assert False, "a missing pass binary must raise"
+    except providers.KeyResolutionError as exc:
+        assert "not installed" in str(exc)
+        assert "together" in str(exc)
+
     # Empty output is a failure too: an empty key would 401 confusingly.
     try:
         providers.KeyResolver(runner=lambda cmd, timeout: "  \n").resolve(passed)
         assert False, "empty pass output must raise"
-    except providers.KeyError_:
+    except providers.KeyResolutionError:
         pass
     print("ok  api key resolution")