Skip to content

python: decode boolean-discriminated unions - #2123

Open
examon wants to merge 4 commits into
mainfrom
sdk-bugfix-393
Open

python: decode boolean-discriminated unions#2123
examon wants to merge 4 commits into
mainfrom
sdk-bugfix-393

Conversation

@examon

@examon examon commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #2122

The problem

scripts/codegen/python.ts captured a union discriminator's JSON Schema const with String(...), so a boolean const became the JavaScript string "true" / "false" before any emitter ran. The dispatch table was typed Array<{ value: string; typeName: string }>, so the type could not survive even if it had been captured.

The generated Python therefore matched strings:

def _load_SessionListEntry(obj: Any) -> "SessionListEntry":
    kind = obj.get("isRemote")
    match kind:
        case "false": return LocalSessionMetadataValue.from_dict(obj)
        case "true": return RemoteSessionMetadataValue.from_dict(obj)
        case _: raise ValueError(f"Unknown SessionListEntry isRemote: {kind!r}")

A JSON boolean decodes to Python True, which never equals "true", so both boolean-discriminated unions in the schema fell through to raise ValueError:

  • sessions.list() raised ValueError: Unknown SessionListEntry isRemote: False for any non-empty session list.
  • QueuedCommandResult failed to decode, and QueuedCommandHandled.to_dict() emitted {"handled": "true"} where the schema declares {"type": "boolean", "const": true} with additionalProperties: false.

The fix

Keep the const's JSON type through codegen and render it as a Python literal.

  • PyDiscriminatorValue = string | boolean, with the capture keeping booleans as booleans and stringifying everything else exactly as before.
  • A literal renderer emits True / False for booleans. JSON.stringify cannot be reused here: it yields lowercase true, which Python parses as a capture pattern rather than a literal, and in a multi-arm match that is a hard SyntaxError.
  • The discriminator ClassVar is annotated bool when the const is boolean, so the encode direction puts a real JSON boolean on the wire.

This mirrors scripts/codegen/go.ts, which already models discriminator values as string | boolean and is unaffected by the bug.

Regenerating changes six lines of python/copilot/generated/rpc.py:

-    handled: ClassVar[str] = "true"
+    handled: ClassVar[bool] = True
-    handled: ClassVar[str] = "false"
+    handled: ClassVar[bool] = False

 def _load_QueuedCommandResult(obj: Any) -> "QueuedCommandResult":
-        case "true": return QueuedCommandHandled.from_dict(obj)
-        case "false": return QueuedCommandNotHandled.from_dict(obj)
+        case True: return QueuedCommandHandled.from_dict(obj)
+        case False: return QueuedCommandNotHandled.from_dict(obj)

 def _load_SessionListEntry(obj: Any) -> "SessionListEntry":
-        case "false": return LocalSessionMetadataValue.from_dict(obj)
-        case "true": return RemoteSessionMetadataValue.from_dict(obj)
+        case False: return LocalSessionMetadataValue.from_dict(obj)
+        case True: return RemoteSessionMetadataValue.from_dict(obj)

No other generated file changes, in any language.

Scope

Deliberately left alone:

  • findPyDiscriminator's mapping.set(String(...)). Those keys feed only the variant-count validity check and the flat-union path, which both boolean unions bypass because they are $ref-based. No union in the schema reaches it today.
  • The ClassVar collapse pass's field lookup, which builds its regex from the raw schema property name (isRemote) and so never matches the snake_cased is_remote. That is a separate defect, and it is currently load-bearing: it is why LocalSessionMetadataValue keeps a real is_remote: bool field that already encodes correctly. Changing it would drop a required constructor parameter.

Verification

Against the published 1.0.8 wheel, the reproducer raises ValueError: Unknown SessionListEntry isRemote: False and exits 1. Against this branch it exits 0:

SessionList(sessions=[LocalSessionMetadataValue(is_remote=False, modified_time='2026-07-26T10:05:00.000Z', session_id='example-local', start_time='2026-07-26T10:00:00.000Z', client_name=None, context=None, is_detached=None, mc_task_id=None, name=None, summary=None)])

Also verified in a scratch environment against the built branch:

probe before after
one local session ValueError LocalSessionMetadataValue
one remote session ValueError RemoteSessionMetadataValue
mixed local + remote list ValueError both variants, round-trips to "isRemote": false/true
_load_QueuedCommandResult on true / false ValueError QueuedCommandHandled / QueuedCommandNotHandled
json.dumps(QueuedCommandHandled().to_dict()) {"handled": "true"} {"handled": true}
handled: "true" (a string) silently dispatched to QueuedCommandHandled ValueError
isRemote missing / None / "yes" / 1 / 0 ValueError (or AssertionError) ValueError

case True: compiles to an identity comparison, so 1 and 0 do not match it despite True == 1. The error path stays intact.

Tests

python/test_rpc_generated.py gains coverage for both unions, routed through the real dispatchers rather than the variant classes (a variant's from_dict ignores the discriminator, so calling it directly would pass even with the bug present):

  • SessionList.from_dict decodes a local and a remote entry in one payload to the right variants.
  • CommandsRespondToQueuedCommandRequest.from_dict decodes both handled values.
  • The encode direction asserts is True / is False identity and that json.dumps produces {"handled": true} / {"handled": false}.
  • A string-discriminated union case guards against a regression on that path.

The new tests fail against the pre-fix generated file and pass after it.

Checks run

  • npm run generate for all five languages plus the pinned nightly cargo fmt step, leaving the tree byte-clean apart from the six intended Python lines.
  • uv run ruff format --check ., uv run ruff check, uv run ty check copilot (two pre-existing warnings in unrelated hand-written files, exit 0).
  • uv run pytest test_rpc_generated.py -v: 7 passed, repeated across runs and on Python 3.11.
  • The offline Python test set: 291 passed.
  • Docs validation: 56 files passed.

The Python generator captured a union discriminator's JSON Schema `const`
with `String()`, so a boolean const became the string "true"/"false" and the
emitted dispatcher matched `case "true":`. A JSON boolean decodes to Python
`True`, which never equals `"true"`, so every boolean-discriminated union
fell through to `raise ValueError`.

Two unions are affected. `sessions.list()` raised
`ValueError: Unknown SessionListEntry isRemote: False` for any non-empty
session list, and `QueuedCommandHandled.to_dict()` put the string `"true"` on
the wire where the schema declares `{"type": "boolean", "const": true}`.

Keep the const's JSON type through codegen and render it as a Python literal
(`True`/`False`), annotating the discriminator `ClassVar` as `bool`. This
mirrors how `go.ts` already models discriminator values. Regenerating changes
six lines of `python/copilot/generated/rpc.py`; no other language changes.
Copilot AI review requested due to automatic review settings July 29, 2026 13:25
@examon
examon requested a review from a team as a code owner July 29, 2026 13:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes Python decoding and serialization for boolean-discriminated RPC unions.

Changes:

  • Preserves boolean discriminator values during Python code generation.
  • Regenerates affected RPC models and dispatchers.
  • Adds decode, encode, round-trip, and string-discriminator regression tests.
Show a summary per file
File Description
scripts/codegen/python.ts Emits correctly typed Python discriminator literals.
python/copilot/generated/rpc.py Uses boolean constants and match arms.
python/test_rpc_generated.py Tests boolean union serialization and dispatch.

Review details

  • Files reviewed: 2/3 changed files
  • Comments generated: 0
  • Review effort level: Medium

@github-actions

This comment has been minimized.

Ensure the rpc sessions.list e2e test persists at least one session entry and asserts the matching session decodes to LocalSessionMetadataValue with is_remote=False, exercising the boolean discriminator path end-to-end.

Use an authed client token from GITHUB_TOKEN (default fakevalue) and enqueue a user turn before save/list so the entry is present without depending on full model completion.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 30, 2026 15:28
Comment thread python/e2e/test_rpc_server_e2e.py Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review details

Comments suppressed due to low confidence (2)

python/e2e/test_rpc_server_e2e.py:308

  • send() starts an asynchronous model turn, but this test has no rpc_server/should_list_find_and_inspect_persisted_session_state.yaml replay snapshot. In CI the proxy therefore returns a no-matching-request error, while outside CI it may forward the request to live CAPI; the fixed 200 ms sleep also does not wait for the turn to finish, so save races background work. This test only needs persisted local metadata, so record a non-model event as before and let the following sessions.save persist it.
            await session.send("Record a turn for sessions.list discriminator coverage", mode="enqueue")
            await asyncio.sleep(0.2)

python/e2e/test_rpc_server_e2e.py:291

  • This factory always creates a stdio client (_make_authed_client, line 97), so the new non-empty sessions.list regression assertion no longer runs over the selected transport. The Python workflow explicitly executes the suite with both default and inprocess, but both matrix cells now exercise this test through stdio. Please use the context's transport-aware client/factory here, or explicitly isolate only the model-dependent part that requires stdio.
        client = _make_authed_client(authed_ctx, token)
  • Files reviewed: 3/4 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@github-actions

This comment has been minimized.

CodeQL flagged the new `except ExceptionGroup: pass` in the sessions.list
teardown as an empty except clause with no explanation. Add the same
explanatory comment the four other identical teardowns in this file already
carry.

Also wrap the enqueue `session.send(...)` call, which was 104 characters and
failed the repo's 100-column ruff lint.
Copilot AI review requested due to automatic review settings July 30, 2026 15:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review details

Comments suppressed due to low confidence (1)

python/e2e/test_rpc_server_e2e.py:309

  • This sends a model turn, but the corresponding replay fixture test/snapshots/rpc_server/should_list_find_and_inspect_persisted_session_state.yaml is absent. In CI the replay proxy fails unmatched model requests instead of forwarding them (test/harness/replayingCapiProxy.ts:529-554); because teardown suppresses the client's ExceptionGroup, this can either make the E2E flaky or let it pass while hiding the failed turn. Please add the snapshot for this prompt, or persist a qualifying session without starting model inference.
            await session.send(
                "Record a turn for sessions.list discriminator coverage", mode="enqueue"
            )
  • Files reviewed: 3/4 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@github-actions

This comment has been minimized.

…ns.list e2e

The sessions.list e2e test enqueued a turn, slept 200ms, then saved and listed
once. On the Windows runners the enqueued turn was not recorded yet when save
ran, so sessions.list came back empty and `assert len(listed.sessions) >= 1`
failed with `assert 0 >= 1`. Linux and macOS happened to win the race.

Replace the fixed sleep with the existing `wait_for_condition` harness helper,
re-saving on each attempt until the session actually appears in sessions.list.
All discriminator assertions are unchanged, so the boolean-discriminator path
this PR fixes is still exercised end-to-end.

`asyncio` was imported only for the removed sleep, so drop the import.
Copilot AI review requested due to automatic review settings July 30, 2026 16:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review details

Comments suppressed due to low confidence (2)

scripts/codegen/python.ts:291

  • pyDiscriminatorValue currently stringifies any non-boolean discriminator (String(constValue)). If the schema ever includes a non-string/non-boolean const discriminator (e.g. null or a number), the generated Python dispatcher will never match at runtime (None/1 vs "null"/"1"), causing guaranteed “Unknown ...” failures. To prevent silently generating broken dispatchers, either (a) validate and throw when constValue is not a string | boolean, or (b) extend PyDiscriminatorValue/pyDiscriminatorValueExpr/pyDiscriminatorValueType to support number | null (and emit None / numeric literals appropriately).
function pyDiscriminatorValue(constValue: unknown): PyDiscriminatorValue {
    return typeof constValue === "boolean" ? constValue : String(constValue);
}

python/e2e/test_rpc_server_e2e.py:382

  • client.stop() is always attempted even if client.start() failed earlier, and the exception handling is narrowly scoped to ExceptionGroup. This can make the test cleanup flaky (masking the primary failure or raising a different teardown error). Consider tracking whether the client successfully started (e.g., a client_started flag set after await client.start()), only calling stop() when started, and/or broadening the teardown handling to avoid overshadowing the original test error while still allowing cleanup failures to be surfaced when they are actionable.
            if session is not None:
                await session.disconnect()
            try:
                await client.stop()
            except ExceptionGroup:
                # Intentional: shutting down the per-test client can race the
                # CLI's own teardown and surface as an aggregated cancellation
                # error from anyio. We don't want it to fail the test.
                pass
  • Files reviewed: 3/4 changed files
  • Comments generated: 0 new
  • Review effort level: Low

@github-actions

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR fixes a Python codegen bug where boolean JSON Schema const discriminators were incorrectly stringified (true"true"), causing ValueError at runtime for SessionListEntry and QueuedCommandResult.

Consistency assessment: no issues.

  • The fix explicitly mirrors the existing behavior in scripts/codegen/go.ts, which already models discriminator values as string | boolean.
  • Per the PR description, npm run generate was run for all five languages and produced no changes outside the six intended Python lines — confirming this bug did not affect other SDKs.
  • The change is a codegen correctness fix, not a new feature, so no other SDK needs a parallel update.

Generated by SDK Consistency Review Agent for #2123 · sonnet46 18.9 AIC · ⌖ 5.38 AIC · ⊞ 6.6K ·

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.

python: boolean-discriminated unions never decode - sessions.list() raises for any non-empty result

4 participants