From 8e422bac2eb4a5b1da17378746f78a77670c5886 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sat, 16 May 2026 17:57:19 -0400 Subject: [PATCH 1/2] Fix Python from_dict() round-trip for optional fields with schema defaults Fixes #1139, #1140, #1141. The Python codegen was embedding JSON-Schema `default` values into `obj.get(key, default)` for optional fields. But the generated dataclass field is always `T | None = None` and `to_dict()` omits the field when `None`, so `from_dict(to_dict(x))` silently mutated unset fields into the schema default: - `SessionTaskCompleteData.summary`: `None` -> `""` - `PermissionPromptRequest.action`: `None` -> `MemoryAction.STORE` - `PermissionRequest.action`: `None` -> `MemoryAction.STORE` Drop `defaultLiteral` from both `emitPyClass` and `emitPyFlatDiscriminatedUnion` so `from_dict()` always uses `obj.get(key)` (matching the dataclass default). Regenerate `session_events.py`. Flip the two codegen-level test assertions that previously locked in the buggy output and add negative assertions. Replace `test_schema_defaults_are_applied_for_missing_optional_fields` (which asserted the bug as expected behavior) with regression tests covering missing-key parsing, explicit-null parsing, and full `from_dict(to_dict(x))` round-trips for all three affected classes. Other languages (Go, .NET, Rust, TypeScript) are unaffected; their generators never read `propSchema.default` for deserialization fallbacks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nodejs/test/python-codegen.test.ts | 6 ++- python/copilot/generated/session_events.py | 6 +-- python/test_event_forward_compatibility.py | 50 ++++++++++++++++++++-- scripts/codegen/python.ts | 30 +------------ 4 files changed, 55 insertions(+), 37 deletions(-) diff --git a/nodejs/test/python-codegen.test.ts b/nodejs/test/python-codegen.test.ts index dc404ea19..4adb7fa06 100644 --- a/nodejs/test/python-codegen.test.ts +++ b/nodejs/test/python-codegen.test.ts @@ -71,11 +71,13 @@ describe("python session event codegen", () => { ); expect(code).toContain("def to_timedelta_int(x: timedelta) -> int:"); expect(code).toContain( - 'action = from_union([from_none, lambda x: parse_enum(SessionSyntheticDataAction, x)], obj.get("action", "store"))' + 'action = from_union([from_none, lambda x: parse_enum(SessionSyntheticDataAction, x)], obj.get("action"))' ); expect(code).toContain( - 'summary = from_union([from_none, from_str], obj.get("summary", ""))' + 'summary = from_union([from_none, from_str], obj.get("summary"))' ); + expect(code).not.toContain('obj.get("action", "store")'); + expect(code).not.toContain('obj.get("summary", "")'); expect(code).toContain("uri: str"); expect(code).toContain("pattern: str"); expect(code).toContain("payload: str"); diff --git a/python/copilot/generated/session_events.py b/python/copilot/generated/session_events.py index 696d5a0e5..4c29f50e0 100644 --- a/python/copilot/generated/session_events.py +++ b/python/copilot/generated/session_events.py @@ -1860,7 +1860,7 @@ def from_dict(obj: Any) -> "PermissionPromptRequest": assert isinstance(obj, dict) kind = parse_enum(PermissionPromptRequestKind, obj.get("kind")) access_kind = from_union([from_none, lambda x: parse_enum(PermissionPromptRequestPathAccessKind, x)], obj.get("accessKind")) - action = from_union([from_none, lambda x: parse_enum(PermissionPromptRequestMemoryAction, x)], obj.get("action", "store")) + action = from_union([from_none, lambda x: parse_enum(PermissionPromptRequestMemoryAction, x)], obj.get("action")) args = from_union([from_none, lambda x: x], obj.get("args")) can_offer_session_approval = from_union([from_none, from_bool], obj.get("canOfferSessionApproval")) capabilities = from_union([from_none, lambda x: from_list(from_str, x)], obj.get("capabilities")) @@ -2025,7 +2025,7 @@ class PermissionRequest: def from_dict(obj: Any) -> "PermissionRequest": assert isinstance(obj, dict) kind = parse_enum(PermissionRequestKind, obj.get("kind")) - action = from_union([from_none, lambda x: parse_enum(PermissionRequestMemoryAction, x)], obj.get("action", "store")) + action = from_union([from_none, lambda x: parse_enum(PermissionRequestMemoryAction, x)], obj.get("action")) args = obj.get("args") can_offer_session_approval = from_union([from_none, from_bool], obj.get("canOfferSessionApproval")) capabilities = from_union([from_none, lambda x: from_list(from_str, x)], obj.get("capabilities")) @@ -3261,7 +3261,7 @@ class SessionTaskCompleteData: def from_dict(obj: Any) -> "SessionTaskCompleteData": assert isinstance(obj, dict) success = from_union([from_none, from_bool], obj.get("success")) - summary = from_union([from_none, from_str], obj.get("summary", "")) + summary = from_union([from_none, from_str], obj.get("summary")) return SessionTaskCompleteData( success=success, summary=summary, diff --git a/python/test_event_forward_compatibility.py b/python/test_event_forward_compatibility.py index 10ba0644a..25b5cc2bc 100644 --- a/python/test_event_forward_compatibility.py +++ b/python/test_event_forward_compatibility.py @@ -17,7 +17,10 @@ ElicitationCompletedAction, ElicitationRequestedMode, ElicitationRequestedSchema, + PermissionPromptRequest, + PermissionPromptRequestKind, PermissionRequest, + PermissionRequestKind, PermissionRequestMemoryAction, SessionEventType, SessionTaskCompleteData, @@ -137,10 +140,49 @@ def test_data_shim_preserves_raw_mapping_values(self): constructed = Data(arguments={"tool_call_id": "call-1"}) assert constructed.to_dict() == {"arguments": {"tool_call_id": "call-1"}} - def test_schema_defaults_are_applied_for_missing_optional_fields(self): - """Generated event models should honor primitive schema defaults during parsing.""" + def test_missing_optional_fields_remain_none_after_parsing(self): + """Generated event models should leave missing optional fields as None. + + Regression test for github/copilot-sdk issues #1139, #1140, and #1141: + the Python codegen previously baked JSON Schema `default` values into + ``obj.get(key, default)`` for optional fields, so ``from_dict()`` returned + the schema default instead of ``None`` and broke ``from_dict(to_dict(x))`` + round-trips for instances where the field was ``None``. + """ + # #1141: PermissionRequest.action defaults to None when missing. request = PermissionRequest.from_dict({"kind": "memory", "fact": "remember this"}) - assert request.action == PermissionRequestMemoryAction.STORE + assert request.action is None + assert PermissionRequestMemoryAction.STORE.value == "store" # sanity + + # #1140: PermissionPromptRequest.action defaults to None when missing. + prompt_request = PermissionPromptRequest.from_dict({"kind": "memory"}) + assert prompt_request.action is None + # #1139: SessionTaskCompleteData.summary defaults to None when missing. task_complete = SessionTaskCompleteData.from_dict({"success": True}) - assert task_complete.summary == "" + assert task_complete.summary is None + + # Explicit JSON null should also map to None. + task_complete_null = SessionTaskCompleteData.from_dict({"success": True, "summary": None}) + assert task_complete_null.summary is None + + def test_optional_fields_round_trip_none(self): + """``from_dict(to_dict(x))`` should equal ``x`` when optional fields are None. + + Regression test for github/copilot-sdk issues #1139, #1140, and #1141. + """ + # #1139: SessionTaskCompleteData round-trip with summary=None. + task = SessionTaskCompleteData(success=None, summary=None) + assert SessionTaskCompleteData.from_dict(task.to_dict()) == task + + # #1140: PermissionPromptRequest round-trip with action=None. + prompt = PermissionPromptRequest(kind=PermissionPromptRequestKind.MEMORY) + assert prompt.action is None + assert "action" not in prompt.to_dict() + assert PermissionPromptRequest.from_dict(prompt.to_dict()) == prompt + + # #1141: PermissionRequest round-trip with action=None. + permission = PermissionRequest(kind=PermissionRequestKind.MEMORY) + assert permission.action is None + assert "action" not in permission.to_dict() + assert PermissionRequest.from_dict(permission.to_dict()) == permission diff --git a/scripts/codegen/python.ts b/scripts/codegen/python.ts index 29c68da0d..a5b655931 100644 --- a/scripts/codegen/python.ts +++ b/scripts/codegen/python.ts @@ -865,22 +865,6 @@ function isPyBase64StringSchema(schema: JSONSchema7): boolean { return schema.format === "byte" || (schema as Record).contentEncoding === "base64"; } -function toPythonLiteral(value: unknown): string | undefined { - if (typeof value === "string") { - return JSON.stringify(value); - } - if (typeof value === "number") { - return Number.isFinite(value) ? String(value) : undefined; - } - if (typeof value === "boolean") { - return value ? "True" : "False"; - } - if (value === null) { - return "None"; - } - return undefined; -} - function extractPyEventVariants(schema: JSONSchema7): PyEventVariant[] { const definitionCollections = collectDefinitionCollections(schema as Record); return getSessionEventVariantSchemas(schema, definitionCollections) @@ -1430,9 +1414,6 @@ function emitPyClass( fieldName: toSnakeCase(propName), isRequired, resolved, - defaultLiteral: isRequired ? undefined : toPythonLiteral( - propSchema.default ?? resolveSchema(propSchema, ctx.definitions)?.default - ), }; }); @@ -1474,9 +1455,7 @@ function emitPyClass( lines.push(` def from_dict(obj: Any) -> "${typeName}":`); lines.push(` assert isinstance(obj, dict)`); for (const field of fieldInfos) { - const sourceExpr = field.defaultLiteral - ? `obj.get(${JSON.stringify(field.jsonName)}, ${field.defaultLiteral})` - : `obj.get(${JSON.stringify(field.jsonName)})`; + const sourceExpr = `obj.get(${JSON.stringify(field.jsonName)})`; lines.push( ` ${field.fieldName} = ${field.resolved.fromExpr(sourceExpr)}` ); @@ -1592,9 +1571,6 @@ function emitPyFlatDiscriminatedUnion( fieldName: toSnakeCase(propName), isRequired: requiredInAll, resolved, - defaultLiteral: requiredInAll ? undefined : toPythonLiteral( - propSchema.default ?? resolveSchema(propSchema, ctx.definitions)?.default - ), }; }); @@ -1620,9 +1596,7 @@ function emitPyFlatDiscriminatedUnion( lines.push(` def from_dict(obj: Any) -> "${typeName}":`); lines.push(` assert isinstance(obj, dict)`); for (const field of fieldInfos) { - const sourceExpr = field.defaultLiteral - ? `obj.get(${JSON.stringify(field.jsonName)}, ${field.defaultLiteral})` - : `obj.get(${JSON.stringify(field.jsonName)})`; + const sourceExpr = `obj.get(${JSON.stringify(field.jsonName)})`; lines.push( ` ${field.fieldName} = ${field.resolved.fromExpr(sourceExpr)}` ); From de1684dd2ebf8126968ea504acc31ec1ba26d2cd Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sat, 16 May 2026 18:11:38 -0400 Subject: [PATCH 2/2] Fix prettier formatting in python-codegen.test.ts The CI prettier check failed on test/python-codegen.test.ts after the assertion update. Apply prettier --write to bring the file back into compliance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nodejs/test/python-codegen.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/nodejs/test/python-codegen.test.ts b/nodejs/test/python-codegen.test.ts index 4adb7fa06..180ac684f 100644 --- a/nodejs/test/python-codegen.test.ts +++ b/nodejs/test/python-codegen.test.ts @@ -73,9 +73,7 @@ describe("python session event codegen", () => { expect(code).toContain( 'action = from_union([from_none, lambda x: parse_enum(SessionSyntheticDataAction, x)], obj.get("action"))' ); - expect(code).toContain( - 'summary = from_union([from_none, from_str], obj.get("summary"))' - ); + expect(code).toContain('summary = from_union([from_none, from_str], obj.get("summary"))'); expect(code).not.toContain('obj.get("action", "store")'); expect(code).not.toContain('obj.get("summary", "")'); expect(code).toContain("uri: str");