feat(browser): backend skeleton + multi-user storage tenancy (PR 1/10)#300
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughA new desktop browser module is introduced with SQLCipher-based encrypted cookie storage, Argon2id key derivation, browser profile persistence, and per-user data isolation. The feature adds two dependencies ( ChangesDesktop Browser Feature
Sequence DiagramsequenceDiagram
actor User
participant Client as Browser Client
participant App as FastAPI App
participant Store as BrowserCookieStore
participant Crypto as crypto.derive_cookie_key
participant SQLCipher as SQLCipher DB
User->>Client: Store cookie (user_id, profile_id, host, cookie)
Client->>App: POST /set_cookie
App->>Crypto: derive_cookie_key(password, user_salt)
Crypto-->>App: hex_key (64 chars)
App->>Store: BrowserCookieStore(db_path, key_hex=hex_key)
App->>Store: await init()
Store->>SQLCipher: PRAGMA key = 'x[hex_key]'
Store->>SQLCipher: CREATE TABLE cookies (encrypted schema)
SQLCipher-->>Store: init complete
App->>Store: await set_cookie(user_id, profile_id, host, name, value, ...)
Store->>SQLCipher: INSERT OR REPLACE (encrypted)
SQLCipher-->>Store: row stored
Store-->>App: success
App-->>Client: cookie saved
User->>Client: Retrieve cookies for host
Client->>App: GET /get_cookies?user_id=...&profile_id=...&host=...
App->>Store: await get_cookies(user_id, profile_id, host)
Store->>SQLCipher: SELECT FROM cookies WHERE ... (encrypted query)
SQLCipher-->>Store: result rows (decrypted by pragma key)
Store-->>App: [cookie_dict, ...]
App-->>Client: cookies (json)
Client-->>User: display cookies
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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 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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 38 minutes and 23 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/routes/desktop_browser/test_router_mounted.py (1)
10-20: ⚡ Quick winThis doesn't actually verify that
include_router(...)happened.The test still passes if Line 919 in
tinyagentos/app.pyis deleted, because it only proves the module imports and the app fixture was created. Either rename this to an import smoke test, or captureFastAPI.include_routerduringcreate_app()and assert that the desktop-browser router was registered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/desktop_browser/test_router_mounted.py` around lines 10 - 20, The test test_desktop_browser_router_present_in_app currently only checks importability and not that create_app actually registered the router; modify the test to patch or spy on FastAPI.include_router when calling create_app() (or the app fixture) and assert that include_router was invoked with tinyagentos.routes.desktop_browser.router (or that the router object was passed/registered with the expected prefix), or alternatively rename the test to make it an explicit "import smoke test" if you don't want to assert registration; reference create_app, FastAPI.include_router, and tinyagentos.routes.desktop_browser.router to locate the code to change.tests/routes/desktop_browser/test_cookie_store.py (1)
43-51: 💤 Low valueConsider narrowing the expected exception type for better test specificity.
The static analysis flags
pytest.raises(Exception)(B017) because it can mask unrelated failures. While the comment explains the SQLCipher version variability, you could import the specific exception type from sqlcipher3 or at minimum assert on the exception message to ensure it's a key/decryption failure rather than an unrelated bug.♻️ Potential refinement
+ from sqlcipher3 import dbapi2 as sqlcipher + # Reopen with wrong key — read must fail (init may also fail # depending on SQLCipher version; either way, the data must # not be retrievable with the wrong key) s2 = BrowserCookieStore(tmp_path / "c.sqlite3", key_hex=WRONG_KEY) - with pytest.raises(Exception): + with pytest.raises((sqlcipher.DatabaseError, sqlcipher.OperationalError)): try: await s2.init() except Exception:Alternatively, keep
Exceptionbut add an assertion on the message to confirm it's crypto-related.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routes/desktop_browser/test_cookie_store.py` around lines 43 - 51, Narrow the test's broad pytest.raises(Exception) by expecting the specific DB/decryption exception (e.g., import and use sqlcipher3.DatabaseError or the concrete exception your sqlcipher lib raises) around the s2.init() call, or if multiple sqlcipher versions differ, keep pytest.raises(Exception) but immediately assert the exception message contains a crypto/key/decryption hint before re-raising; target s2.init() and s2.get_cookies() in your change so the failure is validated as a key/decryption error rather than an unrelated exception.tinyagentos/routes/desktop_browser/store.py (1)
108-108: Replace deprecatedget_event_loop()withget_running_loop().
asyncio.get_event_loop()is deprecated since Python 3.10 when called from a running coroutine. Since this PR targets Python 3.14 compatibility, useasyncio.get_running_loop()instead at lines 108, 160, and 203.♻️ Proposed fix
async def init(self) -> None: - import asyncio from tinyagentos.routes.desktop_browser.schema import COOKIE_SCHEMA def _setup() -> None: # ... unchanged ... - await asyncio.get_event_loop().run_in_executor(None, _setup) + import asyncio + await asyncio.get_running_loop().run_in_executor(None, _setup) self._initialised = TrueApply the same change to
set_cookie()(line 160) andget_cookies()(line 203).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/routes/desktop_browser/store.py` at line 108, Replace deprecated asyncio.get_event_loop() calls used to run blocking helpers in executors with asyncio.get_running_loop() in all three places where run_in_executor is invoked: the _setup call inside the startup sequence and the executor calls inside set_cookie and get_cookies; locate the run_in_executor(None, ...) invocations in functions/methods named similar to the startup/_setup wrapper, set_cookie, and get_cookies and change get_event_loop() to get_running_loop() so the coroutine uses the currently running loop (keeping the rest of the call signature the same).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Around line 14-15: The installer fails building the sqlcipher3 dependency
because the SQLCipher development headers aren't installed; update the install
path by modifying the ensure_linux_deps() function in scripts/install-server.sh
to install the platform-specific dev packages (add libsqlcipher-dev for
Debian/Ubuntu and sqlcipher-devel for Fedora/RHEL) before the pip install step,
or alternatively remove/relocate "sqlcipher3>=0.6.2" from pyproject.toml into an
optional extras group so the base install doesn't require SQLCipher headers;
ensure the change occurs prior to the call that runs pip install -e . so the
build succeeds on clean systems.
In `@tests/routes/desktop_browser/test_crypto.py`:
- Around line 1-49: Add a file-level Ruff suppression for the S106 rule to the
top of the test module that calls derive_cookie_key
(tests/routes/desktop_browser/test_crypto.py) so the literal test passwords
don't trigger lint failures; place a noqa comment immediately after the module
docstring (for example a top-line comment like "# noqa: S106" or the equivalent
Ruff file-level disable) to silence S106 while leaving the tests and
derive_cookie_key references unchanged.
---
Nitpick comments:
In `@tests/routes/desktop_browser/test_cookie_store.py`:
- Around line 43-51: Narrow the test's broad pytest.raises(Exception) by
expecting the specific DB/decryption exception (e.g., import and use
sqlcipher3.DatabaseError or the concrete exception your sqlcipher lib raises)
around the s2.init() call, or if multiple sqlcipher versions differ, keep
pytest.raises(Exception) but immediately assert the exception message contains a
crypto/key/decryption hint before re-raising; target s2.init() and
s2.get_cookies() in your change so the failure is validated as a key/decryption
error rather than an unrelated exception.
In `@tests/routes/desktop_browser/test_router_mounted.py`:
- Around line 10-20: The test test_desktop_browser_router_present_in_app
currently only checks importability and not that create_app actually registered
the router; modify the test to patch or spy on FastAPI.include_router when
calling create_app() (or the app fixture) and assert that include_router was
invoked with tinyagentos.routes.desktop_browser.router (or that the router
object was passed/registered with the expected prefix), or alternatively rename
the test to make it an explicit "import smoke test" if you don't want to assert
registration; reference create_app, FastAPI.include_router, and
tinyagentos.routes.desktop_browser.router to locate the code to change.
In `@tinyagentos/routes/desktop_browser/store.py`:
- Line 108: Replace deprecated asyncio.get_event_loop() calls used to run
blocking helpers in executors with asyncio.get_running_loop() in all three
places where run_in_executor is invoked: the _setup call inside the startup
sequence and the executor calls inside set_cookie and get_cookies; locate the
run_in_executor(None, ...) invocations in functions/methods named similar to the
startup/_setup wrapper, set_cookie, and get_cookies and change get_event_loop()
to get_running_loop() so the coroutine uses the currently running loop (keeping
the rest of the call signature the same).
🪄 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 Plus
Run ID: 904c888b-d000-4d13-9505-d00ee0a45062
📒 Files selected for processing (13)
docs/getting-started.mdpyproject.tomltests/routes/desktop_browser/__init__.pytests/routes/desktop_browser/test_cookie_store.pytests/routes/desktop_browser/test_crypto.pytests/routes/desktop_browser/test_router_mounted.pytests/routes/desktop_browser/test_store_schema.pytests/routes/desktop_browser/test_store_tenancy.pytinyagentos/app.pytinyagentos/routes/desktop_browser/__init__.pytinyagentos/routes/desktop_browser/crypto.pytinyagentos/routes/desktop_browser/schema.pytinyagentos/routes/desktop_browser/store.py
| """Tests for browser-cookie key derivation (Argon2id).""" | ||
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| class TestDeriveCookieKey: | ||
| def test_returns_64_char_hex_string(self): | ||
| from tinyagentos.routes.desktop_browser.crypto import derive_cookie_key | ||
|
|
||
| key = derive_cookie_key(password="hunter2", user_salt=b"u" * 16) | ||
|
|
||
| # SQLCipher needs a 256-bit key, encoded as 64 hex chars | ||
| assert isinstance(key, str) | ||
| assert len(key) == 64 | ||
| assert all(c in "0123456789abcdef" for c in key) | ||
|
|
||
| def test_deterministic_for_same_inputs(self): | ||
| from tinyagentos.routes.desktop_browser.crypto import derive_cookie_key | ||
|
|
||
| a = derive_cookie_key(password="hunter2", user_salt=b"u" * 16) | ||
| b = derive_cookie_key(password="hunter2", user_salt=b"u" * 16) | ||
| assert a == b | ||
|
|
||
| def test_different_passwords_produce_different_keys(self): | ||
| from tinyagentos.routes.desktop_browser.crypto import derive_cookie_key | ||
|
|
||
| a = derive_cookie_key(password="hunter2", user_salt=b"u" * 16) | ||
| b = derive_cookie_key(password="other", user_salt=b"u" * 16) | ||
| assert a != b | ||
|
|
||
| def test_different_salts_produce_different_keys(self): | ||
| from tinyagentos.routes.desktop_browser.crypto import derive_cookie_key | ||
|
|
||
| a = derive_cookie_key(password="hunter2", user_salt=b"a" * 16) | ||
| b = derive_cookie_key(password="hunter2", user_salt=b"b" * 16) | ||
| assert a != b | ||
|
|
||
| def test_rejects_short_salt(self): | ||
| from tinyagentos.routes.desktop_browser.crypto import derive_cookie_key | ||
|
|
||
| with pytest.raises(ValueError, match="salt"): | ||
| derive_cookie_key(password="hunter2", user_salt=b"short") | ||
|
|
||
| def test_rejects_empty_password(self): | ||
| from tinyagentos.routes.desktop_browser.crypto import derive_cookie_key | ||
|
|
||
| with pytest.raises(ValueError, match="password"): | ||
| derive_cookie_key(password="", user_salt=b"u" * 16) |
There was a problem hiding this comment.
Suppress Ruff S106 for this test module.
This file intentionally feeds literal test passwords into a password= parameter, and Ruff already flags the new hunk repeatedly as S106. If lint gates the PR, CI will fail on this file even though the cases are test-only. A file-level ignore is the lowest-noise fix here.
Suggested fix
+# ruff: noqa: S106
"""Tests for browser-cookie key derivation (Argon2id)."""
from __future__ import annotations🧰 Tools
🪛 Ruff (0.15.12)
[error] 11-11: Possible hardcoded password assigned to argument: "password"
(S106)
[error] 21-21: Possible hardcoded password assigned to argument: "password"
(S106)
[error] 22-22: Possible hardcoded password assigned to argument: "password"
(S106)
[error] 28-28: Possible hardcoded password assigned to argument: "password"
(S106)
[error] 29-29: Possible hardcoded password assigned to argument: "password"
(S106)
[error] 35-35: Possible hardcoded password assigned to argument: "password"
(S106)
[error] 36-36: Possible hardcoded password assigned to argument: "password"
(S106)
[error] 43-43: Possible hardcoded password assigned to argument: "password"
(S106)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/routes/desktop_browser/test_crypto.py` around lines 1 - 49, Add a
file-level Ruff suppression for the S106 rule to the top of the test module that
calls derive_cookie_key (tests/routes/desktop_browser/test_crypto.py) so the
literal test passwords don't trigger lint failures; place a noqa comment
immediately after the module docstring (for example a top-line comment like "#
noqa: S106" or the equivalent Ruff file-level disable) to silence S106 while
leaving the tests and derive_cookie_key references unchanged.
Combined follow-up addressing CodeRabbit + final-review notes: - install-server.sh: add libsqlcipher-dev / sqlcipher-devel / sqlcipher across apt / dnf / pacman / apk / brew so fresh Linux/macOS installs don't break on the new sqlcipher3 dep. - store.py: replace deprecated asyncio.get_event_loop() with asyncio.get_running_loop() at all 3 executor call sites. - test_cookie_store.py: narrow pytest.raises(Exception) in the wrong-key test to (DatabaseError, OperationalError, MemoryError) so generic regressions don't pass silently. MemoryError added as it is the observed failure mode on sqlcipher3 0.5.x / Python 3.14 when key is wrong (corrupted page allocation). - test_router_mounted.py: upgrade the trivial assert-not-None router test to a route-set subset check that will catch deletion of the include_router call once PR 2 adds real routes.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (14 files)
Reviewed by grok-code-fast-1:optimized:free · 71,712 tokens |
1 participant
First of ten PRs implementing BrowserApp v2 per the design doc. Foundations only — nothing user-visible. Existing
/api/desktop/proxyis untouched.Summary
tinyagentos/routes/desktop_browser/with empty router (mount point for PR 2+)BrowserStore(regular SQLite) — schema for profiles, history, bookmarks, agent capabilities, push subscriptions, persisted browser-window state. Every row keys on(user_id, …)for OS-grade multi-user isolationBrowserCookieStore(SQLCipher-encrypted) — cookie jar. Per-user 256-bit key derived via Argon2id (OWASP 2024 baseline) from the user's login password. Stored encrypted at restargon2-cffi,sqlcipher3(substituted forpysqlcipher3which doesn't compile on Python 3.14)docs/getting-started.mdfor the system-levelsqlcipherinstall (brew/apt/dnf/Windows)app.pyWhat this does not land
/proxywith SSRF guard + distinct originTest Plan
pytest tests/routes/desktop_browser/ -v— 26 tests passpytest tests/routes/ tests/test_secrets.py -q— 108 tests pass, no regressionsfrom tinyagentos.app import create_app; create_app(...)succeedsSpec
Full design:
docs/superpowers/specs/2026-05-03-browser-app-v2-design.md§4.1, §4.2, §9.Cumulative shipping arc:
Notes for reviewers
pysqlcipher3was the originally-planned dep but doesn't compile on Python 3.14 (_PyLong_AsIntremoved). Swapped tosqlcipher3(drop-in DB-API replacement, ships pre-built wheels for 3.14). Plan and spec amended.libtorrent>=2.0.9doesn't install on Python 3.14 either — unrelated to this PR, surfaces as apip install -e .partial failure but doesn't affect the new code or its tests._initialisedflag onBrowserCookieStoreis currently set but not checked — intentional; lifecycle wiring onapp.statelands in PR 2 alongside the/proxyendpoint that needs it.asyncio.get_event_loop()style,assert self._db is not Nonepattern) noted for PR 2/3 — not blocking; bundling them here would expand diff scope past the foundations cut.Summary by CodeRabbit
Documentation
New Features
Tests