test: refuse SQLite connections into a checkout's data/ directory (#355) - #972
Open
ziomik wants to merge 1 commit into
Open
test: refuse SQLite connections into a checkout's data/ directory (#355)#972ziomik wants to merge 1 commit into
ziomik wants to merge 1 commit into
Conversation
Running pytest from a live deployment let stores fall back to their relative default db_path and open the *production* databases. Add a session-scoped autouse guard in conftest that wraps sqlite3.connect and raises when the resolved path lands under <cwd>/data or <repo root>/data. (Limitation: it does not cover subprocesses.) The guard surfaced production code — not tests — ignoring the configured data directory: - api.py: VoiceStore had db_path="data/voice_calls.db" hardcoded and UserProfileStore was constructed with no argument; both now derive their directory from the configured db_path, keeping the basename so existing production files are not orphaned. - agent_registry.build_system_prompt and dream_runner (two call sites) likewise passed no db_path to UserProfileStore. That single omission was the cause of all 341 test_api.py failures under the guard: the profile store was read while building the system prompt and written by the dream runner. test_daemon's two remaining cases built a bare DaemonConfig(); they now pass working_dir=tmp_path like the rest of the class. Suite: 4574 passed, 2 skipped.
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
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.
Closes #355.
The test guard was the goal, but writing it surfaced production code that ignored the configured data directory. Three files under
src/change behaviour as a result. Please review those hunks as production changes, not as test scaffolding.The guard
Running
pytestfrom a live deployment let stores fall back to their relative defaultdb_pathand open the production databases.tests/conftest.pyadds a session-scoped autouse fixture that wrapssqlite3.connectand raises when the resolved path lands under<cwd>/dataor<repo root>/data.Both roots are checked because they can differ: running pytest from a live deployment against another checkout's tests resolves the stores' relative defaults against the deployment's
data/. The helper handlesstr/bytes/PathLike,file:...?mode=roURIs,:memory:andmode=memory.Known limitation: the guard patches
sqlite3.connectin-process, so it does not cover subprocesses.tests/test_conftest_db_guard.py(new) covers the guard itself with 8 cases.Production bugs it found
api.py—VoiceStorehaddb_path="data/voice_calls.db"hardcoded, andUserProfileStorewas constructed with no argument at all. Both now derive their directory from the configureddb_path, keeping the original basename so existing production files are not orphaned.agent_registry.py(build_system_prompt) anddream_runner.py(two call sites) — same no-argumentUserProfileStore().That single omission was the cause of all 341
test_api.pyfailures under the guard: the profile store is read while building the system prompt and written by the dream runner, so every request path touched it. In a deployment these call sites were reading and writing whateverdata/user_profiles.dbwas relative to the process CWD rather than the configured data directory.A static sweep confirms no other store in
src/is instantiated without an explicitdb_path. The ~23 relative defaults on the store classes themselves remain, but every caller now passes a path.Tests
tests/test_daemon.py's two remaining cases built a bareDaemonConfig()(soworking_dir="."); they now passworking_dir=tmp_pathlike the rest of the class.Full suite from an isolated worktree: 4574 passed, 2 skipped, 0 failed.
ruff checkclean on all touched files.Out of scope
Stores still on WAL rather than rollback journalling:
pinky_identity(×2),pinky_hub,pinky_federation,pinky_memory(×2).🤖 Opened by Engineer