diff --git a/.github/scripts/check_persisted_settings_compat.py b/.github/scripts/check_persisted_settings_compat.py index f792e360ce..e2e50e6d8d 100644 --- a/.github/scripts/check_persisted_settings_compat.py +++ b/.github/scripts/check_persisted_settings_compat.py @@ -145,7 +145,6 @@ def emit(key: str, payload: dict[str, Any]) -> None: acp = ACPAgentSettingsCls( acp_server="claude-code", acp_model="claude-opus-4-6", - acp_env={"OPENAI_API_KEY": "sk-test-acp"}, ) except Exception: acp = None diff --git a/.github/workflows/run-eval.yml b/.github/workflows/run-eval.yml index 794b6497b5..273c7e774d 100644 --- a/.github/workflows/run-eval.yml +++ b/.github/workflows/run-eval.yml @@ -26,7 +26,8 @@ on: sdk_ref: description: SDK commit/ref to evaluate (must be a semantic version like v1.0.0 unless 'Allow unreleased branches' is checked) required: true - default: v1.28.0 + default: v1.29.0 + diff --git a/AGENTS.md b/AGENTS.md index ef16e700b6..ca30438c4f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -142,7 +142,7 @@ When reviewing code, provide constructive feedback: - Remote workspace git operations should call `/api/git/changes` and `/api/git/diff` via the `path` query parameter with slash-normalized strings; building those URLs with `pathlib.Path` leaks host-platform separators and breaks Windows paths. The grep tool now prefers `rg`, then system `grep`, then Python; both the real grep executor and the SDK's terminal-command compatibility fallback should keep that order. For grep parity, the Python fallback should hide dotfiles by default but still let explicit `include` globs surface files like `.env`, matching ripgrep. For glob parity, any symlink-preservation regression test should force the Python fallback path, because ripgrep availability changes whether the fallback implementation runs at all. - Keep path helpers split by purpose: `is_absolute_path_source()` is for cross-platform source/wire syntax detection, while local filesystem writes/validation (for example, the file editor) should use host-native absolute-path semantics so POSIX does not silently accept Windows drive paths as creatable files. - Tool availability filtering belongs in `openhands-sdk/openhands/sdk/tool/registry.py` via `list_usable_tools()`, which preserves registration order and defaults tools to usable unless they expose an `is_usable()` callable. Environment-specific checks like Chromium detection should live on the concrete tool class (`BrowserToolSet.is_usable()`), while agent-server surfaces such as `/server_info` should consume the registry helper rather than re-implement per-tool filtering. -- Pydantic secret field helpers live in `openhands-sdk/openhands/sdk/utils/pydantic_secrets.py`. `serialize_secret()` handles serialization (cipher / `expose_secrets` / default Pydantic masking); `validate_secret()` handles deserialization (cipher decryption, redacted/empty → `None`); `is_redacted_secret()` checks for the sentinel; `REDACTED_SECRET_VALUE` is the canonical sentinel string. For `dict[str, str]` fields whose values are all secrets, wrap each value in `SecretStr` and call `serialize_secret` per value (see `LookupSecret._serialize_secrets` and `ACPAgent._serialize_acp_env`). Do not hand-roll redaction logic in field serializers. +- Pydantic secret field helpers live in `openhands-sdk/openhands/sdk/utils/pydantic_secrets.py`. `serialize_secret()` handles serialization (cipher / `expose_secrets` / default Pydantic masking); `validate_secret()` handles deserialization (cipher decryption, redacted/empty → `None`); `is_redacted_secret()` checks for the sentinel; `REDACTED_SECRET_VALUE` is the canonical sentinel string. For `dict[str, str]` fields whose values are all secrets, wrap each value in `SecretStr` and call `serialize_secret` per value (see `LookupSecret._serialize_secrets`). Do not hand-roll redaction logic in field serializers. - `LookupSecret` normalizes hostless URLs against `OH_INTERNAL_SERVER_URL` (set by `openhands-agent-server.__main__` from the bound host/port, rewriting wildcard binds to loopback) and otherwise falls back to `http://127.0.0.1:8000`, so relative secret URLs can safely target the current agent-server instance. diff --git a/examples/02_remote_agent_server/16_deferred_init.py b/examples/02_remote_agent_server/16_deferred_init.py index c1cc9376eb..bcc3016058 100644 --- a/examples/02_remote_agent_server/16_deferred_init.py +++ b/examples/02_remote_agent_server/16_deferred_init.py @@ -153,14 +153,15 @@ assert resp.status_code == 200 data = resp.json() execution_status = data.get("execution_status", "unknown") - if execution_status in ("stopped", "paused", "error"): + # Terminal states per ConversationExecutionStatus.is_terminal(). + if execution_status in ("finished", "error", "stuck"): break logger.info(f" status: {execution_status} ({elapsed}s elapsed)") time.sleep(2) elapsed += 2 logger.info(f"✅ Conversation finished — status: {execution_status}") - assert execution_status in ("stopped", "paused"), ( + assert execution_status == "finished", ( f"Unexpected final status: {execution_status}" ) diff --git a/openhands-agent-server/openhands/agent_server/api.py b/openhands-agent-server/openhands/agent_server/api.py index b0aae95734..4bea0f6080 100644 --- a/openhands-agent-server/openhands/agent_server/api.py +++ b/openhands-agent-server/openhands/agent_server/api.py @@ -424,7 +424,7 @@ def _sanitize_validation_errors(errors: Sequence[Any]) -> list[dict]: FastAPI's default 422 response includes the raw request ``input`` in each validation error dict. If the request contained secret-bearing fields - (e.g. ``agent.llm.api_key``, ``agent.acp_env``), those values would be + (e.g. ``agent.llm.api_key``, MCP server ``env``), those values would be echoed back to the caller. This helper redacts them. Args: @@ -457,7 +457,7 @@ async def _validation_exception_handler( FastAPI's default 422 handler echoes the raw request body inside the ``detail[].input`` field. When the request contains secrets (e.g. - ``agent.llm.api_key``, ``agent.acp_env``), this would leak credentials + ``agent.llm.api_key``, MCP server ``env``), this would leak credentials in the error response. We intercept the error, redact secret-bearing fields, and return a safe 422 response. diff --git a/openhands-agent-server/openhands/agent_server/persistence/models.py b/openhands-agent-server/openhands/agent_server/persistence/models.py index b3621510ab..ec4cb322b8 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/models.py +++ b/openhands-agent-server/openhands/agent_server/persistence/models.py @@ -69,13 +69,14 @@ def _deep_merge( - Nested dicts are merged recursively. - **Inside a nested map** a ``None`` value **removes** that key — the "unset" primitive a plain deep-merge lacks. It lets a - ``PATCH /api/settings`` diff delete a single map entry (one - ``acp_env`` / MCP ``env`` key) without round-tripping the whole map:: + ``PATCH /api/settings`` diff delete a single map entry (one MCP + ``env`` / ``headers`` key) without round-tripping the whole map:: - {"agent_settings_diff": {"acp_env": {"STALE_KEY": null}}} + {"agent_settings_diff": + {"mcp_config": {"mcpServers": {"svc": {"env": {"STALE_KEY": null}}}}}} - - **At the top level** (a settings *field* like ``confirmation_mode`` or - ``acp_env`` itself) a ``None`` is left as-is and flows to model + - **At the top level** (a settings *field* like ``confirmation_mode``) + a ``None`` is left as-is and flows to model validation — exactly as before this primitive existed. So a stray ``{"confirmation_mode": null}`` still fails loudly (422) instead of silently resetting a field to its default. This scoping is deliberate: diff --git a/openhands-agent-server/openhands/agent_server/settings_router.py b/openhands-agent-server/openhands/agent_server/settings_router.py index 1b7d816f3a..9ee14d5f0c 100644 --- a/openhands-agent-server/openhands/agent_server/settings_router.py +++ b/openhands-agent-server/openhands/agent_server/settings_router.py @@ -177,13 +177,9 @@ async def update_settings( The three ``*_settings_diff`` fields are deep-merged; nested objects merge recursively, and a ``null`` value **inside a nested map deletes that entry** — the "unset" primitive that lets a client remove a single map key without - round-tripping the whole map. To drop one ACP env-var:: + round-tripping the whole map. To remove one MCP server's header:: PATCH /api/settings - {"agent_settings_diff": {"acp_env": {"STALE_KEY": null}}} - - or to remove one MCP server's header:: - {"agent_settings_diff": {"mcp_config": {"mcpServers": {"svc": {"headers": {"X-Old": null}}}}}} diff --git a/openhands-agent-server/pyproject.toml b/openhands-agent-server/pyproject.toml index 77c52635f4..e5ce09422e 100644 --- a/openhands-agent-server/pyproject.toml +++ b/openhands-agent-server/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "openhands-agent-server" -version = "1.28.0" +version = "1.29.0" description = "OpenHands Agent Server - REST/WebSocket interface for OpenHands AI Agent" requires-python = ">=3.12" diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 7d44628992..761345f288 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -84,11 +84,9 @@ from openhands.sdk.tool import Tool # noqa: TC002 from openhands.sdk.tool.builtins.finish import FinishAction, FinishObservation from openhands.sdk.utils import maybe_truncate -from openhands.sdk.utils.deprecation import warn_deprecated from openhands.sdk.utils.pydantic_secrets import ( serialize_secret, validate_secret, - validate_secret_dict, ) from openhands.sdk.utils.redact import redact_text_secrets @@ -1506,49 +1504,6 @@ class ACPAgent(AgentBase): default_factory=list, description="Additional arguments for the ACP server command", ) - acp_env: dict[str, str] = Field( - default_factory=dict, - description=( - "DEPRECATED (removed in 1.29.0): additional environment variables for " - "the ACP server process. Route subprocess env/credentials through " - "state.secret_registry (e.g. agent_context.secrets / " - "StartConversationRequest.secrets) instead." - ), - ) - - @field_validator("acp_env", mode="before") - @classmethod - def _decrypt_acp_env_values(cls, value: Any, info: ValidationInfo) -> Any: - """Decrypt persisted ACP environment values when a cipher is available. - - Mirrors the settings-side ``_decrypt_acp_env_values`` on - :class:`openhands.sdk.settings.model.ACPAgentSettings`. The - settings variant handles the on-disk → memory round-trip, - but the conversation-start path goes - :class:`StartConversationRequest.agent_settings` → the request's - ``_populate_agent_from_settings`` (a ``mode='before'`` - model_validator that runs *without* cipher context) → - ``settings.create_agent()`` → :class:`ACPAgent`. By the time - ``conversation_service.start_conversation`` re-validates the full - :class:`StoredConversation` with the server's cipher in context, - the agent has already been constructed and its ``acp_env`` field - still holds ciphertext. Without a validator here, that ciphertext - survives the re-validation step and reaches the subprocess as the - env-var value — breaking any provider call that interprets the - variable (e.g. an Anthropic request reading a Fernet token in - place of ``ANTHROPIC_BASE_URL``). - - Legacy plaintext values pass through unchanged so first writes - from clients that haven't gone through the encryption pipeline - still validate cleanly. - """ - return validate_secret_dict(value, info, description="ACP env") - - @field_serializer("acp_env", when_used="always") - def _serialize_acp_env(self, value: dict[str, str], info): - """Mask ``acp_env`` values via :func:`serialize_secret`.""" - return {k: serialize_secret(SecretStr(v), info) for k, v in value.items()} - acp_session_mode: str | None = Field( default=None, description=( @@ -2240,9 +2195,7 @@ def _isolate_acp_data_dir( the per-conversation root is what isolation is for. No-ops for an unrecognised command or a provider without a relocation - lever. An explicit ``acp_env`` pin of the data-dir var wins (it has the - highest precedence and is honoured as the materialisation target too), so - leave it untouched. + lever. Claude note: relocating ``CLAUDE_CONFIG_DIR`` is safe under either auth mode. :data:`_ENV_CONFLICT_MAP` is keyed on the OAuth token @@ -2257,12 +2210,10 @@ def _isolate_acp_data_dir( seen by anything the CLI subprocess itself spawns (``git``, ``npm``, ``node``, shells — e.g. ``~/.gitconfig``, ``~/.npmrc``, the npm cache). That is accepted as the cost of isolating Gemini at all; callers that - need a narrower scope can pin ``HOME`` via ``acp_env`` (honoured below) - or leave isolation off for Gemini. + need a narrower scope can leave isolation off for Gemini. - Ordering: this runs *after* the ``secret_registry`` injection and the - ``acp_env`` update in :meth:`_start_acp_server` so an ``acp_env`` pin of - the data-dir var is visible and wins. Relocation is now credential-blind + Ordering: this runs *after* the ``secret_registry`` injection in + :meth:`_start_acp_server`. Relocation is now credential-blind (the auth-conflict strip is keyed on ``CLAUDE_CODE_OAUTH_TOKEN``, not on the config dir), so the data-dir var it sets never affects auth. """ @@ -2270,8 +2221,6 @@ def _isolate_acp_data_dir( if provider is None or provider.data_dir_env_var is None: return env_var = provider.data_dir_env_var - if env_var in self.acp_env: - return data_dir = self._acp_file_secret_dir(state, provider.key) data_dir.mkdir(mode=0o700, parents=True, exist_ok=True) env[env_var] = str(data_dir) @@ -2284,8 +2233,7 @@ def _materialise_file_secrets( For each spec in :attr:`acp_file_secrets` whose secret is registered in ``state.secret_registry``, write its value to the spec's durable per-conversation directory (:meth:`_acp_file_secret_dir`) and set the - controlling env var (``CODEX_HOME`` / ``GOOGLE_APPLICATION_CREDENTIALS``) - unless the caller pinned it via ``acp_env``. + controlling env var (``CODEX_HOME`` / ``GOOGLE_APPLICATION_CREDENTIALS``). Seed-if-absent: a non-empty existing file is preserved, never clobbered — so a token the CLI rewrites on refresh (Codex) survives a recycle, and @@ -2293,44 +2241,24 @@ def _materialise_file_secrets( ``0700`` directories. The blob secret itself is not exported as an env var (callers exclude it via :meth:`_present_file_secret_names`); only the path env var is set. - - If the caller pinned the data-dir env var via the (deprecated) - ``acp_env``, the credential is seeded *where that pin points* so the file - and env stay consistent — and ``acp_env`` keeps its precedence over the - env var. """ for spec in self.acp_file_secrets: name = spec.secret_name value = state.secret_registry.get_secret_value(name) if not value: continue - # Seed where the data-dir env var will actually point: an explicit - # acp_env pin (which wins in env precedence) overrides the default - # per-conversation root, so honor it as the write target too. - pinned = self.acp_env.get(spec.env_var) - if pinned and spec.env_points_to == "dir": - directory = Path(pinned) - target = directory / spec.filename - elif pinned: # env_points_to == "file" - target = Path(pinned) - directory = target.parent - else: - directory = self._acp_file_secret_dir(state, spec.subdir) - target = directory / spec.filename + directory = self._acp_file_secret_dir(state, spec.subdir) + target = directory / spec.filename try: directory.mkdir(mode=0o700, parents=True, exist_ok=True) # Tighten the SDK-owned per-conversation dir in case it - # pre-existed or umask widened mkdir's mode. Skip for an - # externally-pinned acp_env dir (e.g. a deliberately - # group-readable shared mount) so we don't silently narrow - # permissions the user chose. - if not pinned: - directory.chmod(0o700) - # Also clamp the shared SDK-owned `acp/` parent, which - # parents=True may have created under the process umask - # (e.g. 0o755); the leaf chmod above only covers . - # Stop at `acp/` — its parent is the persistence layer's. - directory.parent.chmod(0o700) + # pre-existed or umask widened mkdir's mode. + directory.chmod(0o700) + # Also clamp the shared SDK-owned `acp/` parent, which + # parents=True may have created under the process umask + # (e.g. 0o755); the leaf chmod above only covers . + # Stop at `acp/` — its parent is the persistence layer's. + directory.parent.chmod(0o700) if target.is_file() and target.stat().st_size > 0: # Seed-if-absent: keep the (possibly CLI-refreshed) contents, # but still clamp perms — a pre-existing credential file may @@ -2358,14 +2286,11 @@ def _materialise_file_secrets( directory, ) raise - # acp_env (applied last in _start_acp_server) keeps precedence; only - # set the env var here when the caller did not pin it. - if spec.env_var not in self.acp_env: - env[spec.env_var] = str( - directory if spec.env_points_to == "dir" else target - ) + env[spec.env_var] = str( + directory if spec.env_points_to == "dir" else target + ) for companion in spec.warn_if_unset: - if not env.get(companion) and companion not in self.acp_env: + if not env.get(companion): logger.warning( "ACP file-secret %r materialised but %s is unset; the " "provider may fail to authenticate until it is configured", @@ -2387,12 +2312,11 @@ def _start_acp_server(self, state: ConversationState) -> None: client.mask = state.secret_registry.mask_secrets_in_output # Build the subprocess environment. Precedence, highest first: - # acp_env > state.secret_registry > os.environ > default_environment + # state.secret_registry > os.environ > default_environment # # Conversation credentials intentionally OVERRIDE ambient os.environ: an # explicit per-conversation / provider secret must win over a same-named - # variable in the agent-server's own environment. acp_env (deprecated) - # stays highest. + # variable in the agent-server's own environment. # # agent_context.secrets are seeded into secret_registry at # LocalConversation.__init__ (lower priority than request.secrets), so @@ -2401,17 +2325,6 @@ def _start_acp_server(self, state: ConversationState) -> None: env = default_environment() env.update(os.environ) _strip_inherited_npm_env(env) - if self.acp_env: - warn_deprecated( - "ACPAgent.acp_env", - deprecated_in="1.24.0", - removed_in="1.29.0", - details=( - "Route ACP subprocess env/credentials through " - "state.secret_registry (e.g. agent_context.secrets / " - "StartConversationRequest.secrets) instead." - ), - ) # Reserved file-content credential secrets (Codex auth.json, Gemini # Vertex SA — see _materialise_file_secrets) are written to disk, not # injected as env vars, so exclude their (large blob) names from the @@ -2420,28 +2333,20 @@ def _start_acp_server(self, state: ConversationState) -> None: # Inject the whole registry: an ACP CLI is a black box we can't # name-scan per command (unlike the regular agent's bash tool), so # credentials must be delivered upfront. Registry values override - # ambient os.environ. Skip keys acp_env will set last (avoids a - # redundant LookupSecret.get_value()) and file secrets (materialised to - # disk below). + # ambient os.environ. Skip file secrets (materialised to disk below). env.update( - state.secret_registry.get_all_secrets_as_env_vars( - exclude=set(self.acp_env) | file_secret_names - ) + state.secret_registry.get_all_secrets_as_env_vars(exclude=file_secret_names) ) # Materialise reserved file-content secrets to disk and point their # data-dir env vars (CODEX_HOME / GOOGLE_APPLICATION_CREDENTIALS) at the - # written files. Done before acp_env so an explicit acp_env override of - # those vars still wins. + # written files. self._materialise_file_secrets(state, env) - # acp_env (deprecated) has highest precedence. - env.update(self.acp_env) # Strip CLAUDECODE so nested Claude Code instances don't refuse to start env.pop("CLAUDECODE", None) # Relocate the CLI's data/config root to a per-conversation directory so # sandbox-sharing conversations don't race on a shared HOME (#1019). - # Runs after the registry injection and the acp_env update above so an - # acp_env pin of the data-dir var wins. Independent of the strip below + # Runs after the registry injection above. Independent of the strip below # (keyed on the OAuth token, not the data-dir var), so ordering relative # to it no longer matters for correctness. if self.acp_isolate_data_dir: @@ -2460,7 +2365,7 @@ def _start_acp_server(self, state: ConversationState) -> None: # codex ignores OPENAI_BASE_URL; translate it into the config key it # reads. Reads the *fully assembled* env above, so it fires regardless of # which channel delivered OPENAI_BASE_URL (agent_context.secrets, - # state.secret_registry / StartConversationRequest.secrets, acp_env, + # state.secret_registry / StartConversationRequest.secrets, # os.environ) — i.e. eval, canvas, and cloud all route the same way. args += _codex_base_url_overrides(command, args, env) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 5c45ca45e1..401f571e9a 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -165,7 +165,9 @@ def _decrypt_secrets(cls, value: Any, info: ValidationInfo) -> Any: :class:`StartConversationRequest` (whose ``_populate_agent_from_settings`` validator runs *without* cipher context) and get injected into the agent's system prompt - as-is — same bug class that affected ``ACPAgent.acp_env``. + as-is — same bug class as any secret-bearing dict field that + round-trips without a matching decryption validator (e.g. MCP + ``env`` / ``headers``). ``SecretSource`` entries are dict-shaped on the wire (Pydantic models), so they're skipped by :func:`validate_secret_dict`'s diff --git a/openhands-sdk/openhands/sdk/llm/llm.py b/openhands-sdk/openhands/sdk/llm/llm.py index ced5d2be5b..aedb0c18cf 100644 --- a/openhands-sdk/openhands/sdk/llm/llm.py +++ b/openhands-sdk/openhands/sdk/llm/llm.py @@ -121,22 +121,10 @@ from openhands.sdk.llm.utils.retry_mixin import RetryMixin from openhands.sdk.llm.utils.telemetry import Telemetry from openhands.sdk.logger import ENV_LOG_DIR, get_logger -from openhands.sdk.utils.deprecation import warn_deprecated logger = get_logger(__name__) -# Shared message for the no-op ``_return_metrics`` deprecation (Q1 of #3341). -# Metrics are always returned via ``LLMResponse.metrics``; the parameter has no -# effect and is scheduled for removal after the standard 5-minor-release runway. -# NOTE: ``deprecated_in`` / ``removed_in`` must be passed as string literals at -# each call site — ``check_deprecations.py`` reads them via static AST analysis -# and cannot resolve module-level constants. -_RETURN_METRICS_DETAILS: Final[str] = ( - "The _return_metrics parameter has no effect; metrics are always available " - "via LLMResponse.metrics. Stop passing it." -) - __all__ = ["LLM"] @@ -1296,7 +1284,6 @@ def completion( self, messages: list[Message], tools: Sequence[ToolDefinition] | None = None, - _return_metrics: bool = False, add_security_risk_prediction: bool = False, on_token: TokenCallbackType | None = None, **kwargs, @@ -1309,9 +1296,6 @@ def completion( Args: messages: List of conversation messages. tools: Optional list of tools available to the model. - _return_metrics: Deprecated and ignored; metrics are always returned - via ``LLMResponse.metrics``. Scheduled for removal in - ``1.29.0``. add_security_risk_prediction: Add security_risk field to tool schemas. on_token: Optional callback for streaming tokens. **kwargs: Additional arguments passed to the LLM API. @@ -1335,13 +1319,6 @@ def completion( print(response.content) ``` """ - if _return_metrics: - warn_deprecated( - "LLM.completion(_return_metrics=...)", - deprecated_in="1.24.0", - removed_in="1.29.0", - details=_RETURN_METRICS_DETAILS, - ) _caller_kwargs = kwargs.copy() enable_streaming = bool(kwargs.get("stream", False)) or self.stream if enable_streaming: @@ -1416,7 +1393,6 @@ async def acompletion( self, messages: list[Message], tools: Sequence[ToolDefinition] | None = None, - _return_metrics: bool = False, add_security_risk_prediction: bool = False, on_token: AnyTokenCallbackType | None = None, **kwargs, @@ -1426,13 +1402,6 @@ async def acompletion( Uses ``litellm.acompletion`` under the hood, freeing the event loop while waiting for the LLM provider response. """ - if _return_metrics: - warn_deprecated( - "LLM.acompletion(_return_metrics=...)", - deprecated_in="1.24.0", - removed_in="1.29.0", - details=_RETURN_METRICS_DETAILS, - ) _caller_kwargs = kwargs.copy() enable_streaming = bool(kwargs.get("stream", False)) or self.stream if enable_streaming: @@ -1512,7 +1481,6 @@ def responses( tools: Sequence[ToolDefinition] | None = None, include: list[str] | None = None, store: bool | None = None, - _return_metrics: bool = False, add_security_risk_prediction: bool = False, on_token: TokenCallbackType | None = None, **kwargs, @@ -1526,8 +1494,6 @@ def responses( tools: Optional list of tools available to the model include: Optional list of fields to include in response store: Whether to store the conversation - _return_metrics: Deprecated and ignored; metrics are always returned - via ``LLMResponse.metrics``. Scheduled for removal in ``1.29.0``. add_security_risk_prediction: Add security_risk field to tool schemas on_token: Optional callback for streaming deltas **kwargs: Additional arguments passed to the API @@ -1536,13 +1502,6 @@ def responses( Summary field is always added to tool schemas for transparency and explainability of agent actions. """ - if _return_metrics: - warn_deprecated( - "LLM.responses(_return_metrics=...)", - deprecated_in="1.24.0", - removed_in="1.29.0", - details=_RETURN_METRICS_DETAILS, - ) _caller_kwargs = kwargs.copy() user_enable_streaming = bool(kwargs.get("stream", False)) or self.stream if user_enable_streaming: @@ -1662,7 +1621,6 @@ async def aresponses( tools: Sequence[ToolDefinition] | None = None, include: list[str] | None = None, store: bool | None = None, - _return_metrics: bool = False, add_security_risk_prediction: bool = False, on_token: AnyTokenCallbackType | None = None, **kwargs, @@ -1672,13 +1630,6 @@ async def aresponses( Uses ``litellm.aresponses`` under the hood, freeing the event loop while waiting for the LLM provider response. """ - if _return_metrics: - warn_deprecated( - "LLM.aresponses(_return_metrics=...)", - deprecated_in="1.24.0", - removed_in="1.29.0", - details=_RETURN_METRICS_DETAILS, - ) _caller_kwargs = kwargs.copy() user_enable_streaming = bool(kwargs.get("stream", False)) or self.stream if user_enable_streaming: diff --git a/openhands-sdk/openhands/sdk/llm/router/base.py b/openhands-sdk/openhands/sdk/llm/router/base.py index f841c64a0f..91617297a5 100644 --- a/openhands-sdk/openhands/sdk/llm/router/base.py +++ b/openhands-sdk/openhands/sdk/llm/router/base.py @@ -7,13 +7,12 @@ model_validator, ) -from openhands.sdk.llm.llm import _RETURN_METRICS_DETAILS, LLM +from openhands.sdk.llm.llm import LLM from openhands.sdk.llm.llm_response import LLMResponse from openhands.sdk.llm.message import Message from openhands.sdk.llm.streaming import TokenCallbackType from openhands.sdk.logger import get_logger from openhands.sdk.tool.tool import ToolDefinition -from openhands.sdk.utils.deprecation import warn_deprecated logger = get_logger(__name__) @@ -52,7 +51,6 @@ def completion( self, messages: list[Message], tools: Sequence[ToolDefinition] | None = None, - return_metrics: bool = False, add_security_risk_prediction: bool = False, on_token: TokenCallbackType | None = None, **kwargs, @@ -64,8 +62,6 @@ def completion( Args: messages: List of conversation messages tools: Optional list of tools available to the model - return_metrics: Deprecated and ignored; metrics are always returned - via ``LLMResponse.metrics``. Scheduled for removal in ``1.29.0``. add_security_risk_prediction: Add security_risk field to tool schemas on_token: Optional callback for streaming tokens **kwargs: Additional arguments passed to the LLM API @@ -74,22 +70,13 @@ def completion( Summary field is always added to tool schemas for transparency and explainability of agent actions. """ - if return_metrics: - warn_deprecated( - "RouterLLM.completion(return_metrics=...)", - deprecated_in="1.24.0", - removed_in="1.29.0", - details=_RETURN_METRICS_DETAILS, - ) - # Select appropriate LLM selected_model = self.select_llm(messages) self.active_llm = self.llms_for_routing[selected_model] logger.info(f"RouterLLM routing to {selected_model}...") - # Delegate to selected LLM. ``return_metrics`` is intentionally not - # forwarded: it is a no-op and the warning above already fired. + # Delegate to selected LLM. return self.active_llm.completion( messages=messages, tools=tools, diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index 89c26c518c..bc5c19e595 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -57,7 +57,6 @@ resolve_expose_mode, serialize_secret, validate_secret, - validate_secret_dict, ) from openhands.sdk.utils.redact import sanitize_dict from openhands.sdk.workspace import LocalWorkspace @@ -1279,46 +1278,6 @@ class ACPAgentSettings(AgentSettingsBase): ).model_dump(), }, ) - acp_env: dict[str, str] = Field( - default_factory=dict, - description=( - "DEPRECATED (removed in 1.29.0): extra environment variables passed " - "to the ACP subprocess. Provide arbitrary subprocess env vars through " - "the conversation secrets channel (agent_context.secrets / " - "StartConversationRequest.secrets, which route through " - "state.secret_registry) instead." - ), - json_schema_extra={ - SETTINGS_METADATA_KEY: SettingsFieldMetadata( - label="ACP environment variables", - prominence=SettingProminence.MINOR, - ).model_dump(), - SETTINGS_SECTION_METADATA_KEY: SettingsSectionMetadata( - key="acp", - label="ACP (Agent Client Protocol)", - variant="acp", - ).model_dump(), - }, - ) - - @field_validator("acp_env", mode="before") - @classmethod - def _decrypt_acp_env_values(cls, value: Any, info: ValidationInfo) -> Any: - """Decrypt persisted ACP environment values when a cipher is available. - - Legacy plaintext values pass through unchanged so the next save can - re-encrypt them, matching MCP env/header handling. The matching - on-the-wire validator on :class:`~openhands.sdk.agent.ACPAgent` - handles the conversation-start round-trip; both delegate to the - shared :func:`validate_secret_dict` helper. - """ - return validate_secret_dict(value, info, description="ACP env") - - @field_serializer("acp_env", when_used="always") - def _serialize_acp_env(self, value: dict[str, str], info): - """Mask ``acp_env`` values via :func:`serialize_secret`.""" - return {k: serialize_secret(SecretStr(v), info) for k, v in value.items()} - acp_model: str | None = Field( default=None, description=( @@ -1488,7 +1447,7 @@ def api_key_env_var(self) -> str | None: Delegates to the :data:`~openhands.sdk.settings.acp_providers.ACP_PROVIDERS` registry. Returns ``None`` for ``'custom'`` servers — users manage - credentials entirely via :attr:`acp_env` in that case. + credentials entirely via the conversation secrets channel in that case. """ info = self.provider_info return info.api_key_env_var if info is not None else None @@ -1549,36 +1508,6 @@ def resolve_provider_env(self) -> dict[str, str]: return env - def resolve_acp_env(self) -> dict[str, str]: - """Return the user-supplied ACP subprocess env vars. - - Only the explicit :attr:`acp_env` entries — the user-facing - arbitrary-env-var input that becomes ``ACPAgent.acp_env``. Provider - credentials are **not** folded in here; they ride the conversation - secrets channel → ``state.secret_registry`` (the canonical, - cipher-protected channel the regular agent uses). At spawn time - ``ACPAgent`` injects ``acp_env`` and the registry secrets into the - subprocess env. - - .. deprecated:: 1.24.0 - :attr:`acp_env` is deprecated and will be removed in 1.29.0. Pass - arbitrary subprocess env vars through the conversation secrets - channel instead. - """ - if self.acp_env: - warn_deprecated( - "ACPAgentSettings.acp_env", - deprecated_in="1.24.0", - removed_in="1.29.0", - details=( - "Provide arbitrary ACP subprocess env vars through the " - "conversation secrets channel (agent_context.secrets / " - "StartConversationRequest.secrets, which route through " - "state.secret_registry) instead." - ), - ) - return dict(self.acp_env) - def resolve_acp_command(self) -> list[str]: """Return the effective subprocess command for this settings block. @@ -1687,8 +1616,7 @@ def create_agent(self) -> ACPAgent: which route through ``state.secret_registry``) keyed by the provider's env var name (:attr:`api_key_env_var`), exactly like the regular agent's credentials, and reach the subprocess from the - registry. ``acp_env`` carries only the user's explicit arbitrary - env vars (deprecated). + registry. """ from openhands.sdk.agent import ACPAgent @@ -1730,12 +1658,6 @@ def create_agent(self) -> ACPAgent: # read it from ConversationInfo.agent.acp_server. acp_server=self.acp_server, acp_args=list(self.acp_args), - # Pass acp_env directly rather than via resolve_acp_env() so the - # deprecation warning is not emitted twice on the create_agent path: - # _start_acp_server already warns (ACPAgent.acp_env) at spawn, and - # resolve_acp_env()'s warning (ACPAgentSettings.acp_env) is reserved - # for explicit callers of that public method. - acp_env=dict(self.acp_env), acp_model=self.acp_model, acp_session_mode=self.acp_session_mode, acp_prompt_timeout=self.acp_prompt_timeout, diff --git a/openhands-sdk/openhands/sdk/testing/test_llm.py b/openhands-sdk/openhands/sdk/testing/test_llm.py index 152f2557d7..eacba4e2fc 100644 --- a/openhands-sdk/openhands/sdk/testing/test_llm.py +++ b/openhands-sdk/openhands/sdk/testing/test_llm.py @@ -156,7 +156,6 @@ def completion( self, messages: list[Message], # noqa: ARG002 tools: Sequence[ToolDefinition] | None = None, # noqa: ARG002 - _return_metrics: bool = False, add_security_risk_prediction: bool = False, # noqa: ARG002 on_token: TokenCallbackType | None = None, # noqa: ARG002 **kwargs: Any, # noqa: ARG002 @@ -166,7 +165,6 @@ def completion( Args: messages: Input messages (ignored, but required for API compatibility) tools: Available tools (ignored) - _return_metrics: Whether to return metrics (ignored) add_security_risk_prediction: Add security risk field (ignored) on_token: Streaming callback (ignored) **kwargs: Additional arguments (ignored) @@ -206,7 +204,6 @@ async def acompletion( self, messages: list[Message], tools: Sequence[ToolDefinition] | None = None, - _return_metrics: bool = False, add_security_risk_prediction: bool = False, on_token: AnyTokenCallbackType | None = None, # noqa: ARG002 **kwargs: Any, @@ -230,7 +227,6 @@ async def acompletion( lambda: self.completion( messages=messages, tools=tools, - _return_metrics=_return_metrics, add_security_risk_prediction=add_security_risk_prediction, **kwargs, ), @@ -242,7 +238,6 @@ def responses( tools: Sequence[ToolDefinition] | None = None, include: list[str] | None = None, # noqa: ARG002 store: bool | None = None, # noqa: ARG002 - _return_metrics: bool = False, add_security_risk_prediction: bool = False, on_token: TokenCallbackType | None = None, **kwargs: Any, @@ -255,7 +250,6 @@ def responses( return self.completion( messages=messages, tools=tools, - _return_metrics=_return_metrics, add_security_risk_prediction=add_security_risk_prediction, on_token=on_token, **kwargs, diff --git a/openhands-sdk/openhands/sdk/utils/pydantic_secrets.py b/openhands-sdk/openhands/sdk/utils/pydantic_secrets.py index 7ca99bd0f4..f9c7a5de03 100644 --- a/openhands-sdk/openhands/sdk/utils/pydantic_secrets.py +++ b/openhands-sdk/openhands/sdk/utils/pydantic_secrets.py @@ -142,7 +142,7 @@ def decrypt_str_with_cipher_or_keep( object dying at construction). Building block for the dict-of-string secret-bearing fields - (`acp_env`, `agent_context.secrets`, MCP server `env`/`headers`) + (`agent_context.secrets`, MCP server `env`/`headers`) where each value is a per-key plaintext that's separately encrypted at rest — they can't be typed as :class:`SecretStr` because their keys are user-supplied. @@ -176,10 +176,10 @@ def validate_secret_dict( .. code-block:: python - @field_validator("acp_env", mode="before") + @field_validator("env", mode="before") @classmethod - def _decrypt_acp_env(cls, value, info): - return validate_secret_dict(value, info, description="ACP env") + def _decrypt_env(cls, value, info): + return validate_secret_dict(value, info, description="MCP env") No-ops when the field isn't a dict (lets downstream validation raise the canonical type error), and when no cipher is in context diff --git a/openhands-sdk/openhands/sdk/utils/redact.py b/openhands-sdk/openhands/sdk/utils/redact.py index 0d1ee5153a..bdbaa57384 100644 --- a/openhands-sdk/openhands/sdk/utils/redact.py +++ b/openhands-sdk/openhands/sdk/utils/redact.py @@ -39,7 +39,7 @@ # Keys that should have ALL nested values redacted (not just detected secret keys). # These typically contain environment variables or headers that may include secrets. -REDACT_ALL_VALUES_KEYS = frozenset({"environment", "env", "headers", "acp_env"}) +REDACT_ALL_VALUES_KEYS = frozenset({"environment", "env", "headers"}) # Specific URL query parameter names (lowercased) that should always be redacted, # in addition to any parameter matching SECRET_KEY_PATTERNS via is_secret_key(). diff --git a/openhands-sdk/pyproject.toml b/openhands-sdk/pyproject.toml index 1e88bf80ee..27ae03ceb2 100644 --- a/openhands-sdk/pyproject.toml +++ b/openhands-sdk/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "openhands-sdk" -version = "1.28.0" +version = "1.29.0" description = "OpenHands SDK - Core functionality for building AI agents" requires-python = ">=3.12" diff --git a/openhands-tools/pyproject.toml b/openhands-tools/pyproject.toml index 6b07897351..3bf19a6746 100644 --- a/openhands-tools/pyproject.toml +++ b/openhands-tools/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "openhands-tools" -version = "1.28.0" +version = "1.29.0" description = "OpenHands Tools - Runtime tools for AI agents" requires-python = ">=3.12" diff --git a/openhands-workspace/pyproject.toml b/openhands-workspace/pyproject.toml index b965bec720..90da5bfbfe 100644 --- a/openhands-workspace/pyproject.toml +++ b/openhands-workspace/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "openhands-workspace" -version = "1.28.0" +version = "1.29.0" description = "OpenHands Workspace - Docker and container-based workspace implementations" requires-python = ">=3.12" diff --git a/tests/agent_server/stress/scripts.py b/tests/agent_server/stress/scripts.py index efcea91123..6ccb67169d 100644 --- a/tests/agent_server/stress/scripts.py +++ b/tests/agent_server/stress/scripts.py @@ -47,7 +47,6 @@ def completion( self, messages: list[Message], tools: Sequence[ToolDefinition] | None = None, - _return_metrics: bool = False, add_security_risk_prediction: bool = False, on_token: TokenCallbackType | None = None, **kwargs: Any, @@ -57,7 +56,6 @@ def completion( return super().completion( messages, tools, - _return_metrics, add_security_risk_prediction, on_token, **kwargs, diff --git a/tests/agent_server/test_conversation_router.py b/tests/agent_server/test_conversation_router.py index 924b6a82ce..bddc7f3566 100644 --- a/tests/agent_server/test_conversation_router.py +++ b/tests/agent_server/test_conversation_router.py @@ -123,8 +123,8 @@ def test_search_conversations_default_params( mock_conversation_service.search_conversations.return_value = mock_page # Override the dependency - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -157,8 +157,8 @@ def test_search_conversations_with_all_params( mock_conversation_service.search_conversations.return_value = mock_page # Override the dependency - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -191,8 +191,8 @@ def test_search_conversations_with_all_params( def test_search_conversations_limit_validation(client, mock_conversation_service): """Test search_conversations endpoint with invalid limit values.""" - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -222,8 +222,8 @@ def test_search_conversations_empty_result(client, mock_conversation_service): mock_page = ConversationPage(items=[], next_page_id=None) mock_conversation_service.search_conversations.return_value = mock_page - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -243,8 +243,8 @@ def test_count_conversations_no_filter(client, mock_conversation_service): # Mock the service response mock_conversation_service.count_conversations.return_value = 5 - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -265,8 +265,8 @@ def test_count_conversations_with_status_filter(client, mock_conversation_servic # Mock the service response mock_conversation_service.count_conversations.return_value = 3 - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -292,8 +292,8 @@ def test_count_conversations_zero_result(client, mock_conversation_service): # Mock zero count response mock_conversation_service.count_conversations.return_value = 0 - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -313,8 +313,8 @@ def test_get_conversation_success( # Mock the service response mock_conversation_service.get_conversation.return_value = sample_conversation_info - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -341,8 +341,8 @@ def test_get_conversation_not_found( # Mock the service to return None (conversation not found) mock_conversation_service.get_conversation.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -361,8 +361,8 @@ def test_get_conversation_not_found( def test_get_conversation_invalid_uuid(client, mock_conversation_service): """Test get_conversation endpoint with invalid UUID.""" - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -388,8 +388,8 @@ def test_batch_get_conversations_success( None, ] - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -418,8 +418,8 @@ def test_batch_get_conversations_empty_list(client, mock_conversation_service): # Mock empty response mock_conversation_service.batch_get_conversations.return_value = [] - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -443,8 +443,8 @@ def test_batch_get_conversations_empty_list(client, mock_conversation_service): def test_batch_get_conversations_too_many_ids(client, mock_conversation_service): """Test batch_get_conversations endpoint with too many IDs.""" - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -465,8 +465,8 @@ def test_batch_get_conversations_too_many_ids(client, mock_conversation_service) def test_batch_get_conversations_invalid_uuid(client, mock_conversation_service): """Test batch_get_conversations endpoint with invalid UUID.""" - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -488,8 +488,8 @@ def test_start_conversation_new( True, ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -535,8 +535,8 @@ def test_start_conversation_existing( False, ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -580,8 +580,8 @@ def test_start_conversation_accepts_openhands_agent_settings( updated_at=now, ) mock_conversation_service.start_conversation.return_value = (info, True) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -632,8 +632,8 @@ def test_start_conversation_agent_settings_uses_sdk_default_tools( updated_at=now, ) mock_conversation_service.start_conversation.return_value = (info, True) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -681,8 +681,8 @@ def test_start_conversation_accepts_acp_agent(client, mock_conversation_service) updated_at=now, ) mock_conversation_service.start_conversation.return_value = (acp_info, True) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -718,8 +718,8 @@ def test_start_conversation_accepts_acp_agent_settings( updated_at=now, ) mock_conversation_service.start_conversation.return_value = (acp_info, True) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -732,7 +732,6 @@ def test_start_conversation_accepts_acp_agent_settings( "acp_server": "custom", "acp_command": ["echo", "settings"], "acp_args": ["--verbose"], - "acp_env": {"OPENAI_API_KEY": "sk-acp-env"}, "acp_model": "acp-test-model", "acp_session_mode": "bypassPermissions", "acp_prompt_timeout": 123.0, @@ -746,7 +745,6 @@ def test_start_conversation_accepts_acp_agent_settings( assert request.agent.kind == "ACPAgent" assert request.agent.acp_command == ["echo", "settings"] assert request.agent.acp_args == ["--verbose"] - assert request.agent.acp_env == {"OPENAI_API_KEY": "sk-acp-env"} assert request.agent.acp_model == "acp-test-model" assert request.agent.acp_session_mode == "bypassPermissions" assert request.agent.acp_prompt_timeout == 123.0 @@ -765,8 +763,8 @@ def test_start_conversation_accepts_acp_agent_settings( def test_start_conversation_rejects_invalid_agent_settings( client, mock_conversation_service, agent_settings ): - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -797,8 +795,8 @@ def test_start_conversation_agent_takes_precedence_over_agent_settings( updated_at=now, ) mock_conversation_service.start_conversation.return_value = (info, True) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -824,8 +822,8 @@ def test_start_conversation_agent_takes_precedence_over_agent_settings( def test_start_conversation_rejects_acp_agent_without_kind( client, mock_conversation_service ): - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -846,8 +844,8 @@ def test_start_conversation_rejects_acp_agent_without_kind( def test_start_conversation_invalid_request(client, mock_conversation_service): """Test start_conversation endpoint with invalid request data.""" - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -872,8 +870,8 @@ def test_start_conversation_minimal_request( True, ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -910,8 +908,8 @@ def test_start_conversation_legacy_request_without_agent_kind( True, ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -943,8 +941,8 @@ def test_pause_conversation_success( # Mock the service response - pause successful mock_conversation_service.pause_conversation.return_value = True - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -970,8 +968,8 @@ def test_pause_conversation_failure( # Mock the service response - pause failed mock_conversation_service.pause_conversation.return_value = False - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -995,8 +993,8 @@ def test_delete_conversation_success( # Mock the service response - deletion successful mock_conversation_service.delete_conversation.return_value = True - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1022,8 +1020,8 @@ def test_delete_conversation_failure( # Mock the service response - deletion failed mock_conversation_service.delete_conversation.return_value = False - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1048,8 +1046,8 @@ def test_run_conversation_success( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.run.return_value = None # Successful run - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1076,8 +1074,8 @@ def test_run_conversation_not_found( # Mock the service response - conversation not found mock_conversation_service.get_event_service.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1256,8 +1254,8 @@ def test_switch_acp_model_success( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.switch_acp_model.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: response = client.post( @@ -1277,8 +1275,8 @@ def test_switch_acp_model_not_found( """switch_acp_model returns 404 when the conversation is unknown.""" mock_conversation_service.get_event_service.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: response = client.post( @@ -1299,8 +1297,8 @@ def test_switch_acp_model_non_acp_returns_400( "switch_acp_model is only supported for ACP conversations." ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: response = client.post( @@ -1325,8 +1323,8 @@ def test_switch_acp_model_pre_session_returns_200( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.switch_acp_model.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: response = client.post( @@ -1351,8 +1349,8 @@ def test_switch_acp_model_inactive_service_returns_400( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.switch_acp_model.side_effect = ValueError("inactive_service") - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: response = client.post( @@ -1379,8 +1377,8 @@ def test_switch_acp_model_protocol_error_returns_400( "ACP server rejected model switch to 'bogus': method not found" ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: response = client.post( @@ -1406,8 +1404,8 @@ def test_switch_acp_model_timeout_returns_504( "ACP server did not answer model switch within 600s" ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: response = client.post( @@ -1428,8 +1426,8 @@ def test_run_conversation_already_running( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.run.side_effect = ValueError("conversation_already_running") - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1457,8 +1455,8 @@ def test_run_conversation_other_error( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.run.side_effect = ValueError("some other error") - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1480,8 +1478,8 @@ def test_update_conversation_secrets_success( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.update_secrets.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1518,8 +1516,8 @@ def test_update_conversation_secrets_not_found( # Mock the service response - conversation not found mock_conversation_service.get_event_service.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1550,8 +1548,8 @@ def test_set_conversation_confirmation_policy_success( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.set_confirmation_policy.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1583,8 +1581,8 @@ def test_set_conversation_confirmation_policy_not_found( # Mock the service response - conversation not found mock_conversation_service.get_event_service.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1613,8 +1611,8 @@ def test_update_conversation_success( # Mock the service response - update successful mock_conversation_service.update_conversation.return_value = True - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1645,8 +1643,8 @@ def test_update_conversation_failure( # Mock the service response - update failed mock_conversation_service.update_conversation.return_value = False - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1671,8 +1669,8 @@ def test_update_conversation_invalid_title( ): """Test update_conversation endpoint with invalid title.""" - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1717,8 +1715,8 @@ def test_start_conversation_with_tool_module_qualnames( ) # Override the dependency - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1778,8 +1776,8 @@ def test_start_conversation_without_tool_module_qualnames( ) # Override the dependency - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1821,8 +1819,8 @@ def test_start_conversation_autotitle_defaults_to_true( sample_conversation_info, True, ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1856,8 +1854,8 @@ def test_start_conversation_autotitle_false( sample_conversation_info, True, ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -1897,8 +1895,8 @@ def test_set_conversation_security_analyzer_success( mock_event_service.set_security_analyzer.return_value = None # Override dependency - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) # Make request @@ -1927,8 +1925,8 @@ def test_set_conversation_security_analyzer_with_none( mock_event_service.set_security_analyzer.return_value = None # Override dependency - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) # Make request with None analyzer @@ -1957,8 +1955,8 @@ def test_security_analyzer_endpoint_with_malformed_analyzer_data( mock_event_service.set_security_analyzer.return_value = None # Override dependency - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) # Test with invalid analyzer type (should be rejected) @@ -1983,8 +1981,8 @@ def test_update_secrets_with_string_values( mock_event_service.update_secrets.return_value = None # Override dependency - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2025,8 +2023,8 @@ def test_update_secrets_with_mixed_formats( mock_event_service.update_secrets.return_value = None # Override dependency - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2076,8 +2074,8 @@ def test_switch_conversation_profile_success( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.get_conversation.return_value = mock_conversation - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2104,8 +2102,8 @@ def test_switch_conversation_profile_not_found( """Test switch_conversation_profile endpoint when conversation is not found.""" mock_conversation_service.get_event_service.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2133,8 +2131,8 @@ def test_switch_conversation_profile_nonexistent_profile( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.get_conversation.return_value = mock_conversation - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2159,8 +2157,8 @@ def test_switch_conversation_profile_corrupted_profile( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.get_conversation.return_value = mock_conversation - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2186,8 +2184,8 @@ def test_switch_conversation_llm_success( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.get_conversation.return_value = mock_conversation - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) llm_payload = { @@ -2240,8 +2238,8 @@ def test_switch_conversation_llm_decrypts_encrypted_api_key( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.get_conversation.return_value = mock_conversation - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2286,8 +2284,8 @@ def test_switch_conversation_llm_plaintext_with_cipher_passes_through( mock_conversation_service.get_event_service.return_value = mock_event_service mock_event_service.get_conversation.return_value = mock_conversation - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2316,8 +2314,8 @@ def test_switch_conversation_llm_not_found( """The /switch_llm endpoint returns 404 when the conversation is missing.""" mock_conversation_service.get_event_service.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2343,8 +2341,8 @@ def test_fork_conversation_success( """Test fork endpoint returns 201 with forked conversation info.""" mock_conversation_service.fork_conversation.return_value = sample_conversation_info - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2367,8 +2365,8 @@ def test_fork_conversation_not_found( """Test fork returns 404 when source conversation doesn't exist.""" mock_conversation_service.fork_conversation.return_value = None - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2390,8 +2388,8 @@ def test_fork_conversation_duplicate_id_returns_409( f"Conversation with id {sample_conversation_id} already exists" ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) try: @@ -2417,8 +2415,8 @@ def test_start_conversation_client_tool_registration_error_returns_422( ) ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service + client.app.dependency_overrides[get_conversation_service] = lambda: ( + mock_conversation_service ) request = StartConversationRequest( diff --git a/tests/agent_server/test_goal_loop.py b/tests/agent_server/test_goal_loop.py index ecc7dd00b7..0f5cdb4a18 100644 --- a/tests/agent_server/test_goal_loop.py +++ b/tests/agent_server/test_goal_loop.py @@ -43,7 +43,6 @@ def completion( self, messages, tools=None, - _return_metrics=False, add_security_risk_prediction=False, on_token=None, **kwargs, @@ -53,7 +52,6 @@ def completion( return super().completion( messages, tools, - _return_metrics, add_security_risk_prediction, on_token, **kwargs, diff --git a/tests/agent_server/test_settings_router.py b/tests/agent_server/test_settings_router.py index fe75e2f3b2..fc6557bc2c 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -276,7 +276,7 @@ def test_get_settings_migrates_legacy_openhands_settings_and_resaves_current( assert body["conversation_settings"]["max_iterations"] == 84 -def test_get_settings_migrates_acp_settings_and_resaves_encrypted_env( +def test_get_settings_migrates_acp_settings_and_resaves_encrypted_credentials( client_with_settings, temp_persistence_dir, secret_key ): """ACP settings use the same persisted migration/encryption path.""" @@ -290,7 +290,6 @@ def test_get_settings_migrates_acp_settings_and_resaves_encrypted_env( "acp_server": "custom", "acp_command": ["echo", "settings"], "acp_args": ["--verbose"], - "acp_env": {"OPENAI_API_KEY": _encrypt(cipher, "sk-acp-env")}, "acp_model": "acp-test-model", "acp_session_mode": "bypassPermissions", "acp_prompt_timeout": 123.0, @@ -314,7 +313,6 @@ def test_get_settings_migrates_acp_settings_and_resaves_encrypted_env( assert loaded.agent_settings.agent_kind == "acp" assert loaded.agent_settings.acp_command == ["echo", "settings"] assert loaded.agent_settings.acp_args == ["--verbose"] - assert loaded.agent_settings.acp_env == {"OPENAI_API_KEY": "sk-acp-env"} assert loaded.agent_settings.acp_model == "acp-test-model" assert loaded.agent_settings.acp_session_mode == "bypassPermissions" assert loaded.agent_settings.acp_prompt_timeout == 123.0 @@ -328,7 +326,6 @@ def test_get_settings_migrates_acp_settings_and_resaves_encrypted_env( agent_settings = response.json()["agent_settings"] assert agent_settings["schema_version"] == AGENT_SETTINGS_SCHEMA_VERSION assert agent_settings["agent_kind"] == "acp" - assert agent_settings["acp_env"] == {"OPENAI_API_KEY": "sk-acp-env"} assert agent_settings["llm"]["api_key"] == "sk-acp-llm" patch_response = client_with_settings.patch( @@ -337,18 +334,16 @@ def test_get_settings_migrates_acp_settings_and_resaves_encrypted_env( assert patch_response.status_code == 200, patch_response.text on_disk_text = (temp_persistence_dir / "settings.json").read_text() - assert "sk-acp-env" not in on_disk_text assert "sk-acp-llm" not in on_disk_text on_disk = json.loads(on_disk_text) assert on_disk["schema_version"] == PERSISTED_SETTINGS_SCHEMA_VERSION - assert on_disk["agent_settings"]["acp_env"]["OPENAI_API_KEY"].startswith("gAAAA") + assert on_disk["agent_settings"]["llm"]["api_key"].startswith("gAAAA") assert on_disk["conversation_settings"]["max_iterations"] == 88 reloaded = store.load() assert reloaded is not None assert isinstance(reloaded.agent_settings, ACPAgentSettings) - assert reloaded.agent_settings.acp_env == {"OPENAI_API_KEY": "sk-acp-env"} assert reloaded.conversation_settings.max_iterations == 88 @@ -899,16 +894,16 @@ def test_deep_merge_top_level_null_is_set_not_delete(): def test_deep_merge_nested_null_deletes_entry(): """A ``None`` *inside a nested map* removes that entry; siblings survive.""" merged = _deep_merge( - {"acp_env": {"KEEP": "a", "DROP": "b"}}, - {"acp_env": {"DROP": None}}, + {"env": {"KEEP": "a", "DROP": "b"}}, + {"env": {"DROP": None}}, ) - assert merged == {"acp_env": {"KEEP": "a"}} + assert merged == {"env": {"KEEP": "a"}} def test_deep_merge_nested_null_on_absent_key_is_noop(): """Unsetting a nested key that isn't present is a no-op, not an error.""" - merged = _deep_merge({"acp_env": {"KEEP": "a"}}, {"acp_env": {"MISSING": None}}) - assert merged == {"acp_env": {"KEEP": "a"}} + merged = _deep_merge({"env": {"KEEP": "a"}}, {"env": {"MISSING": None}}) + assert merged == {"env": {"KEEP": "a"}} def test_deep_merge_new_map_embedded_null_stored_as_is(): @@ -986,24 +981,6 @@ def test_update_agent_settings_null_unsets_optional_field() -> None: assert settings.agent_settings.acp_model is None -def test_update_agent_settings_nested_null_removes_map_entry() -> None: - """A null inside a nested map removes that entry (acp_env key removal).""" - settings = PersistedSettings() - settings.update( - { - "agent_settings_diff": { - "agent_kind": "acp", - "acp_server": "claude-code", - "acp_env": {"KEEP": "yes", "DROP": "bye"}, - } - } - ) - assert isinstance(settings.agent_settings, ACPAgentSettings) - - settings.update({"agent_settings_diff": {"acp_env": {"DROP": None}}}) - assert settings.agent_settings.acp_env == {"KEEP": "yes"} - - def test_update_agent_settings_secret_survives_same_kind_merge() -> None: """An existing api_key in agent_settings is preserved across a same-kind update.""" from pydantic import SecretStr @@ -1019,78 +996,6 @@ def test_update_agent_settings_secret_survives_same_kind_merge() -> None: assert settings.agent_settings.llm.api_key.get_secret_value() == "sk-SECRET" -def _seed_acp_settings(persistence_dir: Path, acp_env: dict[str, str]) -> None: - """Write a settings.json with an active ACP agent carrying ``acp_env``.""" - acp = ACPAgentSettings(acp_command=["echo", "hi"], acp_env=acp_env) - persisted = PersistedSettings(agent_settings=acp) - payload = persisted.model_dump(mode="json", context={"expose_secrets": "plaintext"}) - _write_settings_file(persistence_dir, payload) - - -def test_patch_settings_unset_deletes_acp_env_key( - client_with_settings, temp_persistence_dir -): - """PATCH with a ``null`` map value deletes a single ``acp_env`` entry — - the motivating case for the general merge-patch unset (supersedes the - need for a dedicated per-field DELETE endpoint).""" - _seed_acp_settings(temp_persistence_dir, {"KEEP_ME": "keep", "DROP_ME": "drop"}) - - response = client_with_settings.patch( - "/api/settings", - json={"agent_settings_diff": {"acp_env": {"DROP_ME": None}}}, - ) - assert response.status_code == 200 - - acp_env = response.json()["agent_settings"]["acp_env"] - assert "KEEP_ME" in acp_env - assert "DROP_ME" not in acp_env - - -def test_patch_settings_unset_absent_acp_env_key_is_noop( - client_with_settings, temp_persistence_dir -): - """Unsetting a key that isn't present succeeds and changes nothing.""" - _seed_acp_settings(temp_persistence_dir, {"KEEP_ME": "keep"}) - - response = client_with_settings.patch( - "/api/settings", - json={"agent_settings_diff": {"acp_env": {"NEVER_SET": None}}}, - ) - assert response.status_code == 200 - assert set(response.json()["agent_settings"]["acp_env"]) == {"KEEP_ME"} - - -def test_patch_settings_unset_then_add_acp_env_key( - client_with_settings, temp_persistence_dir -): - """Add and delete coexist in one diff: set one key, drop another.""" - _seed_acp_settings(temp_persistence_dir, {"OLD": "x", "DROP_ME": "y"}) - - response = client_with_settings.patch( - "/api/settings", - json={"agent_settings_diff": {"acp_env": {"NEW": "z", "DROP_ME": None}}}, - ) - assert response.status_code == 200 - assert set(response.json()["agent_settings"]["acp_env"]) == {"OLD", "NEW"} - - -def test_patch_settings_null_on_acp_env_field_resets_to_default( - client_with_settings, temp_persistence_dir -): - """A ``null`` on the ``acp_env`` field itself resets it to its default ``{}`` - (RFC 7386 semantics via apply_agent_settings_diff). This is a change from the - old _deep_merge behavior where top-level null would flow to validation and 422.""" - _seed_acp_settings(temp_persistence_dir, {"KEEP_ME": "keep"}) - - response = client_with_settings.patch( - "/api/settings", - json={"agent_settings_diff": {"acp_env": None}}, - ) - assert response.status_code == 200 - after = client_with_settings.get("/api/settings").json() - assert after["agent_settings"]["acp_env"] == {} - - def test_patch_settings_null_on_scalar_field_fails_loudly(client_with_settings): """Regression guard against the silent-reset footgun: ``null`` on a non-optional scalar like ``confirmation_mode`` is rejected (422), not @@ -1115,7 +1020,6 @@ def test_patch_settings_switch_agent_kind_from_acp_to_openhands( # carried into the new variant on a switch. acp = ACPAgentSettings( acp_command=["echo", "test"], - acp_env={"KEY": "value"}, llm=LLM(model="acp-only-model", usage_id="default"), ) persisted = PersistedSettings(agent_settings=acp) @@ -1145,7 +1049,6 @@ def test_patch_settings_switch_agent_kind_from_acp_to_openhands( assert body["agent_settings"]["agent_kind"] == "openhands" # ACP-specific fields should not appear in the response assert "acp_command" not in body["agent_settings"] - assert "acp_env" not in body["agent_settings"] # The restated ``llm`` model wins — the ACP-seeded value is gone. assert body["agent_settings"]["llm"]["model"] == "claude-3-5-sonnet-20241022" @@ -1267,23 +1170,24 @@ def test_patch_settings_same_kind_merge_after_a_switch(client_with_settings): "agent_settings_diff": { "agent_kind": "acp", "acp_command": ["my-cli"], - "acp_env": {"FOO": "bar"}, + "acp_args": ["--foo"], } }, ) assert response.status_code == 200 assert response.json()["agent_settings"]["acp_command"] == ["my-cli"] - # Same-kind follow-up: add an env var. Deep-merge must preserve acp_command - # and the pre-existing env entry. + # Same-kind follow-up: edit a different field. Deep-merge must preserve + # acp_command and acp_args set during the switch. response = client_with_settings.patch( "/api/settings", - json={"agent_settings_diff": {"acp_env": {"BAZ": "qux"}}}, + json={"agent_settings_diff": {"acp_model": "some-model"}}, ) assert response.status_code == 200 body = response.json() assert body["agent_settings"]["acp_command"] == ["my-cli"] - assert set(body["agent_settings"]["acp_env"]) == {"FOO", "BAZ"} + assert body["agent_settings"]["acp_args"] == ["--foo"] + assert body["agent_settings"]["acp_model"] == "some-model" # ── Secrets CRUD tests ────────────────────────────────────────────────── diff --git a/tests/agent_server/test_validation_error_sanitization.py b/tests/agent_server/test_validation_error_sanitization.py index 38d577e343..49681a9fa9 100644 --- a/tests/agent_server/test_validation_error_sanitization.py +++ b/tests/agent_server/test_validation_error_sanitization.py @@ -1,7 +1,7 @@ """Tests for RequestValidationError sanitization in the agent server. Verifies that 422 error responses do not leak sensitive fields such as -``api_key``, ``acp_env``, or other secret-bearing request values. +``api_key``, ``env``, or other secret-bearing request values. Refs: OpenHands/evaluation#385 """ @@ -51,8 +51,8 @@ def test_redacts_api_key_in_input(self): # Non-secret fields should be preserved assert agent_input["llm"]["model"] == "gpt-4" - def test_redacts_acp_env_values(self): - """All values under acp_env should be fully redacted.""" + def test_redacts_env_values(self): + """All values under a redact-all map key (e.g. ``env``) are redacted.""" errors = [ { "type": "value_error", @@ -60,7 +60,7 @@ def test_redacts_acp_env_values(self): "msg": "Invalid value", "input": { "agent": { - "acp_env": { + "env": { "OPENAI_API_KEY": "sk-secret", "DATABASE_URL": "postgres://user:pass@host/db", }, @@ -69,9 +69,9 @@ def test_redacts_acp_env_values(self): } ] result = _sanitize_validation_errors(errors) - acp_env = result[0]["input"]["agent"]["acp_env"] - assert acp_env["OPENAI_API_KEY"] == "" - assert acp_env["DATABASE_URL"] == "" + env = result[0]["input"]["agent"]["env"] + assert env["OPENAI_API_KEY"] == "" + assert env["DATABASE_URL"] == "" def test_preserves_non_secret_fields(self): """Non-secret fields should pass through unchanged.""" @@ -202,7 +202,7 @@ def app_with_validation(self): class SecretPayload(BaseModel): name: str api_key: str - acp_env: dict[str, str] = {} + env: dict[str, str] = {} @app.post("/test-endpoint") async def test_endpoint(payload: SecretPayload): @@ -218,7 +218,7 @@ def test_422_response_redacts_api_key(self, app_with_validation): "/test-endpoint", json={ "api_key": "sk-super-secret-key", - "acp_env": {"PROVIDER_KEY": "provider-secret"}, + "env": {"PROVIDER_KEY": "provider-secret"}, }, ) assert response.status_code == 422 @@ -233,8 +233,8 @@ def test_422_response_redacts_api_key(self, app_with_validation): if "input" in error and isinstance(error["input"], dict): if "api_key" in error["input"]: assert error["input"]["api_key"] == "" - if "acp_env" in error["input"]: - for val in error["input"]["acp_env"].values(): + if "env" in error["input"]: + for val in error["input"]["env"].values(): assert val == "" def test_422_response_preserves_error_structure(self, app_with_validation): @@ -262,7 +262,7 @@ def test_valid_request_unaffected(self, app_with_validation): json={ "name": "test", "api_key": "sk-key", - "acp_env": {}, + "env": {}, }, ) assert response.status_code == 200 diff --git a/tests/cross/test_event_loss_repro.py b/tests/cross/test_event_loss_repro.py index e8dea1b0cb..e9a856d73e 100644 --- a/tests/cross/test_event_loss_repro.py +++ b/tests/cross/test_event_loss_repro.py @@ -108,7 +108,6 @@ def fake_completion_with_finish_tool( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): diff --git a/tests/cross/test_remote_conversation_live_server.py b/tests/cross/test_remote_conversation_live_server.py index 05c2589de0..c57febfcfc 100644 --- a/tests/cross/test_remote_conversation_live_server.py +++ b/tests/cross/test_remote_conversation_live_server.py @@ -185,7 +185,6 @@ def fake_completion( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): # type: ignore[no-untyped-def] @@ -937,7 +936,6 @@ def fake_completion_with_cost( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): # type: ignore[no-untyped-def] @@ -1093,7 +1091,6 @@ def fake_completion_with_finish_tool( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): # type: ignore[no-untyped-def] @@ -1260,7 +1257,6 @@ def fake_completion_with_finish_tool( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): # type: ignore[no-untyped-def] @@ -1411,7 +1407,6 @@ def fake_completion_with_tool_calls( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): # type: ignore[no-untyped-def] @@ -1596,7 +1591,6 @@ def fake_completion_with_finish( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): # type: ignore[no-untyped-def] @@ -1815,7 +1809,6 @@ def fake_completion_with_finish( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): # type: ignore[no-untyped-def] @@ -1953,7 +1946,6 @@ def fake_completion( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): # type: ignore[no-untyped-def] @@ -2198,7 +2190,6 @@ def slow_completion( self, messages, tools, - return_metrics=False, add_security_risk_prediction=False, **kwargs, ): # type: ignore[no-untyped-def] diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index e81399c3a9..fa6a88b9f6 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -6,7 +6,6 @@ import json import threading import uuid -from base64 import urlsafe_b64encode from concurrent.futures import Future from pathlib import Path from types import SimpleNamespace @@ -16,7 +15,7 @@ import pytest from acp.exceptions import RequestError as ACPRequestError from acp.schema import NewSessionResponse, PromptResponse -from pydantic import Field, SecretStr +from pydantic import SecretStr from openhands.sdk.agent.acp_agent import ( ACPAgent, @@ -92,11 +91,6 @@ def _make_agent(**kwargs) -> ACPAgent: return ACPAgent(acp_command=["echo", "test"], **kwargs) -def _make_cipher() -> Cipher: - """Deterministic Fernet cipher for round-trip tests.""" - return Cipher(urlsafe_b64encode(b"a" * 32).decode("ascii")) - - def _make_state(tmp_path) -> ConversationState: agent = _make_agent() workspace = LocalWorkspace(working_dir=str(tmp_path)) @@ -154,10 +148,6 @@ def test_acp_args_default_empty(self): agent = _make_agent() assert agent.acp_args == [] - def test_acp_env_default_empty(self): - agent = _make_agent() - assert agent.acp_env == {} - def test_get_all_llms_yields_sentinel(self): agent = _make_agent() llms = list(agent.get_all_llms()) @@ -217,80 +207,12 @@ def test_roundtrip_serialization(self): agent = ACPAgent( acp_command=["npx", "-y", "claude-agent-acp"], acp_args=["--verbose"], - acp_env={"FOO": "bar"}, ) - # ``acp_env`` is redacted by default, so a value-preserving round-trip - # requires expose_secrets=True (same contract as ``LLM.api_key``). - dumped = agent.model_dump_json(context={"expose_secrets": True}) + dumped = agent.model_dump_json() restored = AgentBase.model_validate_json(dumped) assert isinstance(restored, ACPAgent) assert restored.acp_command == agent.acp_command assert restored.acp_args == agent.acp_args - assert restored.acp_env == agent.acp_env - - def test_acp_env_redacted_by_default(self): - """``acp_env`` values must be masked in default serialization output. - - Regression guard: trace dumps consumed by evaluation tooling embed the - full ACPAgent state under ``history[*].value.agent``. Before masking, - live proxy keys leaked into shareable archives. - """ - agent = ACPAgent( - acp_command=["echo", "test"], - acp_env={ - "OPENAI_API_KEY": "sk-real-secret-do-not-leak", - "GEMINI_API_KEY": "sk-other-secret", - "GEMINI_BASE_URL": "https://llm-proxy.example/", - }, - ) - - # In-memory state still holds the real values — only serialization masks. - assert agent.acp_env["OPENAI_API_KEY"] == "sk-real-secret-do-not-leak" - - # model_dump returns SecretStr objects — real values are hidden. - dumped = agent.model_dump() - for v in dumped["acp_env"].values(): - assert str(v) == REDACTED_SECRET_VALUE - - # JSON path that produced the original leaks must not contain any of - # the real values. - dumped_json = agent.model_dump_json() - assert "sk-real-secret-do-not-leak" not in dumped_json - assert "sk-other-secret" not in dumped_json - assert "https://llm-proxy.example/" not in dumped_json - assert REDACTED_SECRET_VALUE in dumped_json - - def test_acp_env_exposed_with_expose_secrets(self): - """``expose_secrets=True`` returns the real values for transport use.""" - secrets = { - "OPENAI_API_KEY": "sk-real-secret", - "BASE_URL": "https://llm-proxy.example/", - } - agent = ACPAgent(acp_command=["echo", "test"], acp_env=dict(secrets)) - - dumped = agent.model_dump(context={"expose_secrets": True}) - assert dumped["acp_env"] == secrets - - # Round-trip with expose_secrets must reconstruct the original values. - json_blob = agent.model_dump_json(context={"expose_secrets": True}) - restored = AgentBase.model_validate_json(json_blob) - assert isinstance(restored, ACPAgent) - assert restored.acp_env == secrets - - def test_acp_env_serializer_does_not_mutate_in_memory_state(self): - """Serialization must not mutate ``self.acp_env`` — the runtime path - (:meth:`ACPAgent._start_acp_server`) reads it directly to populate the - subprocess environment. - """ - original = {"OPENAI_API_KEY": "sk-real-secret"} - agent = ACPAgent(acp_command=["echo", "test"], acp_env=dict(original)) - - # Multiple dumps in different modes must leave the live dict alone. - agent.model_dump() - agent.model_dump_json() - agent.model_dump(context={"expose_secrets": True}) - - assert agent.acp_env == original def test_deserialization_from_dict(self): data = { @@ -301,104 +223,6 @@ def test_deserialization_from_dict(self): assert isinstance(agent, ACPAgent) assert agent.acp_command == ["echo", "test"] - def test_acp_env_decrypts_ciphertext_with_cipher_in_context(self): - """Round-trip Fernet-encrypted ``acp_env`` values via cipher context. - - Regression for a real production bug in v1.24.0: the on-disk → - ACPAgentSettings → ACPAgent path could leave Fernet ciphertext as - the field value because only the settings-side variant had a - decryption ``field_validator``. The conversation-start flow - validates the full :class:`StoredConversation` with cipher - context after the agent was already constructed (without cipher) - from ``StartConversationRequest.agent_settings`` — and without - the validator here, the ciphertext survives that re-validation - and reaches the ACP subprocess as the env-var value. The - provider call then fails (e.g. Anthropic reads the Fernet token - as ``ANTHROPIC_BASE_URL`` and 400s on URL parsing). - """ - cipher = _make_cipher() - encrypted_key = cipher.encrypt(SecretStr("sk-real")) - encrypted_url = cipher.encrypt(SecretStr("https://api.example.com")) - assert encrypted_key is not None - assert encrypted_url is not None - - # Build the wire payload an agent-server would receive: an - # ACPAgent dict whose ``acp_env`` values are Fernet ciphertext. - data = { - "kind": "ACPAgent", - "acp_command": ["echo", "test"], - "acp_env": { - "ANTHROPIC_API_KEY": encrypted_key, - "ANTHROPIC_BASE_URL": encrypted_url, - }, - } - - restored = AgentBase.model_validate(data, context={"cipher": cipher}) - assert isinstance(restored, ACPAgent) - assert restored.acp_env == { - "ANTHROPIC_API_KEY": "sk-real", - "ANTHROPIC_BASE_URL": "https://api.example.com", - } - - def test_acp_env_no_cipher_in_context_leaves_ciphertext_untouched(self): - """The ``cipher is None`` branch of the validator is exercised on - every code path that round-trips an agent dict without supplying - a cipher (e.g. test serialization helpers, JSON-only diagnostic - dumps). In that mode the ciphertext must survive verbatim — both - because there's nothing to decrypt with, and because mutating it - would defeat a downstream caller that *will* validate again with - the cipher present (the conversation-start re-validation step). - """ - cipher = _make_cipher() - encrypted = cipher.encrypt(SecretStr("sk-real")) - assert encrypted is not None - - data = { - "kind": "ACPAgent", - "acp_command": ["echo", "test"], - "acp_env": {"ANTHROPIC_API_KEY": encrypted}, - } - restored = AgentBase.model_validate(data) - assert isinstance(restored, ACPAgent) - assert restored.acp_env == {"ANTHROPIC_API_KEY": encrypted} - - def test_acp_env_plaintext_passes_through_with_cipher(self): - """First writes from clients that never went through the encryption - pipeline carry plaintext. They must still validate cleanly when the - server happens to have a cipher in context.""" - cipher = _make_cipher() - data = { - "kind": "ACPAgent", - "acp_command": ["echo", "test"], - "acp_env": {"FOO": "plaintext-value"}, - } - restored = AgentBase.model_validate(data, context={"cipher": cipher}) - assert isinstance(restored, ACPAgent) - assert restored.acp_env == {"FOO": "plaintext-value"} - - def test_acp_env_undecryptable_ciphertext_passes_through_with_warning(self, caplog): - """Cipher mismatch / corruption shouldn't crash agent construction. - - Mirrors the MCP env/header pattern: a ciphertext we can't decrypt - is left in place with a logged warning so the operator can repair - it, rather than turning into a hard failure that bricks the - agent. - """ - cipher = _make_cipher() - # Looks like a Fernet token (prefix matches) but isn't a valid - # one — try_decrypt_str returns None. - bogus = "gAAAAA" + ("x" * 80) - data = { - "kind": "ACPAgent", - "acp_command": ["echo", "test"], - "acp_env": {"BUSTED": bogus}, - } - with caplog.at_level("WARNING"): - restored = AgentBase.model_validate(data, context={"cipher": cipher}) - assert isinstance(restored, ACPAgent) - assert restored.acp_env == {"BUSTED": bogus} - assert any("could not be decrypted" in r.message for r in caplog.records) - # --------------------------------------------------------------------------- # Feature validation (init_state guards) @@ -6692,8 +6516,7 @@ class TestACPSecretsEnvInjection: reach the subprocess through ``state.secret_registry``: ``LocalConversation`` seeds ``agent_context.secrets`` into the registry at init (covering callers that never lift them into ``request.secrets``), and - ``_start_acp_server`` injects the registry. ``acp_env`` entries take - precedence over registry secrets. + ``_start_acp_server`` injects the registry. """ @staticmethod @@ -6812,33 +6635,6 @@ def test_static_secret_injected_into_subprocess_env(self, tmp_path): conv.close() assert env.get("GITHUB_TOKEN") == "ghp_test123" - def test_acp_env_takes_precedence_over_agent_context_secret(self, tmp_path): - """An explicit acp_env entry wins over the same key in agent_context.secrets. - - ``agent_context.secrets`` reach env via the registry (seeded at - ``LocalConversation.__init__``); ``acp_env`` is applied last and wins. - """ - from pydantic import SecretStr - - from openhands.sdk.conversation.impl.local_conversation import ( - LocalConversation, - ) - from openhands.sdk.secret import StaticSecret - - agent = _make_agent( - acp_env={"MY_TOKEN": "acp-env-wins"}, - agent_context=AgentContext( - secrets={"MY_TOKEN": StaticSecret(value=SecretStr("secret-panel"))} - ), - ) - conv = LocalConversation(agent, workspace=str(tmp_path)) - try: - with pytest.warns(DeprecationWarning, match=r"ACPAgent\.acp_env"): - env = self._run_start_capturing_env(agent, tmp_path, state=conv.state) - finally: - conv.close() - assert env.get("MY_TOKEN") == "acp-env-wins" - def test_none_value_secret_not_injected(self, tmp_path): """A StaticSecret with value=None is not added to the subprocess env.""" from openhands.sdk.secret import StaticSecret @@ -6865,35 +6661,6 @@ def test_empty_string_secret_not_injected(self, tmp_path): env = self._run_start_capturing_env(agent, tmp_path) assert "EMPTY_SECRET" not in env - def test_acp_env_still_injected(self, tmp_path): - """``acp_env`` (user arbitrary env vars) is still injected at spawn.""" - agent = _make_agent(acp_env={"MY_TOKEN": "acp-env-value"}) - with pytest.warns(DeprecationWarning, match=r"ACPAgent\.acp_env"): - env = self._run_start_capturing_env(agent, tmp_path) - assert env.get("MY_TOKEN") == "acp-env-value" - - def test_empty_acp_env_does_not_warn(self, tmp_path): - """An empty ``acp_env`` must not emit the deprecation warning.""" - import warnings - - agent = _make_agent() - with warnings.catch_warnings(record=True) as caught: - warnings.simplefilter("always") - self._run_start_capturing_env(agent, tmp_path) - assert not [w for w in caught if "acp_env" in str(w.message)] - - -class _CountingLookupSecret(SecretSource): - """A lookup source that records each ``get_value()`` call (to assert it is - *not* invoked when ``acp_env`` shadows the key).""" - - stored_value: str - calls: list[int] = Field(default_factory=list) - - def get_value(self) -> str | None: - self.calls.append(1) - return self.stored_value - class _BrokenSecret(SecretSource): """A source whose ``get_value()`` raises, to verify a failing lookup is @@ -6913,7 +6680,7 @@ class TestACPSecretRegistryEnvInjection: registry at ``LocalConversation.__init__`` (below ``request.secrets``), so the registry is the single channel ``_start_acp_server`` injects from. - Same-key precedence is ``acp_env > secret_registry > os.environ``. + Same-key precedence is ``secret_registry > os.environ``. Registry secrets override ambient ``os.environ`` so an explicit per-conversation/provider secret wins over a same-named server env var. """ @@ -7021,33 +6788,6 @@ def test_registry_lookup_secret_injected_into_subprocess_env(self, tmp_path): ) assert env.get("OPENAI_API_KEY") == "sk-fake-openai" - def test_acp_env_takes_precedence_over_registry_secret(self, tmp_path): - """An explicit ``acp_env`` entry wins over the same key in the registry.""" - agent = _make_agent(acp_env={"GITHUB_TOKEN": "from-acp-env"}) - env = self._run_start_capturing_env( - agent, - tmp_path, - registry_secrets={"GITHUB_TOKEN": "from-registry"}, - ) - assert env.get("GITHUB_TOKEN") == "from-acp-env" - - def test_acp_env_shadow_skips_registry_lookup(self, tmp_path): - """``acp_env`` shadowing a key must not trigger ``get_value()``. - - LookupSecret performs an HTTP request in production; calling it for - a key that ``acp_env`` is about to override wastes a round-trip and - can emit spurious lookup-failure warnings. - """ - secret = _CountingLookupSecret(stored_value="from-registry") - agent = _make_agent(acp_env={"GITHUB_TOKEN": "from-acp-env"}) - env = self._run_start_capturing_env( - agent, - tmp_path, - registry_secrets={"GITHUB_TOKEN": secret}, - ) - assert env.get("GITHUB_TOKEN") == "from-acp-env" - assert secret.calls == [] - def test_request_secret_wins_and_context_only_secret_still_reaches_env( self, tmp_path ): @@ -7088,9 +6828,10 @@ def test_request_secret_wins_and_context_only_secret_still_reaches_env( def test_empty_registry_does_not_change_behaviour(self, tmp_path): """An empty secret_registry must not raise or alter the spawn env.""" - agent = _make_agent(acp_env={"FOO": "bar"}) + agent = _make_agent() env = self._run_start_capturing_env(agent, tmp_path, registry_secrets=None) - assert env.get("FOO") == "bar" + # No secrets to inject; spawn still succeeds and produces an env dict. + assert isinstance(env, dict) def test_failing_registry_lookup_swallowed(self, tmp_path): """A secret source that raises is dropped, not propagated. @@ -7166,7 +6907,7 @@ class TestACPEnvConflictSuppression: route the bearer to a proxy that rejects it, breaking auth silently. _start_acp_server must strip the conflicting vars regardless of where they - came from: acp_env, os.environ, secret_registry, or agent_context.secrets. + came from: os.environ, secret_registry, or agent_context.secrets. The strip is keyed on the token, not on CLAUDE_CONFIG_DIR (#3588). """ @@ -7251,29 +6992,13 @@ async def _fake_filter(_src, _dst): return captured - def test_oauth_token_suppresses_api_key_from_acp_env(self, tmp_path): - """ANTHROPIC_API_KEY from acp_env is stripped when the OAuth token is set.""" - agent = _make_agent( - acp_env={ - "CLAUDE_CODE_OAUTH_TOKEN": "oauth-tok", - "ANTHROPIC_API_KEY": "sk-conflict", - "ANTHROPIC_BASE_URL": "https://proxy.example.com", - } - ) - env = self._run_start_capturing_env(agent, tmp_path) - - assert env["CLAUDE_CODE_OAUTH_TOKEN"] == "oauth-tok" - assert "ANTHROPIC_API_KEY" not in env - assert "ANTHROPIC_BASE_URL" not in env - def test_oauth_token_suppresses_api_key_from_os_environ(self, tmp_path): """ANTHROPIC_API_KEY leaking in from os.environ is stripped too.""" - agent = _make_agent( - acp_env={"CLAUDE_CODE_OAUTH_TOKEN": "oauth-tok"}, - ) + agent = _make_agent() env = self._run_start_capturing_env( agent, tmp_path, + registry_secrets={"CLAUDE_CODE_OAUTH_TOKEN": "oauth-tok"}, extra_os_env={ "ANTHROPIC_API_KEY": "sk-leaked", "ANTHROPIC_BASE_URL": "https://proxy.example.com", @@ -7318,7 +7043,6 @@ def test_oauth_token_suppresses_api_key_from_secrets(self, tmp_path): from openhands.sdk.secret import StaticSecret agent = _make_agent( - acp_env={"CLAUDE_CODE_OAUTH_TOKEN": "oauth-tok"}, agent_context=AgentContext( secrets={ "ANTHROPIC_API_KEY": StaticSecret( @@ -7330,8 +7054,11 @@ def test_oauth_token_suppresses_api_key_from_secrets(self, tmp_path): } ), ) - with pytest.warns(DeprecationWarning, match=r"ACPAgent\.acp_env"): - env = self._run_start_capturing_env(agent, tmp_path) + env = self._run_start_capturing_env( + agent, + tmp_path, + registry_secrets={"CLAUDE_CODE_OAUTH_TOKEN": "oauth-tok"}, + ) assert "CLAUDE_CODE_OAUTH_TOKEN" in env assert "ANTHROPIC_API_KEY" not in env @@ -7339,10 +7066,12 @@ def test_oauth_token_suppresses_api_key_from_secrets(self, tmp_path): def test_no_suppression_without_oauth_token(self, tmp_path): """Without the OAuth token, ANTHROPIC_API_KEY passes through unchanged.""" - agent = _make_agent( - acp_env={"ANTHROPIC_API_KEY": "sk-valid"}, + agent = _make_agent() + env = self._run_start_capturing_env( + agent, + tmp_path, + registry_secrets={"ANTHROPIC_API_KEY": "sk-valid"}, ) - env = self._run_start_capturing_env(agent, tmp_path) assert env.get("ANTHROPIC_API_KEY") == "sk-valid" assert "CLAUDE_CODE_OAUTH_TOKEN" not in env @@ -7352,13 +7081,15 @@ def test_config_dir_alone_does_not_suppress_api_key(self, tmp_path): strip ANTHROPIC_API_KEY. The config dir is a location lever (data-dir isolation), orthogonal to auth mode — keying the strip on it used to delete a working API key during isolation.""" - agent = _make_agent( - acp_env={ + agent = _make_agent() + env = self._run_start_capturing_env( + agent, + tmp_path, + registry_secrets={ "CLAUDE_CONFIG_DIR": "/tmp/claude-isolated", "ANTHROPIC_API_KEY": "sk-valid", - } + }, ) - env = self._run_start_capturing_env(agent, tmp_path) assert env["CLAUDE_CONFIG_DIR"] == "/tmp/claude-isolated" assert env.get("ANTHROPIC_API_KEY") == "sk-valid" @@ -8332,30 +8063,6 @@ def test_reads_reserved_secret_seeded_from_agent_context(self, tmp_path): assert auth_file.read_text(encoding="utf-8") == '{"a": 1}' assert "CODEX_AUTH_JSON" not in env - def test_acp_env_pin_wins_and_credential_seeds_where_it_points(self, tmp_path): - """An explicit acp_env[CODEX_HOME] keeps its precedence, and the - credential is seeded *there* so the file and env stay consistent.""" - from openhands.sdk.secret import StaticSecret - - pinned = tmp_path / "pinned_codex" - # Pre-create the pinned dir with deliberately wide (0755) perms. - pinned.mkdir() - pinned.chmod(0o755) - agent = _make_agent(acp_env={"CODEX_HOME": str(pinned)}) - state = self._state(tmp_path) - state.secret_registry.update_secrets( - {"CODEX_AUTH_JSON": StaticSecret(value=SecretStr('{"k": 1}'))} - ) - env = self._run_start(agent, state, conn=self._make_conn()) - - assert env["CODEX_HOME"] == str(pinned) - # The credential lands under the pinned dir, not the conversation root. - assert (pinned / "auth.json").read_text(encoding="utf-8") == '{"k": 1}' - # The pinned dir's user-chosen perms are NOT silently narrowed... - assert pinned.stat().st_mode & 0o777 == 0o755 - # ...but the credential file itself is still 0600. - assert (pinned / "auth.json").stat().st_mode & 0o777 == 0o600 - def test_fallback_root_when_not_persisted(self, tmp_path): """With no persistence_dir, the file lands under the workspace tree — still seed-if-absent, no TemporaryDirectory.""" @@ -8568,13 +8275,6 @@ def test_unknown_command_no_ops(self, tmp_path): assert "CODEX_HOME" not in env assert "CLAUDE_CONFIG_DIR" not in env - def test_acp_env_pin_wins(self, tmp_path): - agent = self._agent(["codex-acp"], acp_env={"CODEX_HOME": "/pinned/codex"}) - state = self._H._state(tmp_path) - with patch.dict("os.environ", {}, clear=True): - env = self._H._run_start(agent, state, conn=self._H._make_conn()) - assert env["CODEX_HOME"] == "/pinned/codex" - def test_falls_back_to_workspace_when_not_persisted(self, tmp_path): agent = self._agent(["codex-acp"]) state = self._H._state(tmp_path, persisted=False) diff --git a/tests/sdk/context/test_agent_context_serialization.py b/tests/sdk/context/test_agent_context_serialization.py index 96037f14ef..df75ebdd33 100644 --- a/tests/sdk/context/test_agent_context_serialization.py +++ b/tests/sdk/context/test_agent_context_serialization.py @@ -89,7 +89,8 @@ def test_agent_context_secrets_round_trip_through_cipher_context(): """``AgentContext.secrets`` raw-string values must round-trip cleanly when re-validated with a cipher. - Regression for the same bug class as ``ACPAgent.acp_env``: the + Regression for the same bug class as any secret-bearing dict field + (e.g. MCP ``env``/``headers``): the field has a ``field_serializer`` that encrypts under cipher context (via :func:`serialize_secret`) but until now had no matching ``field_validator``. So ciphertext survived diff --git a/tests/sdk/conversation/goal/test_judge.py b/tests/sdk/conversation/goal/test_judge.py index 0be4c2702f..4606310575 100644 --- a/tests/sdk/conversation/goal/test_judge.py +++ b/tests/sdk/conversation/goal/test_judge.py @@ -25,7 +25,6 @@ def completion( self, messages, tools=None, - _return_metrics=False, add_security_risk_prediction=False, on_token=None, **kwargs, @@ -35,7 +34,6 @@ def completion( return super().completion( messages, tools, - _return_metrics, add_security_risk_prediction, on_token, **kwargs, diff --git a/tests/sdk/conversation/remote/test_remote_request_logging.py b/tests/sdk/conversation/remote/test_remote_request_logging.py index 552ac4b689..f0f41a9d98 100644 --- a/tests/sdk/conversation/remote/test_remote_request_logging.py +++ b/tests/sdk/conversation/remote/test_remote_request_logging.py @@ -66,11 +66,11 @@ def test_redacts_secret_keys(self): def test_redacts_all_values_in_environment_keys(self): data = { "environment": {"VAR1": "val1", "VAR2": "val2"}, - "acp_env": {"NESTED": {"deep": "value"}}, + "env": {"NESTED": {"deep": "value"}}, } result = sanitize_dict(data) assert result["environment"] == {"VAR1": "", "VAR2": ""} - assert result["acp_env"] == {"NESTED": {"deep": ""}} + assert result["env"] == {"NESTED": {"deep": ""}} def test_preserves_structure_in_lists(self): data = [{"api_key": "secret"}, {"name": "test"}] @@ -123,7 +123,7 @@ def test_send_request_redacts_structured_error_content(caplog): "input": { "agent": { "llm": {"api_key": "secret-api-key"}, - "acp_env": {"OPENAI_API_KEY": "secret-openai-key"}, + "env": {"OPENAI_API_KEY": "secret-openai-key"}, }, "environment": { "LMNR_PROJECT_API_KEY": "secret-lmnr-key", diff --git a/tests/sdk/llm/test_return_metrics_deprecation.py b/tests/sdk/llm/test_return_metrics_deprecation.py deleted file mode 100644 index 468f417423..0000000000 --- a/tests/sdk/llm/test_return_metrics_deprecation.py +++ /dev/null @@ -1,88 +0,0 @@ -"""Deprecation of the no-op ``_return_metrics`` / ``return_metrics`` parameter. - -Tracks Q1 of #3341: the parameter has no effect (metrics are always returned -via ``LLMResponse.metrics``) and is scheduled for removal in 1.29.0. Until then -it stays in the public signatures and emits a ``DeprecationWarning`` only when a -caller actually passes it. -""" - -import warnings - -import pytest -from litellm.types.utils import ( - Choices, - Message as LiteLLMMessage, - ModelResponse, - Usage, -) -from pydantic import SecretStr - -from openhands.sdk.llm import LLM, Message, TextContent - - -_MSGS = [Message(role="user", content=[TextContent(text="hi")])] - - -def _mock_response(content: str = "ok", model: str = "gpt-4o") -> ModelResponse: - return ModelResponse( - id="resp-1", - choices=[ - Choices( - finish_reason="stop", - index=0, - message=LiteLLMMessage(content=content, role="assistant"), - ) - ], - created=1, - model=model, - object="chat.completion", - usage=Usage(prompt_tokens=10, completion_tokens=5, total_tokens=15), - ) - - -def _llm(model: str = "gpt-4o") -> LLM: - return LLM(model=model, api_key=SecretStr("k"), usage_id="test") - - -def _has_return_metrics_warning(records) -> bool: - return any( - issubclass(r.category, DeprecationWarning) - and "_return_metrics" in str(r.message) - for r in records - ) - - -def test_return_metrics_emits_deprecation_warning(monkeypatch): - llm = _llm() - monkeypatch.setattr( - "openhands.sdk.llm.llm.litellm_completion", lambda **kw: _mock_response() - ) - - with pytest.warns(DeprecationWarning, match="_return_metrics"): - llm.completion(_MSGS, _return_metrics=True) - - -def test_no_warning_when_not_passed(monkeypatch): - llm = _llm() - monkeypatch.setattr( - "openhands.sdk.llm.llm.litellm_completion", lambda **kw: _mock_response() - ) - - with warnings.catch_warnings(record=True) as records: - warnings.simplefilter("always") - llm.completion(_MSGS) - - assert not _has_return_metrics_warning(records) - - -def test_no_warning_when_falsy(monkeypatch): - llm = _llm() - monkeypatch.setattr( - "openhands.sdk.llm.llm.litellm_completion", lambda **kw: _mock_response() - ) - - with warnings.catch_warnings(record=True) as records: - warnings.simplefilter("always") - llm.completion(_MSGS, _return_metrics=False) - - assert not _has_return_metrics_warning(records) diff --git a/tests/sdk/persisted_settings_baselines/v1/agent_settings_acp.json b/tests/sdk/persisted_settings_baselines/v1/agent_settings_acp.json index 028c7d9cdf..1344e73c41 100644 --- a/tests/sdk/persisted_settings_baselines/v1/agent_settings_acp.json +++ b/tests/sdk/persisted_settings_baselines/v1/agent_settings_acp.json @@ -2,16 +2,10 @@ "__expected__": { "agent_kind": "acp", "acp_server": "claude-code", - "acp_model": "claude-opus-4-6", - "acp_env": { - "OPENAI_API_KEY": "sk-acp-fixture" - } + "acp_model": "claude-opus-4-6" }, "schema_version": 1, "agent_kind": "acp", "acp_server": "claude-code", - "acp_model": "claude-opus-4-6", - "acp_env": { - "OPENAI_API_KEY": "sk-acp-fixture" - } + "acp_model": "claude-opus-4-6" } diff --git a/tests/sdk/profiles/test_agent_profile.py b/tests/sdk/profiles/test_agent_profile.py index 41f988652c..ab7b50423f 100644 --- a/tests/sdk/profiles/test_agent_profile.py +++ b/tests/sdk/profiles/test_agent_profile.py @@ -295,8 +295,8 @@ def test_acp_profile_persists_no_secret_fields() -> None: dumped = ACPAgentProfile(name="acp", acp_server="claude-code").model_dump( mode="json" ) - # No embedded credential and no acp_env-style secret bag on the profile. - for key in ("llm", "api_key", "acp_env", "secrets", "agent_context"): + # No embedded credential and no secret bag on the profile. + for key in ("llm", "api_key", "secrets", "agent_context"): assert key not in dumped diff --git a/tests/sdk/profiles/test_resolver.py b/tests/sdk/profiles/test_resolver.py index 04389117e3..aa37d9bb80 100644 --- a/tests/sdk/profiles/test_resolver.py +++ b/tests/sdk/profiles/test_resolver.py @@ -281,8 +281,7 @@ def test_acp_resolves_to_settings_without_credentials( assert settings.acp_args == ["--flag"] assert settings.mcp_config is not None assert list(settings.mcp_config.mcpServers.keys()) == ["fetch"] - # No credential is injected; the deprecated channels stay empty. - assert settings.acp_env == {} + # No credential is injected; the deprecated llm channel stays empty. assert settings.llm.api_key is None assert isinstance(settings.create_agent(), ACPAgent) diff --git a/tests/sdk/test_settings.py b/tests/sdk/test_settings.py index 546806d7ce..e62ba4bbdb 100644 --- a/tests/sdk/test_settings.py +++ b/tests/sdk/test_settings.py @@ -164,7 +164,6 @@ def test_acp_agent_settings_export_schema_has_acp_section() -> None: "acp_server", "acp_command", "acp_args", - "acp_env", "acp_model", "acp_session_mode", "acp_prompt_timeout", @@ -1178,19 +1177,6 @@ def test_acp_resolve_provider_env_custom_server_empty() -> None: assert settings.resolve_provider_env() == {} -def test_acp_resolve_acp_env_returns_only_user_entries() -> None: - # Provider creds are no longer folded into acp_env; resolve_acp_env returns - # only the user's explicit env vars. The provider api_key now flows through - # agent_context.secrets instead (see create_agent). - settings = ACPAgentSettings( - acp_server="claude-code", - llm=LLM(model="claude-opus-4-6", api_key=SecretStr("sk-ui-key")), - acp_env={"MY_CUSTOM_VAR": "value"}, - ) - - assert settings.resolve_acp_env() == {"MY_CUSTOM_VAR": "value"} - - def test_acp_create_agent_ignores_llm_credentials() -> None: # llm.api_key/base_url are deprecated (llm is removed in 1.33.0) and no # longer folded into agent_context.secrets: an LLM-profile base_url would @@ -1206,13 +1192,11 @@ def test_acp_create_agent_ignores_llm_credentials() -> None: base_url="https://llm-proxy.example.com", ), agent_context=context, - acp_env={"MY_VAR": "v"}, ) with pytest.warns(DeprecationWarning, match=r"ACPAgentSettings\.llm is deprecated"): agent = settings.create_agent() - assert agent.acp_env == {"MY_VAR": "v"} # The caller's secrets pass through untouched; no provider creds appear. assert agent.agent_context is not None assert dict(agent.agent_context.secrets or {}) == {"GITHUB_TOKEN": "ghp_test"} @@ -1229,7 +1213,6 @@ def test_acp_create_agent_credentials_warn_even_without_context() -> None: with pytest.warns(DeprecationWarning, match=r"ACPAgentSettings\.llm is deprecated"): agent = settings.create_agent() - assert agent.acp_env == {} assert agent.agent_context is None @@ -1261,24 +1244,6 @@ def test_acp_create_agent_passes_caller_context_through() -> None: assert agent.agent_context is context -def test_acp_env_emits_deprecation_warning() -> None: - # acp_env is deprecated (removed in 1.29.0); using it warns so callers - # migrate to the secret_registry channel before the field is deleted. - settings = ACPAgentSettings(acp_server="claude-code", acp_env={"MY_VAR": "v"}) - with pytest.warns(DeprecationWarning, match=r"ACPAgentSettings\.acp_env"): - assert settings.resolve_acp_env() == {"MY_VAR": "v"} - - -def test_acp_env_empty_does_not_warn() -> None: - import warnings - - settings = ACPAgentSettings(acp_server="claude-code") - with warnings.catch_warnings(record=True) as caught: - warnings.simplefilter("always") - settings.resolve_acp_env() - assert not [w for w in caught if "acp_env" in str(w.message)] - - def test_llm_agent_settings_public_alias_removed() -> None: """The deprecated ``LLMAgentSettings`` public import aliases were removed in v1.24.0; the class itself is retained (internal-only) for the union.""" @@ -1369,62 +1334,6 @@ def test_conversation_settings_agent_settings_field_accepts_both_variants() -> N # --------------------------------------------------------------------------- -def test_acp_agent_settings_acp_env_redacted_by_default() -> None: - settings = ACPAgentSettings( - acp_command=["echo", "test"], - acp_env={"OPENAI_API_KEY": "sk-real-secret"}, - ) - - assert settings.acp_env["OPENAI_API_KEY"] == "sk-real-secret" - assert "sk-real-secret" not in settings.model_dump_json() - assert settings.model_dump(mode="json")["acp_env"] == { - "OPENAI_API_KEY": "**********" - } - - exposed = settings.model_dump(mode="json", context={"expose_secrets": True}) - assert exposed["acp_env"] == {"OPENAI_API_KEY": "sk-real-secret"} - - -def test_acp_agent_settings_acp_env_encrypts_with_cipher() -> None: - """ACP env persistence should mirror other secret-bearing settings. - - The on-disk path encrypts values with a cipher, and loading with the same - cipher must recover plaintext so ACP agents receive usable environment - variables after settings are read back. - """ - from openhands.sdk.utils.cipher import Cipher - - settings = ACPAgentSettings( - acp_command=["echo", "test"], - acp_env={"OPENAI_API_KEY": "sk-real-secret"}, - ) - cipher = Cipher(secret_key="test-encryption-key") - - dumped = settings.model_dump(mode="json", context={"cipher": cipher}) - encrypted_value = dumped["acp_env"]["OPENAI_API_KEY"] - - assert encrypted_value.startswith("gAAAA") - assert "sk-real-secret" not in json.dumps(dumped) - - restored = ACPAgentSettings.model_validate(dumped, context={"cipher": cipher}) - assert restored.acp_env == {"OPENAI_API_KEY": "sk-real-secret"} - - restored_from_persisted = validate_agent_settings( - dumped, context={"cipher": cipher} - ) - assert isinstance(restored_from_persisted, ACPAgentSettings) - assert restored_from_persisted.acp_env == {"OPENAI_API_KEY": "sk-real-secret"} - - legacy_plaintext = ACPAgentSettings.model_validate( - { - "acp_command": ["echo", "test"], - "acp_env": {"OPENAI_API_KEY": "sk-legacy-plaintext"}, - }, - context={"cipher": cipher}, - ) - assert legacy_plaintext.acp_env == {"OPENAI_API_KEY": "sk-legacy-plaintext"} - - def test_openhands_agent_settings_mcp_config_redacts_env_and_headers() -> None: mcp_config = MCPConfig.model_validate( { diff --git a/uv.lock b/uv.lock index cb02664147..795e540f36 100644 --- a/uv.lock +++ b/uv.lock @@ -2458,7 +2458,7 @@ wheels = [ [[package]] name = "openhands-agent-server" -version = "1.28.0" +version = "1.29.0" source = { editable = "openhands-agent-server" } dependencies = [ { name = "aiosqlite" }, @@ -2491,7 +2491,7 @@ requires-dist = [ [[package]] name = "openhands-sdk" -version = "1.28.0" +version = "1.29.0" source = { editable = "openhands-sdk" } dependencies = [ { name = "agent-client-protocol" }, @@ -2543,7 +2543,7 @@ provides-extras = ["boto3"] [[package]] name = "openhands-tools" -version = "1.28.0" +version = "1.29.0" source = { editable = "openhands-tools" } dependencies = [ { name = "binaryornot" }, @@ -2574,7 +2574,7 @@ requires-dist = [ [[package]] name = "openhands-workspace" -version = "1.28.0" +version = "1.29.0" source = { editable = "openhands-workspace" } dependencies = [ { name = "openhands-agent-server" },