Remove acp_env and _return_metrics deprecations (removed_in 1.29.0)

Both features reached their scheduled removal version in 1.29.0:

- Drop the no-op _return_metrics/return_metrics parameter from
  LLM.{completion,acompletion,responses,aresponses}, RouterLLM.completion,
  and the TestLLM doubles. Metrics are always on LLMResponse.metrics.
- Remove the acp_env field end-to-end: ACPAgentSettings and ACPAgent
  (field/validators/serializers), ACPAgentSettings.resolve_acp_env(), the
  ACP spawn-time env-injection/precedence logic, and the
  REDACT_ALL_VALUES_KEYS entry. Arbitrary ACP subprocess env vars now ride
  the conversation secrets channel.

Updates tests, the v1 ACP persisted-settings golden fixture, the
persisted-settings-compat generator, and docstrings/AGENTS.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: Simon Rosenberg

.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

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.
 

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.
 

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:

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}}}}}}
 

openhands-sdk/openhands/sdk/agent/acp_agent.py

@@ -83,11 +83,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,
 )
 
 
@@ -1358,49 +1356,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=(
@@ -2092,9 +2047,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
@@ -2109,21 +2062,17 @@ 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.
         """
         provider = detect_acp_provider_by_command(self.acp_command)
         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)
@@ -2136,53 +2085,32 @@ 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
         a stale pasted blob can't overwrite the live one. Files are ``0600`` in
         ``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 <subdir>.
-                    # 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 <subdir>.
+                # 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
@@ -2210,14 +2138,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",
@@ -2239,12 +2164,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
@@ -2253,17 +2177,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
@@ -2272,28 +2185,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:
@@ -2312,7 +2217,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)