fix: preserve ThinkingPart fields in OpenCode storage round-trip (#156) - #232
fix: preserve ThinkingPart fields in OpenCode storage round-trip (#156)#232Million-mo wants to merge 3 commits into
Conversation
ThinkingPart.id, provider_name, signature, and provider_details were silently dropped during OpenCode storage serialization. The write path (provider.py) only saved content as ReasoningPart.text, and the read path (helpers.py) only restored content — all other fields defaulted to None. This caused send-back to degrade to tags mode: pydantic-ai's _map_response_thinking_part() checks id + provider_name to decide between field mode (reasoning_content) and tags mode. Without these fields, thinking content was wrapped in tags instead of sent via the proper reasoning_content field. Fix: store the extra fields in ReasoningPart.metadata during write, and restore them when constructing ThinkingPart during read. The metadata field already exists on ReasoningPart and is a dict[str, Any], making it a natural extensibility bucket without model changes. Backward compatible: old data without metadata produces ThinkingPart with None defaults, same as before.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Fork PRs don't have access to GitHub Actions secrets, so OPENAI_API_KEY is empty. The OpenAI SDK client constructor validates the key at init time and raises 'Missing credentials' before VCR can intercept HTTP calls. This causes VCR and E2E smoke tests to fail on all fork PRs. Inject a dummy key when the real one is absent. VCR cassette replay and TestModel-based tests never make real HTTP calls, so the dummy key is never used. When the real secret IS available (same-repo PRs), this is a no-op.
949968b to
0e78c01
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that ThinkingPart metadata fields (id, provider_name, signature, and provider_details) are preserved during the OpenCode storage round-trip. It also updates the test configuration to inject a dummy OPENAI_API_KEY for fork PRs to prevent test failures, and adds a new test suite verifying the round-trip behavior. Feedback was provided to simplify the metadata extraction logic in helpers.py using a dictionary mapping and explicit 'is not None' checks to assist static type checkers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| thinking_kwargs: dict[str, Any] = {"content": part.text} | ||
| meta = part.metadata | ||
| if meta: | ||
| if meta.get("thinking_id") is not None: | ||
| thinking_kwargs["id"] = meta["thinking_id"] | ||
| if meta.get("provider_name") is not None: | ||
| thinking_kwargs["provider_name"] = meta["provider_name"] | ||
| if meta.get("signature") is not None: | ||
| thinking_kwargs["signature"] = meta["signature"] | ||
| if meta.get("provider_details") is not None: | ||
| thinking_kwargs["provider_details"] = meta["provider_details"] | ||
| response_parts.append(ThinkingPart(**thinking_kwargs)) |
There was a problem hiding this comment.
We can simplify the mapping of metadata keys to ThinkingPart arguments by using a dictionary mapping and a loop. This reduces nested if statements and makes the code more maintainable. Ensure we use an explicit is not None check on part.metadata directly in the conditional statement to allow static type checkers like mypy to narrow the type correctly, avoiding implicit truthiness checks or intermediate variables.
| thinking_kwargs: dict[str, Any] = {"content": part.text} | |
| meta = part.metadata | |
| if meta: | |
| if meta.get("thinking_id") is not None: | |
| thinking_kwargs["id"] = meta["thinking_id"] | |
| if meta.get("provider_name") is not None: | |
| thinking_kwargs["provider_name"] = meta["provider_name"] | |
| if meta.get("signature") is not None: | |
| thinking_kwargs["signature"] = meta["signature"] | |
| if meta.get("provider_details") is not None: | |
| thinking_kwargs["provider_details"] = meta["provider_details"] | |
| response_parts.append(ThinkingPart(**thinking_kwargs)) | |
| thinking_kwargs: dict[str, Any] = {"content": part.text} | |
| if part.metadata is not None: | |
| mapping = { | |
| "thinking_id": "id", | |
| "provider_name": "provider_name", | |
| "signature": "signature", | |
| "provider_details": "provider_details", | |
| } | |
| for meta_key, kwarg_key in mapping.items(): | |
| if (val := part.metadata.get(meta_key)) is not None: | |
| thinking_kwargs[kwarg_key] = val | |
| response_parts.append(ThinkingPart(**thinking_kwargs)) |
References
- Avoid using implicit truthiness checks or intermediate boolean variables to check for the existence of optional dictionary/mapping parameters when type-narrowing is required. Use explicit
is not Nonechecks directly in the conditional statement so that static type checkers like mypy can correctly narrow the type.
VCR cassettes were recorded against api.ai.rootcloud.info, but fork PRs don't have the OPENAI_BASE_URL secret. Without it, the OpenAI SDK targets api.openai.com, and VCR can't replay the cassette (even with match_on= ['method']) — resulting in 'Connection error' on all VCR tests.
1 participant
Summary
Fixes #156
ThinkingPart.id,provider_name,signature, andprovider_detailswere silently dropped during OpenCode storage serialization. The write path (provider.py) only savedcontentasReasoningPart.text, and the read path (helpers.py) only restoredcontent— all other fields defaulted toNone.This caused send-back to degrade to tags mode: pydantic-ai's
_map_response_thinking_part()checksid + provider_nameto decide between field mode (reasoning_content) and tags mode. Without these fields, thinking content was wrapped in<think>tags instead of sent via the properreasoning_contentfield.Solution
Store the extra fields in
ReasoningPart.metadataduring write, and restore them when constructingThinkingPartduring read. Themetadatafield already exists onReasoningPartand is adict[str, Any], making it a natural extensibility bucket without model changes.Backward compatible: old data without metadata produces
ThinkingPartwithNonedefaults, same as before.Changes
src/agentpool_storage/opencode_provider/provider.pyid/provider_name/signature/provider_detailsintoReasoningPart.metadatasrc/agentpool_storage/opencode_provider/helpers.pymetadatawhen constructingThinkingParttests/sessions/test_opencode_thinking_roundtrip.pyRebase Note
Rebased from
develop/agenticontomain— the original PR (#222) was closed due to branch divergence. Applied cleanly with no conflicts.