Commit 1a0410d
authored
* feat: typed models + schema validation at DB read boundaries (closes #24)
Add models/ package with @DataClass definitions for Workspace, Composer,
Bubble, CliSessionMeta, ExportEntry. Each model has a from_dict() classmethod
that raises SchemaError when critical fields are missing, replacing silent
dict.get() fallbacks at the four database read boundaries identified by
the eval (Section 5.1, Schema Fragility).
Wire into:
- api/workspaces.py:601 — composerData rows in workspace listing
- api/composers.py:57-63 — allComposers envelope in per-workspace fetch
- api/search.py:163-164 — composerData rows during search
- utils/cli_chat_reader.py:93-98 — CLI session meta blob in traverse_blobs
Schema drift surfaces as a printed warning + skipped row, not a silent
empty result. Existing call sites preserve their JSON response shapes
(behavior-preserving wire-in).
* review: address CodeRabbit feedback on PR #30 — tighten schema gates
Six fixes from the CodeRabbit pass on the initial typed-models commit,
plus matching regression tests so each can't silently regress later.
1. api/composers.py: validate the top-level JSON payload is a dict before
key/list checks. A non-object decoded value previously bypassed the
SchemaError path and was swallowed by the surrounding except.
2. models/{cli_session,conversation,workspace,export}.py: same isinstance
guard at the top of every from_dict, so a non-dict raw (list, str, None)
surfaces as SchemaError instead of AttributeError.
3. models/conversation.py: drop the `raw.get(...) or []` fallback on
fullConversationHeadersOnly. The `or []` idiom coerced None/"" / 0
into [] and made the isinstance gate unreachable.
4. models/export.py: replace str(raw[...]) coercion of required string
fields with an isinstance(value, str) gate. Coercion masked drift —
an int log_id would silently turn into a string.
5. models/conversation.py: re-gate createdAt as required per issue #24.
A live workspaceStorage scan confirmed 17/17 composers carry it,
so a missing value is real schema drift, not benign older-record
absence. Two existing test fixtures in test_search_exclusion_filtering
that lacked createdAt updated; the prior tolerated test replaced with
a spec-correct missing-raises test.
6. utils/cli_chat_reader.py: broaden the traverse_blobs catch to
(SchemaError, ValueError, UnicodeDecodeError, TypeError). Malformed
hex, non-UTF-8 bytes, and invalid JSON in the meta blob previously
escaped to the caller; now all four routes return [] uniformly.
Regression tests added:
- tests/test_models.py: non_dict_payload (5 models × 5 bad types),
headers_falsy_non_list, headers_empty_list_is_valid,
export_entry_non_string_required, missing_created_at_raises
- tests/test_cli_chat_reader.py: malformed_hex, non_utf8, invalid_json,
non_dict meta payloads each return [] via traverse_blobs
201 tests pass (was 197); mypy clean on models/; real Cursor data
(17 composers) still loads with zero Schema drift warnings.
* review: route per-row workspace-local composer drift through SchemaError (#30)
CodeRabbit flagged that the inline ``isinstance(c, dict) or not c.get(...)``
skip in api/composers.py masked row-level drift instead of surfacing it the
way the rest of the boundary does. The exact suggested fix
(Composer.from_dict(c)) would have rejected every workspace-local record:
those carry composerId + lastUpdatedAt but not fullConversationHeadersOnly
or createdAt, which only exist on the global cursorDiskKV rows.
Add WorkspaceLocalComposer to models/conversation.py — a slim sibling
model that validates the workspace-local shape (composerId required as a
non-empty string; non-dict raw raises). api/composers.py:list_composers
now wraps each entry in WorkspaceLocalComposer.from_dict, so per-row
drift logs a Schema drift warning and skips, matching every other
read-site in this PR.
Regression coverage in tests/test_models.py: parses good shape;
raises SchemaError for missing/empty/non-string composerId and for
non-dict payloads.
204 tests pass (was 201; +3); mypy clean on models/; real Cursor smoke
test shows zero drift warnings.
* review: log non-schema read failures in list_composers (#30)
CodeRabbit flagged that the bare ``except Exception: pass`` adjacent to
the new SchemaError path swallowed decode / SQLite / non-schema errors
silently and returned a partial composer list with no signal. Replace
with a print that surfaces the exception and ``db_path``, matching the
``Schema drift in ...`` line a few lines up. No behaviour change beyond
observability — the loop still continues to the next workspace.
* docs: drop module docstrings and trim class docstrings to one-liners (#30)
Internal cleanup on the typed-models package — file location and class
name carry the intent; the multi-paragraph rationales were rehashing the
PR description. Function bodies are unchanged.
- models/__init__.py: removed 8-line module docstring
- models/errors.py: dropped module docstring + SchemaError
class docstring trimmed to one line
- models/conversation.py: dropped module docstring; Composer,
WorkspaceLocalComposer, Bubble docstrings
trimmed to one line each
- models/cli_session.py: dropped module docstring; CliSessionMeta
docstring trimmed
- models/workspace.py: dropped module docstring; Workspace
docstring trimmed
- models/export.py: dropped module docstring; ExportEntry
docstring trimmed
- tests/test_models.py: dropped module docstring; removed
now-unused ``from pathlib import Path``
204 tests pass.
* docs: drop task-reference comments and section banners (#30)
Per the review nit: stop narrating in code. Removes:
- tests/test_models.py: 3 ``# --- section ---`` banners and 10
inline comments referencing issue #24 / CodeRabbit findings.
- tests/test_cli_chat_reader.py: 4 ``"""Regression: ..."""``
docstrings and 4 line-comments restating what the test exercises.
- utils/cli_chat_reader.py: 5-line narration on the broadened
except arm — the tuple of exception types is the documentation.
Test names and code already describe the WHAT; the WHY for these cases
is in the PR description and the commit history, not the source.
204 tests still pass.
* review: tighten ID + createdAt type gates across the model layer (#30)
Three related CodeRabbit findings, addressed together:
- Composer.composer_id, Bubble.bubble_id, Workspace.workspace_id now
require ``isinstance(..., str) and value != ""``. Truthiness alone
let non-string IDs (e.g. ``123``) slip past the boundary; aligned
with the pattern already in WorkspaceLocalComposer.
- Composer.createdAt is now type-gated. Presence was already enforced
but any value type (e.g. ``"2026-05-12"``) silently became the
timestamp. Required values must be ``int`` or ``float`` and explicitly
not ``bool`` (since ``bool`` is a subclass of ``int`` in Python and
a boolean is never a real timestamp).
Regression tests pin each new gate:
- test_non_string_composer_id_raises
- test_non_string_bubble_id_raises
- test_non_string_workspace_id_raises
- test_non_numeric_created_at_raises
- test_numeric_created_at_passes
209 tests pass (was 204; +5 new).
* 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
* review: log schema drift + harden get_composer envelope (#30)
CodeRabbit follow-ups to the round-2 wiring pass:
1. The three bubble-loading loops (api/workspaces.py:583, 1025;
api/search.py:149) caught SchemaError, json.JSONDecodeError and
ValueError together and dropped the row silently. That broke the
read-boundary contract — a drifted bubble would simply vanish with
no operator signal. Split the catches: SchemaError now logs
`Schema drift in bubble <bid>: <reason>`; JSONDecodeError /
ValueError still pass silently because that's a parser-noise issue,
not a schema-contract issue.
2. get_composer() at api/composers.py:125 trusted that the per-workspace
blob was a dict with a list-valued allComposers. A drifted local row
like `[]` or `"…"` raised AttributeError on data.get(...) and turned
schema drift into a 500. Mirrored the three envelope guards
list_composers() already applies at line 60–74 (data is dict,
allComposers present, allComposers is list) and wrapped the outer
try with `except SchemaError` so drift logs the db_path and falls
through to the global fallback instead of 500'ing.
3. scripts/ lacked __init__.py while every other top-level package
directory (api/, models/, utils/, tests/) has one. The new
`from scripts import export` calls in tests resolve at runtime via
the sys.path insert in the test header, but mypy was reporting
inconsistent module resolution on scripts.export — a CI typecheck
risk. Added an empty scripts/__init__.py to match convention.
3 new regression tests in tests/test_models_wired_at_read_sites.py:
- test_bubble_schema_drift_is_logged_not_swallowed_silently —
seeds a malformed bubble, captures stdout, asserts the
`Schema drift in bubble <bid>` line is present.
- test_get_composer_handles_non_dict_envelope_via_fallback —
replaces composer.composerData with a list payload, asserts
the endpoint returns 200 via the global fallback (was 500
before the envelope guards).
- test_get_composer_handles_non_list_all_composers_via_fallback —
same shape for non-list allComposers.
Verified locally:
- python -m unittest discover tests: 223 passed (was 220)
- python -m unittest tests.test_models_wired_at_read_sites: 13/13
- mypy resolves scripts.export consistently
* review: 2 CodeRabbit findings on PR #30 — response-shape parity + alias-vote drift guard
* api/composers.py — get_composer() returned three different shapes for
the same conceptual `conversation` field depending on which branch
resolved the composer:
- list_composers (line 89): `c["conversation"] = c.get("conversation") or []`
- get_composer per-workspace (line 157): `return jsonify(local.raw)` — raw, no normalisation
- get_composer global fallback (line 185): `payload.setdefault("conversation", [])` — fills missing but not None
All three sites now use the line-89 idiom, so the endpoint family
returns an identical shape regardless of source.
* api/workspaces.py — `_infer_invalid_workspace_aliases()` was reparsing
raw composer_rows with `json.loads` + bare `except Exception` *before*
the per-row Composer.from_dict gates in list_workspaces() /
get_workspace_tabs() ran. That meant schema-drifted composers could
still cast votes in the workspace-alias inference and misassign
otherwise-valid composers to the wrong workspace. Now gates via
`Composer.from_dict(...)` with a narrow
`(SchemaError, JSONDecodeError, TypeError, ValueError)` catch.
Existing test_majority_vote_alias_selection fixture predated the
strict createdAt gate from issue #24; added `createdAt` to each of
the three composer rows so the fixture reflects real production shape.
New test_drifted_composer_does_not_skew_vote pins the regression:
seeds two well-formed boost-ws votes plus one drifted row that would
vote for team-ws if counted, asserts the alias resolves to boost-ws.
Verified:
- mypy --ignore-missing-imports --no-strict-optional . → Success
- ruff check api/ utils/ scripts/export.py app.py → All checks passed
- unittest discover tests → 224 / 224 OK (was 223 + 1 new regression test)
1 parent 4210226 commit 1a0410d
16 files changed
Lines changed: 1274 additions & 70 deletions
File tree
- api
- models
- scripts
- tests
- utils
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
| 17 | + | |
17 | 18 | | |
18 | 19 | | |
19 | 20 | | |
| |||
42 | 43 | | |
43 | 44 | | |
44 | 45 | | |
45 | | - | |
46 | | - | |
47 | | - | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
48 | 51 | | |
49 | 52 | | |
50 | 53 | | |
| |||
56 | 59 | | |
57 | 60 | | |
58 | 61 | | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
59 | 70 | | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
64 | | - | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
71 | 100 | | |
72 | 101 | | |
73 | 102 | | |
| |||
97 | 126 | | |
98 | 127 | | |
99 | 128 | | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
104 | 168 | | |
105 | 169 | | |
106 | 170 | | |
| |||
116 | 180 | | |
117 | 181 | | |
118 | 182 | | |
119 | | - | |
120 | | - | |
121 | | - | |
122 | | - | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
123 | 195 | | |
124 | 196 | | |
125 | 197 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| 22 | + | |
22 | 23 | | |
23 | 24 | | |
24 | 25 | | |
| |||
148 | 149 | | |
149 | 150 | | |
150 | 151 | | |
151 | | - | |
152 | | - | |
153 | | - | |
154 | | - | |
155 | | - | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
156 | 161 | | |
157 | 162 | | |
158 | 163 | | |
| |||
163 | 168 | | |
164 | 169 | | |
165 | 170 | | |
166 | | - | |
167 | | - | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
168 | 180 | | |
169 | 181 | | |
170 | 182 | | |
171 | | - | |
| 183 | + | |
172 | 184 | | |
173 | 185 | | |
174 | 186 | | |
175 | 187 | | |
176 | | - | |
| 188 | + | |
177 | 189 | | |
178 | 190 | | |
179 | 191 | | |
| |||
245 | 257 | | |
246 | 258 | | |
247 | 259 | | |
248 | | - | |
| 260 | + | |
249 | 261 | | |
250 | 262 | | |
251 | 263 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
| 38 | + | |
38 | 39 | | |
39 | 40 | | |
40 | 41 | | |
| |||
53 | 54 | | |
54 | 55 | | |
55 | 56 | | |
56 | | - | |
57 | | - | |
| 57 | + | |
| 58 | + | |
58 | 59 | | |
59 | 60 | | |
60 | | - | |
| 61 | + | |
61 | 62 | | |
62 | 63 | | |
63 | 64 | | |
| |||
446 | 447 | | |
447 | 448 | | |
448 | 449 | | |
449 | | - | |
450 | | - | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
451 | 459 | | |
452 | 460 | | |
453 | 461 | | |
| |||
583 | 591 | | |
584 | 592 | | |
585 | 593 | | |
586 | | - | |
587 | | - | |
588 | | - | |
589 | | - | |
| 594 | + | |
| 595 | + | |
| 596 | + | |
| 597 | + | |
| 598 | + | |
| 599 | + | |
| 600 | + | |
| 601 | + | |
590 | 602 | | |
591 | 603 | | |
592 | 604 | | |
| |||
603 | 615 | | |
604 | 616 | | |
605 | 617 | | |
606 | | - | |
| 618 | + | |
| 619 | + | |
| 620 | + | |
| 621 | + | |
| 622 | + | |
| 623 | + | |
| 624 | + | |
607 | 625 | | |
608 | | - | |
| 626 | + | |
609 | 627 | | |
610 | 628 | | |
611 | 629 | | |
| |||
614 | 632 | | |
615 | 633 | | |
616 | 634 | | |
617 | | - | |
| 635 | + | |
618 | 636 | | |
619 | 637 | | |
620 | 638 | | |
621 | 639 | | |
622 | 640 | | |
623 | 641 | | |
624 | | - | |
625 | | - | |
626 | | - | |
| 642 | + | |
| 643 | + | |
| 644 | + | |
627 | 645 | | |
628 | 646 | | |
629 | 647 | | |
| |||
1013 | 1031 | | |
1014 | 1032 | | |
1015 | 1033 | | |
1016 | | - | |
1017 | | - | |
1018 | | - | |
1019 | | - | |
| 1034 | + | |
| 1035 | + | |
| 1036 | + | |
| 1037 | + | |
| 1038 | + | |
| 1039 | + | |
| 1040 | + | |
| 1041 | + | |
1020 | 1042 | | |
1021 | 1043 | | |
1022 | 1044 | | |
| |||
1091 | 1113 | | |
1092 | 1114 | | |
1093 | 1115 | | |
1094 | | - | |
1095 | | - | |
| 1116 | + | |
| 1117 | + | |
| 1118 | + | |
| 1119 | + | |
| 1120 | + | |
| 1121 | + | |
| 1122 | + | |
| 1123 | + | |
| 1124 | + | |
| 1125 | + | |
| 1126 | + | |
1096 | 1127 | | |
1097 | 1128 | | |
1098 | 1129 | | |
| |||
1103 | 1134 | | |
1104 | 1135 | | |
1105 | 1136 | | |
1106 | | - | |
| 1137 | + | |
1107 | 1138 | | |
1108 | 1139 | | |
1109 | | - | |
1110 | | - | |
| 1140 | + | |
| 1141 | + | |
1111 | 1142 | | |
1112 | 1143 | | |
1113 | 1144 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
0 commit comments