refactor: slim Database to control-plane store (#599 PR2/3)#603
Conversation
The global Database is now a control-plane store only (auth/api-keys/audit/ interactive-sessions/token-usage). All v2 domain data already lives per-workspace via core/workspace.py. Verified live keep-set (everything else removed): TokenRepository (react_agent + stats_commands persist token usage via MetricsTracker), AuditRepository (audit_logger.create_audit_log), APIKeyRepository (api_key_service / auth), InteractiveSessionRepository (interactive routers), plus BaseRepository. - database.py: 863 -> ~290 lines. Dropped every delegated method on a dropped table (projects/issues/tasks/agents/blockers/memories/context/checkpoints/ git_branches/test/lint/code_reviews/quality_gates/correction/activity/prd/...) and the *_repository compat properties. Kept connection/transaction plumbing. - repositories/__init__.py: trimmed to the control-plane keep-set. - Deleted 16 v1 repositories + codeframe/enforcement/ (reachable only via the now-removed task_repository) + codeframe/git/workflow_manager.py (GitWorkflowManager was never instantiated; only re-exported) and emptied git/__init__.py. Tests: removed ~46 v1 tests of the deleted Database methods/repos/enforcement (tests/persistence/, tests/enforcement/, tests/git/, non-v2 integration DB tests, unit/test_pr_repository, and dropped-table tests under api/blockers/context/debug/ deployment). Trimmed dead fixtures from integration/api/ui conftests and reconciled root collect_ignore. No live source called any dropped-table method (only asyncio.create_task collisions). Verified: pytest --collect-only clean (3437); /health 200; cf --help; ruff + mypy clean (181 files). enforcement/Database tests deferred-none remain. persistence -> platform_store rename is PR3. Part of #599 (PR2 of 3 — does not close).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR refactors persistence into a control-plane-only Database facade, removes many v1 domain repositories, deletes enforcement and git-workflow modules, and removes or updates numerous tests and fixtures that depended on the removed code. ChangesControl-plane persistence refactoring and test cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Review — PR #603: Database slim to control-plane storeSummary: This PR does exactly what it says — reduces A few issues worth addressing before merge: 1. Thread-safety bypass in surviving repositories (medium)
Same issue in Since 2.
3.
4. Async PRAGMA setup duplicated three times (minor) The 4-line block ( 5. Dead
6.
Summary
The deletion surface is very large but I found zero live callers of any removed symbol in the surviving |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/conftest.py (1)
113-126: 💤 Low valueUnused
pyprojectdictionary is dead code.The
pyprojectdict on lines 113-123 is constructed but never used. Line 124-126 writes a hardcoded TOML string directly to the file instead.♻️ Remove the unused dictionary
- # Create pyproject.toml - pyproject = { - "project": { - "name": "test-project", - "version": "0.1.0", - "requires-python": ">=3.11", - }, - "tool": { - "pytest": {"testpaths": ["tests"]}, - "ruff": {"line-length": 100}, - }, - } (test_workspace / "pyproject.toml").write_text( "[project]\nname = 'test-project'\nversion = '0.1.0'\n" )🤖 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 `@tests/integration/conftest.py` around lines 113 - 126, Remove the unused pyproject dictionary declaration and ensure the test writes the intended pyproject content only once; delete the variable named pyproject (the dict built on lines creating "project" and "tool") and either keep the existing (test_workspace / "pyproject.toml").write_text(...) call or replace it by serializing that data structure if you prefer a programmatic write, but do not leave the unused pyproject variable in the file.
🤖 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.
Nitpick comments:
In `@tests/integration/conftest.py`:
- Around line 113-126: Remove the unused pyproject dictionary declaration and
ensure the test writes the intended pyproject content only once; delete the
variable named pyproject (the dict built on lines creating "project" and "tool")
and either keep the existing (test_workspace / "pyproject.toml").write_text(...)
call or replace it by serializing that data structure if you prefer a
programmatic write, but do not leave the unused pyproject variable in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8aa6488-6cac-4b55-b7c2-3a97cecdd700
📒 Files selected for processing (70)
codeframe/enforcement/README.mdcodeframe/enforcement/__init__.pycodeframe/enforcement/adaptive_test_runner.pycodeframe/enforcement/evidence_verifier.pycodeframe/enforcement/language_detector.pycodeframe/enforcement/quality_tracker.pycodeframe/enforcement/skip_pattern_detector.pycodeframe/git/__init__.pycodeframe/git/workflow_manager.pycodeframe/persistence/database.pycodeframe/persistence/repositories/__init__.pycodeframe/persistence/repositories/activity_repository.pycodeframe/persistence/repositories/agent_repository.pycodeframe/persistence/repositories/blocker_repository.pycodeframe/persistence/repositories/checkpoint_repository.pycodeframe/persistence/repositories/context_repository.pycodeframe/persistence/repositories/correction_repository.pycodeframe/persistence/repositories/git_repository.pycodeframe/persistence/repositories/issue_repository.pycodeframe/persistence/repositories/lint_repository.pycodeframe/persistence/repositories/memory_repository.pycodeframe/persistence/repositories/pr_repository.pycodeframe/persistence/repositories/project_repository.pycodeframe/persistence/repositories/quality_repository.pycodeframe/persistence/repositories/review_repository.pycodeframe/persistence/repositories/task_repository.pycodeframe/persistence/repositories/test_repository.pytests/api/conftest.pytests/api/test_api_issues.pytests/api/test_api_metrics.pytests/api/test_api_prd.pytests/api/test_api_session.pytests/api/test_blocker_resolution_api.pytests/api/test_endpoints_database.pytests/api/test_git_api.pytests/api/test_multi_agent_api.pytests/api/test_project_reviews.pytests/api/test_projects_api_progress.pytests/blockers/test_blockers.pytests/conftest.pytests/context/test_checkpoint_restore.pytests/context/test_tier_filtering.pytests/debug/test_fixture_debug.pytests/deployment/test_deployment_contract.pytests/enforcement/test_adaptive_test_runner.pytests/enforcement/test_evidence_verifier.pytests/enforcement/test_language_detector.pytests/enforcement/test_quality_ratchet.pytests/enforcement/test_quality_tracker_enforcement.pytests/enforcement/test_skip_detector.pytests/enforcement/test_skip_pattern_detector.pytests/git/test_git_auto_commit.pytests/git/test_git_workflow_manager.pytests/integration/conftest.pytests/integration/test_blocker_workflow.pytests/integration/test_composite_index.pytests/integration/test_database_operations.pytests/integration/test_project_creation_flow.pytests/integration/test_quickstart_validation.pytests/persistence/test_correction_database.pytests/persistence/test_database.pytests/persistence/test_database_git_branches.pytests/persistence/test_database_issues.pytests/persistence/test_database_schema.pytests/persistence/test_database_todos.pytests/persistence/test_database_typed_returns.pytests/persistence/test_project_agents.pytests/persistence/test_server_database.pytests/ui/conftest.pytests/unit/test_pr_repository.py
💤 Files with no reviewable changes (55)
- codeframe/persistence/repositories/correction_repository.py
- tests/api/test_projects_api_progress.py
- tests/enforcement/test_quality_ratchet.py
- codeframe/enforcement/init.py
- tests/enforcement/test_quality_tracker_enforcement.py
- tests/api/test_git_api.py
- tests/api/test_endpoints_database.py
- tests/enforcement/test_adaptive_test_runner.py
- tests/api/test_blocker_resolution_api.py
- codeframe/enforcement/quality_tracker.py
- codeframe/persistence/repositories/lint_repository.py
- tests/api/test_multi_agent_api.py
- codeframe/enforcement/adaptive_test_runner.py
- codeframe/persistence/repositories/test_repository.py
- codeframe/enforcement/README.md
- tests/context/test_tier_filtering.py
- codeframe/persistence/repositories/activity_repository.py
- codeframe/enforcement/skip_pattern_detector.py
- tests/api/test_api_prd.py
- tests/integration/test_database_operations.py
- tests/integration/test_composite_index.py
- tests/api/test_api_session.py
- codeframe/persistence/repositories/agent_repository.py
- codeframe/persistence/repositories/context_repository.py
- codeframe/persistence/repositories/git_repository.py
- tests/integration/test_quickstart_validation.py
- tests/deployment/test_deployment_contract.py
- codeframe/persistence/repositories/memory_repository.py
- codeframe/persistence/repositories/quality_repository.py
- codeframe/persistence/repositories/blocker_repository.py
- tests/enforcement/test_skip_detector.py
- tests/integration/test_project_creation_flow.py
- codeframe/enforcement/evidence_verifier.py
- tests/enforcement/test_language_detector.py
- codeframe/persistence/repositories/checkpoint_repository.py
- tests/blockers/test_blockers.py
- tests/enforcement/test_evidence_verifier.py
- codeframe/persistence/repositories/issue_repository.py
- tests/git/test_git_workflow_manager.py
- tests/git/test_git_auto_commit.py
- tests/context/test_checkpoint_restore.py
- codeframe/persistence/repositories/review_repository.py
- codeframe/git/workflow_manager.py
- codeframe/persistence/repositories/task_repository.py
- tests/debug/test_fixture_debug.py
- tests/integration/test_blocker_workflow.py
- codeframe/persistence/repositories/project_repository.py
- codeframe/enforcement/language_detector.py
- tests/api/test_project_reviews.py
- tests/api/test_api_issues.py
- tests/api/conftest.py
- codeframe/persistence/repositories/pr_repository.py
- tests/enforcement/test_skip_pattern_detector.py
- tests/api/test_api_metrics.py
- tests/conftest.py
Three in-scope cleanups from the advisory review; the other three findings were pre-existing code untouched by this PR and are out of scope (thread- safety / INSERT duplication / async PRAGMA repetition in surviving repos). - persistence/repositories/base.py: remove dead _get_last_insert_id and _get_last_insert_id_async. Zero callers in the surviving codebase after the v1 repos were removed. Both also had a latent bug (returning lastrowid from a fresh cursor is always None). - persistence/database.py: remove dead _parse_datetime on Database (delegations use BaseRepository._parse_datetime; nothing calls db._parse_datetime). Drop the now-unused datetime import. - persistence/database.py: add a clarifying comment near the token_usage delegations explaining the dual-use (control-plane vs per-workspace). Verified: ruff + mypy clean (181 files), pytest --collect-only clean (3437), /health 200, focused v2 sample of auth + token + interactive tests passes.
Final Triage Summary (PR #603)Cutoffs: original push 2026-05-29T17:48:48Z; review-fix push 2026-05-29T19:24:12Z (commit ef64783). Review passes
Fixed (claude-bot findings in scope for the Database slim)
Skipped (out of scope — pre-existing code I did not touch)
OutcomeNo unresolved Critical/Major findings. After fixes: ruff + mypy clean (181 files), |
Follow-up Review — PR #603 (ef64783)Building on my earlier review, this is a status check against the review-fix commit ef64783. Prior findings — disposition confirmed correct
The three deferred items are correctly scoped out — they exist in code this PR did not modify and belong in a separate hardening pass. New observation: CodeRabbit nitpick (low, non-blocking)CodeRabbit flagged an unused pyproject dictionary in tests/integration/conftest.py lines 113–123 — the dict is constructed but the file write below it uses a hardcoded TOML string instead. This was not addressed in ef64783. It is dead code in a test fixture and will not affect runtime or test outcomes, but it is minor clutter. Worth a one-line cleanup (remove the dict declaration) before or during PR3 if that conftest is touched again — not blocking here. OverallThe three in-scope fixes landed cleanly. The refactoring is thorough — 16 v1 repositories deleted, enforcement/ removed, database.py cut by ~600 lines — with no live callers of any removed symbol in the surviving codebase. Acceptance criteria are satisfied. No unresolved findings blocking merge. The three pre-existing issues (thread-safety, INSERT duplication, PRAGMA repetition) should be tracked in a follow-up issue before the platform store hardening work. |
## Summary Final PR (3 of 3) for #599. Renames the slim control-plane store to its real role, refreshes CLAUDE.md, and removes the last v1 stragglers. - git mv codeframe/persistence -> codeframe/platform_store; updated all 31 imports across codeframe/, tests/, scripts/, plus CI yaml + active docs (CONTRIBUTING.md, PRD.md, TESTING.md) and the embedded Python in tests/e2e/global-setup.ts (caught by codex). - CLAUDE.md: dropped stale codeframe/server/ references (Legacy rule + repo- structure tree line); refreshed tests/agents/ note now that only dependency_resolver remains. - Stragglers: removed orphan sample_project_config fixture, deleted the 5 remaining #597-era v1 tests, un-ignored and fixed tests/api/test_health_endpoint.py (live /health coverage restored), emptied root collect_ignore. - Pre-existing PR2 leftovers caught by CodeRabbit: deleted tests/blockers/test_blocker_expiration_simple.py and tests/lib/test_metrics_tracker.py (both referenced removed Database methods / dropped tables; both deselected by -m v2, so prior CI green was misleading). ## Validation - Tests: pytest -m v2 green (2824 passed); Backend + Frontend CI green - Lint/Type: ruff + mypy clean (181 files); CI Code Quality green - Collect: pytest --collect-only clean (3424); zero active references to codeframe.persistence remain - Boot: TestClient /health -> 200 - Cross-family review: codex (1 P2 fixed) + CodeRabbit (2 inline Major fixed) - claude-bot advisory: 4 minor (3 fixed, 1 partly rebutted -- the flagged "dead" Database guard in tests/ui/conftest.py is actually used by the running_server fixture; simplified to a direct import) - Demo: all 12 PR3 + 9 cumulative #599 acceptance criteria verified ## Cumulative -- #599 fully complete across PRs #602 + #603 + #604 - [x] No legacy multi-agent classes remain (PR1) - [x] core/project.py + dead dirs removed (PR1) - [x] Database exposes only control-plane methods (PR2) - [x] persistence -> platform_store rename + CLAUDE.md (PR3) - [x] codeframe serve boots (every PR) - [x] pytest --collect-only clean; pytest -m v2 green (every PR) - [x] cf --help + golden path; ruff + mypy clean (every PR) - [x] Plan engine kept (verified live via engine_registry -> BuiltinPlanAdapter) Closes #599.
1 participant
Summary
PR 2 of 3 for #599. The global
Databasebecomes a control-plane store only (auth / API keys / audit logs / interactive sessions / token usage). All v2 domain data already lives per-workspace viacore/workspace.py. Net: 70 files, +33 / −24,361.Verified live keep-set (everything else removed — confirmed against live callers, not assumed):
TokenRepository—react_agent+cf statspersist token usage viaMetricsTracker.record_token_usage_sync → db.save_token_usageAuditRepository—lib/audit_logger.create_audit_logAPIKeyRepository—core/api_key_service+auth/*InteractiveSessionRepository— interactive-session routers / WSBaseRepositoryChanges
persistence/database.py: 863 → ~290 lines. Dropped every delegated method backed by a dropped table (projects/issues/tasks/agents/blockers/memories/context/checkpoints/git_branches/test/lint/code_reviews/quality_gates/correction/activity/prd/delete_project_tasks_and_issues/...) and the*_repositorycompat properties. Kept all connection/transaction plumbing and the token + audit delegations.persistence/repositories/__init__.py: trimmed to the keep-set.codeframe/enforcement/(reachable only via the now-removedtask_repository) +codeframe/git/workflow_manager.py(GitWorkflowManagerwas never instantiated — only re-exported) and emptiedgit/__init__.py.tests/persistence/,tests/enforcement/,tests/git/, non-v2 integration DB tests,unit/test_pr_repository, dropped-table tests under api/blockers/context/debug/deployment), trimmed dead fixtures from the integration/api/ui conftests, and reconciled the rootcollect_ignore.Acceptance Criteria (PR2 subset of #599)
Databaseexposes only control-plane methods; no method references a dropped tablepersistence/repositories/keeps only the control-plane repos; the 16 v1 repos removedcodeframe/enforcement/removed (unblocked by the Database slim)codeframe serveboots (/health→ 200);cf --helpworkspytest --collect-onlyclean (3437);uv run pytest -m v2green (2824 passed)Test Plan
asyncio.create_taskcollisions); 0 v2 tests reference a removed symbolpytest --collect-onlyclean;pytest -m v2green (2824 passed, 0 failed)/health→ 200;cf --help/cf status.pyreferences a removed Database method/repo/enforcement/workflow_managerKnown Limitations / Intentionally Deferred
persistence/→platform_store/rename + CLAUDE.md repo-structure update → PR3 (the issue's Part 3).collect_ignoreentries for surviving refactor: remove orphaned v1 web layer (routers + services) #597-era v1 web-layer tests (api/test_project_creation_api.py,auth/test_dual_auth.py, etc.) are left untouched — not part of the Database slim.TokenRepositoryis retained because it's used both here and per-workspace (costs_v2wires it to a raw workspace connection); the globaltoken_usagetable is created by the workspace schema, not the slim control-plane schema.Part of #599 (PR2 of 3 — does not close).
Summary by CodeRabbit