Skip to content

fix(sdk): remove title from LocalConversation.fork() — store it on StoredConversation instead - #4814

Open
BSmick6 wants to merge 8 commits into
OpenHands:mainfrom
BSmick6:claude/fervent-panini-f9fcbb
Open

fix(sdk): remove title from LocalConversation.fork() — store it on StoredConversation instead#4814
BSmick6 wants to merge 8 commits into
OpenHands:mainfrom
BSmick6:claude/fervent-panini-f9fcbb

Conversation

@BSmick6

@BSmick6 BSmick6 commented Sep 1, 2026

Copy link
Copy Markdown

HUMAN:
Inspired by some digging I did for #4617. Turns out there are some duplicate values between StoredConversation and ConversationState. I discovered this because tags is one of them. So I had to worry about syncing, which was actually an already existing issue in update_conversation.

Anyway, while considering whether tags should be pulled from ConversationState, I discovered that, when forking with a title, the title is written into the forked conversation's _state.tags. On its own, that's worth fixing.

In this change, it may look like the title is being removed completely, but the agent explains below how the functionality is preserved via fork_overrides.


AGENT:

Why

Forking a LocalConversation with a title injected {"title": "<value>"} into state.tags, polluting the user-defined tag map. This conflated a display/metadata field with user-defined tags and leaked into tag-based queries and any downstream consumer iterating over tags.

ConversationState has no title field (title belongs on StoredConversation in the agent-server layer). Rather than add a duplicate field, the title parameter on LocalConversation.fork() is deprecated: it is still accepted to avoid a breaking API change, but emits a DeprecationWarning and no longer writes into state.tags.

Summary

  • Deprecates title on LocalConversation.fork() with a DeprecationWarning — the parameter is kept for backwards compatibility but has no effect; ConversationState has no title field and adding one would duplicate StoredConversation
  • Removes the tags["title"] injection workaround from the fork body (the fix for the actual bug)
  • Drops the title=title passthrough in conversation_service.py — the agent server applies title via fork_overrides on StoredConversation independently, so passing it to fork() would only trigger the deprecation warning unnecessarily
  • RemoteConversation.fork(title=...) is unaffected — it routes through the agent server which owns StoredConversation

Issue Number

Fixes #4811

How to Test

uv run pytest tests/sdk/conversation/local/test_fork.py tests/sdk/conversation/remote/test_remote_fork.py -v

All 23 tests pass. test_fork_with_title_is_deprecated verifies that title= emits a DeprecationWarning and that "title" does not appear in state.tags.

For the agent-server path, title on a forked conversation is set via fork_overrides["title"] in conversation_service.py — that path is unchanged.

Video/Screenshots

N/A — pure logic change with no UI impact.

Design Doc

N/A — follow-up discussion on whether the standalone SDK should eventually surface fork titles (e.g. a LocalConversation.title property backed by a proper field) is tracked on #4811.

Type

  • Bug fix
  • Feature
  • Refactor
  • Breaking change
  • Docs / chore

Notes

The title parameter remains fully functional on RemoteConversation.fork() since the agent server handles it correctly via StoredConversation. An intentional eventual removal of the deprecated parameter from LocalConversation.fork() should follow the SDK's deprecation policy.

🤖 Generated with Claude Code

…oredConversation instead

The previous workaround injected the fork title into `state.tags["title"]`,
polluting the user-defined tag map. Since `ConversationState` has no title
field (title lives on `StoredConversation` in the agent-server layer), the
`title` parameter is removed from `LocalConversation.fork()` entirely.

The agent server already applies the title via `fork_overrides` on
`StoredConversation` independent of what `fork()` does, so server-side
behaviour is unchanged.

Fixes OpenHands#4811

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@all-hands-bot

Copy link
Copy Markdown
Collaborator

👋 This PR needs a couple of things fixed before OpenHands can review it:

  • the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

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.

@all-hands-bot

Copy link
Copy Markdown
Collaborator

🚦 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.

@enyst enyst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@enyst
enyst enabled auto-merge (squash) September 1, 2026 21:18
@enyst
enyst disabled auto-merge September 1, 2026 21:34
@enyst

enyst commented Sep 1, 2026

Copy link
Copy Markdown
Member

@OpenHands why is the Python API check failing in CI? Is the workflow correct we should worry, or could it be more lenient?

@openhands-ai

openhands-ai Bot commented Sep 1, 2026

Copy link
Copy Markdown

I'm on it! enyst can track my progress at all-hands.dev

enyst commented Sep 2, 2026

Copy link
Copy Markdown
Member

Hi — I’m OpenHands-GPT-5.6, enyst’s AI agent.

The check is failing for a real and very specific reason: Griffe sees LocalConversation.fork(title) disappear from the published openhands-sdk API. Removing that keyword means existing standalone-SDK code that calls conversation.fork(title="...") will now raise TypeError. The job annotation is:

LocalConversation.fork(title): Parameter was removed
Breaking changes detected (1); require at least minor version bump from 1.44.x, but new is 1.44.1

So the detection is correct, and I would not make the detector ignore this case. The bug fix should stop writing the title into state.tags, but that does not require removing the accepted keyword immediately. The least disruptive fix for this PR is to retain title: str | None = None on LocalConversation.fork() and no longer write it into tags (optionally deprecating it/documenting that local conversations do not persist titles). That fixes the tag pollution without breaking callers. An intentional eventual removal should follow the SDK's deprecation policy rather than being classified as a patch-level bug fix.

There is, however, a workflow-policy mismatch worth making more lenient:

  • #2173 originally made API failures non-blocking on ordinary PRs and blocking only on rel-* PRs.
  • #2432 later made them visibly red on every PR, while explicitly saying they remain optional unless branch protection requires them.
  • gh pr checks 4814 --required confirms Python API is not a required check here; only pre-commit is currently required and failing.
  • Nevertheless, the review bot treats any red check as a reason not to review. That effectively turns this optional diagnostic into a gate.
  • A feature PR also cannot simply bump the package minor version to satisfy this check: the version-bump guard allows version changes only in release PRs.

My recommendation is therefore: worry about the API break in this PR, but separately relax the bot/workflow integration. Either have the bot gate only on required checks, or restore non-release continue-on-error while preserving the annotation/report. The fork origin is not the cause; it only prevents the workflow’s read-only token from posting its sticky report comment.

This comment was generated by an AI agent (OpenHands) on behalf of enyst.

@openhands-ai

This comment was marked as duplicate.

BSmick6 and others added 6 commits September 2, 2026 09:34
…f removing it

Removing the parameter outright is a breaking API change per Griffe's
public-API check. Restore it with a DeprecationWarning so existing callers
don't get a TypeError, while still fixing the tag-pollution bug (the value
is accepted but no longer written into state.tags).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…py docstring

- Replace raw warnings.warn(..., DeprecationWarning) with warn_deprecated()
  from openhands.sdk.utils.deprecation, adding version metadata
  (deprecated_in="1.44.1", removed_in="1.47.0") consistent with other SDK
  deprecations (e.g. LLM.modify_params)
- Update BaseConversation.fork() abstract docstring to note that title is
  deprecated on LocalConversation (still honoured by RemoteConversation)
- Update test match= pattern to match the deprecation library's message format

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…n policy

AGENTS.md specifies removals must target at least 5 minor releases after
deprecated_in. deprecated_in=1.44.1 → removed_in=1.50.0, not 1.47.0.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: fork sets title by injecting into state.tags instead of a dedicated field

3 participants