fix: wrap all production sqlite3.connect() in context managers (closes #17)#23
Merged
Conversation
…#17) 19 production sqlite3.connect() call sites used a bare local variable with no `with` block, no try/finally, and no guaranteed .close() on exception. On a long-lived Flask process, leaks accumulate before the user notices. Two patterns applied: `with closing(sqlite3.connect(...)) as conn:` — used at 15 short-scope sites (composers ×3, cli_chat_reader ×2, cursor_md_exporter ×1, workspaces ×2 small helpers, logs ×2, search ×2, export_api ×1). sqlite3.Connection's own context manager only commits/rolls back, not closes. closing() is what guarantees .close(). `try / except / finally: if conn is not None: conn.close()` — used at 4 long-scope sites (workspaces.get_workspace_tabs ~400 lines, search.search ×2 deep blocks, export_api.export_chats ~340 lines, scripts/export.py). Same cleanup guarantee, no 100-300-line indent shift. Equivalent semantics to a body-wide `with closing(...)` block. Plus utils/workspaces._open_global_db converted to @contextmanager so its two callers write `with _open_global_db(...) as (conn, _):` and the connection is released automatically. Test-side sqlite3.connect calls are out of scope (per-test fixtures, short-lived). Verification: - All 19 production sites classified and confirmed protected (no bare `sqlite3.connect()` outside a closing()/finally guard). - pytest tests/ → 137 / 137 OK. - AST clean on all 8 modified files; contextlib imports correctly added wherever used. - Live HTTP smoke against the running app: home, /api/workspaces, /api/logs, /api/composers, /api/search all return 200 with no Python tracebacks in the log (exercises every wrapping pattern).
📝 WalkthroughWalkthroughThis PR wraps production SQLite connections with ChangesSQLite Connection Lifecycle Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
timon0305
requested review from
bradjin8 and
clean6378-max-it
and removed request for
clean6378-max-it
bradjin8
requested changes
May 8, 2026
…est in README - Route GET /api/workspaces/<id>/tabs through the existing _open_global_db context manager instead of a duplicate sqlite3.connect + outer finally, matching list_workspaces and closing issue #17 review follow-ups. - README: add Tests section with python -m unittest discover tests -v. - test_exclusion_rules: docstring prefers unittest over pytest. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/workspaces.py (1)
1031-1066: ⚡ Quick winParse
messageRequestContextonce.This block runs the same
messageRequestContext:%query twice and JSON-decodes each row twice just to populate two maps. Folding both structures into one pass would remove a full-table scan from a hot endpoint with no behavior change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/workspaces.py` around lines 1031 - 1066, The code currently queries "cursorDiskKV" for keys like "messageRequestContext:%" twice and JSON-decodes each row twice; combine these loops by iterating global_db.execute("SELECT key, value FROM cursorDiskKV WHERE key LIKE 'messageRequestContext:%'") once, JSON.parse row["value"] a single time, then populate both message_request_context_map (append the ctx + "contextId") and project_layouts_map (extract ctx.get("projectLayouts"), JSON-decode each string layout and append layout["rootPath"] when valid) inside the same try/except block; keep the existing checks (parts length, isinstance checks) and existing error handling (except Exception: pass) but remove the duplicated second loop and its JSON loads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/workspaces.py`:
- Around line 163-166: The read-only SQLite URI is being built by interpolating
local_db_path directly into the "file:" string which breaks when the path
contains reserved URI chars; update the sqlite3.connect call to construct a
proper file URI by converting local_db_path to a file URI (use
pathlib.Path(local_db_path).as_uri()) and then append "?mode=ro" before passing
it to sqlite3.connect(..., uri=True) so the with closing(... ) block and
subsequent lconn.execute(...) use a correctly escaped URI.
---
Nitpick comments:
In `@api/workspaces.py`:
- Around line 1031-1066: The code currently queries "cursorDiskKV" for keys like
"messageRequestContext:%" twice and JSON-decodes each row twice; combine these
loops by iterating global_db.execute("SELECT key, value FROM cursorDiskKV WHERE
key LIKE 'messageRequestContext:%'") once, JSON.parse row["value"] a single
time, then populate both message_request_context_map (append the ctx +
"contextId") and project_layouts_map (extract ctx.get("projectLayouts"),
JSON-decode each string layout and append layout["rootPath"] when valid) inside
the same try/except block; keep the existing checks (parts length, isinstance
checks) and existing error handling (except Exception: pass) but remove the
duplicated second loop and its JSON loads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 692dc39b-ea04-4cc5-8160-a8bb6e3fb5b0
📒 Files selected for processing (3)
README.mdapi/workspaces.pytests/test_exclusion_rules.py
✅ Files skipped from review due to trivial changes (2)
- README.md
- tests/test_exclusion_rules.py
…n local_db_path contains characters that must be escaped in URIs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/workspaces.py (1)
480-482:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape the remaining read-only SQLite URIs too.
Line 481 and Line 512 still build
file:URIs by concatenating raw filesystem paths. That leaves_build_composer_id_to_workspace_id()and_open_global_db()exposed to the same?/#/ space path bug you already fixed at Line 166, so workspaces under such paths can still fail to open.Suggested fix
def _build_composer_id_to_workspace_id(workspace_path: str, workspace_entries: list) -> dict: """Build mapping: composerId -> workspaceId from per-workspace state.vscdb.""" mapping = {} for entry in workspace_entries: db_path = os.path.join(workspace_path, entry["name"], "state.vscdb") if not os.path.isfile(db_path): continue try: # closing() guarantees .close() on scope exit (issue `#17`). - with closing(sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)) as conn: + 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() @@ 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) + global_db_uri = Path(global_db_path).resolve().as_uri() + "?mode=ro" + conn = sqlite3.connect(global_db_uri, uri=True)In Python's sqlite3 module, when uri=True is used, should filesystem paths be converted to proper file: URIs (for example via pathlib.Path(...).resolve().as_uri()) before appending ?mode=ro so reserved characters like ? and # are escaped?Also applies to: 512-512
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/workspaces.py` around lines 480 - 482, Replace the raw string concatenation that builds SQLite file: URIs (e.g., sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)) with a proper file URI derived from the filesystem path to escape reserved characters: use pathlib.Path(db_path).resolve().as_uri() to produce the base file://... URI and then append the query string (e.g., ?mode=ro) before calling sqlite3.connect(..., uri=True); apply this change in both _build_composer_id_to_workspace_id and _open_global_db so paths containing ?, #, spaces, etc. are correctly escaped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@api/workspaces.py`:
- Around line 480-482: Replace the raw string concatenation that builds SQLite
file: URIs (e.g., sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)) with a
proper file URI derived from the filesystem path to escape reserved characters:
use pathlib.Path(db_path).resolve().as_uri() to produce the base file://... URI
and then append the query string (e.g., ?mode=ro) before calling
sqlite3.connect(..., uri=True); apply this change in both
_build_composer_id_to_workspace_id and _open_global_db so paths containing ?, #,
spaces, etc. are correctly escaped.
bradjin8
approved these changes
May 8, 2026
wpak-ai
approved these changes
May 8, 2026
timon0305
added a commit
that referenced
this pull request
May 8, 2026
README.md: combine both adjacent additions to the Quick Start area — keep our FLASK_DEBUG opt-in note above the new Tests section that landed on master via PR #23.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
19 production
sqlite3.connect()call sites in this repo used a bare local variable with nowithblock, notry/finally, and no guaranteed.close()on exception. On a long-lived Flask process serving many requests, leaks accumulate before the user notices (file descriptors, SQLite read-locks held longer than needed).Change
Two patterns, each applied where it fits best:
with closing(sqlite3.connect(...)) as conn:— short-scope (15 sites)api/composers.py×3utils/cli_chat_reader.py×2utils/cursor_md_exporter.py×1api/workspaces.py×2 (small helpers)api/logs.py×2api/search.py×2 (wconn+ smallconn)api/export_api.py×1 (wconn)sqlite3.Connection's own context manager only commits / rolls back; it does not.close().contextlib.closing(...)is what guarantees release.try / except / finally: if conn is not None: conn.close()— long-scope (4 sites)api/workspaces.py:get_workspace_tabs(~400 lines usingglobal_db)api/search.py:search×2 deep blocks (~160-line and ~70-line scopes)api/export_api.py:export_chats(~340 lines)scripts/export.py:_conn(cleaned up the existing duplicate close-on-success and close-on-error pattern)Same semantics as wrapping the whole function in
with closing(...), without the 100-300-line indent shift.@contextmanageron the helper that yields a connection — 1 siteapi/workspaces.py:_open_global_dbpreviously returned(conn, path)to callers, who were responsible for.close(). Converted to a@contextmanagerso callers writewith _open_global_db(...) as (conn, _):. Both call sites updated.Test-side
sqlite3.connectcalls are out of scope (per-test fixture lifecycles, short-lived).Test plan
pytest tests/→ 137 / 137 OK (zero behaviour change in the test paths).from contextlib import closing[, contextmanager]correctly added everywhere it's used.sqlite3.connect()call site classified and confirmed protected by eitherclosing(), an outertry/finallyblock in the same function, or@contextmanager.GET /→ HTTP 200GET /api/workspaces→ HTTP 200 (exerciseswith closing()+@contextmanager+ smallwith closing()in_build_composer_id_to_workspace_id)GET /api/logs→ HTTP 200 (bothwith closing()paths)GET /api/composers→ HTTP 200 (all 3with closing()sites)GET /api/search?q=test&type=all→ HTTP 200 (bothtry/finallysites + smallwith closing())Closes #17.
Summary by CodeRabbit