Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions surfaces/cli/llm_auth/providers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
from dataclasses import dataclass
from typing import Literal

from surfaces.cli.wizard.config import PROVIDER_BY_VALUE, SUPPORTED_PROVIDERS, ProviderOption
from surfaces.cli.wizard.config import (
PROVIDER_BY_VALUE,
SUPPORTED_PROVIDERS,
ProviderOption,
WizardCredentialKind,
)

AuthKind = Literal["api_key", "cli_subscription"]

Expand Down Expand Up @@ -70,7 +75,7 @@ def all_names(self) -> tuple[str, ...]:
def _api_key_profiles() -> tuple[ProviderAuthProfile, ...]:
profiles: list[ProviderAuthProfile] = []
for provider in SUPPORTED_PROVIDERS:
if provider.credential_kind != "api_key":
if provider.credential_kind != WizardCredentialKind.API_KEY:
continue
profiles.append(
ProviderAuthProfile(
Expand Down
6 changes: 3 additions & 3 deletions surfaces/cli/llm_auth/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
provider_for_profile,
resolve_auth_profile,
)
from surfaces.cli.wizard.config import PROVIDER_BY_VALUE, ProviderOption
from surfaces.cli.wizard.config import PROVIDER_BY_VALUE, ProviderOption, WizardCredentialKind
from surfaces.cli.wizard.env_sync import sync_provider_env
from surfaces.cli.wizard.validation import validate_provider_credentials

Expand Down Expand Up @@ -88,7 +88,7 @@ def configure_api_key_provider(
) -> AuthSetupResult:
"""Validate and persist an API-key provider credential."""
provider = provider_for_profile(profile)
if provider.credential_kind != "api_key" or not provider.api_key_env:
if provider.credential_kind != WizardCredentialKind.API_KEY or not provider.api_key_env:
raise AuthSetupError(f"{provider.label} does not use an OpenSRE-managed API key.")

normalized_key = api_key.strip()
Expand Down Expand Up @@ -165,7 +165,7 @@ def configure_cli_subscription_provider(
) -> AuthSetupResult:
"""Configure a CLI-backed subscription provider such as ChatGPT/Codex or Claude Code."""
provider = provider_for_profile(profile)
if provider.credential_kind != "cli" or provider.adapter_factory is None:
if provider.credential_kind != WizardCredentialKind.CLI or provider.adapter_factory is None:
raise AuthSetupError(f"{provider.label} is not a CLI-backed subscription provider.")

adapter = provider.adapter_factory()
Expand Down
10 changes: 7 additions & 3 deletions surfaces/cli/wizard/_ui.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
WARNING,
)
from surfaces.cli.llm_auth.persist import AuthSetupError, persist_api_key_secret
from surfaces.cli.wizard.config import PROVIDER_BY_VALUE, ProviderOption
from surfaces.cli.wizard.config import PROVIDER_BY_VALUE, ProviderOption, WizardCredentialKind
from surfaces.cli.wizard.integration_health import IntegrationHealthResult
from surfaces.cli.wizard.probes import ProbeResult
from surfaces.cli.wizard.prompts import select as select_prompt
Expand Down Expand Up @@ -121,8 +121,12 @@ def _local_defaults() -> dict[str, str | bool | None]:
api_key_env = _string_value(
local.get("api_key_env"), api_key_provider.api_key_env if api_key_provider else ""
)
is_cli = bool(raw_provider_option and raw_provider_option.credential_kind == "cli")
is_host = bool(api_key_provider and api_key_provider.credential_kind == "host")
is_cli = bool(
raw_provider_option and raw_provider_option.credential_kind == WizardCredentialKind.CLI
)
is_host = bool(
api_key_provider and api_key_provider.credential_kind == WizardCredentialKind.HOST
)
is_oauth_backend = bool(raw_provider_value and raw_provider_value != provider_value)
raw_auth_method = local.get("auth_method")
auth_method = (
Expand Down
4 changes: 2 additions & 2 deletions surfaces/cli/wizard/env_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
write_env_lines,
)
from config.llm_auth.auth_method import LLM_AUTH_METHOD_ENV
from surfaces.cli.wizard.config import ProviderOption
from surfaces.cli.wizard.config import ProviderOption, WizardCredentialKind


def sync_reasoning_model_env(
Expand Down Expand Up @@ -162,7 +162,7 @@ def sync_provider_env(
# A ``host`` credential (e.g. the Ollama host) is non-secret runtime config
# that the wizard persists to ``.env`` — keep it as an active key so this
# sync does not strip it back out in the same wizard run.
if provider.credential_kind == "host" and provider.api_key_env:
if provider.credential_kind == WizardCredentialKind.HOST and provider.api_key_env:
active_non_secret.add(provider.api_key_env)
keys_to_remove -= active_non_secret

Expand Down
30 changes: 22 additions & 8 deletions surfaces/cli/wizard/flow.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,12 @@
from surfaces.cli.wizard.azure_openai import (
choose_provider_model,
)
from surfaces.cli.wizard.config import PROVIDER_BY_VALUE, SUPPORTED_PROVIDERS, ProviderOption
from surfaces.cli.wizard.config import (
PROVIDER_BY_VALUE,
SUPPORTED_PROVIDERS,
ProviderOption,
WizardCredentialKind,
)
from surfaces.cli.wizard.configurators.github import (
DEFAULT_GITHUB_MCP_MODE,
DEFAULT_GITHUB_MCP_URL,
Expand All @@ -71,6 +76,7 @@
from surfaces.cli.wizard.integration_health import IntegrationHealthResult
from surfaces.cli.wizard.llm_credential import (
CANCEL,
DEFERRED,
OK,
REPICK,
UNSAVED,
Expand Down Expand Up @@ -804,8 +810,8 @@ def run_wizard(_argv: list[str] | None = None) -> int:

if change_provider:
if auth_method == API_KEY_AUTH_METHOD and provider.credential_kind not in (
"cli",
"none",
WizardCredentialKind.CLI,
WizardCredentialKind.NONE,
):
credential_outcome, model = _prompt_validated_llm_credential(
provider,
Expand All @@ -818,7 +824,11 @@ def run_wizard(_argv: list[str] | None = None) -> int:
if credential_outcome == REPICK:
force_repick = True
continue
if credential_outcome == UNVERIFIED:
if credential_outcome == DEFERRED:
# No key to hand: finish onboarding with the provider chosen
# and nothing persisted, rather than ending the wizard.
credential_state = DEFERRED
Comment thread
greptile-apps[bot] marked this conversation as resolved.
elif credential_outcome == UNVERIFIED:
credential_state = UNVERIFIED
elif credential_outcome == UNSAVED:
credential_state = UNSAVED
Expand All @@ -836,16 +846,20 @@ def run_wizard(_argv: list[str] | None = None) -> int:
os.environ.update(azure_env)
else:
if auth_method == API_KEY_AUTH_METHOD and provider.credential_kind not in (
"cli",
"none",
WizardCredentialKind.CLI,
WizardCredentialKind.NONE,
):
has_api_key = bool(defaults["has_api_key"])
legacy_api_key = str(defaults["legacy_api_key"] or "").strip()
# A ``host`` credential (e.g. the Ollama host) is not a secret api key: never
# migrate a stale legacy ``api_key`` value into it — that would leak a
# secret-shaped value into .env and point the runtime at a bogus host. Fall
# through to the host prompt instead.
if not has_api_key and legacy_api_key and provider.credential_kind != "host":
if (
not has_api_key
and legacy_api_key
and provider.credential_kind != WizardCredentialKind.HOST
):
migration_outcome = _persist_llm_credential_with_recovery(
provider, legacy_api_key, session_env_sink=session_env_sink
)
Expand Down Expand Up @@ -885,7 +899,7 @@ def run_wizard(_argv: list[str] | None = None) -> int:
provider_extra_env = azure_env
os.environ.update(azure_env)

if model_provider.credential_kind == "cli":
if model_provider.credential_kind == WizardCredentialKind.CLI:
cli_out = _run_cli_llm_onboarding(
model_provider,
display_label=(
Expand Down
33 changes: 23 additions & 10 deletions surfaces/cli/wizard/llm_credential.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
from surfaces.cli.wizard.azure_openai import (
choose_provider_model,
)
from surfaces.cli.wizard.config import PROJECT_ENV_PATH, ProviderOption
from surfaces.cli.wizard.config import PROJECT_ENV_PATH, ProviderOption, WizardCredentialKind
from surfaces.cli.wizard.endpoint_prompt import (
ensure_endpoint_settings as ensure_provider_endpoint_settings,
)
Expand All @@ -51,8 +51,8 @@
#: What became of the credential the wizard just collected. ``unsaved`` outranks
#: ``unverified``: a credential that never landed anywhere is the more urgent thing
#: to say on the summary screen.
CredentialState = Literal["ok", "unverified", "unsaved"]
CredentialOutcome = Literal["ok", "unverified", "unsaved", "repick", "cancel"]
CredentialState = Literal["ok", "unverified", "unsaved", "deferred"]
CredentialOutcome = Literal["ok", "unverified", "unsaved", "deferred", "repick", "cancel"]

# Recovery/outcome vocabulary. These are the byte-identical string values the wizard
# menus, ``_choose`` defaults, and ``run_wizard`` branches all resolve against; named
Expand All @@ -68,6 +68,10 @@
OK: Final = "ok"
UNSAVED: Final = "unsaved"
UNVERIFIED: Final = "unverified"
#: The user has no key to hand. Onboarding finishes; the key is added later with
#: ``opensre auth login <provider>`` or the provider env var. One value for both
#: the prompt outcome and the recorded state, as with ``unsaved``/``unverified``.
DEFERRED: Final = "deferred"

_LLM_CREDENTIAL_MAX_ATTEMPTS = 10 # mirrors _run_cli_llm_onboarding's retry budget

Expand Down Expand Up @@ -113,14 +117,16 @@ def _credential_line_for_saved_summary(
if provider.value == "openai":
return "OpenAI OAuth tokens (Codex CLI)"
return f"{_provider_choice_label(provider)} OAuth session"
if provider.credential_kind == "host":
if provider.credential_kind == WizardCredentialKind.HOST:
# A ``host`` credential (e.g. the Ollama host URL) is written to the project
# ``.env`` by ``_persist_llm_credential``, never the keyring — the summary must
# name .env, not the system keychain, in both the verified and unverified cases.
if credential_state == UNVERIFIED:
return "project .env (unverified)"
return "project .env"
if provider.credential_kind != "cli":
if provider.credential_kind != WizardCredentialKind.CLI:
if credential_state == DEFERRED:
return f"not set yet — run `opensre auth login {provider.value}` when you have a key"
if credential_state == UNSAVED:
return "not saved — re-enter next run"
if credential_state == UNVERIFIED:
Expand All @@ -135,7 +141,7 @@ def _credential_line_for_saved_summary(
def _persist_llm_credential(provider: ProviderOption, value: str) -> bool:
"""Persist one prompted credential where the runtime will actually read it.

``credential_kind == "host"`` values (e.g. the Ollama host URL) are plain
``credential_kind == WizardCredentialKind.HOST`` values (e.g. the Ollama host URL) are plain
runtime configuration, not secrets: the runtime resolves them from the
environment only, never the keyring, so they belong in the project ``.env``.
Everything else keeps the keyring path.
Expand All @@ -145,7 +151,7 @@ def _persist_llm_credential(provider: ProviderOption, value: str) -> bool:
(permission denied, read-only fs, full disk) fails soft exactly like a keyring
failure — it never propagates and crashes onboarding (#3591).
"""
if provider.credential_kind == "host":
if provider.credential_kind == WizardCredentialKind.HOST:
try:
sync_env_values({provider.api_key_env: value})
except OSError as exc:
Expand Down Expand Up @@ -249,7 +255,7 @@ def _persist_llm_credential_with_recovery(
why this is also correct at the legacy-key migration site, where there is no
prompt to return to.

Reachable for ``credential_kind == "host"`` only when the ``.env`` write fails:
Reachable for ``credential_kind == WizardCredentialKind.HOST`` only when the ``.env`` write fails:
``_persist_llm_credential`` returns ``False`` on an ``OSError`` write error, so the
host lands on the same recovery menu the keyring path uses (#3591). A successful
host write returns ``True`` and never reaches this menu.
Expand Down Expand Up @@ -353,15 +359,22 @@ def _prompt_validated_llm_credential(
for _attempt in range(_LLM_CREDENTIAL_MAX_ATTEMPTS):
try:
value = _prompt_value(
f"{credential_display} ({env_key})",
f"{credential_display} ({env_key}) — leave blank to set up later",
# Only a ``host`` credential may be pre-filled. _prompt_value returns the
# default on empty input, and a secret provider's credential_default is a
# placeholder (Azure's is an endpoint URL) — offering it would let a bare
# Enter persist a URL as the API key.
default=provider.credential_default if provider.credential_kind == "host" else "",
default=provider.credential_default
if provider.credential_kind == WizardCredentialKind.HOST
else "",
secret=provider.credential_secret,
# A blank answer is "I do not have this yet", not a mistake to
# re-prompt: this was the only step that could end onboarding.
allow_empty=provider.credential_kind != WizardCredentialKind.HOST,
back_on_cancel=True,
)
if not value:
return DEFERRED, model
except WizardBack: # must precede KeyboardInterrupt: WizardBack subclasses it
return REPICK, model
except KeyboardInterrupt:
Expand Down
75 changes: 71 additions & 4 deletions tests/cli/wizard/test_flow.py
Original file line number Diff line number Diff line change
Expand Up @@ -4479,15 +4479,20 @@ def _save_api_key(provider, value, **_kwargs):
def test_run_wizard_azure_empty_key_input_never_persists_the_endpoint_placeholder(
monkeypatch, tmp_path
) -> None:
"""Enter on an empty Azure key prompt must NOT save the endpoint placeholder as the key."""
"""Enter on an empty Azure key prompt must NOT save the endpoint placeholder.

Azure's ``credential_default`` is an endpoint URL, so a pre-filled prompt
would let a bare Enter persist it as the API key. A blank answer now defers
setup instead of re-prompting, and must still persist nothing.
"""
monkeypatch.delenv("AZURE_OPENAI_BASE_URL", raising=False)
monkeypatch.delenv("AZURE_OPENAI_API_VERSION", raising=False)

placeholder = flow.PROVIDER_BY_VALUE["azure-openai"].credential_default
assert placeholder == "https://your-resource.openai.azure.com" # sanity: the trap exists

password_calls: list[dict[str, object]] = []
password_asks = iter(["", "az-real-key"]) # bare Enter, then a real key
password_asks = iter([""]) # bare Enter: no key to hand
validated_keys: list[str] = []
saved_llm_keys: list[tuple[str, str]] = []

Expand Down Expand Up @@ -4545,8 +4550,9 @@ def _validate(*, provider, api_key, model):
assert all(call.get("default") == "" for call in password_calls)
assert placeholder not in validated_keys
assert all(value != placeholder for _provider, value in saved_llm_keys)
assert validated_keys == ["az-real-key"]
assert saved_llm_keys == [("azure-openai", "az-real-key")]
# Deferred: nothing validated, nothing persisted, wizard still completes.
assert validated_keys == []
assert saved_llm_keys == []


def test_run_wizard_ollama_host_prompt_keeps_its_localhost_default(monkeypatch, tmp_path) -> None:
Expand Down Expand Up @@ -5035,3 +5041,64 @@ def _capture_choose(_prompt, _choices, default=None, **_kwargs):

# Assert
assert seen["default"] == "focused"


def test_run_wizard_blank_llm_key_defers_setup_instead_of_ending(monkeypatch, tmp_path) -> None:
"""A blank key finishes onboarding; it must not cancel or loop forever.

The credential prompt was the one step with no "later": its outcomes were
repick, cancel, save-anyway and continue-unsaved. A user without a key to
hand had to abandon the wizard.
"""
# Arrange
saved_llm_keys: list[tuple[str, str]] = []
validator_calls: list[tuple[str, str]] = []
password_asks = 0

def _mock_select(*_args, **_kwargs):
prompt = str(_args[0]) if _args else ""
m = MagicMock()
if "Choose your LLM provider" in prompt:
m.ask.return_value = "openai"
elif "auth method" in prompt:
m.ask.return_value = "api_key"
elif "model" in prompt:
m.ask.return_value = "gpt-5.4-mini"
elif "integration" in prompt.lower():
m.ask.return_value = "skip"
else:
m.ask.return_value = "quickstart"
return m

def _mock_password(*_args, **_kwargs):
nonlocal password_asks
password_asks += 1
m = MagicMock()
m.ask.return_value = "" # the user has no key to hand
return m

def _validate(*, provider, api_key, model):
validator_calls.append((provider.value, api_key))
return ValidationResult(ok=True, detail="unexpected")

monkeypatch.setattr(_ui, "select_prompt", _mock_select)
monkeypatch.setattr(flow.questionary, "password", _mock_password)
monkeypatch.setattr(llm_credential, "validate_provider_credentials", _validate)
monkeypatch.setattr(_ui, "get_store_path", lambda: tmp_path / "opensre.json")
monkeypatch.setattr(flow, "probe_local_target", lambda _path: ProbeResult("local", True, "ok"))
monkeypatch.setattr(flow, "save_local_config", lambda **_kwargs: tmp_path / "opensre.json")
monkeypatch.setattr(flow, "sync_provider_env", lambda **_kwargs: tmp_path / ".env")
monkeypatch.setattr(
_ui,
"save_api_key",
lambda provider, value, **_kwargs: _stub_save_recording(saved_llm_keys, provider, value),
)

# Act
exit_code = flow.run_wizard()

# Assert
assert exit_code == 0, "a deferred key must not fail onboarding"
assert saved_llm_keys == [], "nothing may be persisted for a blank key"
assert validator_calls == [], "a blank key must not be sent to the provider"
assert password_asks == 1, "the prompt must accept the blank answer, not re-ask"