feat(settings): add verification.critic_api_key with fallback to LLM key (#3494)

Co-authored-by: openhands <openhands@all-hands.dev>

Author: xingyaoww

openhands-sdk/openhands/sdk/settings/model.py

@@ -48,6 +48,7 @@
     decrypt_str_with_cipher_or_keep,
     resolve_expose_mode,
     serialize_secret,
+    validate_secret,
     validate_secret_dict,
 )
 from openhands.sdk.utils.redact import sanitize_dict
@@ -353,6 +354,34 @@ class VerificationSettings(BaseModel):
             ).model_dump()
         },
     )
+    critic_api_key: str | SecretStr | None = Field(
+        default=None,
+        description=(
+            "API key used to authenticate with the critic service. "
+            "When None, the LLM's ``api_key`` is reused, which preserves "
+            "the auto-configuration path for the All-Hands LLM proxy."
+        ),
+        json_schema_extra={
+            SETTINGS_METADATA_KEY: SettingsFieldMetadata(
+                label="Critic API Key",
+                prominence=SettingProminence.CRITICAL,
+                depends_on=("critic_enabled",),
+            ).model_dump()
+        },
+    )
+
+    @field_validator("critic_api_key")
+    @classmethod
+    def _validate_critic_api_key(
+        cls, v: str | SecretStr | None, info: ValidationInfo
+    ) -> SecretStr | None:
+        return validate_secret(v, info)
+
+    @field_serializer("critic_api_key", when_used="always")
+    def _serialize_critic_api_key(
+        self, v: SecretStr | None, info: SerializationInfo
+    ) -> Any:
+        return serialize_secret(v, info)
 
 
 def _default_llm_settings() -> LLM:
@@ -913,8 +942,15 @@ def build_condenser(self, llm: LLM) -> LLMSummarizingCondenser | None:
     def build_critic(self) -> CriticBase | None:
         """Create an :class:`APIBasedCritic` from these settings.
 
-        Returns ``None`` when the critic is disabled or when the LLM
-        has no ``api_key`` (the critic service requires authentication).
+        Returns ``None`` when the critic is disabled or when no API key
+        is available (the critic service requires authentication).
+
+        If ``verification.critic_api_key`` is set it is used to
+        authenticate with the critic service; otherwise the LLM's
+        ``api_key`` is reused. This preserves the existing
+        auto-configuration path for the All-Hands LLM proxy while
+        letting deployments route the critic through a different
+        provider (e.g. an LLM proxy with its own credential).
 
         If ``verification.critic_server_url`` or
         ``verification.critic_model_name`` are set they override the
@@ -924,7 +960,7 @@ def build_critic(self) -> CriticBase | None:
         if not self.verification.critic_enabled:
             return None
 
-        api_key = self.llm.api_key
+        api_key = self.verification.critic_api_key or self.llm.api_key
         if api_key is None:
             return None
 

tests/sdk/agent/test_acp_agent.py

@@ -62,6 +62,23 @@
 # ---------------------------------------------------------------------------
 
 
+class _FakeLookupSecret(SecretSource):
+    """Module-level stand-in for ``LookupSecret`` used by registry tests.
+
+    Defined at module scope (not inside a test method) so its ``__qualname__``
+    does not contain ``<locals>``. ``DiscriminatedUnionMixin`` rejects
+    subclasses whose qualname contains ``<locals>`` during ``SecretSource``
+    union validation, and any such local subclass leaks into the global
+    ``__subclasses__`` registry — breaking unrelated serialization tests
+    that run later on the same xdist worker.
+    """
+
+    stored_value: str
+
+    def get_value(self) -> str | None:
+        return self.stored_value
+
+
 def _make_agent(**kwargs) -> ACPAgent:
     return ACPAgent(acp_command=["echo", "test"], **kwargs)
 

tests/sdk/test_settings.py

@@ -122,6 +122,15 @@ def test_llm_agent_settings_export_schema_groups_sections() -> None:
         is SettingProminence.CRITICAL
     )
 
+    # The critic API key must surface in the schema as a CRITICAL, secret
+    # field that depends on critic_enabled — this is what the GUI uses to
+    # render a masked input gated on the toggle.
+    critic_api_key = v_fields["verification.critic_api_key"]
+    assert critic_api_key.secret is True
+    assert critic_api_key.value_type == "string"
+    assert critic_api_key.prominence is SettingProminence.CRITICAL
+    assert critic_api_key.depends_on == ["verification.critic_enabled"]
+
 
 def test_acp_agent_settings_export_schema_has_acp_section() -> None:
     schema = ACPAgentSettings.export_schema()
@@ -554,6 +563,77 @@ def test_llm_create_agent_no_critic_without_api_key() -> None:
     assert agent.critic is None
 
 
+def test_llm_create_agent_critic_uses_explicit_api_key() -> None:
+    """When ``verification.critic_api_key`` is set, the critic authenticates
+    with it instead of the LLM key. The LLM's own key is preserved untouched
+    so the main agent loop still talks to its provider."""
+    settings = OpenHandsAgentSettings(
+        llm=LLM(model="m", api_key=SecretStr("llm-key")),
+        verification=VerificationSettings(
+            critic_enabled=True,
+            critic_api_key=SecretStr("critic-key"),
+        ),
+    )
+    agent = settings.create_agent()
+    assert isinstance(agent.critic, APIBasedCritic)
+    assert isinstance(agent.critic.api_key, SecretStr)
+    assert agent.critic.api_key.get_secret_value() == "critic-key"
+    # LLM key unaffected.
+    assert isinstance(agent.llm.api_key, SecretStr)
+    assert agent.llm.api_key.get_secret_value() == "llm-key"
+
+
+def test_llm_create_agent_critic_falls_back_to_llm_api_key() -> None:
+    """Without ``verification.critic_api_key``, the legacy behavior holds:
+    the critic reuses the LLM key (auto-config path for the All-Hands proxy)."""
+    settings = OpenHandsAgentSettings(
+        llm=LLM(model="m", api_key=SecretStr("llm-key")),
+        verification=VerificationSettings(critic_enabled=True),
+    )
+    agent = settings.create_agent()
+    assert isinstance(agent.critic, APIBasedCritic)
+    assert isinstance(agent.critic.api_key, SecretStr)
+    assert agent.critic.api_key.get_secret_value() == "llm-key"
+
+
+def test_llm_create_agent_critic_with_only_critic_api_key() -> None:
+    """If the LLM has no key but ``critic_api_key`` is supplied, the critic
+    is still built — its credential is independent of the LLM's."""
+    settings = OpenHandsAgentSettings(
+        llm=LLM(model="m", api_key=None),
+        verification=VerificationSettings(
+            critic_enabled=True,
+            critic_api_key=SecretStr("critic-only-key"),
+        ),
+    )
+    agent = settings.create_agent()
+    assert isinstance(agent.critic, APIBasedCritic)
+    assert isinstance(agent.critic.api_key, SecretStr)
+    assert agent.critic.api_key.get_secret_value() == "critic-only-key"
+
+
+def test_verification_settings_critic_api_key_roundtrip() -> None:
+    """``critic_api_key`` survives dump → validate when secrets are exposed,
+    and validates from both plain strings and SecretStr inputs."""
+    settings = VerificationSettings(
+        critic_enabled=True,
+        critic_api_key="plain-string-key",
+    )
+    assert isinstance(settings.critic_api_key, SecretStr)
+    assert settings.critic_api_key.get_secret_value() == "plain-string-key"
+
+    dumped = settings.model_dump(context={"expose_secrets": "plaintext"})
+    assert dumped["critic_api_key"] == "plain-string-key"
+
+    restored = VerificationSettings.model_validate(dumped)
+    assert isinstance(restored.critic_api_key, SecretStr)
+    assert restored.critic_api_key.get_secret_value() == "plain-string-key"
+
+    # Empty strings normalize to None (consistent with LLM.api_key handling).
+    empty = VerificationSettings(critic_enabled=True, critic_api_key="")
+    assert empty.critic_api_key is None
+
+
 def test_llm_create_agent_critic_with_iterative_refinement() -> None:
     settings = OpenHandsAgentSettings(
         llm=LLM(model="m", api_key=SecretStr("k")),