From 6e234186e20326d2f4aa28af539e9eb0ead01886 Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Thu, 30 Jul 2026 14:52:53 -0700 Subject: [PATCH] fix: tolerate null for optional strings in wire models Optional server columns come back as an explicit JSON null once a caller clears them, but 20 wire fields were typed as a bare `str` with an "" default. Pydantic rejects null for those, and because the failure happens during list validation, one cleared row takes down the entire response: $ goodeye templates list ValidationError: 1 validation error for TemplateList items.1.outcome Input should be a valid string Clearing `outcome` on a single template broke `templates list` and `templates search` outright, including rows that parsed fine. Add a `NullableStr` alias (BeforeValidator coercing None to "") and apply it to every optional string field that already defaulted to "". Consumers keep a plain `str`, so callers that format the value stay safe, and non-string types are still rejected. This matches the module's stated intent that the wire models be "deliberately minimal and permissive so minor additive server changes do not break old CLI releases." Co-Authored-By: Claude Opus 5 (1M context) --- pyproject.toml | 2 +- src/goodeye_cli/wire.py | 64 ++++++++++----- tests/test_wire_nullable_strings.py | 121 ++++++++++++++++++++++++++++ uv.lock | 2 +- 4 files changed, 165 insertions(+), 24 deletions(-) create mode 100644 tests/test_wire_nullable_strings.py diff --git a/pyproject.toml b/pyproject.toml index 232892d..affb80b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "goodeye" -version = "0.25.3" +version = "0.25.4" description = "Goodeye CLI: a private home for the skills your AI follows and the verifiers its work must pass, from the terminal." readme = "README.md" license = { file = "LICENSE" } diff --git a/src/goodeye_cli/wire.py b/src/goodeye_cli/wire.py index 472faa5..614592b 100644 --- a/src/goodeye_cli/wire.py +++ b/src/goodeye_cli/wire.py @@ -8,9 +8,29 @@ from __future__ import annotations from datetime import datetime -from typing import Any +from typing import Annotated, Any -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, BeforeValidator, ConfigDict, Field + + +def _empty_when_null(value: Any) -> Any: + """Coerce an explicit JSON null to an empty string. + + Optional server columns arrive as null once a caller clears them, rather + than being omitted from the payload. Every field annotated with + ``NullableStr`` already defaults to "", so an absent value is expected; + without this coercion one null row fails validation and takes down the + whole response, including the rows that parsed fine. + """ + return "" if value is None else value + + +NullableStr = Annotated[str, BeforeValidator(_empty_when_null)] +"""A string field that tolerates null on the wire and reads as "" in Python. + +Use for any optional text the server may return as null. Consumers keep a +plain ``str``, so callers that format or concatenate the value stay safe. +""" class _WireBase(BaseModel): @@ -108,8 +128,8 @@ class WorkflowSummary(_WireBase): id: str name: str current_version: int - description: str = "" - outcome: str = "" + description: NullableStr = "" + outcome: NullableStr = "" tags: list[str] = Field(default_factory=list) updated_at: datetime | None = None owner_user_id: str | None = None @@ -136,8 +156,8 @@ class WorkflowSearchItem(_WireBase): match_reason: str slug: str | None = None name: str | None = None - description: str = "" - outcome: str = "" + description: NullableStr = "" + outcome: NullableStr = "" tags: list[str] = Field(default_factory=list) @@ -207,8 +227,8 @@ class WorkflowDetail(_WireBase): name: str version: int body: str - description: str = "" - outcome: str = "" + description: NullableStr = "" + outcome: NullableStr = "" tags: list[str] = Field(default_factory=list) owner_user_id: str | None = None updated_at: datetime | None = None @@ -255,7 +275,7 @@ class WorkflowFilePatchResult(_WireBase): version: int version_token: str name: str - slug: str = "" + slug: NullableStr = "" changed: list[str] = Field(default_factory=list) deleted: list[str] = Field(default_factory=list) carried_forward: int = 0 @@ -369,7 +389,7 @@ class DesignCheckCriterion(_WireBase): criterion: str passed: bool - reason: str = "" + reason: NullableStr = "" class DesignChecks(_WireBase): @@ -395,8 +415,8 @@ class TemplateSummary(_WireBase): handle: str owner_user_id: str latest_version: int - description: str = "" - outcome: str = "" + description: NullableStr = "" + outcome: NullableStr = "" tags: list[str] = Field(default_factory=list) publishing_handle: str safety_verification_status: str = "unverified" @@ -415,11 +435,11 @@ class TemplateSearchItem(_WireBase): id: str rank: int match_reason: str - slug: str = "" - name: str = "" - handle: str = "" - description: str = "" - outcome: str = "" + slug: NullableStr = "" + name: NullableStr = "" + handle: NullableStr = "" + description: NullableStr = "" + outcome: NullableStr = "" tags: list[str] = Field(default_factory=list) @@ -486,8 +506,8 @@ class TemplateDetail(_WireBase): owner_user_id: str version: int body: str - description: str = "" - outcome: str = "" + description: NullableStr = "" + outcome: NullableStr = "" tags: list[str] = Field(default_factory=list) release_notes: str | None = None publishing_handle: str @@ -797,7 +817,7 @@ class ImageGeneratorSummary(_WireBase): generator_id: str name: str - description: str = "" + description: NullableStr = "" current_version: int version_token: str status: str @@ -816,7 +836,7 @@ class ImageGeneratorDetail(_WireBase): generator_id: str name: str - description: str = "" + description: NullableStr = "" current_version: int version: int version_token: str @@ -836,7 +856,7 @@ class ImageGeneratorDeployResult(_WireBase): generator_id: str name: str - description: str = "" + description: NullableStr = "" current_version: int version: int version_token: str diff --git a/tests/test_wire_nullable_strings.py b/tests/test_wire_nullable_strings.py new file mode 100644 index 0000000..c4ba74c --- /dev/null +++ b/tests/test_wire_nullable_strings.py @@ -0,0 +1,121 @@ +"""Tests that optional wire strings tolerate an explicit JSON null. + +Optional server columns come back as null once a caller clears them. These +fields already default to "", so a null must read the same way rather than +failing validation and taking the whole response down with it. +""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from goodeye_cli.wire import ( + TemplateList, + TemplateSearchResponse, + TemplateSummary, + WorkflowDetail, + WorkflowList, + WorkflowSearchResponse, + WorkflowSummary, +) + + +def _template_row(**overrides: object) -> dict[str, object]: + row: dict[str, object] = { + "id": "tmpl-1", + "slug": "writing-humanizer", + "name": "writing-humanizer", + "handle": "example", + "owner_user_id": "user-1", + "latest_version": 10, + "publishing_handle": "example", + } + row.update(overrides) + return row + + +def _workflow_row(**overrides: object) -> dict[str, object]: + row: dict[str, object] = {"id": "skill-1", "name": "example-skill", "current_version": 1} + row.update(overrides) + return row + + +@pytest.mark.parametrize("field", ["outcome", "description"]) +def test_template_summary_accepts_null(field: str) -> None: + summary = TemplateSummary.model_validate(_template_row(**{field: None})) + assert getattr(summary, field) == "" + + +@pytest.mark.parametrize("field", ["outcome", "description"]) +def test_workflow_summary_accepts_null(field: str) -> None: + summary = WorkflowSummary.model_validate(_workflow_row(**{field: None})) + assert getattr(summary, field) == "" + + +def test_null_outcome_does_not_fail_the_whole_template_list() -> None: + """One cleared row must not take down the rows that parsed fine.""" + payload = { + "items": [_template_row(id="tmpl-1"), _template_row(id="tmpl-2", outcome=None)], + "next_cursor": None, + } + listing = TemplateList.model_validate(payload) + assert [item.id for item in listing.items] == ["tmpl-1", "tmpl-2"] + assert listing.items[1].outcome == "" + + +def test_null_outcome_does_not_fail_the_whole_workflow_list() -> None: + payload = {"items": [_workflow_row(), _workflow_row(id="skill-2", outcome=None)]} + listing = WorkflowList.model_validate(payload) + assert listing.items[1].outcome == "" + + +def test_template_search_accepts_null_strings() -> None: + payload = { + "items": [ + { + "id": "tmpl-1", + "rank": 1, + "match_reason": "name match", + "slug": None, + "name": None, + "handle": None, + "description": None, + "outcome": None, + } + ], + "query": "humanize", + "limit": 10, + } + item = TemplateSearchResponse.model_validate(payload).items[0] + assert (item.slug, item.name, item.handle, item.description, item.outcome) == ("",) * 5 + + +def test_workflow_search_accepts_null_strings() -> None: + payload = { + "items": [{"id": "skill-1", "rank": 1, "match_reason": "m", "description": None}], + "query": "humanize", + "limit": 10, + } + assert WorkflowSearchResponse.model_validate(payload).items[0].description == "" + + +def test_workflow_detail_accepts_null_outcome() -> None: + detail = WorkflowDetail.model_validate( + {"id": "skill-1", "name": "n", "version": 1, "body": "b", "outcome": None} + ) + assert detail.outcome == "" + + +def test_absent_and_present_values_are_unchanged() -> None: + """The coercion must only touch null, leaving normal payloads alone.""" + absent = TemplateSummary.model_validate(_template_row()) + assert absent.outcome == "" + present = TemplateSummary.model_validate(_template_row(outcome="Ship faster.")) + assert present.outcome == "Ship faster." + + +def test_wrong_types_are_still_rejected() -> None: + """Tolerating null must not turn the field into an anything-goes field.""" + with pytest.raises(ValidationError): + TemplateSummary.model_validate(_template_row(outcome=123)) diff --git a/uv.lock b/uv.lock index 8c0e996..9ea194a 100644 --- a/uv.lock +++ b/uv.lock @@ -176,7 +176,7 @@ wheels = [ [[package]] name = "goodeye" -version = "0.25.3" +version = "0.25.4" source = { editable = "." } dependencies = [ { name = "httpx" },