fix(core): gate workspace schema migrations behind PRAGMA user_version (#733)

[frankbria]

Summary

Fixes #733 — get_workspace() ran _ensure_schema_upgrades() (~30 DDL statements + multiple write commits, each an fsync in WAL) on every call, i.e. on every authenticated v2 request via get_v2_workspace.

• New SCHEMA_VERSION = 1 module constant, stamped into PRAGMA user_version by _init_database and after a completed migration run.
• _ensure_schema_upgrades() now returns after a single PRAGMA user_version read when the DB is current — zero DDL, zero write commits.
• Legacy DBs (user_version = 0) run the unchanged, idempotent migration path exactly once, then get stamped.
• Bump rule documented at the constant: increment whenever _init_database/_ensure_schema_upgrades gain a migration.

Acceptance criteria (from issue)

• ✅ Migrations gated behind a PRAGMA user_version check
• ✅ Steady-state requests issue zero DDL and zero write commits — proven in-test by observing PRAGMA data_version (unchanged across repeated get_workspace() calls) from an independent connection

Tests

• TestSchemaVersionGate: fresh-workspace stamp, steady-state zero-writes (via data_version), legacy-DB migration + re-stamp.
• Fixed test_task_without_requirement_ids_in_existing_db: its legacy-schema simulation now also resets user_version = 0, as a genuine pre-migration DB would have.
• Full CI subset: 4038 passed / 1 failed → fixed → related suites green. ruff clean. Cross-family codex review: no findings.

Known limitations

• Migrations are per-DB idempotent and safe under concurrent first-load (both processes run the same idempotent DDL, then stamp).
• Forgetting to bump SCHEMA_VERSION when adding a migration would skip it on existing DBs; the bump rule is documented at the constant and at both stamp sites.

Summary by CodeRabbit

• Bug Fixes

  • Workspace upgrades now run only when needed, reducing unnecessary database work on repeated loads.
  • Older workspaces are now reliably brought up to date, preserving expected task data and indexes.
  • Fresh workspaces are marked as up to date immediately after initialization.

• Tests

  • Added coverage for schema version tracking, steady-state loading, and legacy workspace upgrades.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 52 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 888c11f6-9cdc-48ce-8653-6bbc0a0fdddf

📥 Commits

Reviewing files that changed from the base of the PR and between 9ef8a4c and 3a5dd3d.

📒 Files selected for processing (1)

• codeframe/core/workspace.py

Walkthrough

Adds a SCHEMA_VERSION constant and PRAGMA user_version stamping to codeframe/core/workspace.py to gate _ensure_schema_upgrades, skipping DDL when the database is already current. Tests are added/updated to verify version stamping, no writes on steady-state loads, and legacy migration behavior.

Changes

Schema version gating

Layer / File(s)	Summary
SCHEMA_VERSION constant and stamping logic codeframe/core/workspace.py	Adds SCHEMA_VERSION = 1, stamps PRAGMA user_version after fresh initialization and after completing upgrades, and adds an early-return gate in _ensure_schema_upgrades when the stored version is already current.
Tests validating version gating and migration tests/core/test_workspace.py, tests/core/test_task_requirement_ids.py	Adds TestSchemaVersionGate verifying fresh stamping, no writes on warm loads, and legacy DB migration; updates the requirement_ids test to reset user_version to 0 to force the upgrade path.

Estimated code review effort: 2 (Simple) | ~12 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant get_workspace
  participant EnsureSchemaUpgrades
  participant SQLiteDB

  Caller->>get_workspace: request workspace
  get_workspace->>EnsureSchemaUpgrades: _ensure_schema_upgrades()
  EnsureSchemaUpgrades->>SQLiteDB: read PRAGMA user_version
  alt version >= SCHEMA_VERSION
    EnsureSchemaUpgrades->>SQLiteDB: close connection
    EnsureSchemaUpgrades-->>get_workspace: return early, no DDL
  else version < SCHEMA_VERSION
    EnsureSchemaUpgrades->>SQLiteDB: run DDL upgrades
    EnsureSchemaUpgrades->>SQLiteDB: PRAGMA user_version = SCHEMA_VERSION
    EnsureSchemaUpgrades-->>get_workspace: return
  end

Loading

Possibly related PRs

• frankbria/codeframe#686: Both PRs modify the same workspace initialization and schema-upgrade path in codeframe/core/workspace.py.

Poem

A version stamp, so neat and small,
Saves my burrow from DDL sprawl. 🐇
No more digging schemas each time I hop—
One check, one gate, and steady loads stop.
Hooray for user_version, my new best friend!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly states the main change: gating workspace schema migrations behind PRAGMA user_version for issue #733.
Linked Issues check	✅ Passed	The changes gate migrations on PRAGMA user_version, skip steady-state DDL/write commits, and preserve one-time migration for legacy databases.
Out of Scope Changes check	✅ Passed	The added tests and schema versioning support directly serve the migration-gating goal and do not introduce unrelated behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/733-gate-schema-migrations

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Code Review — fix(core): gate workspace schema migrations behind PRAGMA user_version (#733)

Overall: This is a clean, well-scoped performance fix. The approach is correct, the PR description is thorough, and the PRAGMA data_version trick in the steady-state test is a clever and reliable signal. A few things worth noting before merge:

---

What's working well

• The >= SCHEMA_VERSION guard (rather than ==) correctly future-proofs the gate: a DB that was stamped with a newer version won't be downgraded back through the migration path.
• The data_version assertion in test_steady_state_load_issues_zero_writes is a direct proof of the invariant — much stronger than mocking or checking call counts.
• test_task_without_requirement_ids_in_existing_db now correctly simulates a pre-migration DB by resetting user_version = 0. Good catch; without that the test was giving a false green.
• Stamping happens in both _init_database and _ensure_schema_upgrades, so both code paths produce a consistent state.

---

Issues / suggestions

1. Connection leak in the early-return path (_ensure_schema_upgrades)

conn = _open_db(db_path)
cursor = conn.cursor()

if cursor.execute("PRAGMA user_version").fetchone()[0] >= SCHEMA_VERSION:
    conn.close()   # ← fine when it runs, but…
    return

If _open_db succeeds but the PRAGMA read raises (edge case: corrupt page, threading error), conn.close() is never reached. The rest of the function also closes manually without a try/finally. The existing codebase pattern is the same, so this isn't a regression introduced here — but the early-return path is new and it's a good opportunity to tighten it:

conn = _open_db(db_path)
try:
    cursor = conn.cursor()
    if cursor.execute("PRAGMA user_version").fetchone()[0] >= SCHEMA_VERSION:
        return
    # ... all migration DDL ...
    cursor.execute(f"PRAGMA user_version = {SCHEMA_VERSION}")
    conn.commit()
finally:
    conn.close()

This eliminates the duplicate conn.close() call at the bottom and is exception-safe.

2. f-string for PRAGMA value

cursor.execute(f"PRAGMA user_version = {SCHEMA_VERSION}")

SQLite's PRAGMA doesn't support ? placeholders, so the f-string is unavoidable here — this is the standard pattern. Worth adding a one-line comment so future readers don't flag it as an injection risk:

# PRAGMA doesn't accept ? placeholders; SCHEMA_VERSION is a module-level int constant.
cursor.execute(f"PRAGMA user_version = {SCHEMA_VERSION}")

3. test_legacy_db_is_migrated_and_stamped only validates one index

The test drops idx_tasks_external_url and checks it's re-created. This adequately tests the gate mechanism (version check → migrations run → stamp). It doesn't prove all migrations ran, but that's covered by the broader existing test suite via the pre-existing idempotency tests. The scope is fine for this PR.

4. Concurrent first-load on an unversioned DB

Acknowledged in the PR description. Two processes can race through the migration path simultaneously; since every DDL statement is CREATE … IF NOT EXISTS, both will succeed and then both will stamp the same version. Safe, but the stamp is not atomic with the migration run — a crash between the last DDL and the stamp leaves version at 0, causing one redundant re-run on next open. This is acceptable for a first pass and matches the "documented known limitation" framing.

---

Minor nit

SCHEMA_VERSION = 1 lives at module level, between Workspace and _get_state_dir. Consider grouping it near the other top-level constants (STATE_DB_NAME, CODEFRAME_DIR) so it's easier to find when someone needs to bump it.

---

Summary

The fix is correct and the test suite directly proves the intended invariant. The connection-leak risk in the early-return path is the only substantive concern — worth a try/finally wrap. Everything else is minor polish.

[coderabbitai]

🧹 Nitpick comments (1)

codeframe/core/workspace.py (1)

57-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a regression test asserting schema/version parity.

Nothing currently enforces that a future column/table addition to _init_database/_ensure_schema_upgrades is paired with a SCHEMA_VERSION bump — relies entirely on the comment convention. A test that hashes/inspects the full schema and compares it against a recorded SCHEMA_VERSION-keyed fixture would catch drift early, but this is speculative hardening beyond the current fix.

🤖 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 `@codeframe/core/workspace.py` around lines 57 - 61, Add a regression test for
schema/version parity around SCHEMA_VERSION in workspace.py, since the current
comment-only convention can drift from _init_database and
_ensure_schema_upgrades. Update the test to inspect or snapshot the full schema
created by those functions and assert it matches the recorded SCHEMA_VERSION
fixture, so any future table/column/index change in Workspace initialization or
migration paths requires a corresponding version bump.

🤖 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 `@codeframe/core/workspace.py`:
- Around line 57-61: Add a regression test for schema/version parity around
SCHEMA_VERSION in workspace.py, since the current comment-only convention can
drift from _init_database and _ensure_schema_upgrades. Update the test to
inspect or snapshot the full schema created by those functions and assert it
matches the recorded SCHEMA_VERSION fixture, so any future table/column/index
change in Workspace initialization or migration paths requires a corresponding
version bump.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d9491a2c-df67-4fd4-bab0-2d2d4d3ddaf4

📥 Commits

Reviewing files that changed from the base of the PR and between cfefbcf and 9ef8a4c.

📒 Files selected for processing (3)

• codeframe/core/workspace.py
• tests/core/test_task_requirement_ids.py
• tests/core/test_workspace.py

[frankbria]

Review feedback triage (commit 3a5dd3d):

• ✅ claude-review Add Claude Code GitHub Workflow #1 (connection leak on new early-return path): applied — the user_version probe now closes the connection if the read itself raises.
• ✅ claude-review Brainstorming: Integrate remaining general concepts into specification #2 (f-string PRAGMA): one-line comment added at both stamp sites (PRAGMA doesn't accept ? placeholders; value is a module int constant).
• ✅ claude-review nit (constant placement): SCHEMA_VERSION moved next to CODEFRAME_DIR/STATE_DB_NAME.
• ⏭️ claude-review feat(sprint-4): Multi-Agent Coordination System - P0 & P1 Complete #3/feat: Project schema refactoring with source types and workspace management #4 (test scope, concurrent first-load): no change — reviewer assessed both as acceptable; concurrency behavior is documented in the PR body (idempotent DDL, worst case one redundant re-run after a crash between DDL and stamp).
• ⏭️ CodeRabbit nitpick (schema/version-parity regression test): skipped — CodeRabbit itself flags it as speculative hardening beyond this fix; the bump rule is documented at the constant. Can be added later if drift actually bites.

[claude]

Follow-up Review — commit 3a5dd3d

All three substantive items from my first review are addressed. Confirming:

✅ Connection leak (#1): The early-return path now wraps the PRAGMA user_version read in try/except and explicitly calls conn.close() before raise, and again on the early-return branch. The chosen pattern (explicit close at each exit point) is functionally equivalent to the try/finally I suggested and handles both failure modes correctly.

✅ f-string PRAGMA comments (#2): Both stamp sites in _init_database and _ensure_schema_upgrades carry the one-liner explaining why ? placeholders can't be used here.

✅ Constant placement (nit): SCHEMA_VERSION = 1 now sits immediately after STATE_DB_NAME/CODEFRAME_DIR with a clear bump-rule docstring.

The two non-blocking points (#3 test scope, #4 concurrent first-load) were correctly left as-is per the original review.

This PR is ready to merge. The fix is correct, minimal, and directly proven by the data_version invariant in test_steady_state_load_issues_zero_writes.