diff --git a/.pr/4694/automated-tests.txt b/.pr/4694/automated-tests.txt deleted file mode 100644 index 00743650a6..0000000000 --- a/.pr/4694/automated-tests.txt +++ /dev/null @@ -1,49 +0,0 @@ -PR #4694 validation summary -Validated source commit: d07c896cc8e740403cd6dae6ed99459d01fd857d -Upstream main: 6a1e4d0f08dcdd02786526a51b7965e1877008bc -Timestamp: 2026-09-09T05:32:14Z - -PASS make build - -PASS uv run pre-commit run --files - Ruff format, Ruff lint, pycodestyle, pyright, import rules, and tool - registration all passed where applicable. - -RED uv run pytest \ - tests/agent_server/test_event_service.py::TestEventServiceRun::test_wait_for_run_completion_waits_for_task_finalization \ - tests/agent_server/test_event_service.py::TestEventServiceRun::test_wait_for_run_completion_timeout_does_not_cancel_run -q - At tests-only commit e424d8902, both failed because EventService had no - wait_for_run_completion method. - -GREEN uv run pytest \ - tests/agent_server/test_event_service.py::TestEventServiceRun::test_wait_for_run_completion_waits_for_task_finalization \ - tests/agent_server/test_event_service.py::TestEventServiceRun::test_wait_for_run_completion_timeout_does_not_cancel_run -q - 2 passed in 0.25s. - -PASS uv run pytest \ - tests/agent_server/test_event_service.py \ - tests/agent_server/test_api_authentication.py \ - tests/agent_server/test_openapi_contract.py \ - tests/agent_server/test_openapi_discriminator.py -q - 149 passed in 45.09s. - -PASS TMUX_TMPDIR=/tmp/oh-responses-4694-final-cross uv run pytest \ - tests/cross/test_remote_conversation_live_server.py::test_openai_chat_completions_gateway_over_real_server \ - tests/cross/test_remote_conversation_live_server.py::test_openai_gateway_replays_frozen_llm_fixtures -q - 2 passed in 19.05s. - -PASS OH_PERSISTENCE_DIR= \ - TMUX_TMPDIR= uv run pytest -m stress -q - 13 passed, 9992 deselected in 132.09s. - -Stress diagnostic: without an isolated persistence directory, the full stress -run passed 12/13 and the timing-ratio assertion in parallel-subagents missed its -budget. That same test also failed on isolated upstream source when it inherited -the host's profiles/plugins. With an empty CI-like persistence directory it -passed alone in 2.02s, and the complete 13-test stress suite passed. - -OpenAPI diagnostic: before replacing the generic OpenAI SDK Response schema, -the branch renamed the canonical OpenHands Action component and failed -test_action_schema_has_discriminator. Isolated upstream main passed the same -test. The narrowed gateway response model restored the canonical schema; all 12 -OpenAPI contract/discriminator tests then passed. diff --git a/.pr/4694/concurrency.json b/.pr/4694/concurrency.json deleted file mode 100644 index 004322571d..0000000000 --- a/.pr/4694/concurrency.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "result": "PASS", - "mode": "concurrent", - "request_count": 3, - "outputs_isolated": true, - "conversation_ids_unique": true -} diff --git a/.pr/4694/live-official-sdk.json b/.pr/4694/live-official-sdk.json deleted file mode 100644 index 61c9865256..0000000000 --- a/.pr/4694/live-official-sdk.json +++ /dev/null @@ -1,16 +0,0 @@ -{ - "result": "PASS", - "mode": "live", - "openai_sdk": "2.33.0", - "model_profile": "openhands_sonnet", - "typed_response": true, - "manual_replay_recalled_nonce": true, - "fresh_conversation_for_replay": true, - "native_conversation_persisted_with_store_false": true, - "invalid_auth_status": 401, - "previous_response_id_status": 400, - "store_true_status": 400, - "stream_true_status": 400, - "unknown_profile_status": 404, - "usage_nonzero": true -} diff --git a/.pr/4694/restart-replay.json b/.pr/4694/restart-replay.json deleted file mode 100644 index 7e23ee37cc..0000000000 --- a/.pr/4694/restart-replay.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "result": "PASS", - "mode": "restart", - "old_native_conversation_status": 404, - "manual_replay_recalled_nonce": true, - "fresh_conversation_after_storage_reset": true -} diff --git a/.pr/4694/validation.md b/.pr/4694/validation.md deleted file mode 100644 index 51b6a9558d..0000000000 --- a/.pr/4694/validation.md +++ /dev/null @@ -1,78 +0,0 @@ -# PR #4694 validation report - -Verdict: ready for review as a stateless-first Responses API gateway. - -Validated source commit: `d07c896cc8e740403cd6dae6ed99459d01fd857d` - -Base: upstream `main` at `6a1e4d0f08dcdd02786526a51b7965e1877008bc` - -Validated at: `2026-09-09T05:32:14Z` - -Environment: macOS 26.5.2, Python 3.13.11, OpenAI Python SDK 2.33.0. - -## Review-driven changes - -- Kept the earlier `store: true` review fix: it returns `400` instead of - promising a retrievable response that the gateway cannot provide. -- Removed the conversation-ID approximation for `previous_response_id`. It did - not identify an exact response turn, accepted forged suffixes, could branch - from the wrong head, and reused the first conversation's model while reporting - the later request's model. The gateway now returns `400` and tells clients to - replay input items. -- Moved top-level `instructions` and system/developer input into the agent's real - system context. They are no longer encoded as user text. -- Added an EventService run-completion primitive so fresh gateway requests wait - through stop-hook retries and pending event publication, without cancelling - the agent run if the HTTP waiter times out. -- Replaced the OpenAI SDK's full generic `Response` type as the FastAPI response - schema with the gateway's supported response shape. The generic type imported - another `Action` schema, renamed OpenHands' canonical `Action` component, and - broke the existing OpenAPI discriminator contract. -- Corrected the documentation: `store: false` is protocol-level statelessness, - not a data-retention control. The backing native OpenHands conversation still - follows normal agent-server persistence. - -## Evidence matrix - -| Scenario | Result | Evidence | -| --- | --- | --- | -| Tests-only red commit followed by green implementation | PASS | [`automated-tests.txt`](automated-tests.txt) | -| EventService lifecycle, auth, OpenAPI contracts | PASS, 149 tests | [`automated-tests.txt`](automated-tests.txt) | -| Real TCP server with official SDK and deterministic LLM fixtures | PASS, 2 tests | [`automated-tests.txt`](automated-tests.txt) | -| Clean-environment agent-server stress suite | PASS, 13 tests | [`automated-tests.txt`](automated-tests.txt) | -| Hosted Sonnet through official `OpenAI` client | PASS | [`live-official-sdk.json`](live-official-sdk.json) | -| Replay actual output items into a fresh conversation | PASS | [`live-official-sdk.json`](live-official-sdk.json) | -| Restart with the old conversation storage absent, then replay client items | PASS | [`restart-replay.json`](restart-replay.json) | -| Three simultaneous hosted requests through `AsyncOpenAI` | PASS | [`concurrency.json`](concurrency.json) | -| Invalid bearer token | PASS, `401` | [`live-official-sdk.json`](live-official-sdk.json) | -| `previous_response_id`, `store: true`, and `stream: true` | PASS, explicit `400` | [`live-official-sdk.json`](live-official-sdk.json) | -| Unknown OpenHands profile | PASS, `404` | [`live-official-sdk.json`](live-official-sdk.json) | - -The live runs used an isolated subprocess server, isolated settings/workspace/ -conversation directories, a copied single profile, bearer authentication, and -the official sync and async OpenAI clients. Evidence is sanitized: it contains -no credentials, environment dump, raw profile data, raw conversation state, or -telemetry. - -## Scope recommendation - -Merge the stateless path in this PR. Clients can preserve context by sending the -prior user input plus the returned output items in the next request. This also -works after a server restart with no prior native conversation available. - -Implement `previous_response_id` separately only with durable -response-ID-to-exact-turn mapping, restart recovery, old-turn branching/forking, -model and instruction replacement, exact-ID validation, and same-parent -concurrency tests. A conversation UUID alone is not sufficient. - -Streaming is useful follow-up scope because agent runs are long-lived. Stored -Responses objects and a `GET /v1/responses/{id}` endpoint are lower priority than -streaming and exact continuation because native OpenHands conversations already -provide persistence. Unsupported OpenAI generation/tool-selection fields remain -documented as ignored; they should be implemented or explicitly rejected before -claiming broader Responses API compatibility. - -## Contract references - -- [Create a response](https://developers.openai.com/api/reference/cli/resources/responses/methods/create) -- [Conversation state](https://developers.openai.com/api/docs/guides/conversation-state) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 3f6988b6a3..b7f62ca53b 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1957,7 +1957,7 @@ def activate_file_credential_binding( self._file_credential_bindings[secret_name] = binding def restart_for_updated_credentials(self, secret_names: Collection[str]) -> None: - configured = {spec.secret_name for spec in self.acp_file_secrets} + configured = {spec.secret_name for spec in self._active_file_secrets()} with self._file_credential_lock: self._replace_file_credentials_on_next_materialisation.update( configured.intersection(secret_names) @@ -2452,7 +2452,7 @@ def _present_file_secret_names(self, state: ConversationState) -> set[str]: (their values are file blobs, not env vars the subprocess can reference by name). """ - configured = {spec.secret_name for spec in self.acp_file_secrets} + configured = {spec.secret_name for spec in self._active_file_secrets()} if not configured: return set() return set(state.secret_registry.secret_sources) & configured @@ -2587,6 +2587,42 @@ def _resolved_provider(self) -> ACPProviderInfo | None: self.acp_server or "" ) or detect_acp_provider_by_command(self.acp_command) + def _active_file_secrets(self) -> list[ACPFileSecretSpec]: + """The file-secret specs that apply to the provider this agent runs. + + Drops the specs that are *another registered provider's* reserved + credential and keeps everything else, so a harness added upstream cannot + change how this provider's conversation treats a secret carrying the new + reserved name (see #4923). A name several providers share stays: it is + this provider's too. + + Deliberately no provenance test. :attr:`acp_file_secrets` defaults to the + union across the registry, but a persisted conversation carries whatever + that union was when it was written, so comparing against today's default + would read an older list as a caller override and silently stop scoping + after an upgrade. Filtering by ownership needs no such distinction, and a + spec for a CLI outside the registry is owned by nobody and always applies. + + An unrecognised server keeps every spec, matching + :meth:`_strip_conflicting_env`: without an identity we cannot tell whose + credential a reserved name belongs to. + """ + provider = self._resolved_provider() + if provider is None: + return list(self.acp_file_secrets) + own = {spec.secret_name for spec in provider.file_secrets} + owned_elsewhere = { + spec.secret_name + for key, info in ACP_PROVIDERS.items() + if key != provider.key + for spec in info.file_secrets + } - own + return [ + spec + for spec in self.acp_file_secrets + if spec.secret_name not in owned_elsewhere + ] + def _strip_conflicting_env(self, env: dict[str, str]) -> None: """Remove env vars that would defeat this provider's own credential. @@ -2615,7 +2651,7 @@ def _strip_conflicting_env(self, env: dict[str, str]) -> None: def _materialise_file_secrets( self, state: ConversationState, env: dict[str, str] ) -> None: - for spec in self.acp_file_secrets: + for spec in self._active_file_secrets(): name = spec.secret_name with self._file_credential_lock: replace_existing = ( @@ -3091,7 +3127,7 @@ async def _init() -> tuple[ or detect_acp_provider_by_agent_name(agent_name) ) configured = _preconfigured_credentials( - auth_provider, self.acp_file_secrets, env + auth_provider, self._active_file_secrets(), env ) if configured: logger.info( diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 28bf16d08c..17c4c5eddd 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -9379,6 +9379,171 @@ def test_settings_pass_file_secrets_through_create_agent(self): } +class TestACPFileSecretProviderScoping: + """``acp_file_secrets`` defaults to the union across every registered + provider, but only the running provider's specs apply (#4923). Without the + scoping, registering a harness upstream changes how an unrelated provider's + conversation treats a secret carrying the new reserved name. + """ + + _H = TestACPFileSecretMaterialisation + + @staticmethod + def _names(agent): + return {spec.secret_name for spec in agent._active_file_secrets()} + + @pytest.mark.parametrize( + "acp_server,expected", + [ + ("claude-code", set()), + ("codex", {"CODEX_AUTH_JSON"}), + ("gemini-cli", {"GOOGLE_APPLICATION_CREDENTIALS_JSON"}), + ("kimi-code", {"KIMI_CODE_CONFIG_TOML"}), + ("pi", {"PI_AUTH_JSON"}), + ("opencode", set()), + ], + ) + def test_scopes_to_the_running_provider(self, acp_server, expected): + agent = _make_agent(acp_server=acp_server) + assert self._names(agent) == expected + + def test_no_provider_claims_another_providers_reserved_secret(self): + """The property the scoping exists to hold, asserted over the registry + so a provider added upstream is covered without editing this test.""" + from openhands.sdk.settings.acp_providers import ACP_PROVIDERS + + for key, info in ACP_PROVIDERS.items(): + others = { + spec.secret_name + for other, other_info in ACP_PROVIDERS.items() + if other != key + for spec in other_info.file_secrets + } - {spec.secret_name for spec in info.file_secrets} + assert self._names(_make_agent(acp_server=key)).isdisjoint(others) + + def test_unrecognised_server_keeps_the_union(self): + """No identity means we cannot tell whose credential a reserved name + belongs to, so stay conservative — as ``_strip_conflicting_env`` does.""" + from openhands.sdk.settings.acp_providers import default_acp_file_secrets + + agent = ACPAgent(acp_command=["some-unknown-acp-server"]) + assert self._names(agent) == { + spec.secret_name for spec in default_acp_file_secrets() + } + + def test_custom_command_resolves_the_provider_from_the_command(self): + agent = ACPAgent(acp_command=["codex-acp"]) + assert self._names(agent) == {"CODEX_AUTH_JSON"} + + def test_specs_outside_the_registry_always_apply(self): + """A downstream CLI's spec is owned by no registered provider, so it is + never filtered out — whichever harness the conversation runs.""" + from openhands.sdk import ACPFileSecretSpec + + custom = ACPFileSecretSpec( + secret_name="MYCLI_TOKEN_JSON", + filename="token.json", + env_var="MYCLI_HOME", + subdir="mycli", + ) + agent = _make_agent(acp_server="codex", acp_file_secrets=[custom]) + assert self._names(agent) == {"MYCLI_TOKEN_JSON"} + + from openhands.sdk.settings.acp_providers import default_acp_file_secrets + + agent = _make_agent( + acp_server="codex", + acp_file_secrets=[custom, *default_acp_file_secrets()], + ) + assert self._names(agent) == {"MYCLI_TOKEN_JSON", "CODEX_AUTH_JSON"} + + def test_a_list_persisted_before_a_provider_was_added_still_scopes(self): + """The upgrade case. A conversation written when the registry held only + Codex and Gemini carries that two-spec list; resumed on a newer SDK it + must still scope, which a comparison against today's default could not + do — the stored list no longer equals it. + """ + from openhands.sdk.settings.acp_providers import ACP_PROVIDERS + + pre_upgrade = [ + *ACP_PROVIDERS["codex"].file_secrets, + *ACP_PROVIDERS["gemini-cli"].file_secrets, + ] + agent = _make_agent(acp_server="claude-code", acp_file_secrets=pre_upgrade) + assert self._names(agent) == set() + + agent = _make_agent(acp_server="codex", acp_file_secrets=pre_upgrade) + assert self._names(agent) == {"CODEX_AUTH_JSON"} + + def test_a_name_several_providers_share_is_kept(self): + """``owned_elsewhere`` subtracts the running provider's own names, so a + spec two providers both claim is not filtered from either.""" + from openhands.sdk import ACPFileSecretSpec + + shared = ACPFileSecretSpec( + secret_name="GOOGLE_APPLICATION_CREDENTIALS_JSON", + filename="gcloud-credentials.json", + env_var="GOOGLE_APPLICATION_CREDENTIALS", + subdir="gemini-cli", + ) + agent = _make_agent(acp_server="gemini-cli", acp_file_secrets=[shared]) + assert self._names(agent) == {"GOOGLE_APPLICATION_CREDENTIALS_JSON"} + + def test_empty_specs_stay_empty(self): + agent = _make_agent(acp_server="codex", acp_file_secrets=[]) + assert self._names(agent) == set() + + def test_scoping_survives_a_serialization_round_trip(self): + """A resumed conversation must scope too — including one written by an + older SDK, which ``test_a_list_persisted_before_a_provider_was_added_ + still_scopes`` covers.""" + agent = _make_agent(acp_server="claude-code") + restored = ACPAgent.model_validate(agent.model_dump()) + assert self._names(restored) == set() + + def test_other_providers_blob_stays_a_plain_env_var(self, tmp_path): + """End to end through ``_start_acp_server``: on a claude-code + conversation ``PI_AUTH_JSON`` is delivered as an ordinary env var, not + materialised to disk with ``PI_CODING_AGENT_DIR`` set.""" + from openhands.sdk.secret import StaticSecret + + agent = _make_agent(acp_server="claude-code") + state = self._H._state(tmp_path) + state.secret_registry.update_secrets( + {"PI_AUTH_JSON": StaticSecret(value=SecretStr("blob"))} + ) + + with patch.dict("os.environ", {}, clear=True): + env = self._H._run_start( + agent, state, conn=self._H._make_conn(agent_name="claude-agent-acp") + ) + + assert env.get("PI_AUTH_JSON") == "blob" + assert "PI_CODING_AGENT_DIR" not in env + assert not (agent._acp_file_secret_dir(state, "pi") / "auth.json").exists() + + def test_own_blob_still_materialises(self, tmp_path): + """The counterpart: on a pi conversation the same secret is written to + disk and the data-dir var points at it.""" + from openhands.sdk.secret import StaticSecret + + agent = _make_agent(acp_server="pi") + state = self._H._state(tmp_path) + state.secret_registry.update_secrets( + {"PI_AUTH_JSON": StaticSecret(value=SecretStr("blob"))} + ) + + with patch.dict("os.environ", {}, clear=True): + env = self._H._run_start( + agent, state, conn=self._H._make_conn(agent_name="pi-acp") + ) + + target = agent._acp_file_secret_dir(state, "pi") / "auth.json" + assert target.read_text() == "blob" + assert env.get("PI_CODING_AGENT_DIR") == str(target.parent) + assert "PI_AUTH_JSON" not in env + + # --------------------------------------------------------------------------- # Per-conversation CLI data-dir isolation (issue #1019) # ---------------------------------------------------------------------------