Conversation
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
The PR has two independent changes:
-
agent.py: Moves async response secret masking into a worker thread (
asyncio.to_thread) while releasing the conversation state lock, preventing event-loop blocking when uncachedLookupSecretsources need network I/O. This reuses the existing_released_state_lock_during_iopattern already used for the LLM completion call. -
check_sdk_api_breakage.py: Converts
_ACCEPTED_REMOVED_MEMBERSfrom afrozensetto adictwith per-member justification strings, and addsLLM.modify_params(removed in #4954) as an accepted removal. Clean refactor, well-tested.
Behavioral asymmetry (eval flag)
The async path now masks the message before classify_response and passes the already-masked message to the response handlers. The sync path (_step) does not mask before classify_response — masking happens later in _emit_message_event.
This means for tool-call responses in the async path, the "thought" content stored in ActionEvent is now masked (<secret-hidden>), while in the sync path it remains unmasked. This is arguably more correct (secrets shouldn't appear in action event thoughts), but it is a behavioral difference between sync and async execution paths.
The agent's actual decision-making (tool calls, arguments) is unaffected since _mask_secrets only touches content and reasoning_content, not tool_calls. However, since this changes what gets recorded in events, I'm flagging it for a human maintainer to evaluate whether lightweight evals are needed before merge.
Test quality
The new test is solid — it uses a real loopback HTTP server, a real TestLLM, and exercises the full async agent path to verify masking end-to-end. Not just mocks.
CI script change
The frozenset → dict conversion is clean. The per-member justification strings are a nice improvement over the previous hardcoded message. The test verifies the exact acceptance behavior including negative cases.
[RISK ASSESSMENT]
- Overall PR: 🟡 MEDIUM — The agent.py change introduces a behavioral asymmetry between sync and async paths (masked vs unmasked thought content in tool-call action events). The change is unlikely to affect benchmark performance since tool call arguments are untouched, but it does change what gets persisted in events. No eval evidence is provided. A human maintainer should decide whether lightweight evals are warranted.
VERDICT: Needs human maintainer judgment on eval impact before merge.
KEY INSIGHT: The async path now masks earlier than sync, creating a subtle asymmetry in action event thought content — functionally harmless but worth a deliberate eval decision.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review: fix(sdk): resolve async response secrets outside the event loop
Summary
This PR makes two changes:
-
agent.py: For non-tool-call responses in the async path (
_astep), secret masking now runs in a worker thread viaasyncio.to_threadwhile the conversation state lock is released via_released_state_lock_during_io(). This prevents blocking the event loop when uncachedLookupSecretsources need to resolve via HTTP. -
check_sdk_api_breakage.py: Converts
_ACCEPTED_REMOVED_MEMBERSfrom afrozensetto adictwith per-member justification strings, and addsLLM.modify_paramsas an accepted removal (removed upstream in PR #4954 after its deprecation runway).
Analysis
agent.py change: The approach is sound. _released_state_lock_during_io() is already used for the LLM network call at line 937, and the same pattern is correctly applied here. The SecretRegistry._exported_values dict is protected by its own RLock, so concurrent access from the worker thread while the state lock is released is safe. After the first masking call caches resolved secret values, the subsequent synchronous _mask_secrets call inside _emit_message_event (response_dispatch.py:305) uses the cache and doesn't trigger blocking I/O. Text masking is idempotent, so the double-masking is a minor inefficiency, not a correctness issue.
check_sdk_api_breakage.py change: Clean refactor. The dict lookup preserves the existing _is_accepted_removed_member behavior (in check), and the per-member justification is surfaced in the diagnostic notice. The new test correctly verifies that only modify_params (not drop_params) is accepted, and that the diagnostic references PR #4954.
Tests: Both new tests exercise real code paths — the loopback server test validates actual HTTP resolution + masking through the real agent execution path, and the parity test confirms sync/async behavioral equivalence for tool thoughts.
Risk Assessment
🟡 MEDIUM — This PR changes agent execution behavior (the async response masking path). Per the repo review policy, PRs that change agent behavior should not be approved without eval evidence. The PR description does not include a link to the eval monitor showing a completed benchmark run, and no human maintainer has confirmed eval results.
Recommendation: Flagging for human maintainer review. Please run lightweight evals to confirm no regression in agent response handling before merging.
Verdict
No material bugs found. The code is correct, well-tested, and the approach is consistent with existing patterns. Leaving a COMMENT (not APPROVE) per the eval-risk policy for agent behavior changes.
a851881 to
3786250
Compare
|
🚦 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. |
1eb2747 to
846a78f
Compare
3786250 to
1a84afa
Compare
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR moves non-tool-call response secret masking in _astep off the event loop: it runs _mask_secrets in a worker thread (asyncio.to_thread) while releasing the conversation state lock via _released_state_lock_during_io(). This avoids blocking the event loop when uncached LookupSecret resolution performs network I/O (potentially calling back into the same agent-server). Tool-call responses are correctly excluded, preserving sync/async parity for tool thought handling.
The tests exercise real agent execution paths (loopback HTTP server, TestLLM, real Conversation.arun/run) rather than mocks — good.
Findings
Redundant double-masking for non-tool-call responses (minor, non-blocking)
For CONTENT and REASONING_ONLY | EMPTY responses, _astep now masks the message at line 1029, then passes the already-masked message into _handle_content_response / _handle_no_content_response, which call _emit_message_event -> _mask_secrets a second time. The second call is idempotent and cheap (secrets are already cached from the first call, so no I/O), so this is not a correctness bug — just a redundant pass. Not worth blocking on, but worth being aware of.
Eval/benchmark risk — flagging for human maintainer review
This PR changes the async agent execution path (secret masking timing and thread context). Per the repo review policy, changes to agent behavior should be validated with lightweight evals before merging. No eval monitor link or human-confirmed benchmark results are present in the PR description or comments. I am leaving a COMMENT rather than APPROVE so a human maintainer can decide whether to run evals before merging.
Risk Assessment
MEDIUM — The change is well-scoped and the approach is sound (releasing the lock for I/O, offloading to a worker thread). The primary risk is behavioral: it alters the async execution path timing for secret resolution, which could plausibly affect benchmark performance. Recommend a human maintainer run lightweight evals before merging.
Verdict: Worth merging after eval validation. The code is correct and the tests are solid.
|
All review threads are now resolved, including the non-blocking double-masking observation. The focused regression tests and full CI pass, and the official stack is repaired. Please approve this top PR if the async secret-masking change looks good. I will re-request review now.\n\n_This comment was created by an AI agent (OpenHands) on behalf of @neubig._ |
1a84afa to
8cef037
Compare
There was a problem hiding this comment.
Review from Astra:
P2 — The final masking pass can still block the event loop and leave a secret unmasked. The new code masks in a worker thread, but _emit_message_event subsequently calls the same resolving masker synchronously.
If update_secrets() adds a LookupSecret while the first lookup is pending, the first pass’s snapshot misses it. The final pass discovers it and performs its HTTP request on the event loop.
I reproduced this with real TestLLM, Conversation.arun(), and loopback HTTP servers:
- The event loop stalled for 30.03 seconds.
- After the lookup timed out, the emitted response contained
<secret-hidden> beta-dummy-credential-456: the newly registered dummy credential remained unmasked.
That contradicts the resolved review thread’s assumption that the second pass necessarily performs no external I/O. Async emission should never invoke an I/O-capable masker synchronously.
|
Addressed the asynchronous lookup race in 24679a6. Async response emission no longer calls an I/O-capable masker synchronously. It resolves in a worker thread, rechecks the registry under the conversation lock, and repeats when another secret was registered. Secret updates now acquire the same state lock. A real TestLLM/Conversation.arun regression chains three loopback lookups, registering the next secret during each previous lookup. The run finishes within the five-second guard and masks all three values. All three focused tests pass in 1.65 seconds; type checking and pre-commit checks pass. Requested all-hands-bot review on the new head. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Taste Rating: 🟢 Good taste
The PR cleanly offloads blocking secret resolution (e.g. LookupSecret HTTP calls) from the event loop to a worker thread during async response processing, while releasing the conversation state lock around the I/O boundary.
Key design observations:
-
Retry loop for concurrent secret registration — The
while Trueloop in_astepsnapshotssecret_sourcesbefore masking, releases the lock for theasyncio.to_threadcall, then re-acquires the lock and checks if the registry is stable. Ifupdate_secretsregistered a new source during the lookup, the loop re-masks. This correctly handles the edge case where aLookupSecretcallback registers additional secrets. Both reads ofsecret_sourceshappen under the lock, so the comparison is race-free. -
mask_secrets=Falseflag — Passing this to_handle_content_response/_handle_no_content_responsein the async path prevents a redundant second masking pass in_emit_message_event, since masking already happened in the worker thread. The sync_steppath still uses the defaultmask_secrets=True, preserving sync/async behavioral parity (both produce a fully masked message). -
update_secretslock acquisition — Wrappingself._state.secret_registry.update_secrets(secrets)inwith self._state:is the correct fix. Before this PR,update_secretsmodifiedsecret_sourceswithout the state lock — safe in the sync-only world but now necessary since the async path releases the lock during masking, and a worker-thread lookup callback can callupdate_secretsconcurrently. -
Tool-call responses are not pre-masked — The
if response_type is not LLMResponseType.TOOL_CALLS:guard ensures tool-call thoughts go through the existing shared path (_get_action_event), matching sync behavior. The parity test verifies this. -
Tests are solid — All three tests use real loopback HTTP servers and real agent execution paths (TestLLM + Conversation.arun), not mocks. The chained-lookup test directly exercises the retry loop by registering new
LookupSecretsources during each HTTP response.
Prior review concerns (behavioral asymmetry from pre-masking before classification, redundant double-masking) are resolved in this commit: classification happens first, and mask_secrets=False skips the second pass.
Risk Assessment: 🟢 LOW
This PR changes the timing and mechanism of secret masking (worker thread vs event loop) but does not change agent decision-making, tool calling, prompt templates, or loop logic. The LLM sees the same message; only the emitted MessageEvent has secrets masked, which was already the case before. No eval/benchmark impact is expected.
Verdict: ✅ Worth merging
Key Insight: The retry-until-stable pattern is the right primitive for reconciling unlocked I/O with a mutable secret registry — it avoids per-method locking on the registry while guaranteeing the final emitted message is fully masked.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
Code review for PR #4967 (fix(sdk): resolve async response secrets outside the event loop) has been posted to GitHub as an APPROVE review. Summary of review findings:
Risk Assessment: 🟢 LOW — changes masking timing/mechanism but not agent decision-making, tool calling, or loop logic. No eval/benchmark impact expected. Review posted: #4967 (review) This comment was posted by an AI agent (OpenHands). |
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR offloads non-tool-call response secret masking in _astep to a worker thread via asyncio.to_thread, releasing the conversation state lock during the I/O boundary. A retry-until-stable loop handles the case where a LookupSecret callback registers new sources during the lookup. The mask_secrets=False flag passed to the response handlers prevents the redundant synchronous second masking pass in _emit_message_event. update_secrets is now properly wrapped in with self._state:.
Analysis
Retry loop (agent.py:1026-1037): Correct. Snapshots secret_sources under the lock, releases the lock for the asyncio.to_thread masking call, re-acquires the lock and compares. If new sources were registered during the lookup, re-masks. Both reads of secret_sources happen under the state lock, so the comparison is race-free.
mask_secrets=False (response_dispatch.py:324-326): Correctly prevents the synchronous second masking pass that the prior P2 review (enyst, commit 8cef037) flagged as a blocking-I/O risk on the event loop. The retry loop ensures the message is fully masked before being passed to the handlers.
update_secrets lock (local_conversation.py:2772-2773): Correct fix. Before this PR, update_secrets modified secret_sources without the state lock — safe in a sync-only world but now necessary since the async path releases the lock and the retry loop relies on consistent lock-protected snapshots.
Tool-call exclusion: The if response_type is not LLMResponseType.TOOL_CALLS guard correctly preserves sync/async parity for tool thoughts. The parity test verifies this.
Tests: All three tests use real loopback HTTP servers, TestLLM, and real Conversation.arun/run — not mocks. The third test directly exercises the retry loop by chaining LookupSecret registrations during HTTP responses.
Risk Assessment
🟡 MEDIUM — The code is correct and well-tested. The prior P2 concern (synchronous blocking on the event loop) is fully resolved. However, this changes the async agent execution path (masking timing and thread context). Per the repo eval-risk policy, changes to agent behavior should be validated with lightweight evals before merging. No eval monitor link or human-confirmed benchmark results are present in the PR description or comments.
Recommendation: Flagging for human maintainer review. The change is unlikely to affect benchmark performance since it only alters masking timing, not agent decision-making — but a human maintainer should confirm.
Verdict: No material bugs found. Worth merging after eval validation.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
The PR moves async response secret masking into a worker thread (asyncio.to_thread) while releasing the conversation state lock, preventing event-loop blocking when uncached LookupSecret sources require network I/O. It also fixes update_secrets to correctly hold the state lock when mutating the secret registry, aligning with the repo concurrency convention.
The design is sound:
- Registry stability loop: The
while Trueloop takes a snapshot ofsecret_sourcesunder the lock, releases the lock for the masking I/O, then reacquires and checks if new sources were registered during the await. This correctly handles the case where aLookupSecretcallback registers additional secrets (demonstrated by the third test). - Sync/async parity for tool calls: TOOL_CALLS responses are explicitly excluded from pre-masking (
if response_type is not LLMResponseType.TOOL_CALLS), so both paths pass unmasked message content to the tool-call handlers. Themask_secrets=Falseflag passed to content/no-content handlers prevents double-masking. The second test verifies sync/async thought parity. - Concurrency safety:
mask_secrets_in_outputtakeslist(self.secret_sources)(snapshot), and_exported_valuesis protected by its own RLock.update_secretsonly adds entries (never removes), so concurrent reads during the lock-released window can miss new secrets but not crash — the stability loop catches this. update_secretslocking fix: Correctly wraps the registry mutation inwith self._state:, matching the ~13 existing call sites that protect state access.
Test quality
Tests use real loopback HTTP servers, real TestLLM, and exercise full async agent paths — not mocks. The third test (secrets registered during lookup) directly validates the while True stability loop with a cascading lookup pattern.
Eval risk
This touches async agent execution paths (when/where masking happens), but the masking result is identical — secrets end up masked in MessageEvent either way. Agent decision-making is unaffected: tool call arguments are never masked, and classify_response runs on the unmasked message in both paths. The update_secrets locking change could slightly affect concurrency timing but not correctness. Flagging for a human maintainer to decide whether lightweight evals are warranted.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW — The change relocates existing masking logic to a thread and fixes a locking bug. The end-to-end masking behavior is preserved. The
while Trueloop has no max-iteration guard, but unbounded looping would require a pathological recursive secret-registration pattern that is not realistic.
VERDICT: Worth merging. Core logic is sound, concurrency approach is well-designed, and tests cover the key scenarios including the dynamic-registration edge case.
KEY INSIGHT: The registry stability loop elegantly handles the self-referential case where resolving a secret triggers registration of new secrets, without requiring a separate notification mechanism.
|
This comment was posted by an AI agent (OpenHands). |
|
|
Please review the current head and its linked live evidence. If there are no remaining actionable findings, please submit an APPROVE review rather than a COMMENT-only review. If changes are still needed, identify them explicitly. This request is for code-review approval; it does not waive any separate human-note, CI, or dependency requirements. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review: fix(sdk): resolve async response secrets outside the event loop
Taste Rating: 🟢 Good taste
The change moves non-tool-response secret masking to a worker thread in the async path (_astep), releasing the conversation state lock around the I/O boundary so loopback LookupSecret HTTP calls don't deadlock the event loop. The convergence loop correctly handles cascading lookups (where resolving one secret registers another). The update_secrets lock fix is a legitimate thread-safety improvement — the previous code accessed self._state.secret_registry without holding the state lock.
Analysis
-
Concurrency design: The
while Trueconvergence loop snapshotssecret_sourcesunder the lock, releases the lock for theto_threadmasking call, then re-checks under the lock. Snapshot and comparison are both under the lock — no TOCTOU. The_released_state_lock_during_iocontext manager is deadlock-safe (no-op if the thread doesn't own the lock). -
Sync/async parity: Tool-call responses skip pre-masking in both paths (tool masking happens separately via
mask_secrets_in_model). Non-tool responses: sync path masks inline in_emit_message_event(defaultmask_secrets=True), async path pre-masks and passesmask_secrets=False. Equivalent behavior. Testtest_async_tool_thought_matches_sync_behaviorverifies this. -
Thread safety of
_mask_secretsin worker thread:mask_secrets_in_outputuseslist(self.secret_sources)(key snapshot) and_exported_values_lockfor the values. CPython GIL makes dict reads safe against concurrentupdate_secretswrites. Acceptable. -
update_secretslock fix: Previously unlocked access toself._state.secret_registry; now properly wrapped inwith self._state:. This aligns with the repo convention that allself._state.*accesses must hold the lock.
No material issues found
No bugs, security concerns, or design flaws identified. The change is focused and proportional to the problem (31s → <1s Canvas stall from loopback secret deadlock). Tests cover the three key scenarios: loopback resolution, sync/async parity, and cascading registration during lookup.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW
- Changes async execution mechanics for secret masking only; no change to agent reasoning, prompts, tool calling, or planning logic.
- No version bumps, no dependency changes, no breaking API changes.
VERDICT: ✅ Worth merging
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
The PR moves non-tool-call response secret masking in _astep off the event loop: _mask_secrets runs in a worker thread (asyncio.to_thread) while the conversation state lock is released via _released_state_lock_during_io(). A retry-until-stable loop handles the case where a LookupSecret callback registers new sources during the lookup. The mask_secrets=False flag passed to _handle_content_response / _handle_no_content_response prevents the redundant synchronous second masking pass in _emit_message_event — this resolves the P2 blocking-I/O concern raised by @enyst. update_secrets is now correctly wrapped in with self._state:.
Analysis
Retry loop (agent.py:1026–1037): Correct. Snapshots secret_sources under the lock, releases the lock for the asyncio.to_thread masking call, reacquires the lock and compares. Both reads of secret_sources happen under the state lock, so the comparison is race-free. mask_secrets_in_output iterates list(self.secret_sources) (snapshot), and _exported_values is protected by its own RLock, so concurrent update_secrets calls during the lock-released window are safe.
mask_secrets=False (response_dispatch.py:257, 282, 311): Correctly prevents the synchronous second masking pass that the prior P2 review flagged. The retry loop ensures the message is fully masked before being passed to the handlers.
update_secrets lock (local_conversation.py:2772–2773): Correct fix. FIFOLock is reentrant per thread, so sync-path calls from within run() are safe. Async-path calls from callback threads can acquire the released lock.
Sync/async parity: Tool-call responses are excluded from pre-masking (if response_type is not LLMResponseType.TOOL_CALLS), preserving parity. The second test verifies this. Both paths produce fully masked messages for non-tool-call responses.
Tests: All three use real loopback HTTP servers, TestLLM, and full agent execution paths — not mocks. The third test directly exercises the retry loop with cascading LookupSecret registrations.
No material bugs found. The code is unchanged since the last review at bcd7240; the only new commits are docs evidence and automated .pr/ cleanup.
Risk Assessment
🟡 MEDIUM — The code is correct and well-tested. However, this changes the async agent execution path (masking timing and thread context). Per the repo eval-risk policy, changes to agent behavior should be validated with lightweight evals before merging. No eval monitor link or human-confirmed benchmark results are present. The change is unlikely to affect benchmark performance since it only alters masking timing, not agent decision-making — but a human maintainer should confirm.
Verdict: No material findings. Worth merging after eval validation.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
4 participants
HUMAN:
User request, quoted verbatim from @neubig: “OK, create the new PRs and stack 3403 on top of the conversation-scoped API one.”
AGENT:
Why
Uncached secret resolution may perform blocking callbacks during async agent response processing; avoid blocking the event loop while holding conversation state.
Summary
Issue Number
Split from existing PR #3403 at the author’s request; no separate issue was created.
How to Test
uv run pytest -q tests/sdk/agent/test_async_secret_masking.py— 2 passed using real agent execution paths.Video/Screenshots
Not applicable; this changes asynchronous SDK execution behavior.
Design Doc
Not needed for this focused bug fix.
Type
Notes
This is the top PR in the official two-PR stack with #4971.
This PR description was updated by an AI agent (OpenHands) on behalf of @neubig.
Live Agent Canvas evidence
Live isolated Canvas comparison: the same provider reply took 31.014 seconds before and 0.957 seconds after. Before, the saved-secret loopback froze Canvas and timed out; after, the reply was masked and Settings remained responsive.
Before GIF · After GIF · Report and reproduction · Measured evidence. These immutable links retain the artifacts if
.pr/is later cleaned up.🐳 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-slimnikolaik/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:6d4fe23-pythonRun
All tags pushed for this build
About Multi-Architecture Support
6d4fe23-python) is a multi-arch manifest supporting both amd64 and arm646d4fe23-python-amd64) are also available if needed