Skip to content

Commit 017d408

Browse files
committed
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 4b8548a commit 017d408

3 files changed

Lines changed: 62 additions & 7 deletions

File tree

api/composers.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,14 @@ def get_composer(composer_id):
154154
# JSON the list endpoint hid.
155155
print(f"Schema drift in workspace-local composer {composer_id}: {e}")
156156
continue
157-
return jsonify(local.raw)
157+
# Match list_composers() at line 89 and the global
158+
# fallback below: `conversation` is normalised to []
159+
# whether it's absent or None, so the response shape
160+
# is identical regardless of which branch resolved
161+
# the composer (CodeRabbit on PR #30).
162+
payload = dict(local.raw)
163+
payload["conversation"] = payload.get("conversation") or []
164+
return jsonify(payload)
158165
except SchemaError as e:
159166
print(f"Schema drift in {db_path}: {e}")
160167
except (OSError, sqlite3.Error, json.JSONDecodeError, ValueError):
@@ -182,7 +189,7 @@ def get_composer(composer_id):
182189
print(f"Schema drift in composer {composer_id}: {e}")
183190
return jsonify({"error": "Composer schema drift"}), 404
184191
payload = dict(composer.raw)
185-
payload.setdefault("conversation", [])
192+
payload["conversation"] = payload.get("conversation") or []
186193
return jsonify(payload)
187194
except (OSError, sqlite3.Error, json.JSONDecodeError, ValueError):
188195
pass

api/workspaces.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -447,8 +447,15 @@ def _infer_invalid_workspace_aliases(
447447
if mapped not in invalid_workspace_ids:
448448
continue
449449
try:
450-
cd = json.loads(row["value"])
451-
except Exception:
450+
# Validate via Composer.from_dict so a schema-drifted row can't
451+
# steer the alias vote and misassign otherwise-valid composers to
452+
# the wrong workspace. The downstream per-row loops in
453+
# list_workspaces() / get_workspace_tabs() already drop drift,
454+
# but this helper runs BEFORE that loop and its votes shape
455+
# invalid_workspace_aliases for every other composer.
456+
composer = Composer.from_dict(json.loads(row["value"]), composer_id=cid)
457+
cd = composer.raw
458+
except (SchemaError, json.JSONDecodeError, TypeError, ValueError):
452459
continue
453460
inferred = _determine_project_for_conversation(
454461
cd,

tests/test_invalid_workspace_aliases.py

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,13 @@
1111

1212
class TestInvalidWorkspaceAliases(unittest.TestCase):
1313
def test_majority_vote_alias_selection(self):
14+
# `createdAt` is required by Composer.from_dict (issue #24's strict
15+
# numeric-millis gate). Production rows always carry it; the original
16+
# fixture predated the gate.
1417
composer_rows = [
15-
{"key": "composerData:cid-1", "value": json.dumps({"fullConversationHeadersOnly": []})},
16-
{"key": "composerData:cid-2", "value": json.dumps({"fullConversationHeadersOnly": []})},
17-
{"key": "composerData:cid-3", "value": json.dumps({"fullConversationHeadersOnly": []})},
18+
{"key": "composerData:cid-1", "value": json.dumps({"createdAt": 1_715_000_000_000, "fullConversationHeadersOnly": []})},
19+
{"key": "composerData:cid-2", "value": json.dumps({"createdAt": 1_715_000_000_000, "fullConversationHeadersOnly": []})},
20+
{"key": "composerData:cid-3", "value": json.dumps({"createdAt": 1_715_000_000_000, "fullConversationHeadersOnly": []})},
1821
]
1922
composer_id_to_ws = {
2023
"cid-1": "invalid-ws",
@@ -46,6 +49,44 @@ def test_majority_vote_alias_selection(self):
4649

4750
self.assertEqual(aliases.get("invalid-ws"), "boost-ws")
4851

52+
def test_drifted_composer_does_not_skew_vote(self):
53+
# CodeRabbit regression check: a schema-drifted composer
54+
# (e.g. missing createdAt) must NOT cast a vote, because if it did
55+
# it could outweigh well-formed composers and misassign every other
56+
# composer mapped to the same invalid workspace.
57+
composer_rows = [
58+
# Two well-formed votes for boost-ws
59+
{"key": "composerData:cid-1", "value": json.dumps({"createdAt": 1_715_000_000_000, "fullConversationHeadersOnly": []})},
60+
{"key": "composerData:cid-2", "value": json.dumps({"createdAt": 1_715_000_000_000, "fullConversationHeadersOnly": []})},
61+
# One drifted row that, if counted, would vote for team-ws
62+
# (no createdAt → Composer.from_dict raises SchemaError → skip)
63+
{"key": "composerData:cid-3", "value": json.dumps({"fullConversationHeadersOnly": []})},
64+
]
65+
composer_id_to_ws = {"cid-1": "invalid-ws", "cid-2": "invalid-ws", "cid-3": "invalid-ws"}
66+
project_layouts_map = {
67+
"cid-1": [normalize_file_path(r"d:\_Cpp_Digest\boostbacklog")],
68+
"cid-2": [normalize_file_path(r"d:\_Cpp_Digest\boostbacklog")],
69+
"cid-3": [normalize_file_path(r"d:\_Cpp_Digest\team-brain")],
70+
}
71+
workspace_path_map = {
72+
normalize_file_path(r"d:\_cpp_digest\boostbacklog"): "boost-ws",
73+
normalize_file_path(r"d:\_cpp_digest\team-brain"): "team-ws",
74+
}
75+
76+
aliases = _infer_invalid_workspace_aliases(
77+
composer_rows=composer_rows,
78+
project_layouts_map=project_layouts_map,
79+
project_name_map={},
80+
workspace_path_map=workspace_path_map,
81+
workspace_entries=[],
82+
bubble_map={},
83+
composer_id_to_ws=composer_id_to_ws,
84+
invalid_workspace_ids={"invalid-ws"},
85+
)
86+
87+
# cid-3 is dropped (drift), so boost-ws wins 2-0 (not 2-1)
88+
self.assertEqual(aliases.get("invalid-ws"), "boost-ws")
89+
4990

5091
if __name__ == "__main__":
5192
unittest.main()

0 commit comments

Comments
 (0)