-
Notifications
You must be signed in to change notification settings - Fork 5
fix: preserve _meta during ACP serialization #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -199,6 +199,7 @@ def rename_types(output_path: Path) -> list[str]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| content = _apply_field_overrides(content) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| content = _apply_default_overrides(content) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| content = _add_description_comments(content) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| content = _ensure_custom_base_model(content) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| alias_lines = [f"{old} = {new}" for old, new in sorted(RENAME_MAP.items())] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| alias_block = BACKCOMPAT_MARKER + "\n" + "\n".join(alias_lines) + "\n" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -220,6 +221,37 @@ def rename_types(output_path: Path) -> list[str]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return warnings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _ensure_custom_base_model(content: str) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if "class BaseModel(_BaseModel):" in content: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return content | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lines = content.splitlines() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for idx, line in enumerate(lines): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not line.startswith("from pydantic import "): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| imports = [part.strip() for part in line[len("from pydantic import ") :].split(",")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| has_config = any(part == "ConfigDict" for part in imports) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new_imports = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for part in imports: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if part == "BaseModel": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new_imports.append("BaseModel as _BaseModel") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| has_alias = True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new_imports.append(part) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not has_alias: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new_imports.append("BaseModel as _BaseModel") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not has_config: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new_imports.append("ConfigDict") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lines[idx] = "from pydantic import " + ", ".join(new_imports) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| insert_idx = idx + 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lines.insert(insert_idx, "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lines.insert(insert_idx + 1, "class BaseModel(_BaseModel):") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lines.insert(insert_idx + 2, " model_config = ConfigDict(populate_by_name=True)") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lines.insert(insert_idx + 3, "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+229
to
+251
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not line.startswith("from pydantic import "): | |
| continue | |
| imports = [part.strip() for part in line[len("from pydantic import ") :].split(",")] | |
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | |
| has_config = any(part == "ConfigDict" for part in imports) | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| has_alias = True | |
| else: | |
| new_imports.append(part) | |
| if not has_alias: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if not has_config: | |
| new_imports.append("ConfigDict") | |
| lines[idx] = "from pydantic import " + ", ".join(new_imports) | |
| insert_idx = idx + 1 | |
| lines.insert(insert_idx, "") | |
| lines.insert(insert_idx + 1, "class BaseModel(_BaseModel):") | |
| lines.insert(insert_idx + 2, " model_config = ConfigDict(populate_by_name=True)") | |
| lines.insert(insert_idx + 3, "") | |
| break | |
| if line.startswith("from pydantic import "): | |
| imports = [part.strip() for part in line[len("from pydantic import ") :].split(",")] | |
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | |
| has_config = any(part == "ConfigDict" for part in imports) | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| has_alias = True | |
| else: | |
| new_imports.append(part) | |
| if not has_alias: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if not has_config: | |
| new_imports.append("ConfigDict") | |
| lines[idx] = "from pydantic import " + ", ".join(new_imports) | |
| insert_idx = idx + 1 | |
| lines.insert(insert_idx, "") | |
| lines.insert(insert_idx + 1, "class BaseModel(_BaseModel):") | |
| lines.insert(insert_idx + 2, " model_config = ConfigDict(populate_by_name=True)") | |
| lines.insert(insert_idx + 3, "") | |
| break |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| from acp.schema import AgentMessageChunk, TextContentBlock | ||
| from acp.utils import serialize_params | ||
|
|
||
|
|
||
| def test_serialize_params_uses_meta_aliases() -> None: | ||
| chunk = AgentMessageChunk( | ||
| sessionUpdate="agent_message_chunk", | ||
| content=TextContentBlock(type="text", text="demo", field_meta={"inner": "value"}), | ||
| field_meta={"outer": "value"}, | ||
| ) | ||
|
|
||
| payload = serialize_params(chunk) | ||
|
|
||
| assert payload["_meta"] == {"outer": "value"} | ||
| assert payload["content"]["_meta"] == {"inner": "value"} | ||
|
|
||
|
|
||
| def test_serialize_params_omits_meta_when_absent() -> None: | ||
| chunk = AgentMessageChunk( | ||
| sessionUpdate="agent_message_chunk", | ||
| content=TextContentBlock(type="text", text="demo"), | ||
| ) | ||
|
|
||
| payload = serialize_params(chunk) | ||
|
|
||
| assert "_meta" not in payload | ||
| assert "_meta" not in payload["content"] | ||
|
|
||
|
|
||
| def test_field_meta_can_be_set_by_name_on_models() -> None: | ||
| chunk = AgentMessageChunk( | ||
| sessionUpdate="agent_message_chunk", | ||
| content=TextContentBlock(type="text", text="demo", field_meta={"inner": "value"}), | ||
| field_meta={"outer": "value"}, | ||
| ) | ||
|
|
||
| assert chunk.field_meta == {"outer": "value"} | ||
| assert chunk.content.field_meta == {"inner": "value"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditions
if not has_aliasandif not has_configwill always be False when the loop at line 235-240 already foundBaseModeland converted it to the alias. Since line 232-233 check for existing alias/config, and lines 236-238 ensure the alias is added whenBaseModelis found, these additional checks are redundant. The logic should either rely on the existing checks (lines 232-233) or simplify this section.