feat(sdk): add cleanup LLM profile for outward agent text

[smolpaws]

HUMAN:

Reviewed the design against the ask_oracle pattern (#3673) and ran the new unit tests plus the profile-store suite locally. Holding for a maintainer nod on the outward-path hook location before wiring it into agent-server surfaces.

AGENT:

Why

An agent's reasoning can be sound while the surface of a message is broken — mojibake from mis-encoded emoji, stray control characters, or a format that does not fit the target channel. This was seen live: an agent repeatedly sent ð / â garbage to humans in a Slack thread. The big reasoning model should not have to police its own output every turn; a small dedicated pass is cheaper and more reliable.

Closes #4343

Summary

• Add clean_outward_text(text, *, cipher=None) in openhands-sdk (openhands/sdk/llm/cleanup_profile.py). It resolves a saved LLM profile named cleanup (CLEANUP_PROFILE_NAME) by convention and runs one stateless completion to repair the message surface only — no meaning change, no added content.
• Export clean_outward_text and CLEANUP_PROFILE_NAME from openhands.sdk.llm.
• Mirror the ask_oracle pattern (feat(sdk): add ask_oracle tool #3673): no agent setting, no wiring. The pass is fail-open — a missing profile, a call error, or an empty reply all return the original text unchanged. make_llm_completion is imported lazily to avoid a circular import at package init.

Issue Number

Closes #4343

How to Test

Run the new unit tests and the profile-store suite:

uv run pytest tests/sdk/llm/test_cleanup_profile.py -q      # 7 passed
uv run pytest tests/sdk/llm/test_llm_profile_store.py -q    # 63 passed

The tests exercise real code paths:

• Happy path — a saved cleanup profile (a capturing TestLLM) repairs "…nudge! ð" → "…nudge."; the call sends exactly [system, user], no tools, no history.
• Missing profile — a real empty LLMProfileStore dir → original text returned unchanged.
• Call failure (exhausted scripted LLM) and empty reply → original text returned.
• Blank input short-circuits without loading a profile.

End-to-end usage: save an LLM under the profile name cleanup via LLMProfileStore().save("cleanup", llm, include_secrets=True), then call clean_outward_text(draft) on the outward path. With no cleanup profile saved, the feature is off and text passes through untouched.

Lint/format/type checks pass locally: ruff check, ruff format --check, pyright — all clean; pre-commit hooks (including import-dependency rules and tool-subclass registration) passed on commit.

Open questions

• Where should the outward-path hook live so it covers agent-server outbound surfaces without touching internal agent/tool messages? This PR adds the primitive; wiring the call site is intentionally left for a follow-up once the hook location is agreed.

---

🐾 From the #proj-agent-canvas thread.

Co-authored-by: smolpaws engel@enyst.org

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR adds an opt-in clean_outward_text primitive that routes an agent's outward, human-facing text through a conventionally-named cleanup LLM profile for surface repair (mojibake, control chars, formatting). It mirrors the ask_oracle pattern: no agent setting, no wiring, fail-open. The change is purely additive — no existing call path is touched — and the feature is off by default (returns the original text when no cleanup profile is saved).

Risk: LOW. Additive, opt-in, fail-open, and well-scoped. I ran the new suite (tests/sdk/llm/test_cleanup_profile.py, 7 passed) and the profile-store suite (tests/sdk/llm/test_llm_profile_store.py, 63 passed) locally; both are green.

Correctness & security

• The fail-open design is sound: FileNotFoundError (missing profile) is handled separately, and a broad except Exception covers load/call failures, so a cleanup problem can never block or corrupt an outward message. LLMProfileStore.load re-raises corruption as ValueError, which the broad handler correctly catches.
• str.format(text=text) is one-pass, so braces in the draft text stay literal — no format-string injection risk.
• No secrets are handled beyond existing Cipher plumbing; the Cipher import is correctly TYPE_CHECKING-only.
• The cleanup prompt is well-constructed to forbid content additions; the trust assumption that the cleanup model obeys it is inherent to the feature and acceptable.

Findings

1. No async variant. clean_outward_text is sync-only. The repo documents that async paths propagate through the full call chain (LLM.acompletion -> ... -> LocalConversation.arun), and amake_llm_completion already exists, so adding aclean_outward_text would be trivial. The outward surface in agent-server is async, so a sync-only primitive will force the eventual call site into an executor wrapper or a second API addition. Since wiring is explicitly deferred, this isn't blocking, but providing the async entry point now (while the primitive is the only thing this PR ships) avoids a follow-up API change.

2. make_llm_completion carries agent-action semantics into a toolless text pass. That helper hard-codes add_security_risk_prediction=True and its docstring describes injecting security-risk/summary fields into tool schemas. For a cleanup completion with no tools this is a no-op (empty tool list -> nothing to decorate), so there is no behavioral bug, and vision_inspect already uses the same helper with tools=[] as precedent. Flagging only as a minor consistency note: the cleanup pass is not an agent action, so a direct llm.completion/llm.responses call would communicate intent more clearly. Not worth changing if you want to keep parity with vision_inspect.

3. LLMProfileStore() is constructed on every call. Each invocation reads from the default profile dir and acquires a file lock. On a hot outward path (every agent message) that is disk I/O + locking per message. Acceptable while the feature is unwired and opt-in, but worth keeping in mind when the call site lands.

4. Test gap: cipher forwarding is untested. clean_outward_text(..., cipher=...) threads the cipher to LLMProfileStore.load, but no test asserts it arrives there — the mocks accept cipher: Any = None and never check. A small assertion that the cipher reaches load would lock in the contract for the encrypted-secrets path.

Tests

Coverage is good for the documented behavior: happy path, missing profile, call failure, empty reply, and blank-input short-circuit all use real TestLLM/LLMProfileStore code paths rather than mocks of the unit under test. One note: the happy-path test asserts the scripted reply is returned verbatim, so it verifies pass-through of the LLM reply rather than actual mojibake repair — fine for a unit test, just worth knowing the repair logic itself isn't exercised here.

Overall this is a clean, well-scoped primitive. The main forward-looking suggestion is adding the async variant before the wiring PR so the async call site doesn't need a workaround.

[all-hands-bot]

LLMProfileStore() is constructed (and the profile read from disk + locked) on every call. On a hot outward path this is per-message disk I/O. Fine while the feature is unwired and opt-in, but worth caching or lazy-loading once the call site lands.

[smolpaws]

Agreed, and left as-is for now on purpose. The feature is unwired and opt-in, so this path is cold today. The right place to cache/lazy-load the store is the call site, once the outward hook lands and we know the per-message cadence — doing it inside the primitive now would be premature. Noting it here so it isn't lost when the wiring PR arrives.

[all-hands-bot]

make_llm_completion hard-codes add_security_risk_prediction=True and injects security/summary fields into tool schemas. With no tools this is a harmless no-op, and vision_inspect uses the same helper with tools=[] as precedent — but the cleanup pass is a pure text repair, not an agent action. A direct llm.completion/llm.responses call would communicate intent more clearly. Not blocking; keep parity with vision_inspect if you prefer.

[smolpaws]

Kept it, for parity with vision_inspect, which calls the same helper with tools=[]. With no tools the add_security_risk_prediction=True is a no-op (nothing to decorate), so there's no behavioral difference, and reusing the shared helper keeps retry/router/streaming handling consistent across the SDK. If you'd rather the intent read as a pure text pass, I'm happy to switch to a direct llm.completion/llm.responses call — just say the word. 🐾

[VascoSch92]

Do you need this?

[smolpaws]

Yes — it's needed. Cipher is imported under TYPE_CHECKING only (to keep it out of the runtime import graph), and it's used in the annotations cipher: Cipher | None. Without from __future__ import annotations those annotations evaluate at def-time and raise NameError: name 'Cipher' is not defined on import. I just verified: removing the line breaks from openhands.sdk.llm import clean_outward_text. ruff/pyright stay green either way (they're static), so the future import is the thing that actually keeps it working at runtime. 🐾

[smolpaws]

Thanks for the reviews 🐾 Pushed e971a68 addressing the feedback:

1. Async variant — done. Added aclean_outward_text(), since the agent-server outward surface is async and amake_llm_completion already exists. Sync and async now share _load_cleanup_llm / _cleanup_messages / _repaired_or_original, so there's one code path and one fail-open contract. Both are exported. This means the eventual wiring PR won't need an executor wrapper or a follow-up API change.

4. Cipher test gap — done. Added test_cipher_is_forwarded_to_profile_load, which asserts the cipher argument actually reaches LLMProfileStore.load (sentinel identity check). Also added tools == [] to the happy-path assertions.

2. make_llm_completion semantics. Kept it, for parity with vision_inspect (which calls the same helper with tools=[]). As the review notes, with no tools the add_security_risk_prediction=True is a no-op, so there's no behavioral difference — and reusing the shared helper keeps retry/router/streaming handling consistent. Happy to switch to a direct llm.completion if you'd prefer the clearer intent.

3. LLMProfileStore() per call. Left as-is for now — the feature is unwired and opt-in, so this is cold. It's the right thing to cache/lazy-load at the call site once the outward hook lands (that PR knows the message cadence); doing it here would be premature. Noted so it isn't lost.

Re: from __future__ import annotations (Vasco's line note) — replied inline: it's required. Cipher is TYPE_CHECKING-only and used in cipher: Cipher | None; without the future import the annotation evaluates at def-time and raises NameError on import (verified).

Tests: 11 passing (added async happy-path / missing-profile / call-failure + the cipher test). ruff, pyright, and pre-commit all green.

[all-hands-bot]

🚦 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 @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: e971a680054f9aa4056c628af5b97c0df6132e7c
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/cb3f8975-8854-4fe8-8276-b3e51f99ebb7

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

⚠️ OpenHands PR Reviewer encountered a problem at commit e971a680054f (status: stuck).

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

Reviewed the new head (e971a68) which adds the async variant aclean_outward_text and a cipher-forwarding test on top of the original commit. The change adds a clean, fail-open primitive that resolves a saved cleanup LLM profile and runs a single stateless completion to repair the surface of outward agent text — no agent setting, no wiring, mirroring the vision_inspect pattern (lazy make_llm_completion, tools=[], profile-store lookup).

Correctness

• Fail-open contract is correct and consistently applied across both sync and async paths: missing profile -> original text; load failure -> original text; call failure -> original text; empty/whitespace reply -> original text; blank input short-circuits before loading a profile.
• _load_cleanup_llm catches FileNotFoundError specifically (feature-off) and broad Exception (corrupt profile / lock timeout) -> None with a warning, so a cleanup problem can never block or corrupt an outward message.
• _repaired_or_original joins TextContent blocks and strips; empty result falls back to the original. Correct.
• The stateless contract is verified by the happy-path test: messages are exactly [system, user], tools == [], no history.

Tests

Coverage is solid for a primitive of this size: happy path (sync + async), missing profile (sync + async), call failure (sync + async), empty reply, blank input (parametrized), and cipher forwarding (sync). The CapturingTestLLM exercises the real completion path through make_llm_completion rather than mocking the helper.

Minor coverage gap (non-blocking): the async path has no cipher-forwarding or empty-reply test. Both are covered transitively via the shared _load_cleanup_llm / _repaired_or_original helpers, so this is a low-priority follow-up rather than a gap that affects confidence.

Security

• No secret leakage: failures are logged with the exception message only, never the draft text. The cipher is forwarded to LLMProfileStore.load for at-rest decryption rather than handled ad hoc.
• The cleanup prompt is narrowly scoped to surface repair and explicitly forbids adding facts/links/promises — appropriate for a pass that touches human-facing text.

Maintainability

• Sync/async share _load_cleanup_llm, _cleanup_messages, and _repaired_or_original; the only duplication is the ~10-line blank-check + lazy-import + try/except shell, consistent with the repo's sync/async pairing convention.
• from __future__ import annotations is required and correct (the Cipher type is under TYPE_CHECKING only).
• The prior review round's three threads are addressed in this head: the async variant was added, and the per-call store construction / make_llm_completion-vs-direct-call points were acknowledged as deferred to the wiring call site (reasonable given the feature is unwired and opt-in today).

Risk Assessment

Low risk. This is an additive, opt-in, fail-open primitive with no wiring into agent-server surfaces yet. It cannot block or corrupt outward messages by construction. The only runtime cost when enabled is one extra small-LLM completion per outward message, controlled entirely by whether a cleanup profile is saved. No material issues found on the current head.