Skip to content
Closed
86 changes: 83 additions & 3 deletions openhands-sdk/openhands/sdk/context/agent_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from pydantic import (
BaseModel,
Field,
PrivateAttr,
SecretStr,
field_serializer,
field_validator,
Expand Down Expand Up @@ -118,6 +119,61 @@ class AgentContext(BaseModel):
json_schema_extra={"acp_compatible": True},
)

# Snapshot of the skills that ``_load_auto_skills`` added on top of
# the caller-supplied list. The serializer drops only the ones still
# equal to this snapshot — if a downstream consumer replaces an
# auto-loaded skill via ``model_copy(update={'skills': merged})``
# (OpenHands' ``_create_agent_with_skills`` does this when it merges
# in sandbox / repo skills with overlapping names), the replacement
# has different field values and survives serialization. The same
# auto-load will re-run on the receiving end and rebuild the
# auto-loaded subset that wasn't replaced. For a stock configuration
# that turns both flags on (~40 skills bundled under
# ``~/.openhands/skills``) the resolved list is ~260 KB per
# ``AgentContext`` — every ``GET`` on a stored conversation carried
# that. See software-agent-sdk#3301.
_auto_loaded_skills: dict[str, Skill] = PrivateAttr(default_factory=dict)

@field_serializer("skills", when_used="always", mode="wrap")
def _serialize_skills(self, value: list[Skill], handler, info) -> Any:
"""Drop unmodified auto-loaded skills from the serialized output.

The runtime keeps the full resolved list on ``self.skills`` so
prompt rendering and downstream consumers behave exactly as
today. Only the wire payload changes: callers re-loading the
model will trigger ``_load_auto_skills`` again, which rebuilds
the auto-loaded subset deterministically from the same
``load_user_skills`` / ``load_public_skills`` /
``marketplace_path`` configuration.

Equality (not just name match) is required because consumers
like OpenHands' ``_create_agent_with_skills`` replace
auto-loaded skills in-place with their own version under the
same name. A name-only filter would silently drop those
replacements and the receiver would auto-reload the stock
version on the next deserialization.

Opt-out via ``context={"preserve_full_skills": True}``: paths
that need a stable snapshot of the resolved skill catalog (the
most important one being ``ConversationState._save_base_state``
— persistence of the conversation to disk) pass this flag so
the serializer is a no-op. Without it, a paused conversation
resumed after the ``~/.openhands/skills`` directory or the
public marketplace updated would silently pick up the *new*
skill content via ``_load_auto_skills``. The API-response path
skips the flag and gets the byte-size win.

``mode="wrap"`` + ``handler`` delegation preserves caller
options like ``exclude_none``, ``exclude_defaults``, and
nested ``include`` / ``exclude`` for the surviving skills —
manual ``s.model_dump(...)`` would have ignored them.
"""
if info.context and info.context.get("preserve_full_skills"):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: model_dump(round_trip=True) still goes through the lossy API-wire path here and emits skills: [] for auto-loaded skills. round_trip=True is Pydantic's standard signal for a dump that callers can persist/reload without semantic loss; with this serializer, a caller using it for an AgentContext snapshot will reload whatever the current external skill source returns instead of the in-memory catalog they dumped. Please treat info.round_trip like preserve_full_skills and add a regression test for AgentContext(load_public_skills=True).model_dump(round_trip=True) preserving the full skill list.

return handler(value)
auto = self._auto_loaded_skills
kept = [s for s in value if auto.get(s.name) != s]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: This filter still uses the copied private snapshot even if the auto-load config was changed after validation. For example, ctx.model_copy(update={"load_public_skills": False}) keeps the runtime auto-loaded skills but serializes them as []; on AgentContext.model_validate(...) they are not reloaded because the flag is now false. Changing marketplace_path has the same stale-snapshot problem, except the receiver may reload a different catalog. Please either tie the snapshot to the (load_user_skills, load_public_skills, marketplace_path) config and no-op when it no longer matches, or clear/recompute the snapshot when those fields are updated.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: filtering to kept before delegating to handler changes Pydantic's index-based nested include/exclude semantics for skills. Once auto-loaded entries are removed, the caller's indices are applied to the compacted list, so an include/exclude targeting an auto-loaded index can incorrectly serialize or drop an explicit skill. Skip trimming when nested include/exclude targets skills, or apply the trim after serialization while preserving the original index mapping.

return handler(kept)

@field_serializer("secrets", when_used="always")
def _serialize_secrets(
self, value: Mapping[str, SecretValue] | None, info
Expand Down Expand Up @@ -148,7 +204,25 @@ def _validate_skills(cls, v: list[Skill], _info):

@model_validator(mode="after")
def _load_auto_skills(self):
"""Load user and/or public skills if enabled."""
"""Load user and/or public skills if enabled.

Names of skills added here are tracked in
``_auto_loaded_skill_names`` so the serializer can drop them
from the wire payload (this validator re-runs on every model
load, so the same skills repopulate without needing to be
persisted).

Migration: stored conversations created before the serializer
change carry the resolved auto-loaded skill list inlined on
``skills``. When such a conversation is loaded back, the names
match ``existing_names`` and the new-append branch is skipped
— but we still mark them as auto-loaded if the persisted skill
equals what the loader would produce now. Without this, every
old conversation would keep the bloated payload until rewritten.
A persisted skill that no longer matches the loader's current
output (user edited the file, marketplace updated, etc.) stays
treated as explicit so the on-disk content wins.
"""
if not self.load_user_skills and not self.load_public_skills:
return self

Expand All @@ -160,10 +234,16 @@ def _load_auto_skills(self):
marketplace_path=self.marketplace_path,
)

existing_names = {skill.name for skill in self.skills}
existing_by_name = {skill.name: skill for skill in self.skills}
for name, skill in auto_skills.items():
if name not in existing_names:
existing = existing_by_name.get(name)
if existing is None:
self.skills.append(skill)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: appending auto-loaded skills in the after-validator doesn't mark skills as set. As a result, AgentContext(load_public_skills=True).model_dump(round_trip=True, exclude_unset=True) (and the preserve_full_skills variant) omits skills entirely before the serializer can preserve the snapshot, so reloading can pick up a different current skill catalog. If round-trip/preserve dumps are meant to be lossless, mark skills as set when the validator mutates it or avoid relying on a field serializer for this path.

self._auto_loaded_skills[name] = skill

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: This stores the same mutable Skill object that is appended to self.skills, so it is not actually a snapshot. Since Skill is mutable today, an in-place caller edit like ctx.skills[0].content = "custom" mutates the private copy too; equality still succeeds and model_dump() drops the customized skill instead of preserving it. Store an independent snapshot (deep copy or stable serialized fingerprint) and add a regression test for in-place modification.

elif existing == skill:
# Migration path for conversations stored before the
# serializer change — see the docstring.
self._auto_loaded_skills[name] = skill
else:
logger.debug(
f"Skipping auto-loaded skill '{name}' (already in explicit skills)"
Expand Down
12 changes: 11 additions & 1 deletion openhands-sdk/openhands/sdk/conversation/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,18 @@ def _save_base_state(self, fs: FileStore) -> None:

If a cipher is configured, secrets will be encrypted. Otherwise, they
will be redacted (serialized as '**********').

``preserve_full_skills`` is set so the persisted snapshot
includes auto-loaded skills inline — without this the conversation
would silently pick up *new* skill content from
``~/.openhands/skills`` / the public marketplace on resume, since
``AgentContext._load_auto_skills`` re-runs on deserialization.
Persistence needs the freeze-at-create-time snapshot; only the
API-response path opts into the trim. See software-agent-sdk#3301.
"""
context = {"cipher": self._cipher} if self._cipher else None
context: dict[str, Any] = {"preserve_full_skills": True}
if self._cipher:
context["cipher"] = self._cipher
# Warn if secrets exist but no cipher is configured
if not self._cipher and self.secret_registry.secret_sources:
logger.warning(
Expand Down
Loading
Loading