Skip to content

Commit 16f861d

Browse files
committed
review: wire typed models load-bearing across read sites (#30)
Closes five blocking findings from Brad's review pass: 1. Bubble / Workspace / ExportEntry were defined and unit-tested but had zero production callers — the PR's own "Used at" table didn't match the code. Wired each at the cited sites: * Bubble in api/workspaces.py (both bubble loaders, line 584 + 1023) * Bubble in api/search.py:150 * Workspace in api/composers.py:44 and api/workspaces.py:54 * ExportEntry reader in scripts/export.py:66 — paired with writer updates so the on-disk shape matches what the model accepts. 2. get_workspace_tabs() in api/workspaces.py bypassed Composer.from_dict while list_workspaces() in the same file validated. The two primary conversation-browsing paths now agree on what counts as a valid row. 3. get_composer() returned the raw dict from either per-workspace DB or global storage with no validation. Now goes through WorkspaceLocalComposer.from_dict (per-workspace, drift logged + skipped) and Composer.from_dict (global fallback, drift -> 404 + log). 4. WorkspaceLocalComposer.from_dict result was discarded after the gate. The typed object is now load-bearing: the sort key reads local.last_updated_at and the JSON's composerId/lastUpdatedAt are written from the validated fields, not the raw dict. 5. SchemaError message always read "missing required field" even on type/shape mismatches. Now reads "invalid field" when hint is set and "missing required field" only when hint is None, so log grepping can tell the two drift modes apart. Plus one forward-looking note: added a WHY comment at the numeric-only createdAt check in models/conversation.py pointing at the 17/17 live scan that justifies the strict isinstance(int, float) gate. 11 new regression tests: - tests/test_models_wired_at_read_sites.py (new file, 10 tests) pinning each from_dict call site with spy + side_effect patches so a future refactor that drops the typed model fails loudly. - tests/test_models.py one new test asserting the SchemaError wording distinction holds. Verified locally: - python -m unittest discover tests: 220 passed, OK (was 209) - python -m unittest tests.test_models_wired_at_read_sites: 10/10 pass - python app.py boots clean, curl / returns 200
1 parent 59ae103 commit 16f861d

8 files changed

Lines changed: 497 additions & 48 deletions

File tree

api/composers.py

Lines changed: 39 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313

1414
from utils.workspace_path import resolve_workspace_path
1515
from utils.path_helpers import to_epoch_ms
16-
from models import SchemaError, WorkspaceLocalComposer
16+
from models import Composer, SchemaError, Workspace, WorkspaceLocalComposer
1717

1818
bp = Blueprint("composers", __name__)
1919

@@ -41,9 +41,11 @@ def list_composers():
4141

4242
workspace_folder = None
4343
try:
44-
wd = _read_json_file(wj_path)
45-
workspace_folder = wd.get("folder")
46-
except Exception:
44+
workspace = Workspace.from_dict(_read_json_file(wj_path), workspace_id=name)
45+
workspace_folder = workspace.folder
46+
except (SchemaError, OSError, ValueError):
47+
# Missing / malformed workspace.json is non-fatal — the row still
48+
# contributes its composer data, just without a folder hint.
4749
pass
4850

4951
try:
@@ -72,21 +74,27 @@ def list_composers():
7274
)
7375
for c in all_composers:
7476
try:
75-
WorkspaceLocalComposer.from_dict(c)
77+
local = WorkspaceLocalComposer.from_dict(c)
7678
except SchemaError as e:
7779
print(f"Schema drift in {db_path}: {e}")
7880
continue
81+
# Use the typed view downstream so the dataclass is
82+
# load-bearing, not just a filter (Brad's review): the
83+
# sort key and the JSON's composerId both read off the
84+
# validated values, not the raw dict.
85+
c["composerId"] = local.composer_id
86+
c["lastUpdatedAt"] = local.last_updated_at
7987
c["conversation"] = c.get("conversation") or []
8088
c["workspaceId"] = name
8189
c["workspaceFolder"] = workspace_folder
82-
composers.append(c)
90+
composers.append((local, c))
8391
except SchemaError as e:
8492
print(f"Schema drift in {db_path}: {e}")
8593
except Exception as e:
8694
print(f"Failed reading composers from {db_path}: {e}")
8795

88-
composers.sort(key=lambda c: to_epoch_ms(c.get("lastUpdatedAt")), reverse=True)
89-
return jsonify(composers)
96+
composers.sort(key=lambda pair: to_epoch_ms(pair[0].last_updated_at), reverse=True)
97+
return jsonify([c for _, c in composers])
9098

9199
except Exception as e:
92100
print(f"Failed to get composers: {e}")
@@ -117,9 +125,17 @@ def get_composer(composer_id):
117125
if row and row[0]:
118126
data = json.loads(row[0])
119127
for c in (data.get("allComposers") or []):
120-
if c.get("composerId") == composer_id:
121-
return jsonify(c)
122-
except Exception:
128+
if isinstance(c, dict) and c.get("composerId") == composer_id:
129+
try:
130+
local = WorkspaceLocalComposer.from_dict(c)
131+
except SchemaError as e:
132+
# Same drift list_composers() logs and skips at line ~78,
133+
# so a single-composer fetch can't silently return malformed
134+
# JSON the list endpoint hid.
135+
print(f"Schema drift in workspace-local composer {composer_id}: {e}")
136+
continue
137+
return jsonify(local.raw)
138+
except (OSError, sqlite3.Error, json.JSONDecodeError, ValueError):
123139
pass
124140

125141
# Fallback: global storage
@@ -135,10 +151,18 @@ def get_composer(composer_id):
135151

136152
if row and row[0]:
137153
raw = row[0] if isinstance(row[0], str) else row[0].decode("utf-8")
138-
composer = json.loads(raw)
139-
composer.setdefault("conversation", [])
140-
return jsonify(composer)
141-
except Exception:
154+
try:
155+
composer = Composer.from_dict(json.loads(raw), composer_id=composer_id)
156+
except SchemaError as e:
157+
# Don't return malformed JSON to the client — surface the drift
158+
# as a 404 + log, matching the silent-skip behaviour of the
159+
# list endpoints for the same row.
160+
print(f"Schema drift in composer {composer_id}: {e}")
161+
return jsonify({"error": "Composer schema drift"}), 404
162+
payload = dict(composer.raw)
163+
payload.setdefault("conversation", [])
164+
return jsonify(payload)
165+
except (OSError, sqlite3.Error, json.JSONDecodeError, ValueError):
142166
pass
143167

144168
return jsonify({"error": "Composer not found"}), 404

api/search.py

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
from utils.path_helpers import normalize_file_path, get_workspace_folder_paths, to_epoch_ms
1919
from utils.text_extract import extract_text_from_bubble
2020
from utils.cli_chat_reader import list_cli_projects, traverse_blobs, messages_to_bubbles
21-
from models import Composer, SchemaError
21+
from models import Bubble, Composer, SchemaError
2222

2323
bp = Blueprint("search", __name__)
2424

@@ -147,11 +147,12 @@ def search():
147147
if len(parts) >= 3:
148148
bid = parts[2]
149149
try:
150-
b = json.loads(row["value"])
151-
if isinstance(b, dict):
152-
text = extract_text_from_bubble(b)
153-
bubble_map[bid] = {"text": text, "raw": b}
154-
except Exception:
150+
bubble = Bubble.from_dict(json.loads(row["value"]), bubble_id=bid)
151+
text = extract_text_from_bubble(bubble.raw)
152+
bubble_map[bid] = {"text": text, "raw": bubble.raw}
153+
except (SchemaError, json.JSONDecodeError, ValueError):
154+
# Skip malformed bubble rows — search must keep returning
155+
# results from the well-formed ones.
155156
pass
156157

157158
# Search through composerData

api/workspaces.py

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
)
3434
from utils.text_extract import extract_text_from_bubble, format_tool_action
3535
from utils.exclusion_rules import build_searchable_text, is_excluded_by_rules
36-
from models import Composer, SchemaError
36+
from models import Bubble, Composer, SchemaError, Workspace
3737

3838
bp = Blueprint("workspaces", __name__)
3939

@@ -51,11 +51,11 @@ def _get_workspace_display_name(workspace_path: str, workspace_id: str) -> str:
5151
return "Other chats"
5252
wj_path = os.path.join(workspace_path, workspace_id, "workspace.json")
5353
try:
54-
wd = _read_json_file(wj_path)
55-
name = get_workspace_display_name(wd)
54+
workspace = Workspace.from_dict(_read_json_file(wj_path), workspace_id=workspace_id)
55+
name = get_workspace_display_name(workspace.raw)
5656
if name:
5757
return name
58-
except Exception:
58+
except (SchemaError, OSError, ValueError):
5959
pass
6060
return workspace_id
6161

@@ -581,10 +581,11 @@ def list_workspaces():
581581
if len(parts) >= 3:
582582
bid = parts[2]
583583
try:
584-
b = json.loads(row["value"])
585-
if isinstance(b, dict):
586-
bubble_map[bid] = b
587-
except Exception:
584+
bubble = Bubble.from_dict(json.loads(row["value"]), bubble_id=bid)
585+
bubble_map[bid] = bubble.raw
586+
except (SchemaError, json.JSONDecodeError, ValueError):
587+
# Skip malformed bubble rows — read-many path, one bad row
588+
# must not 500 the endpoint.
588589
pass
589590

590591
# Process each composer
@@ -1019,10 +1020,10 @@ def get_workspace_tabs(workspace_id):
10191020
if len(parts) >= 3:
10201021
bid = parts[2]
10211022
try:
1022-
b = json.loads(row["value"])
1023-
if isinstance(b, dict):
1024-
bubble_map[bid] = b
1025-
except Exception:
1023+
bubble = Bubble.from_dict(json.loads(row["value"]), bubble_id=bid)
1024+
bubble_map[bid] = bubble.raw
1025+
except (SchemaError, json.JSONDecodeError, ValueError):
1026+
# Skip malformed rows — one bad bubble must not 500 the tabs endpoint.
10261027
pass
10271028

10281029
# Load codeBlockDiffs
@@ -1097,8 +1098,17 @@ def get_workspace_tabs(workspace_id):
10971098
for row in composer_rows:
10981099
composer_id = row["key"].split(":")[1]
10991100
try:
1100-
cd = json.loads(row["value"])
1101-
1101+
composer = Composer.from_dict(json.loads(row["value"]), composer_id=composer_id)
1102+
except SchemaError as e:
1103+
# Skip the same drift list_workspaces() drops at line 605 so the two
1104+
# primary conversation paths agree on what counts as a valid composer.
1105+
print(f"Schema drift in composer {composer_id}: {e}")
1106+
continue
1107+
except (json.JSONDecodeError, TypeError, ValueError):
1108+
continue
1109+
try:
1110+
cd = composer.raw
1111+
11021112
# Determine project
11031113
pid = _determine_project_for_conversation(
11041114
cd, composer_id, project_layouts_map,
@@ -1109,11 +1119,11 @@ def get_workspace_tabs(workspace_id):
11091119
if not pid and mapped_ws in invalid_workspace_ids:
11101120
pid = invalid_workspace_aliases.get(mapped_ws)
11111121
assigned = pid if pid else "global"
1112-
1122+
11131123
if assigned not in matching_ws_ids:
11141124
continue
1115-
1116-
headers = cd.get("fullConversationHeadersOnly") or []
1125+
1126+
headers = composer.full_conversation_headers_only
11171127

11181128
# Build bubbles
11191129
bubbles = []

models/conversation.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ def from_dict(cls, raw: dict[str, Any], *, composer_id: str) -> "Composer":
3838
raise SchemaError("Composer", "createdAt")
3939

4040
created_at = raw.get("createdAt")
41+
# Numeric-only on purpose: a 2026-05 scan of 17/17 live composers on
42+
# disk stored createdAt as int milliseconds. If Cursor ever switches
43+
# to ISO strings, those rows would disappear from list/search via a
44+
# drift warning — relax the check at that point, don't silently coerce.
4145
if not isinstance(created_at, (int, float)) or isinstance(created_at, bool):
4246
raise SchemaError(
4347
"Composer",

models/errors.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,12 @@ def __init__(self, model: str, field: str, *, hint: str | None = None) -> None:
88
self.model = model
99
self.field = field
1010
self.hint = hint
11-
message = f"{model}: missing required field '{field}'"
11+
# Distinguish "absent" from "present-but-wrong-shape" so log grepping can
12+
# tell missing-key drift apart from type-mismatch drift. Hint is only
13+
# populated for shape mismatches (e.g. "expected list, got dict"), so its
14+
# presence is the signal.
1215
if hint:
13-
message = f"{message} ({hint})"
16+
message = f"{model}: invalid field '{field}' ({hint})"
17+
else:
18+
message = f"{model}: missing required field '{field}'"
1419
super().__init__(message)

scripts/export.py

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
aggregate_session_stats,
3939
)
4040
from utils.cursor_md_exporter import cursor_cli_session_to_markdown
41+
from models import ExportEntry, SchemaError
4142

4243
_logger = logging.getLogger(__name__)
4344

@@ -62,13 +63,13 @@ def _load_manifest_entries(manifest_path: str) -> dict:
6263
if not line:
6364
continue
6465
try:
65-
entry = json.loads(line)
66-
log_id = entry.get("log_id")
67-
if log_id:
68-
existing[log_id] = entry
69-
except Exception as e:
70-
_logger.debug("Skipping malformed manifest line in %s: %s", manifest_path, e)
71-
except Exception as e:
66+
entry = ExportEntry.from_dict(json.loads(line))
67+
existing[entry.log_id] = entry.raw
68+
except (SchemaError, json.JSONDecodeError, ValueError) as e:
69+
# Pre-PR-30 manifests lack title/workspace — skip them so the
70+
# next export rebuilds the entry under the new schema.
71+
_logger.debug("Skipping manifest line in %s: %s", manifest_path, e)
72+
except OSError as e:
7273
_logger.debug("Failed to read manifest %s: %s", manifest_path, e)
7374
return existing
7475

@@ -818,7 +819,8 @@ def assign_workspace(cd, cid):
818819

819820
rel_path = os.path.join(today, ws_slug, "chat", filename)
820821
exported.append({"id": composer_id, "rel_path": rel_path, "content": md,
821-
"out_path": out_path, "updatedAt": updated_at})
822+
"out_path": out_path, "updatedAt": updated_at,
823+
"title": title, "workspace": ws_display_name})
822824
count += 1
823825

824826
# --- Cursor CLI sessions ---
@@ -915,6 +917,8 @@ def assign_workspace(cd, cid):
915917
"content": md,
916918
"out_path": out_path,
917919
"updatedAt": updated_ms,
920+
"title": title,
921+
"workspace": ws_name,
918922
})
919923
count += 1
920924

@@ -947,6 +951,8 @@ def assign_workspace(cd, cid):
947951
for e in exported:
948952
existing[e["id"]] = {
949953
"log_id": e["id"],
954+
"title": e["title"],
955+
"workspace": e["workspace"],
950956
"path": os.path.relpath(e["out_path"], out_dir),
951957
"updated_at": datetime.fromtimestamp(e["updatedAt"] / 1000).isoformat() if e["updatedAt"] else datetime.now().isoformat(),
952958
}
@@ -960,6 +966,8 @@ def assign_workspace(cd, cid):
960966
for e in exported:
961967
global_existing[e["id"]] = {
962968
"log_id": e["id"],
969+
"title": e["title"],
970+
"workspace": e["workspace"],
963971
"path": e["out_path"],
964972
"updated_at": datetime.fromtimestamp(e["updatedAt"] / 1000).isoformat() if e["updatedAt"] else datetime.now().isoformat(),
965973
}

tests/test_models.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,22 @@ def test_schema_error_inherits_value_error(self) -> None:
191191
return
192192
self.fail("SchemaError did not propagate as ValueError")
193193

194+
def test_schema_error_message_distinguishes_missing_from_invalid(self) -> None:
195+
# Operators grep logs for drift. "missing required field" must only fire
196+
# when a key is absent; type / shape mismatches must read "invalid field"
197+
# so the two failure modes can be told apart.
198+
missing = SchemaError("Composer", "createdAt")
199+
self.assertIn("missing required field", str(missing))
200+
self.assertNotIn("invalid field", str(missing))
201+
202+
type_mismatch = SchemaError(
203+
"Composer", "createdAt",
204+
hint="expected timestamp number, got str",
205+
)
206+
self.assertIn("invalid field", str(type_mismatch))
207+
self.assertNotIn("missing required field", str(type_mismatch))
208+
self.assertIn("expected timestamp number, got str", str(type_mismatch))
209+
194210
def test_non_dict_payload_raises_schema_error(self) -> None:
195211
bad_payloads: tuple[object, ...] = ([], "not a dict", 42, None, ("a", "b"))
196212
for bad in bad_payloads:

0 commit comments

Comments
 (0)