review: address CodeRabbit findings on PR #31 (5 fixes + tests)

timon0305 · timon0305 · commit 349a5e5bebeb · 2026-05-12T15:25:27.000+02:00
Five CodeRabbit findings on the api/workspaces.py split, addressed
together with regression coverage:

1. services/cli_tabs.py: wrap ``messages_to_bubbles(...)`` in try/except.
   ``traverse_blobs(...)`` was already guarded; one malformed session
   from this second call would 500 the whole tabs endpoint instead of
   being skipped like the traverse failures.
   Test: tests/test_cli_tabs.py mocks one session to throw, verifies the
   endpoint returns 200 with the failing session skipped and the healthy
   session intact.

2. services/workspace_db.py: build SQLite ``file:`` URIs via
   ``Path(...).resolve().as_uri()`` instead of f-string interpolation.
   The naive form breaks on paths containing spaces or other reserved
   characters. The resolver code already used the safe form; aligning
   the db helpers here.
   Test: tests/test_workspace_db_special_paths.py creates a workspace
   inside ``Cursor User/workspaceStorage/`` (path with a space),
   verifies the mapping and global-DB open both succeed.

3. services/workspace_listing.py + workspace_tabs.py: accept
   dict-shaped ``projectLayouts`` entries in addition to JSON-string
   ones. The resolver code already handled both shapes; this brings
   the listing/tabs paths in line so dict layouts no longer leave
   ``project_layouts_map`` empty (which silently fell composers
   back to the "global" bucket).
   Test: tests/test_project_layouts_dict_shape.py runs
   ``list_workspace_projects`` against both shapes, asserts the
   composer is routed to its workspace in both.

4. services/workspace_resolver.py: ``_get_project_from_file_path``
   now uses ``os.path.commonpath`` instead of ``startswith`` so a
   workspace rooted at ``/repo/app`` doesn't sibling-match
   ``/repo/app2/...``. ``ValueError`` from ``commonpath`` (e.g.
   mismatched drive on Windows) is caught as "not within".
   Test: tests/test_project_path_boundary.py covers the sibling-prefix
   case, the file-outside-any-workspace case, and the regular
   inside-workspace case.

5. services/workspace_tabs.py: mark synthetic "Tool Action" bubbles
   with ``synthetic=True`` and skip them in the response-time pass.
   Synthetic bubbles get a fresh ``datetime.now()`` timestamp; without
   the flag they were treated as AI responses from the last user
   message, inflating ``responseTimeMs`` / ``totalResponseTimeMs``
   with values unrelated to model latency. The flag stays internal —
   the tab serializer only copies type/text/timestamp/metadata to
   the wire payload.

186 tests pass (was 178; +8 new).
diff --git a/services/cli_tabs.py b/services/cli_tabs.py
@@ -34,7 +34,11 @@ def _get_cli_workspace_tabs(workspace_id: str):
                 print(f"CLI: could not read session {session_id}: {e}")
                 continue
 
-            bubbles = messages_to_bubbles(messages, created_ms)
+            try:
+                bubbles = messages_to_bubbles(messages, created_ms)
+            except Exception as e:
+                print(f"CLI: could not convert session {session_id} to bubbles: {e}")
+                continue
             if not bubbles:
                 continue
 
diff --git a/services/workspace_db.py b/services/workspace_db.py
@@ -4,6 +4,7 @@
 import os
 import sqlite3
 from contextlib import closing, contextmanager
+from pathlib import Path
 
 from utils.path_helpers import get_workspace_folder_paths
 from utils.workspace_descriptor import _read_json_file
@@ -47,7 +48,10 @@ def _build_composer_id_to_workspace_id(workspace_path: str, workspace_entries: l
             continue
         try:
             # closing() guarantees .close() on scope exit (issue #17).
-            with closing(sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)) as conn:
+            # Path.as_uri() percent-encodes reserved chars; ``f"file:{path}"``
+            # breaks sqlite URI parsing on paths with spaces, ``#``, etc.
+            db_uri = Path(db_path).resolve().as_uri() + "?mode=ro"
+            with closing(sqlite3.connect(db_uri, uri=True)) as conn:
                 row = conn.execute(
                     "SELECT value FROM ItemTable WHERE [key] = 'composer.composerData'"
                 ).fetchone()
@@ -72,7 +76,8 @@ def _open_global_db(workspace_path: str):
     if not os.path.isfile(global_db_path):
         yield None, global_db_path
         return
-    conn = sqlite3.connect(f"file:{global_db_path}?mode=ro", uri=True)
+    db_uri = Path(global_db_path).resolve().as_uri() + "?mode=ro"
+    conn = sqlite3.connect(db_uri, uri=True)
     conn.row_factory = sqlite3.Row
     try:
         yield conn, global_db_path
diff --git a/services/workspace_listing.py b/services/workspace_listing.py
@@ -66,11 +66,11 @@ def list_workspace_projects(workspace_path: str, rules: list) -> list[dict]:
                             for layout in layouts:
                                 if isinstance(layout, str):
                                     try:
-                                        obj = json.loads(layout)
-                                        if isinstance(obj, dict) and obj.get("rootPath"):
-                                            project_layouts_map[cid].append(obj["rootPath"])
+                                        layout = json.loads(layout)
                                     except Exception:
-                                        pass
+                                        continue
+                                if isinstance(layout, dict) and layout.get("rootPath"):
+                                    project_layouts_map[cid].append(layout["rootPath"])
                     except Exception:
                         pass
 
diff --git a/services/workspace_resolver.py b/services/workspace_resolver.py
@@ -112,7 +112,11 @@ def _get_project_from_file_path(
             wd = _read_json_file(entry["workspaceJsonPath"])
             for folder in get_workspace_folder_paths(wd):
                 wp = normalize_file_path(folder)
-                if normalized_path.startswith(wp) and len(wp) > best_len:
+                try:
+                    is_within_workspace = os.path.commonpath([normalized_path, wp]) == wp
+                except ValueError:
+                    is_within_workspace = False
+                if is_within_workspace and len(wp) > best_len:
                     best_len = len(wp)
                     best_match = entry["name"]
         except Exception:
diff --git a/services/workspace_tabs.py b/services/workspace_tabs.py
@@ -138,11 +138,11 @@ def assemble_workspace_tabs(
                         for layout in layouts:
                             if isinstance(layout, str):
                                 try:
-                                    obj = json.loads(layout)
-                                    if isinstance(obj, dict) and obj.get("rootPath"):
-                                        project_layouts_map[cid].append(obj["rootPath"])
+                                    layout = json.loads(layout)
                                 except Exception:
-                                    pass
+                                    continue
+                            if isinstance(layout, dict) and layout.get("rootPath"):
+                                project_layouts_map[cid].append(layout["rootPath"])
                 except Exception:
                     pass
 
@@ -351,6 +351,7 @@ def assemble_workspace_tabs(
                             "type": "ai",
                             "text": f"**Tool Action:**{diff_text}",
                             "timestamp": int(datetime.now().timestamp() * 1000),
+                            "synthetic": True,
                         })
 
                 bubbles.sort(key=lambda b: b.get("timestamp") or 0)
@@ -360,7 +361,7 @@ def assemble_workspace_tabs(
                 for b in bubbles:
                     if b["type"] == "user":
                         last_user_ts = b.get("timestamp")
-                    elif b["type"] == "ai" and last_user_ts is not None:
+                    elif b["type"] == "ai" and last_user_ts is not None and not b.get("synthetic"):
                         ts = b.get("timestamp")
                         if ts and ts > last_user_ts:
                             meta = b.setdefault("metadata", {})
diff --git a/tests/test_cli_tabs.py b/tests/test_cli_tabs.py
@@ -0,0 +1,72 @@
+from __future__ import annotations
+
+import os
+import sys
+import unittest
+from unittest.mock import patch
+
+from flask import Flask
+
+REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+if REPO_ROOT not in sys.path:
+    sys.path.insert(0, REPO_ROOT)
+
+from services.cli_tabs import _get_cli_workspace_tabs
+
+
+def _make_app():
+    app = Flask(__name__)
+    app.config["TESTING"] = True
+    app.config["EXCLUSION_RULES"] = []
+    return app
+
+
+def _fake_project(project_id: str, sessions: list[dict]) -> dict:
+    return {
+        "project_id": project_id,
+        "workspace_name": "Test Project",
+        "workspace_path": "/tmp/test",
+        "last_updated_ms": 1_715_000_000_000,
+        "sessions": sessions,
+    }
+
+
+def _fake_session(session_id: str) -> dict:
+    return {
+        "session_id": session_id,
+        "db_path": f"/tmp/{session_id}.db",
+        "meta": {"createdAt": 1_715_000_000_000, "name": "Session"},
+    }
+
+
+class TestMessagesToBubblesFailureIsolation(unittest.TestCase):
+    def test_failing_session_does_not_500_endpoint(self) -> None:
+        app = _make_app()
+        project = _fake_project("proj-1", [
+            _fake_session("sess-bad"),
+            _fake_session("sess-good"),
+        ])
+
+        def fake_messages_to_bubbles(messages, created_ms):
+            if messages == ["bad"]:
+                raise RuntimeError("simulated bubble conversion failure")
+            return [{"type": "user", "text": "hi", "timestamp": created_ms}]
+
+        def fake_traverse_blobs(db_path):
+            return ["bad"] if "sess-bad" in db_path else ["ok"]
+
+        with app.test_request_context("/api/workspaces/cli:proj-1/tabs"), \
+             patch("services.cli_tabs.list_cli_projects", return_value=[project]), \
+             patch("services.cli_tabs.traverse_blobs", side_effect=fake_traverse_blobs), \
+             patch("services.cli_tabs.messages_to_bubbles", side_effect=fake_messages_to_bubbles):
+            response = _get_cli_workspace_tabs("cli:proj-1")
+
+        self.assertEqual(response.status_code, 200)
+        payload = response.get_json()
+        tab_ids = [t["id"] for t in payload["tabs"]]
+        self.assertNotIn("sess-bad", tab_ids)
+        self.assertIn("sess-good", tab_ids)
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/tests/test_project_layouts_dict_shape.py b/tests/test_project_layouts_dict_shape.py
@@ -0,0 +1,86 @@
+from __future__ import annotations
+
+import json
+import os
+import sqlite3
+import sys
+import tempfile
+import unittest
+
+REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+if REPO_ROOT not in sys.path:
+    sys.path.insert(0, REPO_ROOT)
+
+from services.workspace_listing import list_workspace_projects
+
+
+def _make_workspace_storage(parent: str, *, layout_as_dict: bool) -> str:
+    ws_root = os.path.join(parent, "workspaceStorage")
+    global_root = os.path.join(parent, "globalStorage")
+    os.makedirs(ws_root, exist_ok=True)
+    os.makedirs(global_root, exist_ok=True)
+
+    ws_dir = os.path.join(ws_root, "ws-a")
+    os.makedirs(ws_dir, exist_ok=True)
+    target_folder = os.path.join(parent, "real-project")
+    os.makedirs(target_folder, exist_ok=True)
+    with open(os.path.join(ws_dir, "workspace.json"), "w") as f:
+        json.dump({"folder": f"file://{target_folder}"}, f)
+    sqlite3.connect(os.path.join(ws_dir, "state.vscdb")).close()
+
+    layout_payload: object
+    if layout_as_dict:
+        layout_payload = {"rootPath": target_folder}
+    else:
+        layout_payload = json.dumps({"rootPath": target_folder})
+
+    gdb = os.path.join(global_root, "state.vscdb")
+    conn = sqlite3.connect(gdb)
+    conn.execute("CREATE TABLE cursorDiskKV ([key] TEXT PRIMARY KEY, value TEXT)")
+    conn.execute(
+        "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
+        (
+            "composerData:cmp-1",
+            json.dumps({
+                "name": "Test composer",
+                "createdAt": 1_715_000_000_000,
+                "lastUpdatedAt": 1_715_000_500_000,
+                "fullConversationHeadersOnly": [{"bubbleId": "b-1"}],
+            }),
+        ),
+    )
+    conn.execute(
+        "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
+        (
+            "messageRequestContext:cmp-1:ctx-1",
+            json.dumps({"projectLayouts": [layout_payload]}),
+        ),
+    )
+    conn.execute(
+        "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
+        ("bubbleId:cmp-1:b-1", json.dumps({"type": "user", "text": "hello"})),
+    )
+    conn.commit()
+    conn.close()
+    return ws_root
+
+
+class TestProjectLayoutsDictShape(unittest.TestCase):
+    def _assert_assigned_to_workspace(self, ws_root: str) -> None:
+        projects = list_workspace_projects(ws_root, rules=[])
+        ids = [p["id"] for p in projects]
+        self.assertIn("ws-a", ids, msg=f"expected composer routed to ws-a, got {ids}")
+
+    def test_string_shaped_layout(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            ws_root = _make_workspace_storage(tmp, layout_as_dict=False)
+            self._assert_assigned_to_workspace(ws_root)
+
+    def test_dict_shaped_layout(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            ws_root = _make_workspace_storage(tmp, layout_as_dict=True)
+            self._assert_assigned_to_workspace(ws_root)
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/tests/test_project_path_boundary.py b/tests/test_project_path_boundary.py
@@ -0,0 +1,64 @@
+from __future__ import annotations
+
+import json
+import os
+import sys
+import tempfile
+import unittest
+
+REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+if REPO_ROOT not in sys.path:
+    sys.path.insert(0, REPO_ROOT)
+
+from services.workspace_resolver import _get_project_from_file_path
+
+
+def _write_workspace_json(parent: str, name: str, folder: str) -> dict:
+    ws_dir = os.path.join(parent, name)
+    os.makedirs(ws_dir, exist_ok=True)
+    wj = os.path.join(ws_dir, "workspace.json")
+    with open(wj, "w") as f:
+        json.dump({"folder": folder}, f)
+    return {"name": name, "workspaceJsonPath": wj}
+
+
+class TestPrefixCollision(unittest.TestCase):
+    def test_sibling_prefix_does_not_match(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            app = os.path.join(tmp, "repo", "app")
+            app2 = os.path.join(tmp, "repo", "app2")
+            os.makedirs(app, exist_ok=True)
+            os.makedirs(app2, exist_ok=True)
+
+            entries = [
+                _write_workspace_json(tmp, "ws-app", app),
+                _write_workspace_json(tmp, "ws-app2", app2),
+            ]
+
+            file_in_app2 = os.path.join(app2, "src", "main.py")
+            self.assertEqual(
+                _get_project_from_file_path(file_in_app2, entries),
+                "ws-app2",
+            )
+
+    def test_file_outside_any_workspace_returns_none(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            app = os.path.join(tmp, "repo", "app")
+            os.makedirs(app, exist_ok=True)
+            entries = [_write_workspace_json(tmp, "ws-app", app)]
+
+            unrelated = os.path.join(tmp, "elsewhere", "file.py")
+            self.assertIsNone(_get_project_from_file_path(unrelated, entries))
+
+    def test_file_inside_workspace_still_matches(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            app = os.path.join(tmp, "repo", "app")
+            os.makedirs(app, exist_ok=True)
+            entries = [_write_workspace_json(tmp, "ws-app", app)]
+
+            inside = os.path.join(app, "src", "main.py")
+            self.assertEqual(_get_project_from_file_path(inside, entries), "ws-app")
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/tests/test_workspace_db_special_paths.py b/tests/test_workspace_db_special_paths.py
