Skip to content

Commit 4a3d8ba

Browse files
alexkromanclaude
andauthored
Extract the OS keyring into core/keyring_store (#215)
`core/config.py` sat at exactly the 500-line gate with zero headroom (the next addition would break the max-file-length check). This splits the OS keyring access — the one external secret dependency — onto its natural seam. ## What changed - **New `core/keyring_store.py`** — the single importer of `keyring`, holding `KEYRING_SERVICE` plus `set_secret` / `get_secret` / `restore_secret` / `delete_secret` / `usable`. - **`core/config.py` (500 → 442)** — reads and writes secrets through that wrapper and no longer imports `keyring` directly, so the documented "secrets live in the keyring, not the dotfile" boundary is now structural. `config.keyring_usable` stays as a thin delegator so the auth-state facade (`doctor` / `login` and their tests) is unchanged. The two `clear_*` paths collapse onto the shared `delete_secret`. - Tests that reached into config's keyring internals repoint to `keyring_store` (`config.keyring` → `keyring_store.keyring`, `config.KEYRING_SERVICE` → `keyring_store.KEYRING_SERVICE`). No behavior change. Full `scripts/check.sh` gate passes — 100% patch coverage, mutation gate, CodeQL, import-linter contracts (the new module lands in the `core` layer). > Note: separated from #214 (eval/stream refactor, currently in the merge queue) so it lands independently. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018Bkx51XDcYBdnot1EF8h9U --- _Generated by [Claude Code](https://claude.ai/code/session_018Bkx51XDcYBdnot1EF8h9U)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent c3e797f commit 4a3d8ba

5 files changed

Lines changed: 126 additions & 89 deletions

File tree

aai_cli/AGENTS.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,9 @@ between layers is enforced — higher may import lower, never the reverse:
3131
- **`ui/`** — Rich rendering: `output`, `render`, `theme`, `steps`, `follow`,
3232
`help_text`, `typer_patches`, `update_check`.
3333
- **`core/`** — the Rich-free library layer: `client`, `config`,
34-
`config_builder`, `environments`, `env`, `errors`, `llm`, `telemetry`,
35-
`debuglog`, `remotefs`, `sync_stt`, `hotkey`, `ws`, `youtube`, `wer`,
36-
`argscan`, `jsonshape`, `timeparse`, `microphone`, `procs`, `stdio`,
34+
`config_builder`, `keyring_store`, `environments`, `env`, `errors`, `llm`,
35+
`telemetry`, `debuglog`, `remotefs`, `sync_stt`, `hotkey`, `ws`, `youtube`,
36+
`wer`, `argscan`, `jsonshape`, `timeparse`, `microphone`, `procs`, `stdio`,
3737
`choices`. Contract 4 also forbids `rich` here, so "no Rich below the UI
3838
layer" is structural.
3939

@@ -139,7 +139,7 @@ heavily-reworked commands with long bodies; small commands keep the inline
139139
### Cross-cutting state (resolution order matters)
140140

141141
- **`app/context.py`**`AppState` (profile, env) is attached to the Typer context in the root `@app.callback()`. `run_command` is the standard command wrapper.
142-
- **`core/config.py`** — profiles persisted in `config.toml` (via `platformdirs`); the **API key lives only in the OS keyring** (`KEYRING_SERVICE = "assemblyai-cli"`), never in a dotfile. Key resolution order: `--api-key` flag (validation paths only) → `ASSEMBLYAI_API_KEY` env → keyring. **Run commands deliberately expose no `--api-key` flag** so keys can't leak into `ps`/shell history.
142+
- **`core/config.py`** — profiles persisted in `config.toml` (via `platformdirs`); the **API key lives only in the OS keyring**, never in a dotfile. The keyring access itself is factored into **`core/keyring_store.py`** (the single importer of `keyring`, holding `KEYRING_SERVICE = "assemblyai-cli"` + `set_secret`/`get_secret`/`restore_secret`/`delete_secret`/`usable`), so the "secrets never touch the dotfile" split is structural; `config` reads/writes secrets through it and only `config.keyring_usable` re-surfaces the probe on the auth facade. Key resolution order: `--api-key` flag (validation paths only) → `ASSEMBLYAI_API_KEY` env → keyring. **Run commands deliberately expose no `--api-key` flag** so keys can't leak into `ps`/shell history.
143143
- **`core/environments.py`** — a frozen `Environment` (api_base, streaming_host, llm_gateway_base, ams_base, stytch_*). `DEFAULT_ENV` is **`production`**; use `--sandbox` (or `--env sandbox000` / `AAI_ENV`) to target the sandbox. The active environment is a process-global set once at startup; precedence: `--env``AAI_ENV` → profile's stored env → default. A credential is only valid against the environment that minted it.
144144
- **`core/client.py`** — thin wrappers over the `assemblyai` SDK (`transcribe`, `list_transcripts`, `stream_audio`, etc.). It normalizes SDK exceptions: auth failures become a single clean `auth_failure()` `CLIError`; everything else becomes `APIError`. New SDK calls should follow this try/except shape.
145145
- **`core/errors.py`** — the `CLIError` hierarchy (each with `error_type` + `exit_code`). `ui/output.py` emits errors to **stderr**; stdout stays clean for pipelines. `--json` switches to machine-readable output; it is never auto-enabled — `output.resolve_json()` deliberately keeps human text the default even when piped or agent-run.

aai_cli/core/config.py

Lines changed: 17 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,13 @@
88
import uuid
99
from pathlib import Path
1010

11-
import keyring
12-
import keyring.errors # keyring.errors is not re-exported by keyring/__init__
1311
import platformdirs
1412
import tomli_w
1513
from pydantic import BaseModel, ConfigDict, Field, ValidationError
1614

17-
from aai_cli.core import debuglog, env
15+
from aai_cli.core import debuglog, env, keyring_store
1816
from aai_cli.core.errors import CLIError, NotAuthenticated
1917

20-
KEYRING_SERVICE = "assemblyai-cli"
2118
ENV_API_KEY = "ASSEMBLYAI_API_KEY"
2219
DEFAULT_PROFILE = "default"
2320

@@ -203,80 +200,29 @@ def set_active_profile(name: str) -> None:
203200
_dump(cfg)
204201

205202

206-
def _keyring_set(username: str, secret: str) -> None:
207-
"""Write a secret to the OS keyring, turning backend failures into a clean error.
208-
209-
A locked keychain, or an existing entry whose ACL is bound to another app, makes
210-
keyring raise a KeyringError (e.g. macOS errSecInvalidOwnerEdit, -25244). Surface
211-
it as a CLIError so the command prints a fixable message instead of a traceback.
212-
"""
213-
try:
214-
keyring.set_password(KEYRING_SERVICE, username, secret)
215-
except keyring.errors.KeyringError as exc:
216-
raise CLIError(
217-
f"Your OS keyring rejected the write ({exc}).",
218-
error_type="keyring_error",
219-
suggestion=(
220-
"Unlock your keyring, or remove the stale 'assemblyai-cli' entry and "
221-
"retry (macOS: security delete-generic-password -s assemblyai-cli). "
222-
"On a headless machine without a keyring, set ASSEMBLYAI_API_KEY instead."
223-
),
224-
) from exc
225-
226-
227-
def _keyring_restore(username: str, prior: str | None) -> None:
228-
"""Best-effort restore of a keyring entry to a snapshot value, for login rollback.
229-
230-
Suppresses keyring errors (including a delete of an absent entry) so a failed
231-
rollback never masks the original write error that triggered it.
232-
"""
233-
with contextlib.suppress(keyring.errors.KeyringError):
234-
if prior is None:
235-
keyring.delete_password(KEYRING_SERVICE, username)
236-
else:
237-
keyring.set_password(KEYRING_SERVICE, username, prior)
238-
239-
240203
def set_api_key(profile: str, api_key: str) -> None:
241204
validate_profile(profile)
242-
_keyring_set(profile, api_key)
205+
keyring_store.set_secret(profile, api_key)
243206
cfg = _load()
244207
cfg.profiles.setdefault(profile, Profile())
245208
if cfg.active_profile is None:
246209
cfg.active_profile = profile
247210
_dump(cfg)
248211

249212

250-
def _keyring_get(username: str) -> str | None:
251-
"""Read a secret, treating an unusable keyring backend as "nothing stored".
252-
253-
Headless machines (containers, CI, servers) routinely have no keyring backend at
254-
all, so keyring raises NoKeyringError on every read. That state must read as "not
255-
signed in" — ASSEMBLYAI_API_KEY still works there — never as a crash.
256-
"""
257-
try:
258-
return keyring.get_password(KEYRING_SERVICE, username)
259-
except keyring.errors.KeyringError:
260-
return None
261-
262-
263213
def get_api_key(profile: str) -> str | None:
264-
return _keyring_get(profile)
214+
return keyring_store.get_secret(profile)
265215

266216

267217
def keyring_usable() -> bool:
268-
"""True when the OS keyring backend can be read.
218+
"""True when the OS keyring backend can be read (delegates to ``keyring_store``).
269219
270-
Headless boxes (containers, CI, bare SSH) often have no keyring backend, so
271-
``keyring`` raises on every access. ``assembly doctor`` uses this to tell a user with
272-
no key that the *backend* is the problem — and to recommend ASSEMBLYAI_API_KEY —
273-
rather than pointing at `assembly login`, whose browser flow also can't persist there.
220+
Kept on ``config`` as part of the auth-state facade: ``assembly doctor``/`login`
221+
call it to tell a user with no key that the *backend* is the problem — and to
222+
recommend ASSEMBLYAI_API_KEY — rather than pointing at the browser flow, which
223+
also can't persist on a keyring-less box.
274224
"""
275-
try:
276-
keyring.get_password(KEYRING_SERVICE, "__probe__")
277-
except keyring.errors.KeyringError:
278-
return False
279-
return True
225+
return keyring_store.usable()
280226

281227

282228
def get_profile_env(profile: str) -> str | None:
@@ -308,10 +254,7 @@ def set_profile_email(profile: str, email: str) -> None:
308254

309255

310256
def clear_api_key(profile: str) -> None:
311-
# KeyringError, not just PasswordDeleteError: with no backend at all (headless
312-
# boxes) delete raises NoKeyringError, and "nothing stored" is already the goal.
313-
with contextlib.suppress(keyring.errors.KeyringError):
314-
keyring.delete_password(KEYRING_SERVICE, profile)
257+
keyring_store.delete_secret(profile)
315258

316259

317260
SESSION_KEYRING_PREFIX = "session" # keyring username: f"{prefix}:{profile}"
@@ -328,7 +271,7 @@ def set_session(profile: str, *, session_jwt: str, session_token: str, account_i
328271
key. The JWT is short-lived; an expired session surfaces as NotAuthenticated.
329272
"""
330273
validate_profile(profile)
331-
_keyring_set(
274+
keyring_store.set_secret(
332275
_session_username(profile),
333276
StoredSession(jwt=session_jwt, token=session_token).model_dump_json(),
334277
)
@@ -339,7 +282,7 @@ def set_session(profile: str, *, session_jwt: str, session_token: str, account_i
339282

340283
def get_session(profile: str) -> dict[str, str] | None:
341284
"""The stored {'jwt', 'token'} for a profile, or None if absent/corrupt."""
342-
raw = _keyring_get(_session_username(profile))
285+
raw = keyring_store.get_secret(_session_username(profile))
343286
if not raw:
344287
return None
345288
try:
@@ -356,8 +299,7 @@ def get_account_id(profile: str) -> int | None:
356299

357300

358301
def clear_session(profile: str) -> None:
359-
with contextlib.suppress(keyring.errors.KeyringError):
360-
keyring.delete_password(KEYRING_SERVICE, _session_username(profile))
302+
keyring_store.delete_secret(_session_username(profile))
361303
cfg = _load()
362304
prof = cfg.profiles.get(profile)
363305
if prof and prof.account_id is not None:
@@ -385,8 +327,8 @@ def persist_login(
385327
restored best-effort.
386328
"""
387329
validate_profile(profile)
388-
prior_api_key = _keyring_get(profile)
389-
prior_session = _keyring_get(_session_username(profile))
330+
prior_api_key = keyring_store.get_secret(profile)
331+
prior_session = keyring_store.get_secret(_session_username(profile))
390332
prior_cfg = _load()
391333
done = False
392334
try:
@@ -404,8 +346,8 @@ def persist_login(
404346
done = True
405347
finally:
406348
if not done:
407-
_keyring_restore(profile, prior_api_key)
408-
_keyring_restore(_session_username(profile), prior_session)
349+
keyring_store.restore_secret(profile, prior_api_key)
350+
keyring_store.restore_secret(_session_username(profile), prior_session)
409351
_dump(prior_cfg)
410352

411353

aai_cli/core/keyring_store.py

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
"""The OS keyring as the CLI's secret store.
2+
3+
config.toml holds only non-secret profile settings; every secret — the API key and
4+
the browser-login session blob — lives in the OS keyring instead, keyed under
5+
``KEYRING_SERVICE``. This module is the single place that imports ``keyring``, so the
6+
"secrets never touch the dotfile" boundary is structural: ``config`` reads and writes
7+
them through this typed wrapper and never opens the keyring directly.
8+
9+
Every backend call is wrapped because a keyring is routinely unusable — a locked
10+
keychain, an ACL-bound entry, or (on headless boxes) no backend at all — and those
11+
must surface as a clean ``CLIError`` or read as "nothing stored", never a traceback.
12+
"""
13+
14+
from __future__ import annotations
15+
16+
import contextlib
17+
18+
import keyring
19+
import keyring.errors # keyring.errors is not re-exported by keyring/__init__
20+
21+
from aai_cli.core.errors import CLIError
22+
23+
KEYRING_SERVICE = "assemblyai-cli"
24+
25+
26+
def set_secret(username: str, secret: str) -> None:
27+
"""Write a secret to the OS keyring, turning backend failures into a clean error.
28+
29+
A locked keychain, or an existing entry whose ACL is bound to another app, makes
30+
keyring raise a KeyringError (e.g. macOS errSecInvalidOwnerEdit, -25244). Surface
31+
it as a CLIError so the command prints a fixable message instead of a traceback.
32+
"""
33+
try:
34+
keyring.set_password(KEYRING_SERVICE, username, secret)
35+
except keyring.errors.KeyringError as exc:
36+
raise CLIError(
37+
f"Your OS keyring rejected the write ({exc}).",
38+
error_type="keyring_error",
39+
suggestion=(
40+
"Unlock your keyring, or remove the stale 'assemblyai-cli' entry and "
41+
"retry (macOS: security delete-generic-password -s assemblyai-cli). "
42+
"On a headless machine without a keyring, set ASSEMBLYAI_API_KEY instead."
43+
),
44+
) from exc
45+
46+
47+
def get_secret(username: str) -> str | None:
48+
"""Read a secret, treating an unusable keyring backend as "nothing stored".
49+
50+
Headless machines (containers, CI, servers) routinely have no keyring backend at
51+
all, so keyring raises NoKeyringError on every read. That state must read as "not
52+
signed in" — ASSEMBLYAI_API_KEY still works there — never as a crash.
53+
"""
54+
try:
55+
return keyring.get_password(KEYRING_SERVICE, username)
56+
except keyring.errors.KeyringError:
57+
return None
58+
59+
60+
def restore_secret(username: str, prior: str | None) -> None:
61+
"""Best-effort restore of a keyring entry to a snapshot value, for login rollback.
62+
63+
Suppresses keyring errors (including a delete of an absent entry) so a failed
64+
rollback never masks the original write error that triggered it.
65+
"""
66+
with contextlib.suppress(keyring.errors.KeyringError):
67+
if prior is None:
68+
keyring.delete_password(KEYRING_SERVICE, username)
69+
else:
70+
keyring.set_password(KEYRING_SERVICE, username, prior)
71+
72+
73+
def delete_secret(username: str) -> None:
74+
"""Delete a keyring entry, treating an absent entry or missing backend as success.
75+
76+
KeyringError, not just PasswordDeleteError: with no backend at all (headless
77+
boxes) delete raises NoKeyringError, and "nothing stored" is already the goal.
78+
"""
79+
with contextlib.suppress(keyring.errors.KeyringError):
80+
keyring.delete_password(KEYRING_SERVICE, username)
81+
82+
83+
def usable() -> bool:
84+
"""True when the OS keyring backend can be read.
85+
86+
Headless boxes (containers, CI, bare SSH) often have no keyring backend, so
87+
``keyring`` raises on every access. ``assembly doctor`` uses this to tell a user with
88+
no key that the *backend* is the problem — and to recommend ASSEMBLYAI_API_KEY —
89+
rather than pointing at `assembly login`, whose browser flow also can't persist there.
90+
"""
91+
try:
92+
keyring.get_password(KEYRING_SERVICE, "__probe__")
93+
except keyring.errors.KeyringError:
94+
return False
95+
return True

tests/test_config.py

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import pytest
44

5-
from aai_cli.core import config
5+
from aai_cli.core import config, keyring_store
66
from aai_cli.core.errors import CLIError, NotAuthenticated
77

88

@@ -29,8 +29,8 @@ def test_resolve_falls_back_to_keyring():
2929

3030

3131
def test_get_session_rejects_non_string_jwt():
32-
config.keyring.set_password(
33-
config.KEYRING_SERVICE,
32+
keyring_store.keyring.set_password(
33+
keyring_store.KEYRING_SERVICE,
3434
config._session_username("default"),
3535
json.dumps({"jwt": 123, "token": "tok"}),
3636
)
@@ -66,7 +66,7 @@ def test_set_api_key_keyring_failure_raises_clean_error(monkeypatch):
6666
def boom(*_a, **_k):
6767
raise keyring.errors.PasswordSetError("denied by keychain")
6868

69-
monkeypatch.setattr(config.keyring, "set_password", boom)
69+
monkeypatch.setattr(keyring_store.keyring, "set_password", boom)
7070
with pytest.raises(CLIError) as exc:
7171
config.set_api_key("default", "sk_abc")
7272
assert "keyring" in exc.value.message.lower()
@@ -84,7 +84,7 @@ def test_keyring_usable_false_when_backend_raises(monkeypatch):
8484
def boom(*_a, **_k):
8585
raise keyring.errors.NoKeyringError("no backend on this headless box")
8686

87-
monkeypatch.setattr(config.keyring, "get_password", boom)
87+
monkeypatch.setattr(keyring_store.keyring, "get_password", boom)
8888
assert config.keyring_usable() is False
8989

9090

@@ -94,7 +94,7 @@ def test_set_session_keyring_failure_raises_clean_error(monkeypatch):
9494
def boom(*_a, **_k):
9595
raise keyring.errors.PasswordSetError("denied by keychain")
9696

97-
monkeypatch.setattr(config.keyring, "set_password", boom)
97+
monkeypatch.setattr(keyring_store.keyring, "set_password", boom)
9898
with pytest.raises(CLIError) as exc:
9999
config.set_session("default", session_jwt="j", session_token="t", account_id=1)
100100
assert "keyring" in exc.value.message.lower()
@@ -123,14 +123,14 @@ def _fail_on_session_write(monkeypatch):
123123
"""
124124
import keyring.errors
125125

126-
real_set = config.keyring.set_password
126+
real_set = keyring_store.keyring.set_password
127127

128128
def selective(service, username, secret):
129129
if username.startswith(config.SESSION_KEYRING_PREFIX + ":"):
130130
raise keyring.errors.PasswordSetError("denied by keychain")
131131
return real_set(service, username, secret)
132132

133-
monkeypatch.setattr(config.keyring, "set_password", selective)
133+
monkeypatch.setattr(keyring_store.keyring, "set_password", selective)
134134

135135

136136
def test_persist_login_rolls_back_to_empty_when_session_write_fails(monkeypatch):

tests/test_config_session.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
from aai_cli.core import config
1+
from aai_cli.core import config, keyring_store
22

33

44
def test_set_and_get_session_roundtrips():
@@ -26,5 +26,5 @@ def test_clear_session_is_safe_when_absent():
2626
def test_get_session_returns_none_for_corrupt_blob():
2727
import keyring
2828

29-
keyring.set_password(config.KEYRING_SERVICE, "session:default", "not-json")
29+
keyring.set_password(keyring_store.KEYRING_SERVICE, "session:default", "not-json")
3030
assert config.get_session("default") is None

0 commit comments

Comments
 (0)