review: Brad's PR #33 pass — XDG isolation, --no-composer fix, flag coverage

timon0305 · timon0305 · commit c1ae9d14835a · 2026-05-13T19:21:51.000+02:00
Six follow-ups across two threads of Brad's review.

Production fixes:

* scripts/export.py get_global_state_dir() now honors XDG_STATE_HOME
  (returns $XDG_STATE_HOME/cursor-chat-browser), falling back to the
  historical ~/.cursor-chat-browser only when the env var is unset.
  Without this, the --since last test leaked state into the developer's
  real home directory; the test still passed but only by timestamp
  coincidence and corrupted ~/.cursor-chat-browser/export_state.json
  on every CI run.

* scripts/export.py --no-composer was a parsed-but-unused flag —
  opts["include_composer"] was set but never consulted. Added the
  missing guard at the IDE-composer iteration site so the flag now
  actually skips composer rows.

Test additions to tests/test_cli_export_e2e.py:

* Both DB-seeding helpers (_make_global_state_db, _make_workspace_storage)
  now use contextlib.closing(sqlite3.connect(...)). A mid-setup
  exception used to leak the handle and on Windows that blocks
  TemporaryDirectory.cleanup().

* New TestGetGlobalStateDir regression class — two tests pinning
  both branches of the XDG behavior so this can't silently regress.

* test_no_composer_skips_ide_composer_data — fixture seeds composer
  data exclusively, so --no-composer must produce zero markdown.

* test_exclude_rules_filters_matching_composer — writes a rules file
  containing "E2E" (substring of the seeded composer's title) and
  asserts the seeded composer is filtered out before any markdown is
  written.

Doc fix in tests/cli-export-qa-checklist.md:

* `curl -sI` sends HTTP/1.1 by default and the dev server replies in
  kind. Listing HTTP/1.0 first would have triggered false-fail reports
  from testers seeing HTTP/1.1.

Verified locally:
  - python3 -m unittest tests.test_cli_export_e2e: 9 passed
  - python3 -m unittest discover tests: 187 passed, OK
  - ~/.cursor-chat-browser never created during the run
  - python3 app.py boots clean; curl -sI returns HTTP/1.1 200 OK
diff --git a/scripts/export.py b/scripts/export.py
@@ -129,6 +129,13 @@ def resolve_workspace_path() -> str:
 
 
 def get_global_state_dir() -> str:
+    # Honor XDG_STATE_HOME when set so the export state file (and manifest)
+    # can be redirected — required for hermetic test runs and useful for
+    # users following the XDG Base Directory spec. Falls back to the
+    # historical ~/.cursor-chat-browser location when the env var is unset.
+    xdg = os.environ.get("XDG_STATE_HOME")
+    if xdg:
+        return os.path.join(xdg, "cursor-chat-browser")
     return os.path.join(str(Path.home()), ".cursor-chat-browser")
 
 
@@ -486,8 +493,9 @@ def assign_workspace(cd, cid):
     exported = []
     count = 0
 
-    # Process IDE composers
-    for row in ide_composer_rows:
+    # Process IDE composers (skipped entirely when --no-composer was passed)
+    include_composer = opts.get("include_composer", True)
+    for row in ide_composer_rows if include_composer else []:
         composer_id = row["key"].split(":")[1]
         try:
             cd = json.loads(row["value"])
diff --git a/tests/cli-export-qa-checklist.md b/tests/cli-export-qa-checklist.md
@@ -68,7 +68,7 @@ python app.py
 ```
 
 - [ ] Process stays running for at least 30 s without crash
-- [ ] `curl -sI http://127.0.0.1:3000/` returns `HTTP/1.0 200 OK` (or 200 over 1.1)
+- [ ] `curl -sI http://127.0.0.1:3000/` returns `HTTP/1.1 200 OK` (or `HTTP/1.0 200 OK` under some dev-server configurations)
 - [ ] Home page lists at least one workspace card (assuming real Cursor data)
 - [ ] No `Exception` / `Traceback` in server log
 
diff --git a/tests/test_cli_export_e2e.py b/tests/test_cli_export_e2e.py
@@ -9,7 +9,7 @@
 import tempfile
 import unittest
 import zipfile
-from contextlib import contextmanager
+from contextlib import closing, contextmanager
 from pathlib import Path
 
 REPO_ROOT = Path(__file__).resolve().parent.parent
@@ -25,36 +25,38 @@
 
 
 def _make_global_state_db(path: str, *, last_updated_ms: int = 1_715_000_500_000) -> None:
-    conn = sqlite3.connect(path)
-    conn.execute("CREATE TABLE cursorDiskKV ([key] TEXT PRIMARY KEY, value TEXT)")
-    conn.execute(
-        "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
-        (
-            f"composerData:{HAPPY_COMPOSER_ID}",
-            json.dumps({
-                "name": "Export E2E conversation",
-                "createdAt": 1_715_000_000_000,
-                "lastUpdatedAt": last_updated_ms,
-                "fullConversationHeadersOnly": [
-                    {"bubbleId": HAPPY_BUBBLE_ID, "type": 1},
-                ],
-                "modelConfig": {"modelName": "gpt-4o"},
-            }),
-        ),
-    )
-    conn.execute(
-        "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
-        (
-            f"bubbleId:{HAPPY_COMPOSER_ID}:{HAPPY_BUBBLE_ID}",
-            json.dumps({
-                "text": "hello from the e2e test fixture",
-                "type": "user",
-                "createdAt": 1_715_000_400_000,
-            }),
-        ),
-    )
-    conn.commit()
-    conn.close()
+    # closing() guarantees conn.close() even if an exec/commit raises
+    # mid-setup. On Windows TemporaryDirectory.cleanup() refuses to delete
+    # an open SQLite file, so a leaked handle would fail the whole test.
+    with closing(sqlite3.connect(path)) as conn:
+        conn.execute("CREATE TABLE cursorDiskKV ([key] TEXT PRIMARY KEY, value TEXT)")
+        conn.execute(
+            "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
+            (
+                f"composerData:{HAPPY_COMPOSER_ID}",
+                json.dumps({
+                    "name": "Export E2E conversation",
+                    "createdAt": 1_715_000_000_000,
+                    "lastUpdatedAt": last_updated_ms,
+                    "fullConversationHeadersOnly": [
+                        {"bubbleId": HAPPY_BUBBLE_ID, "type": 1},
+                    ],
+                    "modelConfig": {"modelName": "gpt-4o"},
+                }),
+            ),
+        )
+        conn.execute(
+            "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
+            (
+                f"bubbleId:{HAPPY_COMPOSER_ID}:{HAPPY_BUBBLE_ID}",
+                json.dumps({
+                    "text": "hello from the e2e test fixture",
+                    "type": "user",
+                    "createdAt": 1_715_000_400_000,
+                }),
+            ),
+        )
+        conn.commit()
 
 
 def _make_workspace_storage(parent: str, *, last_updated_ms: int = 1_715_000_500_000) -> str:
@@ -70,14 +72,13 @@ def _make_workspace_storage(parent: str, *, last_updated_ms: int = 1_715_000_500
     with open(os.path.join(ws_dir, "workspace.json"), "w", encoding="utf-8") as f:
         json.dump({"folder": project_folder}, f)
     local_db = os.path.join(ws_dir, "state.vscdb")
-    conn = sqlite3.connect(local_db)
-    conn.execute("CREATE TABLE ItemTable ([key] TEXT PRIMARY KEY, value TEXT)")
-    conn.execute(
-        "INSERT INTO ItemTable ([key], value) VALUES (?, ?)",
-        ("composer.composerData", json.dumps({"allComposers": [{"composerId": HAPPY_COMPOSER_ID}]})),
-    )
-    conn.commit()
-    conn.close()
+    with closing(sqlite3.connect(local_db)) as conn:
+        conn.execute("CREATE TABLE ItemTable ([key] TEXT PRIMARY KEY, value TEXT)")
+        conn.execute(
+            "INSERT INTO ItemTable ([key], value) VALUES (?, ?)",
+            ("composer.composerData", json.dumps({"allComposers": [{"composerId": HAPPY_COMPOSER_ID}]})),
+        )
+        conn.commit()
 
     _make_global_state_db(os.path.join(global_root, "state.vscdb"), last_updated_ms=last_updated_ms)
     return ws_root
@@ -115,6 +116,38 @@ def _run_export(argv: list[str], *, workspace_path: str, state_dir: str):
                 os.environ[key] = prior
 
 
+class TestGetGlobalStateDir(unittest.TestCase):
+    """Regression: get_global_state_dir() must honor XDG_STATE_HOME.
+
+    Before the fix it hardcoded ~/.cursor-chat-browser, which leaked test
+    state into the developer's real home directory and made the
+    --since last test pass only by timestamp coincidence.
+    """
+
+    def setUp(self):
+        self._prior = os.environ.get("XDG_STATE_HOME")
+
+    def tearDown(self):
+        if self._prior is None:
+            os.environ.pop("XDG_STATE_HOME", None)
+        else:
+            os.environ["XDG_STATE_HOME"] = self._prior
+
+    def test_uses_xdg_state_home_when_set(self):
+        os.environ["XDG_STATE_HOME"] = "/tmp/some-xdg-root"
+        self.assertEqual(
+            export_script.get_global_state_dir(),
+            "/tmp/some-xdg-root/cursor-chat-browser",
+        )
+
+    def test_falls_back_to_home_when_xdg_unset(self):
+        os.environ.pop("XDG_STATE_HOME", None)
+        self.assertEqual(
+            export_script.get_global_state_dir(),
+            os.path.join(str(Path.home()), ".cursor-chat-browser"),
+        )
+
+
 class TestCliExportEndToEnd(unittest.TestCase):
     def setUp(self):
         self._tmp = tempfile.TemporaryDirectory()
@@ -238,6 +271,49 @@ def test_since_last_skips_already_exported_records(self):
             msg="--since last should not rewrite the already-exported markdown",
         )
 
+    # ─── --no-composer ─────────────────────────────────────────────────────
+
+    def test_no_composer_skips_ide_composer_data(self):
+        # The fixture seeds IDE composer data exclusively (no CLI sessions
+        # under CLI_CHATS_PATH). --no-composer must therefore drop the
+        # seeded composer and exit with "No conversations found.", leaving
+        # the output dir empty of .md files.
+        with _run_export(["--out", self.out_dir, "--no-zip", "--no-composer"],
+                         workspace_path=self.workspace_path,
+                         state_dir=self.state_dir):
+            pass
+
+        md_files = list(Path(self.out_dir).rglob("*.md"))
+        self.assertEqual(
+            md_files, [],
+            msg=f"--no-composer must produce zero markdown; got {[p.name for p in md_files]}",
+        )
+
+    # ─── --exclude-rules ───────────────────────────────────────────────────
+
+    def test_exclude_rules_filters_matching_composer(self):
+        # A rule whose word matches the composer's title must drop it
+        # before any markdown is written. The fixture's composer is named
+        # "Export E2E conversation" so a single-token rule of "E2E" is
+        # enough to match via the case-insensitive substring check.
+        rules_path = os.path.join(self.tmpdir, "exclusion-rules.txt")
+        with open(rules_path, "w", encoding="utf-8") as f:
+            f.write("E2E\n")
+
+        with _run_export(
+            ["--out", self.out_dir, "--no-zip", "--exclude-rules", rules_path],
+            workspace_path=self.workspace_path,
+            state_dir=self.state_dir,
+        ):
+            pass
+
+        md_files = list(Path(self.out_dir).rglob("*.md"))
+        matched = [p for p in md_files if HAPPY_COMPOSER_ID[:8] in p.name]
+        self.assertEqual(
+            matched, [],
+            msg=f"--exclude-rules failed to filter seeded composer; got {[p.name for p in md_files]}",
+        )
+
 
 if __name__ == "__main__":
     unittest.main()
