Skip to content

Commit 28f8466

Browse files
fix(models): address review feedback on typed Bubble accessors
1 parent 84ed410 commit 28f8466

5 files changed

Lines changed: 100 additions & 10 deletions

File tree

api/composers.py

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,13 @@ def list_composers() -> tuple[Response, int] | Response:
9898
# load-bearing, not just a filter (Brad's review): the
9999
# sort key and the JSON's composerId both read off the
100100
# validated values, not the raw dict.
101-
c["composerId"] = local.composer_id
102-
c["lastUpdatedAt"] = local.last_updated_at
103-
c["conversation"] = c.get("conversation") or []
104-
c["workspaceId"] = name
105-
c["workspaceFolder"] = workspace_folder
106-
composers.append((local, c))
101+
payload = local.cursor_storage_payload()
102+
payload["composerId"] = local.composer_id
103+
payload["lastUpdatedAt"] = local.last_updated_at
104+
payload["conversation"] = payload.get("conversation") or []
105+
payload["workspaceId"] = name
106+
payload["workspaceFolder"] = workspace_folder
107+
composers.append((local, payload))
107108
except SchemaError as e:
108109
_logger.warning(
109110
"Schema drift in %s: %s (%s)",

models/conversation.py

Lines changed: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,50 @@
2727
_logger = logging.getLogger(__name__)
2828

2929

30+
def _filter_str_list_elements(
31+
value: list[Any],
32+
*,
33+
model: str,
34+
record_id: str,
35+
field: str,
36+
) -> list[str]:
37+
filtered: list[str] = []
38+
for item in value:
39+
if isinstance(item, str):
40+
filtered.append(item)
41+
else:
42+
_logger.warning(
43+
"Schema drift in %s %s: invalid %s element (expected str, got %s)",
44+
model,
45+
record_id,
46+
field,
47+
type(item).__name__,
48+
)
49+
return filtered
50+
51+
52+
def _filter_dict_list_elements(
53+
value: list[Any],
54+
*,
55+
model: str,
56+
record_id: str,
57+
field: str,
58+
) -> list[FileUriDict]:
59+
filtered: list[FileUriDict] = []
60+
for item in value:
61+
if isinstance(item, dict):
62+
filtered.append(cast(FileUriDict, item))
63+
else:
64+
_logger.warning(
65+
"Schema drift in %s %s: invalid %s element (expected dict, got %s)",
66+
model,
67+
record_id,
68+
field,
69+
type(item).__name__,
70+
)
71+
return filtered
72+
73+
3074
@dataclass(frozen=True)
3175
class Composer:
3276
"""Cursor conversation row from globalStorage cursorDiskKV; requires fullConversationHeadersOnly + createdAt."""
@@ -287,7 +331,12 @@ def relevant_files(self) -> list[str]:
287331
type(value).__name__,
288332
)
289333
return []
290-
return cast(list[str], value)
334+
return _filter_str_list_elements(
335+
value,
336+
model="Bubble",
337+
record_id=self.bubble_id,
338+
field="relevantFiles",
339+
)
291340

292341
@property
293342
def attached_file_code_chunks_uris(self) -> list[FileUriDict]:
@@ -301,7 +350,12 @@ def attached_file_code_chunks_uris(self) -> list[FileUriDict]:
301350
type(value).__name__,
302351
)
303352
return []
304-
return cast(list[FileUriDict], value)
353+
return _filter_dict_list_elements(
354+
value,
355+
model="Bubble",
356+
record_id=self.bubble_id,
357+
field="attachedFileCodeChunksUris",
358+
)
305359

306360
@property
307361
def context(self) -> BubbleContextDict:

models/conversation_types.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@ class FileUriDict(TypedDict, total=False):
1212

1313

1414
class BubbleMetadataDict(TypedDict, total=False):
15-
"""Storage ``metadata`` blob on a bubble row."""
15+
"""Storage ``metadata`` blob on a bubble row (subset we read here).
16+
17+
Rich display metadata (tool calls, token counts, thinking) is assembled in
18+
:func:`utils.display_bubble.build_storage_bubble_metadata` from typed
19+
bubble fields, not from this dict alone.
20+
"""
1621

1722
modelName: str
1823

tests/test_blob_parsing_fuzz.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,8 @@ def _assemble_workspace_bubble(bubble_id: object, value: object) -> dict | None:
184184
except (json.JSONDecodeError, TypeError, ValueError):
185185
return None
186186
try:
187-
return Bubble.from_dict(parsed, bubble_id=bubble_id)._raw # type: ignore[arg-type]
187+
bubble = Bubble.from_dict(parsed, bubble_id=bubble_id)
188+
return bubble.cursor_storage_payload() # type: ignore[arg-type]
188189
except SchemaError:
189190
return None
190191

@@ -205,6 +206,7 @@ def test_never_raises_unhandled(self, raw: dict, bubble_id: str) -> None:
205206
if bubble is None:
206207
return
207208
self.assertEqual(bubble.bubble_id, bubble_id)
209+
# from_dict stores the parsed dict by reference (no copy on construction).
208210
self.assertIs(bubble._raw, raw)
209211

210212
@given(raw=_BUBBLE_RAW_ANY, bubble_id=_BUBBLE_ID_ANY)

tests/test_raw_accessors.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,34 @@ def test_bubble_relevant_files_empty_when_key_missing(self) -> None:
4646
with self.assertNoLogs("models.conversation", level="WARNING"):
4747
self.assertEqual(bubble.relevant_files, [])
4848

49+
def test_bubble_relevant_files_skips_non_str_elements(self) -> None:
50+
bubble = Bubble.from_dict(
51+
{"relevantFiles": ["/good.py", 123, None, "/also.py"]},
52+
bubble_id="b-rel",
53+
)
54+
with self.assertLogs("models.conversation", level="WARNING") as logs:
55+
self.assertEqual(bubble.relevant_files, ["/good.py", "/also.py"])
56+
self.assertTrue(any("relevantFiles" in m for m in logs.output), logs.output)
57+
58+
def test_bubble_attached_file_uris_skips_non_dict_elements(self) -> None:
59+
bubble = Bubble.from_dict(
60+
{
61+
"attachedFileCodeChunksUris": [
62+
{"path": "/a.py"},
63+
"bad",
64+
{"path": "/b.py"},
65+
]
66+
},
67+
bubble_id="b-uri",
68+
)
69+
with self.assertLogs("models.conversation", level="WARNING") as logs:
70+
uris = bubble.attached_file_code_chunks_uris
71+
self.assertEqual(uris, [{"path": "/a.py"}, {"path": "/b.py"}])
72+
self.assertTrue(
73+
any("attachedFileCodeChunksUris" in m for m in logs.output),
74+
logs.output,
75+
)
76+
4977
def test_project_layouts_silent_when_key_missing(self) -> None:
5078
with self.assertNoLogs("models.raw_access", level="WARNING"):
5179
layouts = message_request_context_project_layouts({}, composer_id="cmp-1")

0 commit comments

Comments
 (0)