Skip to content

Commit ffd22ea

Browse files
NikitasT2003claude
andcommitted
fix(setup): disclose where an existing API key came from
When the wizard found an API key, it said "found existing X. Use it?" without naming the source. A user whose shell had OPENROUTER_API_KEY exported thought the wizard was hallucinating a key — they had no such entry in .env and didn't remember exporting one. Trust damaged, and there was no clear path to fix it short of reading the code. Changes - _mask_key(): shows prefix + last 4 chars only, hides the middle, so the user can recognise which key the wizard latched onto without the key being visible in output. - _resolve_api_key(): names the source explicitly — "from shell environment" or "from the .env file in this workspace" — and shows the masked preview every time an existing key is found. - When the shell has the key exported AND the user says "no, use a different one", warn that shell env takes precedence over .env at runtime and show the exact command to unset it (POSIX + PowerShell variants). - Non-interactive / quickstart paths also print the source now so CI logs are legible. Tests (11 new, 264 total) - TestMaskKey: prefix preservation, last-4 exposure, middle redaction, short-key / empty-key handling. - TestKeySourceAttribution: shell-only case, .env-only case, both-present precedence (shell wins), no-key-anywhere case, shell-overrides warning presence + unset command presence. - TestNonInteractiveReuse: shell + .env reuse messages both name the source and show the masked preview. All 253 tests pass (1 skipped for Windows symlink rights). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 9860ba8 commit ffd22ea

2 files changed

Lines changed: 305 additions & 6 deletions

File tree

src/thesis_agent/cli.py

Lines changed: 63 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,24 @@ def _resolve_provider(
395395
)
396396

397397

398+
def _mask_key(key: str) -> str:
399+
"""Show the shape of a key without leaking it.
400+
401+
Preserves the recognisable prefix (e.g. `sk-or-`, `sk-ant-`) and the last
402+
4 characters, hiding the middle. `sk-or-abc…wxyz`. For unusually short
403+
keys we just print the length.
404+
"""
405+
k = (key or "").strip()
406+
if not k:
407+
return "<empty>"
408+
if len(k) < 12:
409+
return f"<{len(k)} chars>"
410+
# Prefix runs up to the second dash if present, otherwise first 6 chars.
411+
prefix_end = k.find("-", k.find("-") + 1)
412+
prefix = k[: prefix_end + 1] if 0 < prefix_end < 15 else k[:6]
413+
return f"{prefix}{k[-4:]}"
414+
415+
398416
def _resolve_api_key(
399417
*,
400418
info: dict,
@@ -406,6 +424,8 @@ def _resolve_api_key(
406424
"""Return a key, or None if the user chose to skip.
407425
408426
Precedence: --api-key flag > shell env > .env > interactive prompt (with retry).
427+
Always tells the user *where* an existing key came from, and shows a
428+
masked preview so they can recognise it before reusing.
409429
"""
410430
import questionary
411431

@@ -414,26 +434,63 @@ def _resolve_api_key(
414434

415435
shell_key = (os.environ.get(info["key_env"]) or "").strip() or None
416436
env_key = _read_env_var(info["key_env"])
417-
existing = shell_key or env_key
437+
438+
# Source attribution: shell env wins at runtime (it overrides .env when
439+
# the agent boots via python-dotenv), so honour that precedence here too.
440+
if shell_key:
441+
existing = shell_key
442+
source = "shell environment"
443+
hint = (
444+
"Your shell has this variable exported (likely from your profile "
445+
"or a parent process). It will override any value in .env."
446+
)
447+
elif env_key:
448+
existing = env_key
449+
source = "the .env file in this workspace"
450+
hint = ""
451+
else:
452+
existing = None
453+
source = ""
454+
hint = ""
418455

419456
if existing and not force:
420-
# Non-interactive or quickstart: reuse silently.
457+
preview = _mask_key(existing)
458+
# Non-interactive or quickstart: still announce the source + preview.
421459
if non_interactive or quickstart:
422-
console.print(f" [dim]reusing existing {info['key_env']} from shell/.env[/]")
460+
console.print(
461+
f" [dim]reusing {info['key_env']} [cyan]{preview}[/] "
462+
f"from {source}[/]"
463+
)
423464
return existing
465+
466+
console.print(f" found {info['key_env']} [cyan]{preview}[/] [dim](from {source})[/]")
467+
if hint:
468+
console.print(f" [dim]{hint}[/]")
424469
try:
425470
use_existing = _ask(
426471
questionary.confirm,
427-
f"Found an existing {info['key_env']}. Use it?",
472+
"Use this key?",
428473
default=True,
429474
)
430475
except _Cancelled:
431476
raise
432477
if use_existing:
433478
console.print(" [green]reusing existing key.[/]")
434479
return existing
435-
# Clear transition so the user sees the flow moved on.
436-
console.print(" [dim]ok — enter a new key.[/]\n")
480+
# User said No. Warn them about shell-env precedence before taking a new key.
481+
if shell_key:
482+
console.print(
483+
" [yellow]note:[/] your shell env has "
484+
f"[cyan]{info['key_env']}[/] exported. Any new key you paste "
485+
"will be written to [cyan].env[/], but the shell env will "
486+
"still take precedence until you unset it."
487+
)
488+
console.print(
489+
f" to unset: [cyan]unset {info['key_env']}[/] "
490+
f"(bash/zsh) or [cyan]Remove-Item Env:{info['key_env']}[/] (PowerShell)\n"
491+
)
492+
else:
493+
console.print(" [dim]ok — enter a new key.[/]\n")
437494

438495
if non_interactive:
439496
console.print(f"[red]no {info['key_env']} provided (required for --non-interactive)[/]")

tests/test_wizard_key_source.py

Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
"""Wizard must be honest about where an existing key came from.
2+
3+
Context: a user reported the wizard finding an OpenRouter key they were sure
4+
they hadn't set. Investigation showed their shell had `OPENROUTER_API_KEY`
5+
exported from a parent process. The wizard was correct to detect it, but its
6+
output did not distinguish shell env from `.env`, making the key look like a
7+
phantom. These tests lock in the fix: the wizard names the source and shows
8+
a masked preview.
9+
"""
10+
11+
from __future__ import annotations
12+
13+
import sys
14+
from pathlib import Path
15+
from typing import Any
16+
17+
import pytest
18+
from typer.testing import CliRunner
19+
20+
from thesis_agent import cli
21+
22+
runner = CliRunner()
23+
24+
25+
class _Prompt:
26+
def __init__(self, answers: list[Any]):
27+
self._answers = answers
28+
29+
def ask(self):
30+
if not self._answers:
31+
raise AssertionError("script exhausted")
32+
a = self._answers.pop(0)
33+
if isinstance(a, BaseException):
34+
raise a
35+
return a
36+
37+
38+
class _FakeChoice:
39+
def __init__(self, label: str, value: Any = None):
40+
self.title = label
41+
self.value = value if value is not None else label
42+
43+
def __eq__(self, other):
44+
return isinstance(other, _FakeChoice) and self.value == other.value
45+
46+
def __hash__(self):
47+
return hash(self.value)
48+
49+
50+
class _FakeQuestionary:
51+
Choice = _FakeChoice
52+
53+
def __init__(self, script: list[Any]):
54+
self._script = script
55+
56+
def _n(self):
57+
return _Prompt([self._script.pop(0)])
58+
59+
def select(self, *a, **k):
60+
return self._n()
61+
62+
def confirm(self, *a, **k):
63+
return self._n()
64+
65+
def password(self, *a, **k):
66+
return self._n()
67+
68+
def text(self, *a, **k):
69+
return self._n()
70+
71+
72+
@pytest.fixture
73+
def ws(tmp_path: Path, monkeypatch):
74+
monkeypatch.chdir(tmp_path)
75+
for v in (
76+
"THESIS_PROVIDER",
77+
"ANTHROPIC_API_KEY",
78+
"OPENROUTER_API_KEY",
79+
"OPENROUTER_BASE_URL",
80+
"OPENROUTER_SITE_URL",
81+
"OPENROUTER_SITE_NAME",
82+
):
83+
monkeypatch.delenv(v, raising=False)
84+
monkeypatch.setattr(cli, "_validate_key", lambda *a, **kw: (True, "ok"))
85+
monkeypatch.setattr(cli, "_copy_examples", lambda: 0)
86+
return tmp_path
87+
88+
89+
def _install_fake_q(monkeypatch, script: list[Any]):
90+
monkeypatch.setitem(sys.modules, "questionary", _FakeQuestionary(script))
91+
92+
93+
# ---------------------------------------------------------------------------
94+
# _mask_key
95+
# ---------------------------------------------------------------------------
96+
97+
class TestMaskKey:
98+
def test_openrouter_key_preserves_prefix_and_last_four(self):
99+
out = cli._mask_key("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")
100+
assert out.startswith("sk-or-")
101+
assert out.endswith("c264")
102+
assert "…" in out
103+
# Middle must be masked — the long hex body should not appear
104+
assert "71b768574b63558d" not in out
105+
106+
def test_anthropic_key_preserves_prefix(self):
107+
out = cli._mask_key("sk-ant-abcdefghijklmnop")
108+
assert out.startswith("sk-ant-")
109+
assert out.endswith("mnop")
110+
assert "abcdefghij" not in out
111+
112+
def test_very_short_key_only_reports_length(self):
113+
out = cli._mask_key("xyz")
114+
assert "xyz" not in out
115+
assert "chars" in out
116+
117+
def test_empty_key(self):
118+
assert cli._mask_key("") == "<empty>"
119+
120+
121+
# ---------------------------------------------------------------------------
122+
# Source attribution: shell env vs .env
123+
# ---------------------------------------------------------------------------
124+
125+
class TestKeySourceAttribution:
126+
def test_key_from_shell_env_is_labelled(self, ws: Path, monkeypatch):
127+
"""When shell env has the key, wizard must say 'shell environment'
128+
so the user knows where the phantom key came from."""
129+
monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-v1-from-shell-xxxx9999")
130+
_install_fake_q(monkeypatch, [
131+
"openrouter", # provider
132+
True, # use existing? → yes
133+
False, # add attribution headers? → no (bool confirm)
134+
True, # create workspace?
135+
False, # copy examples?
136+
])
137+
result = runner.invoke(cli.app, ["setup"])
138+
assert result.exit_code == 0, result.stdout
139+
assert "shell environment" in result.stdout
140+
# Masked preview must show — last 4 chars visible, middle hidden
141+
assert "9999" in result.stdout
142+
assert "v1-from-shell" not in result.stdout # body masked
143+
144+
def test_key_from_env_file_is_labelled(self, ws: Path, monkeypatch):
145+
"""When only .env has the key (no shell env), wizard must say '.env'."""
146+
(ws / ".env").write_text(
147+
"THESIS_PROVIDER=openrouter\nOPENROUTER_API_KEY=sk-or-v1-in-dotenv-1234\n",
148+
encoding="utf-8",
149+
)
150+
_install_fake_q(monkeypatch, [
151+
"openrouter",
152+
True, # use existing
153+
False, # attribution headers
154+
True, # create ws
155+
False, # examples
156+
])
157+
result = runner.invoke(cli.app, ["setup"])
158+
assert result.exit_code == 0, result.stdout
159+
assert ".env" in result.stdout
160+
assert "shell environment" not in result.stdout
161+
assert "1234" in result.stdout
162+
assert "in-dotenv" not in result.stdout
163+
164+
def test_warns_when_shell_overrides_dotenv(self, ws: Path, monkeypatch):
165+
"""Critical UX case: user says 'no, use a different key'. If shell
166+
still has the old one exported, it will *win* at runtime. Wizard
167+
must warn + show how to unset."""
168+
monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-v1-stale-aaaa1234")
169+
_install_fake_q(monkeypatch, [
170+
"openrouter",
171+
False, # don't reuse — enter fresh
172+
"sk-or-v1-fresh-brand-new-bbbb5678", # password
173+
False, # attribution headers
174+
True, False,
175+
])
176+
result = runner.invoke(cli.app, ["setup"])
177+
assert result.exit_code == 0, result.stdout
178+
# The warning must name the shell + the env var + show how to unset.
179+
assert "shell env" in result.stdout.lower()
180+
assert "precedence" in result.stdout.lower() or "override" in result.stdout.lower()
181+
assert "unset OPENROUTER_API_KEY" in result.stdout or "Remove-Item" in result.stdout
182+
183+
def test_no_key_anywhere_shows_no_source(self, ws: Path, monkeypatch):
184+
_install_fake_q(monkeypatch, [
185+
"anthropic",
186+
"sk-ant-fresh",
187+
True, False,
188+
])
189+
result = runner.invoke(cli.app, ["setup"])
190+
assert result.exit_code == 0, result.stdout
191+
assert "shell environment" not in result.stdout
192+
assert "found ANTHROPIC_API_KEY" not in result.stdout
193+
194+
def test_shell_env_takes_precedence_over_dotenv(self, ws: Path, monkeypatch):
195+
"""When both are present, shell wins (matches python-dotenv runtime)."""
196+
(ws / ".env").write_text(
197+
"THESIS_PROVIDER=openrouter\nOPENROUTER_API_KEY=sk-or-v1-in-env-aaaa\n",
198+
encoding="utf-8",
199+
)
200+
monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-v1-shell-zzzz1111")
201+
_install_fake_q(monkeypatch, [
202+
"openrouter",
203+
True, False,
204+
True, False,
205+
])
206+
result = runner.invoke(cli.app, ["setup"])
207+
assert result.exit_code == 0, result.stdout
208+
# Source must be shell (not .env)
209+
assert "shell environment" in result.stdout
210+
assert "1111" in result.stdout # shell key's preview wins
211+
assert "aaaa" not in result.stdout # .env key hidden
212+
213+
214+
# ---------------------------------------------------------------------------
215+
# Non-interactive mode still reuses silently but names the source
216+
# ---------------------------------------------------------------------------
217+
218+
class TestNonInteractiveReuse:
219+
def test_reuse_message_names_shell_env(self, ws: Path, monkeypatch):
220+
monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-v1-ci-env-yyyy9999")
221+
result = runner.invoke(cli.app, [
222+
"setup", "--non-interactive",
223+
"--provider", "openrouter",
224+
"--skip-validation", "--skip-examples",
225+
])
226+
assert result.exit_code == 0, result.stdout
227+
assert "shell environment" in result.stdout
228+
assert "9999" in result.stdout # masked preview
229+
230+
def test_reuse_message_names_env_file(self, ws: Path, monkeypatch):
231+
(ws / ".env").write_text(
232+
"THESIS_PROVIDER=openrouter\nOPENROUTER_API_KEY=sk-or-v1-persisted-wwww0000\n",
233+
encoding="utf-8",
234+
)
235+
result = runner.invoke(cli.app, [
236+
"setup", "--non-interactive",
237+
"--provider", "openrouter",
238+
"--skip-validation", "--skip-examples",
239+
])
240+
assert result.exit_code == 0, result.stdout
241+
assert ".env file" in result.stdout or "env file" in result.stdout.lower()
242+
assert "0000" in result.stdout

0 commit comments

Comments
 (0)