Merge PR #52: support OPENAI_BASE_URL in env overrides; fix resolve_auth key priority

- Add OPENAI_BASE_URL to env override chain - Fix resolve_auth() to check provider-specific env var before flat api_key - Infer provider=openai when api_format=openai - Prefix bare version models (e.g. '5.4' -> 'gpt-5.4') for OpenAI providers - Fix 4 pre-existing test env var leaks - Preserved main's credential_slot logic while adopting new env-var-first priority Co-authored-by: siaochuan <siaochuan@users.noreply.github.com>
2026-04-07 12:07:57 +00:00
parent be1f7161ea 07b528b5e9
commit dfaa4b26f7
2 changed files with 109 additions and 22 deletions
@@ -249,8 +249,12 @@ def resolve_model_setting(
            return _CLAUDE_ALIAS_TARGETS[normalized]
        return normalize_anthropic_model_name(configured)

-    if provider in {"openai", "openai_codex", "copilot"} and normalized in {"default", "best"}:
-        return "gpt-5.4"
+    if provider in {"openai", "openai_codex", "copilot"}:
+        if normalized in {"default", "best"}:
+            return "gpt-5.4"
+        # Bare version numbers like "5.4" → "gpt-5.4"
+        if normalized and normalized[0].isdigit():
+            return f"gpt-{configured}"

    return configured

@@ -457,6 +461,11 @@ class Settings(BaseModel):
        profile_name, profile = self.resolve_profile()
        next_provider = (self.provider or "").strip() or profile.provider
        next_api_format = (self.api_format or "").strip() or profile.api_format
+        # When api_format switches to "openai" but provider is still the
+        # default "anthropic", infer provider as "openai" so model resolution
+        # and other provider-dependent logic uses the correct path.
+        if next_api_format == "openai" and next_provider == "anthropic":
+            next_provider = "openai"
        next_base_url = self.base_url if self.base_url is not None else profile.base_url
        flat_model = (self.model or "").strip()
        resolved_profile_model = resolve_model_setting(
@@ -576,6 +585,31 @@ class Settings(BaseModel):
            )

        storage_provider = auth_source_provider_name(auth_source)
+
+        # Look up the provider-specific environment variable first.  The flat
+        # ``self.api_key`` field is a legacy single-slot value that usually
+        # holds an Anthropic key.  When the active profile points at a
+        # *different* provider (e.g. ``openai_api_key``), blindly returning
+        # ``self.api_key`` sends the wrong credential to the wrong backend.
+        # Checking the env var first ensures the correct key is used when the
+        # user has both ANTHROPIC_API_KEY and OPENAI_API_KEY configured.
+        env_var = {
+            "anthropic_api_key": "ANTHROPIC_API_KEY",
+            "openai_api_key": "OPENAI_API_KEY",
+            "dashscope_api_key": "DASHSCOPE_API_KEY",
+        }.get(auth_source)
+        if env_var:
+            env_value = os.environ.get(env_var, "")
+            if env_value:
+                return ResolvedAuth(
+                    provider=provider or storage_provider,
+                    auth_kind="api_key",
+                    value=env_value,
+                    source=f"env:{env_var}",
+                    state="configured",
+                )
+
+        # Fall back to the flat api_key field (settings.json / --api-key).
        explicit_key = "" if profile.credential_slot else self.api_key
        if explicit_key:
            return ResolvedAuth(
@@ -600,22 +634,6 @@ class Settings(BaseModel):
                    state="configured",
                )

-        env_var = {
-            "anthropic_api_key": "ANTHROPIC_API_KEY",
-            "openai_api_key": "OPENAI_API_KEY",
-            "dashscope_api_key": "DASHSCOPE_API_KEY",
-        }.get(auth_source)
-        if env_var:
-            env_value = os.environ.get(env_var, "")
-            if env_value:
-                return ResolvedAuth(
-                    provider=provider or storage_provider,
-                    auth_kind="api_key",
-                    value=env_value,
-                    source=f"env:{env_var}",
-                    state="configured",
-                )
-
        stored = load_credential(storage_provider, "api_key")
        if stored:
            return ResolvedAuth(
@@ -650,7 +668,11 @@ def _apply_env_overrides(settings: Settings) -> Settings:
    if model:
        updates["model"] = model

-    base_url = os.environ.get("ANTHROPIC_BASE_URL") or os.environ.get("OPENHARNESS_BASE_URL")
+    base_url = (
+        os.environ.get("ANTHROPIC_BASE_URL")
+        or os.environ.get("OPENAI_BASE_URL")
+        or os.environ.get("OPENHARNESS_BASE_URL")
+    )
    if base_url:
        updates["base_url"] = base_url

@@ -46,6 +46,7 @@ class TestSettings:

    def test_resolve_api_key_missing_raises(self, monkeypatch):
        monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False)
+        monkeypatch.delenv("OPENAI_API_KEY", raising=False)
        s = Settings()
        with pytest.raises(ValueError, match="No API key found"):
            s.resolve_api_key()
@@ -64,14 +65,72 @@ class TestSettings:
        assert s.model != updated.model
        assert s is not updated

+    def test_resolve_auth_prefers_env_over_flat_api_key_for_openai(self, monkeypatch):
+        """When api_format=openai, resolve_auth() should use OPENAI_API_KEY
+        from the environment rather than the flat api_key field which may
+        contain an Anthropic key from settings.json."""
+        monkeypatch.setenv("OPENAI_API_KEY", "sk-openai-correct")
+        monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False)
+        s = Settings(api_key="sk-ant-wrong-provider", api_format="openai")
+        s = s.sync_active_profile_from_flat_fields()
+        auth = s.resolve_auth()
+        assert auth.value == "sk-openai-correct"
+        assert "OPENAI" in auth.source
+
+    def test_resolve_auth_falls_back_to_flat_api_key(self, monkeypatch):
+        """When no provider-specific env var is set, resolve_auth() should
+        still fall back to the flat api_key field."""
+        monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False)
+        monkeypatch.delenv("OPENAI_API_KEY", raising=False)
+        s = Settings(api_key="sk-fallback-key")
+        s = s.sync_active_profile_from_flat_fields()
+        auth = s.resolve_auth()
+        assert auth.value == "sk-fallback-key"
+
+    def test_env_overrides_picks_up_openai_base_url(self, tmp_path: Path, monkeypatch):
+        """_apply_env_overrides should pick up OPENAI_BASE_URL for relay
+        providers that use OpenAI-compatible format."""
+        monkeypatch.delenv("ANTHROPIC_BASE_URL", raising=False)
+        monkeypatch.delenv("OPENHARNESS_BASE_URL", raising=False)
+        monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False)
+        monkeypatch.setenv("OPENAI_BASE_URL", "https://relay.example.com/v1")
+        monkeypatch.setenv("OPENAI_API_KEY", "sk-relay-key")
+        path = tmp_path / "settings.json"
+        path.write_text(json.dumps({}))
+        s = load_settings(path)
+        assert s.base_url == "https://relay.example.com/v1"
+
+    def test_anthropic_base_url_takes_precedence_over_openai(self, tmp_path: Path, monkeypatch):
+        """ANTHROPIC_BASE_URL should take precedence over OPENAI_BASE_URL."""
+        monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False)
+        monkeypatch.setenv("ANTHROPIC_BASE_URL", "https://anthropic-relay.example.com")
+        monkeypatch.setenv("OPENAI_BASE_URL", "https://openai-relay.example.com/v1")
+        path = tmp_path / "settings.json"
+        path.write_text(json.dumps({}))
+        s = load_settings(path)
+        assert s.base_url == "https://anthropic-relay.example.com"
+

 class TestLoadSaveSettings:
-    def test_load_missing_file_returns_defaults(self, tmp_path: Path):
+    def test_load_missing_file_returns_defaults(self, tmp_path: Path, monkeypatch):
+        monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False)
+        monkeypatch.delenv("OPENAI_API_KEY", raising=False)
+        monkeypatch.delenv("ANTHROPIC_BASE_URL", raising=False)
+        monkeypatch.delenv("OPENAI_BASE_URL", raising=False)
+        monkeypatch.delenv("OPENHARNESS_BASE_URL", raising=False)
+        monkeypatch.delenv("ANTHROPIC_MODEL", raising=False)
+        monkeypatch.delenv("OPENHARNESS_MODEL", raising=False)
        path = tmp_path / "nonexistent.json"
        s = load_settings(path)
        assert s == Settings().materialize_active_profile()

-    def test_load_existing_file(self, tmp_path: Path):
+    def test_load_existing_file(self, tmp_path: Path, monkeypatch):
+        monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False)
+        monkeypatch.delenv("OPENAI_API_KEY", raising=False)
+        monkeypatch.delenv("ANTHROPIC_BASE_URL", raising=False)
+        monkeypatch.delenv("OPENAI_BASE_URL", raising=False)
+        monkeypatch.delenv("ANTHROPIC_MODEL", raising=False)
+        monkeypatch.delenv("OPENHARNESS_MODEL", raising=False)
        path = tmp_path / "settings.json"
        path.write_text(json.dumps({"model": "claude-opus-4-20250514", "verbose": True, "fast_mode": True}))
        s = load_settings(path)
@@ -80,7 +139,13 @@ class TestLoadSaveSettings:
        assert s.fast_mode is True
        assert s.api_key == ""  # default preserved

-    def test_save_and_load_roundtrip(self, tmp_path: Path):
+    def test_save_and_load_roundtrip(self, tmp_path: Path, monkeypatch):
+        monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False)
+        monkeypatch.delenv("OPENAI_API_KEY", raising=False)
+        monkeypatch.delenv("ANTHROPIC_BASE_URL", raising=False)
+        monkeypatch.delenv("OPENAI_BASE_URL", raising=False)
+        monkeypatch.delenv("ANTHROPIC_MODEL", raising=False)
+        monkeypatch.delenv("OPENHARNESS_MODEL", raising=False)
        path = tmp_path / "settings.json"
        original = Settings(api_key="sk-roundtrip", model="claude-opus-4-20250514", verbose=True)
        save_settings(original, path)