Repository navigation
fix(sdk): mask model output in the durable MessageEvent - #4783
Conversation
Only tool output was masked on the standard Agent path: bash output in the terminal executor and MCP tool output. The model's own response text was never masked, so a secret the model echoed back was persisted verbatim into the MessageEvent — the copy that is stored, replayed to the LLM, rendered, exported and POSTed to webhooks. Mask at _emit_message_event, the single point where both _handle_content_response and _handle_no_content_response build that event, shared by the sync and async paths. reasoning_content is masked as well since it is the only text a reasoning-only response carries. Signed provider payloads (thinking_blocks, responses_reasoning_item) are left untouched so their signatures stay valid. An empty registry short-circuits, so the non-secret path copies nothing. Fixes #4678
Endpoint auditContract: pinned release artifact
Actionable client-only calls (0)none Actionable server-only operations (18)
Documented non-divergences (13)Client calls intentionally absent from the filtered contract (11)
Reason: Operational Agent Server endpoints intentionally excluded from the filtered public release artifact.
Reason: Client-ahead API stacked on the pending Agent Server meta-profiles implementation.
Reason: Client-ahead API stacked on the pending Agent Server pre-flight LLM validation endpoint. Server operations covered by an exposed browser URL (2)
Reason: RemoteWorkspace.startWorkspaceSession exposes these authenticated URLs for browser iframe and file requests; they are not HttpClient method calls. |
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Review feedback: trim the docstring to the one non-obvious constraint and drop the section banner in the tests. Also removes an empty-registry short-circuit that was wrong. Masking reads _exported_values, which is a separate field from secret_sources: EventService.activate_credential_binding pops a name from secret_sources via model_copy while the resolved value stays tracked, so the guard skipped masking a live credential that mask_secrets_in_output would still have masked. Calling it unconditionally is both correct and cheap — an empty _exported_values makes it a no-op. Covered by test_content_response_masks_value_dropped_from_secret_sources.
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
enyst
left a comment
There was a problem hiding this comment.
Well. If we mask in Observations, they should never get here… well, unless the user did a trick.
Well, I don’t feel strongly about it, I do think too much code always leads to bugs, and repeating it all over… well, actually, it’s kind of good, right? Repeating it a lot means the next agent will be reminded of it every time it gets it into context. 🤔 Let’s do it…
my point is that If i give a token to the LLM I don't want that my token is saved into the trajectories |
HUMAN:
Fix leakage of secrets.
AGENT:
Why
SecretRegistry.mask_secrets_in_outputhad, outsideacp_agent.py, exactly two non-test call sites —openhands-tools/openhands/tools/terminal/impl.py:371(bash output) andopenhands-sdk/openhands/sdk/mcp/tool.py:187(MCP tool output). Both are tool output. Nothing masked the model's own response text on the standardAgentpath.That copy is the durable one. A secret the model echoes back was persisted verbatim into the
MessageEventthat is stored, replayed to the LLM on the next turn, rendered, exported, and POSTed to webhooks.This is the off-ACP half of the problem. On ACP the situation was already the better one:
_handle_updatemasks each chunk and_finalize_successful_turnre-masks the joined text at the persistence boundary, so the durable ACP record was already safe (covered today byTestACPStepMasksPersistedTurn::test_finish_action_masks_secret_split_across_chunks). Off ACP there was no masking at either end.Summary
MessageEventat_emit_message_event(openhands-sdk/openhands/sdk/agent/response_dispatch.py) — the single point where both_handle_content_responseand_handle_no_content_responsebuild that event, shared by the sync (step) and async (astep) paths.reasoning_contentalongsidecontent: it is the only text a reasoning-only response carries, so maskingcontentalone would be a no-op for exactly one of the two callers.secret_sourceslist (see Notes).Issue Number
Fixes #4678
How to Test
New regression tests in
tests/sdk/agent/test_response_dispatch.py, driven through the existing_run_single_stepharness (a realagent.step()with a cannedLLMResponse, not a direct call to the masking helper):To confirm they actually catch the bug, revert only the source file and re-run — all three masking tests fail against the previous behaviour:
Wider suites, to show nothing else moved:
The two failures are
tests/cross/test_remote_conversation_live_server.py::test_openai_chat_completions_gateway_over_real_serverand::test_settings_and_secrets_api_with_live_server. Both are pre-existing onorigin/main— verified by stashing this branch's changes and re-running the same file on a clean tree, which produces the identical two failures.pre-commit run --files <both changed files>passes (ruff format, ruff lint, pycodestyle, pyright, import rules, tool registration).Video/Screenshots
Not applicable — no user-facing surface. The observable behaviour is the assertion output above.
Type
Notes
Acceptance criteria. Both are met. "A secret split across two chunks is masked in the durable message on both the
Agentand ACP paths" — the ACP half already held and is covered by the existing test named above; theAgenthalf istest_content_response_masks_secret_joined_from_stream_chunks. "Model response text containing a registered secret is masked on the standardAgentpath" —test_content_response_masks_registered_secret.Why no stream snapshot. The issue sketches a snapshot compiled at stream open. That is not needed for these acceptance criteria and is deliberately left out of this PR: deltas are accumulated before the
Messageis constructed, so the durable copy is built from the joined text and the join-boundary mask catches a secret that straddles two chunks. Masking the deltas themselves belongs with the streaming work in #4671.Self-review catch (fixed in 37341bd). The first push guarded the mask with
if not registry.secret_sources: return message. That was wrong. Masking reads_exported_values, a separate field fromsecret_sources, andEventService.activate_credential_bindingpops a name out ofsecret_sourcesviamodel_copywhile the resolved value stays tracked for masking. In that window the guard skipped masking a live credential thatmask_secrets_in_outputwould still have masked — a leak in exactly the credential-binding flow. Reproduced directly against the registry:The guard is gone; masking is unconditional, which is also cheaper to reason about (an empty
_exported_valuesmakes it a no-op). Regression test:test_content_response_masks_value_dropped_from_secret_sources, verified to fail with the guard restored.Deliberately not masked.
thinking_blocksandresponses_reasoning_itemare signed provider payloads; rewriting their text invalidates the signature replayed on the next request.Known gap, not in scope here.
ActionEvent.thoughton the tool-call path (agent.py:1318) is still unmasked —_handle_tool_callsdoes not route through_emit_message_event. Happy to fold it in if you would rather it landed together; I kept this PR to the one chokepoint the acceptance criteria name.Lock cost.
agent.step()runs under the state lock, andmask_secrets_in_outputresolves uncached sources, which may do blocking I/O. Exposure is unchanged in practice: it is one call per agent step, it already has failed-lookup backoff, values are normally resolved and cached well before the model replies, and with no secrets registered it degrades to an empty loop.terminal/impl.pyalready calls it from inside a step.Not verified locally.
.github/scripts/check_sdk_api_breakage.pycould not run here (griffe fails to fetch the baseline from PyPI in this environment). The change adds no public API — one private static method, no changed signatures.🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:37341bd-pythonRun
All tags pushed for this build
About Multi-Architecture Support
37341bd-python) is a multi-arch manifest supporting both amd64 and arm6437341bd-python-amd64) are also available if needed