refactor(workspace): one definition of the schema, shared by both paths (#1060)

[frankbria]

Closes #1060.

The drift was already real, and measurable

_init_database (fresh) and _ensure_schema_upgrades (existing) carried 28 copy-pasted CREATE TABLE IF NOT EXISTS statements. Measured before touching anything:

only on the create path : blockers, checkpoints, events, prds, tasks, workspace
                          + idx_blockers_status, idx_blockers_workspace,
                            idx_events_workspace, idx_tasks_status, idx_tasks_workspace
only on the upgrade path: keeps

So a workspace reaching the upgrade path without one of those six never got it back — and that only ever surfaces as a runtime error on whichever path is less exercised, which is the one nobody tests.

Fix

_create_core_tables / _create_core_indexes are now the single definition, called by both paths — the pattern _create_token_usage_schema already demonstrated, applied to the rest. Column migrations (ALTER TABLE) and data backfill stay in _ensure_schema_upgrades, since DDL alone can't express them.

	before	after
CREATE TABLE statements	28	18 (the canonical set)
workspace.py	1206 lines	1043

With every table guaranteed to exist, the upgrade path's four if not cursor.fetchone(): <CREATE> else: <ALTER> blocks lose their create branch, and six unconditional re-CREATEs become dead.

Ordering is load-bearing — two [P1]s the review caught

My first version called the whole shared schema early on the upgrade path. That put index creation ahead of the migrations those indexes depend on, and both cases brick a real legacy workspace rather than merely diverging:

• idx_prds_chain / idx_prds_depends_on index columns an older workspace doesn't have yet. Created before the guarded ALTER TABLEs, get_workspace() raises no such column: chain_id and the workspace won't open at all.
• idx_tasks_external_url is UNIQUE. A workspace that imported the same GitHub issue twice needs _dedupe_external_urls first, or the index raises IntegrityError with no way back in — regressing the recovery path [P1.25] Harden workspace DB integrity: foreign keys, safe UNIQUE index upgrade, single DDL, no control-plane contamination #943 added on purpose.

So tables and indexes are separate now. _init_database runs both back to back (a new database has nothing to migrate); _ensure_schema_upgrades creates tables early — pure IF NOT EXISTS, always safe — and calls the indexes last, after every column migration and data fixup. Sharing is preserved; only sequencing differs, and the docstrings say why.

My convergence test could not have caught either, which is the more useful finding: dropping a whole table is too clean a simulation of "legacy", because the table comes back complete. Real legacy databases have tables that exist with missing columns, or that hold data a newer index rejects.

Verification

• test_each_table_dropped_alone_is_restored is the AC's "valuable part": it drops each table in turn, upgrades, and diffs sqlite_master against a fresh workspace — so a table added to only one path fails here rather than in production. Plus test_dropping_everything_rebuilds_the_whole_schema and a source-level guard that no CREATE TABLE appears twice.
• Two tests for the shapes the drop test can't reach: a prds table existing without chain_id, and a tasks table holding duplicate external_urls. Both RED before the ordering fix.
• Behaviour unchanged (AC3): new workspaces initialise, existing ones reopen with their data, a legacy workspace upgrades and stays readable.
• Mutation-checked: removing the shared call from the upgrade path fails 3.
• Full non-e2e gate green; ruff check clean.

Known limitations

• keeps (previously upgrade-path-only) is intentionally not added to the canonical schema — it isn't in the fresh schema and nothing in codeframe/ reads it, so promoting it would have been scope creep dressed as convergence. The convergence test compares against fresh-as-reference, so it does not require it.
• The upgrade path still contains ALTER TABLE and backfill logic that is inherently order-dependent. This PR removes the duplication, not the sequencing — _create_core_indexes must stay last, which the docstring states and the two legacy-shape tests enforce.

[github-actions]

Claude finished @frankbria's task in 7m 1s —— View job

---

Precision bug-hunting review — complete

Reviewed PR #1103 for concrete defects only (logic errors, data loss, security, race conditions, breaking contracts). Style/architecture/coverage are covered by a separate review.

• Gather PR context (gh pr view, gh pr diff, existing comments)
• Read the changed files and surrounding callers/callees
• Confirm each candidate defect against real code
• Post inline finding + summary

Result: 1 finding (posted inline on codeframe/core/workspace.py:796 + summary table below).

The two ordering P1s the PR body calls out (idx_prds_chain/idx_prds_depends_on after the chain_id/depends_on ALTERs; UNIQUE idx_tasks_external_url after _dedupe_external_urls) are genuinely fixed — I traced the column migrations to lines 670–704 and the dedupe to 764, both before the index call at 796. The remaining issue is a separate one: the UNIQUE index is now created twice on the upgrade path — guarded at 767, then unguarded again at 796 via _create_core_indexes, which reintroduces the very IntegrityError brick the guard exists to survive (#943).

See the inline comment for the full failure trace and the summary comment for the table.

[frankbria]

Third-party review — codex review --base main

Two [P1] findings, both real, both bugs I introduced, and both worse than the drift I was fixing — a diverged schema fails at some later runtime call; these two stop the workspace opening at all.

[P1] Preserve PRD column migrations before indexing

On workspaces created before prds.chain_id/prds.depends_on existed, calling _create_core_schema() now reaches these index statements before the later upgrade code has added the missing columns, so get_workspace() fails with no such column: chain_id instead of migrating.

[P1] Dedupe imported issue URLs before creating the unique index

For existing workspaces that already contain duplicate non-null external_url rows … _create_core_schema() now creates the unique index before _dedupe_external_urls() runs, so upgrade raises IntegrityError and bricks the workspace. This regresses the recovery path below.

Both confirmed by writing them as tests first — RED before the fix, green after.

The root mistake is worth naming precisely: I treated "share the DDL" as a pure de-duplication, when the two paths were never running the same DDL in the same context. The fresh path has no legacy columns and no dirty rows, so order is irrelevant there; the upgrade path is defined by having both. Merging them without splitting tables from indexes silently imposed the fresh path's ordering assumptions on the upgrade path.

Fixed by splitting _create_core_tables from _create_core_indexes. Tables are created early on the upgrade path (pure IF NOT EXISTS, always safe); indexes run last, after every ALTER TABLE and after _dedupe_external_urls. Sharing is preserved — only sequencing differs — and both docstrings state the constraint so the next person doesn't re-merge them.

The part I'd flag for anyone reviewing this pattern

My convergence test could not have caught either finding. It drops whole tables, and a dropped table comes back complete — so it never produces a table that exists with missing columns, or one holding data a newer UNIQUE index rejects. Those are precisely what "legacy" means in the wild.

That's a gap in the test I wrote to make drift impossible, so it got two tests of its own: test_a_prds_table_without_chain_id_still_upgrades and test_duplicate_external_urls_are_deduped_before_the_unique_index. A whole-table-drop is a convergence test; those two are ordering tests, and the refactor turned ordering into something worth testing.

No Critical findings; both P1s accepted and fixed, neither dismissed.

[claude]

Review

The refactor's core idea (one _create_core_tables/_create_core_indexes definition shared by both paths, tables-then-migrations-then-indexes on the upgrade path) is sound, and the two P1s already caught and fixed in the codex review comment (index-before-migration for prds.chain_id, index-before-dedupe for idx_tasks_external_url) were real and the fixes look correct in isolation. The new test_schema_convergence_1060.py suite is a good addition — the whole-table-drop convergence test plus the two legacy-shape ordering tests are exactly the right level of test for this kind of drift bug.

However, I think the second P1 fix is now incomplete and reintroduces a narrower version of the same regression it was meant to close.

The unguarded second idx_tasks_external_url attempt can still brick a workspace (codeframe/core/workspace.py:766-796)

_ensure_schema_upgrades still contains the original, carefully guarded block: it calls _dedupe_external_urls(conn, cursor), then attempts CREATE UNIQUE INDEX IF NOT EXISTS idx_tasks_external_url ON tasks(workspace_id, external_url) inside a try/except sqlite3.IntegrityError, logging a warning and continuing ('workspace still opens normally') if it fails.

...and then, about 25 lines later, _create_core_indexes(cursor) runs at the very end (line 796) and issues the exact same CREATE UNIQUE INDEX IF NOT EXISTS idx_tasks_external_url ON tasks(workspace_id, external_url) statement again — this time with no try/except around it.

If the index already got created by the guarded attempt, the second call is a harmless no-op (IF NOT EXISTS). But if it did not — e.g. _dedupe_external_urls's own SELECT scan raises sqlite3.Error and it silently returns without deduping (workspace.py:563-565), or dedup otherwise leaves a duplicate pair behind — the guarded attempt logs a warning and continues, and then the second, unguarded attempt in _create_core_indexes hits the same IntegrityError on the same leftover duplicate data, except this time nothing catches it. It propagates straight out of _ensure_schema_upgrades into get_workspace() (no try/except at that call site either), which is precisely the 'bricked, no recovery path' outcome #943 was written to prevent, and that this PR's own description says it preserved.

This is a corner case (only reached when dedup itself fails to fully clean up), which is presumably why the new test_duplicate_external_urls_are_deduped_before_the_unique_index test doesn't catch it — it only exercises the happy path where dedup succeeds and the index is created once. It doesn't exercise 'dedup best-effort fails, then the fallback catch-all index pass runs into the same data.'

Suggested fix: since _create_core_indexes is now the single authoritative place indexes get created, the dedupe-then-guarded-create block at 764-778 is redundant with it and could be deleted entirely (call _dedupe_external_urls alone, then let the final _create_core_indexes(cursor) create the index) — but then _create_core_indexes itself needs the try/except around just that one statement, since it's the only index in that function with a uniqueness constraint that live data can violate. Right now neither location reliably guarantees the 'never brick, just lose the optimization' property the code comments claim.

Minor: other inline index statements are now dead duplicates (not bugs, just leftover cruft)

Two more spots in _ensure_schema_upgrades still create indexes inline that _create_core_indexes(cursor) (called at line 796) will recreate a second time: idx_prds_parent/idx_prds_chain/idx_prds_depends_on (workspace.py:707-710) right after the prds column migrations, and idx_run_engine_log_ws_engine/idx_engine_stats_ws (workspace.py:780-790) — visible in the diff as CREATE INDEX calls left behind when the CREATE TABLE run_engine_log / engine_stats blocks above them were deleted.

These are non-unique indexes, so the duplication is harmless at runtime (IF NOT EXISTS), just redundant. But it works against the PR's own stated goal of 'one definition of the schema, shared by both paths,' and it's not caught by TestNoDdlIsDuplicated.test_no_create_table_statement_appears_twice since that regex only checks CREATE TABLE, not CREATE INDEX. Worth a follow-up cleanup pass (or widening that guard test to also catch duplicated CREATE INDEX names).

Everything else

_create_core_tables/_create_core_indexes/_create_core_schema split and docstrings are clear and explain why the ordering matters, which will help the next person avoid re-merging them. The batch_runs migration simplification (dropping the if-not-exists-CREATE-else-ALTER branch now that _create_core_tables guarantees the table) is correct — a freshly-created batch_runs already has every column, so the guarded ALTER loop becomes a no-op for it. No other CI/test-coverage concerns beyond the gap noted above; ruff/test claims in the description are consistent with what's in the diff.

[github-actions]

GLM precision review — 1 finding

Scoped to concrete defects only (logic errors, data loss, security, race conditions, breaking contracts). Style, naming, architecture, and test coverage are covered by a separate review.

severity	file:line	finding
major	codeframe/core/workspace.py:796	_create_core_indexes re-creates the UNIQUE idx_tasks_external_url unguarded, defeating the IntegrityError recovery net (#943) that the guarded creation at 766–778 exists to provide. If _dedupe_external_urls returns early (its scan SELECT raises sqlite3.Error, caught at line 563) leaving duplicate (workspace_id, external_url) rows, line 767's failure is caught but the unguarded re-creation here raises and propagates out of _ensure_schema_upgrades → get_workspace(), bricking the workspace. Fresh path is immune (empty table); upgrade-path-only.

No Critical findings. The two ordering P1s the PR calls out (idx_prds_chain/idx_prds_depends_on after their column ALTERs; the UNIQUE index after _dedupe_external_urls) are correctly fixed — verified by tracing the column migrations to lines 670–704 and dedupe to 764, both before the index call at 796.

Did not approve or request changes.

[github-actions]

Claude finished @frankbria's task in 4m 18s —— View job

---

Precision bug-hunting review — re-checked on sync

The author pushed a fix moving the IntegrityError guard inside _create_core_indexes. I re-traced the synced code for the prior finding and hunted for new concrete defects.

• Gather PR context (gh pr view, gh pr diff, existing comments)
• Read workspace.py changed regions + callers/callees
• Read tests/core/test_schema_convergence_1060.py
• Confirm each candidate defect against real code
• Post summary (no new findings)

Scoped to concrete defects only (logic errors, data loss, security, race conditions, breaking contracts).

Result: ✅ no new defects — the prior finding is resolved

The defect all three reviewers raised on the previous commit (the unguarded second CREATE UNIQUE INDEX IF NOT EXISTS idx_tasks_external_url at the old line 796) is fixed correctly:

• The guard now lives inside _create_core_indexes alongside the statement it protects (workspace.py:493–504), so every caller inherits it instead of one call site carrying protection that sharing the DDL silently dropped. The source now contains exactly one such statement — pinned by test_the_index_is_created_exactly_once_in_the_source, which is the right test: two attempts is precisely how one ends up unguarded.
• Ordering is intact: _dedupe_external_urls(conn, cursor) runs at workspace.py:782 (inside the tasks-exists block, which _create_core_tables at 623 guarantees is entered), and _create_core_indexes(cursor) runs last at :801, after the chain_id/depends_on ALTERs (:684–718) and the dedupe. So the UNIQUE index is only attempted once the columns exist and the data has been cleaned.
• test_undedupable_duplicates_warn_instead_of_bricking monkeypatches _dedupe_external_urls to a no-op so duplicates genuinely survive, then asserts _ensure_schema_upgrades completes and stamps user_version — exactly the path that used to raise out to get_workspace() (:989, unguarded). This is the regression test the prior version lacked.

Candidate defects I checked and ruled out

• Subsequent CREATE INDEX statements after the caught IntegrityError. _create_core_indexes continues with ~20 more index statements after the try/except. SQLite rolls back only the failed statement; the transaction stays usable, so the remaining non-unique indexes are created normally. No rollback needed inside the function. Safe.
• Narrow except sqlite3.IntegrityError vs. a missing column. A missing external_url would raise OperationalError, not IntegrityError, and escape the guard. But _create_core_tables always adds external_url (:237–238, guarded ALTER) before any caller reaches _create_core_indexes, so that state is unreachable on both paths. The catch only needs to handle duplicate data, which it does.
• Fresh-path correctness without dedupe. _create_core_schema/_init_database is fresh-only (writes a temp DB at :895–897, never an existing one), so the absent dedupe call there is correct — an empty table can't violate uniqueness, and the except arm is unreachable there (as the docstring states).
• batch_runs simplification. Dropping the if-not-exists-CREATE else ALTER branch is correct: _create_core_tables guarantees the table for new DBs, and the now-unconditional ALTER loop (:635–653) backfills columns on legacy DBs where CREATE TABLE IF NOT EXISTS is a no-op. The two batch_runs indexes, previously created only on the create branch, now come from _create_core_indexes — a net improvement.
• Table set coverage / keeps. Every table from both original paths is present in _create_core_tables (18 CREATE TABLE incl. token_usage). keeps appears nowhere in codeframe/ (only the English word in comments), so excluding it from the canonical schema is correct, not a regression.

One non-defect, already covered by the separate review

The inline idx_prds_parent/idx_prds_chain/idx_prds_depends_on (:721–723) and idx_run_engine_log_ws_engine/idx_engine_stats_ws (:785–794) are now created twice — once inline, once by _create_core_indexes at :801. They are non-unique and IF NOT EXISTS, so this is harmless redundancy, not a defect; it's a cleanup item for the style/architecture review (which has already flagged it), and the test_no_create_table_statement_appears_twice guard deliberately checks CREATE TABLE only. Noting it only so it's clear I didn't overlook it.

No Critical/major/minor defects to report on this sync. The fix is sound.

[claude]

Review

Checked the current state of workspace.py against the three earlier automated reviews (Claude bot, GLM, codex) and the fix pushed after them.

The unguarded second idx_tasks_external_url creation (flagged by the earlier Claude/GLM reviews) is now fixed. _create_core_indexes (workspace.py:465-524) is the single call site: the CREATE UNIQUE INDEX is wrapped in try/except sqlite3.IntegrityError there, and the block that used to attempt it a second time inside _ensure_schema_upgrades (around line 764) now only calls _dedupe_external_urls - the comment at 778-781 explicitly documents why the second attempt was removed. Good regression coverage too: test_undedupable_duplicates_warn_instead_of_bricking monkeypatches _dedupe_external_urls to no-op and asserts the upgrade still completes instead of raising IntegrityError, and test_the_index_is_created_exactly_once_in_the_source greps for exactly one occurrence of the CREATE UNIQUE INDEX statement so this can't silently regress a third time. Both are exactly the kind of test that would have caught the bug the second review round found.

The two original P1s (idx_prds_chain/idx_prds_depends_on before their ALTERs; the UNIQUE index before dedupe) are correctly resolved by moving indexes to run last on the upgrade path (_create_core_indexes called at line 801, after all ALTER TABLE migrations and _dedupe_external_urls), and _create_core_tables/_create_core_schema cleanly separate "always safe" DDL from order-dependent DDL. _dedupe_external_urls also does the right thing on data: keeps the oldest row (by rowid), blanks rather than deletes the duplicates, and catches sqlite3.Error on its own scan so a corrupt/locked read can't crash the upgrade either.

Minor, non-blocking:

• idx_prds_parent/idx_prds_chain/idx_prds_depends_on (workspace.py:721-723) and idx_run_engine_log_ws_engine/idx_engine_stats_ws (786-794) are still created inline in _ensure_schema_upgrades in addition to _create_core_indexes recreating them at the end. Harmless at runtime (IF NOT EXISTS, non-unique), but it's leftover duplication against the PR's own "one definition, shared by both paths" goal - already called out in the earlier Claude review as a good follow-up cleanup, and test_no_create_table_statement_appears_twice doesn't catch it since it only regexes CREATE TABLE, not CREATE INDEX.
• Cosmetic: only one blank line separates _create_core_schema from _init_database (workspace.py:535-537), vs. two elsewhere in the file - won't fail ruff check since E3xx isn't in the default rule selection here, just a minor consistency nit.

Test suite (test_schema_convergence_1060.py) is thorough and well-targeted: the generic drop-each-table convergence test, the two "shape a table-drop can't produce" ordering tests (missing column, duplicate data), the source-level duplicate-DDL guard, and the behavior-unchanged smoke tests all read as exactly the right level of test for this class of drift bug. No further concerns - this is in good shape to merge once the maintainers are satisfied with the third-party review round.

[github-actions]

Claude finished @frankbria's task in 2m 29s —— View job

---

Precision bug-hunting review — re-checked on sync (commit 8872620)

The new commit de-duplicates index DDL: it removes five inline CREATE INDEX statements from _ensure_schema_upgrades (idx_prds_parent, idx_prds_chain, idx_prds_depends_on, idx_run_engine_log_ws_engine, idx_engine_stats_ws) and relies on _create_core_indexes — already called last — to own them. I re-traced the synced code for new concrete defects and re-confirmed the prior findings hold. Scoped to defects only (logic errors, data loss, security, races, breaking contracts); style/architecture/coverage are a separate review.

• Gather PR context (gh pr view, gh pr diff, latest-commit diff)
• Confirm all 5 removed indexes exist in _create_core_indexes
• Verify ordering: indexes still run after the column ALTERs + dedupe they depend on
• Re-confirm the prior UNIQUE-index guard (#943) is intact
• Confirm no CREATE INDEX is duplicated in source (the new guard test's claim)

Result: ✅ no new defects

What I verified on this sync:

• All 5 removed indexes are present in _create_core_indexes (workspace.py:510–512, 517–518), so removing the inline copies does not drop any index from the upgrade path — convergence against a fresh workspace (test_each_table_dropped_alone_is_restored) would otherwise fail here, not in production.
• Ordering is intact. _create_core_indexes runs at workspace.py:791, after the prds.chain_id/depends_on ALTERs (685–719) that idx_prds_chain/idx_prds_depends_on index, and after _create_core_tables (624) guarantees run_engine_log/engine_stats exist for the two engine indexes. This commit only deleted inline copies; it did not move _create_core_indexes earlier, so the load-bearing sequencing the two P1s forced is unchanged.
• Prior finding still fixed. The IntegrityError guard for the UNIQUE idx_tasks_external_url lives inside _create_core_indexes alongside the statement (493–504), and the source contains exactly one such statement — so the [P1.25] Harden workspace DB integrity: foreign keys, safe UNIQUE index upgrade, single DDL, no control-plane contamination #943 brick the earlier rounds flagged is not reintroduced a third time. SQLite rolls back only the failed CREATE UNIQUE INDEX statement on IntegrityError, so the remaining ~20 indexes in the same function still build and the final conn.commit() (795) persists them.
• New source-level guard is sound. test_no_create_index_statement_appears_twice regexes CREATE (?:UNIQUE )?INDEX IF NOT EXISTS <name>; all 27 index names in the file are unique, so it passes and would catch a future duplicate. (Runtime note, not a defect: _create_token_usage_schema is invoked twice on the upgrade path — via _create_core_tables:460 then directly at 628 — but every statement is IF NOT EXISTS, so it is idempotent and the source-level guard is the right level.)

Candidate non-defects I checked and ruled out (listed only for transparency):

• Two consecutive bare conn.commit() calls at workspace.py:783 and 785, plus the now-stale # Add indexes for PRD version chain queries comment over a bare commit at 721–722, are leftovers from the removed inline-index block. Redundant/misleading, but not runtime defects — conn.commit() with nothing pending is a no-op and workspaces still upgrade correctly. Cleanup items for the style pass, out of scope here.

No Critical/major/minor defects to report on this sync. The index de-duplication is correct: every removed index is recreated exactly once, last, by the shared definition, with ordering and the #943 guard preserved.

[frankbria]

Both minor items fixed — the first turned out to matter more than "leftover cleanup".

Duplicate CREATE INDEX statements. Five of them: idx_prds_parent, idx_prds_chain, idx_prds_depends_on, idx_run_engine_log_ws_engine, idx_engine_stats_ws. Harmless at runtime as you say, but shipping a PR about duplicated DDL that still contains duplicated DDL isn't a defensible place to stop.

Your observation about why my guard missed them is the useful part: it regexed CREATE TABLE only. So the fix is test_no_create_index_statement_appears_twice, and that test is what found all five — not me re-reading the file. It also caught a mistake in my first removal attempt: I deleted matching lines, which for the multi-line statements left an orphaned cursor.execute( with a dangling "ON ..." continuation. Six tests went red with sqlite3.OperationalError: near \"ON\". Redone statement-aware.

workspace.py is now 1068 lines, down from 1206, with 44 DDL statements each appearing exactly once.

Blank line — fixed. And thanks for noting ruff check won't catch it: E3xx isn't in this repo's selected rules, so that consistency is by hand, which is worth knowing rather than assuming the linter has it.

---

Four review rounds on this one, and the pattern across them is consistent enough to name: every finding was about something the shared definition changed other than duplication — ordering against migrations, ordering against data fixups, a guard left behind at a call site, and finally indexes not covered by my own duplication guard. The de-duplication itself was never the risky part; the implicit context each path had accumulated around its copy was.

[claude]

Review

I read through the four prior review rounds (Claude bot, GLM, codex, and the second Claude pass) plus the final synced diff. The two original [P1]s are correctly fixed and I re-verified the trace myself: idx_prds_chain/idx_prds_depends_on are created in _create_core_indexes (workspace.py:465-524), called only at the end of _ensure_schema_upgrades (:791), after the chain_id/depends_on ALTERs (:685-719) and after _dedupe_external_urls (:780). The previously-flagged unguarded second idx_tasks_external_url attempt is also gone: there is now exactly one CREATE UNIQUE INDEX statement in the source, guarded with try/except sqlite3.IntegrityError inside _create_core_indexes itself (:493-504), and the upgrade path only calls _dedupe_external_urls before falling through to the shared indexer. test_undedupable_duplicates_warn_instead_of_bricking exercises exactly the 'dedupe fails to fully clean up' corner case the second Claude review called out as untested. The five previously-duplicated CREATE INDEX statements (idx_prds_parent, idx_prds_chain, idx_prds_depends_on, idx_run_engine_log_ws_engine, idx_engine_stats_ws) are gone too, and test_no_create_index_statement_appears_twice now guards the class of bug that let them slip past the original CREATE TABLE-only regex.

Three small things nobody flagged yet, all harmless rather than bugs:

• _create_token_usage_schema(cursor) is now called twice on the upgrade path: once inside _create_core_tables (workspace.py:460), and again explicitly right after _create_core_tables(cursor) returns in _ensure_schema_upgrades (:626-628, with a comment that predates this refactor and now describes something the shared function already does). Harmless (IF NOT EXISTS), but it is the same class of duplication this PR exists to remove, just at the call-site level instead of the DDL-text level, so neither of the two 'no duplicate DDL' source-guard tests catches it since both regex literal CREATE statements, not function calls. Worth deleting the now-redundant explicit call+comment at :626-628 in a follow-up.
• Two bare, back-to-back conn.commit() calls with nothing between them at workspace.py:783 and :785, leftover from the inline runs/run_logs/diagnostic_reports/run_engine_log/engine_stats/execution-trace table blocks that used to live here and each carried their own commit. No-op today, just dead code worth a pass.
• Stale comment at workspace.py:721, '# Add indexes for PRD version chain queries', immediately followed by only a bare conn.commit() — the indexes it refers to moved into _create_core_indexes in this same PR, so the comment now describes nothing that happens at that line.

None of these affect correctness — SQLite commits are no-ops with nothing pending, and calling an IF NOT EXISTS schema function twice is safe — but given the PR's explicit thesis is 'one definition, no duplication,' they're worth a quick cleanup pass so the file fully matches its own stated goal.

Everything else: the _create_core_tables/_create_core_indexes/_create_core_schema split, the docstrings explaining why tables-then-migrations-then-indexes ordering is load-bearing, and test_schema_convergence_1060.py's four-layer coverage (whole-table-drop convergence, source-level no-duplicate-DDL guards, behavior-unchanged smoke tests, and the two 'shapes a table-drop can't produce' ordering tests) are all solid. SCHEMA_VERSION correctly stays at 5 since this is a pure consolidation with no schema content change. No security concerns — this is local SQLite DDL with no user-controlled input reaching these statements.

[github-actions]

Claude finished @frankbria's task in 2m 5s —— View job

---

Precision bug-hunting review — re-checked on sync (commit e03d326)

The new commit removes the three leftover items the prior round flagged as cleanup-only: the now-redundant explicit _create_token_usage_schema(cursor) call, the stale # Add indexes for PRD version chain queries comment, and one of two consecutive bare conn.commit() calls. Re-traced the synced code for new concrete defects and re-confirmed the prior findings hold. Scoped to defects only (logic errors, data loss, security, races, breaking contracts); style/architecture/coverage are a separate review.

• Gather PR context (gh pr view, gh pr diff, latest-commit diff)
• Read the changed regions of workspace.py + callers/callees
• Confirm each candidate defect against real code
• Post summary (no new findings)

✅ GLM review: no new defects found

What I verified on this sync:

• token_usage is still created on the upgrade path. The removed explicit call was genuinely redundant: _create_core_tables(cursor) (called at workspace.py:624) already calls _create_token_usage_schema(cursor) at :460, and that function is pure CREATE TABLE/INDEX IF NOT EXISTS (idempotent, :116–133). No ALTER or other statement in _ensure_schema_upgrades references token_usage, so nothing depended on the second call site's placement between _create_core_tables and the batch_runs migrations. Removing it changes no observable behavior.
• Commit removal changes no transaction semantics. The deleted line was the second of two consecutive conn.commit() calls at the bottom of _ensure_schema_upgrades; conn.commit() with nothing pending is a no-op, so the remaining conn.commit() at :778 still flushes the tasks/dedupe work before _create_core_indexes(cursor) at :784. No pending DDL/data is lost.
• Comment removal is cosmetic. The bare conn.commit() at :717 (inside the prds-exists block) is still present and still a no-op; only the misleading comment above it was deleted.

Prior findings still hold (no regression on this commit)

• The IntegrityError guard for the UNIQUE idx_tasks_external_url lives inside _create_core_indexes alongside the statement (:493–504), and the source contains exactly one such statement — so the [P1.25] Harden workspace DB integrity: foreign keys, safe UNIQUE index upgrade, single DDL, no control-plane contamination #943 brick the earlier rounds flagged is not reintroduced.
• Ordering is intact: _create_core_indexes runs last (:784), after the prds.chain_id/depends_on ALTERs (:681–715) and _dedupe_external_urls (:775). This commit deleted nothing from those sections.

Candidate non-defects I checked and ruled out (listed only for transparency):

• The now-single bare conn.commit() at :717 (formerly under the stale comment) remains redundant with the commit just above it at :715. Harmless no-op, not a runtime defect — a cleanup item for the style pass, out of scope here.

No Critical/major/minor defects to report on this sync. The cleanup is correct and behavior-preserving.

[frankbria]

All three cleaned up in e03d326.

Worth saying why I didn't leave them as a follow-up: each is the same shape as what this PR removes, so shipping "one definition, no duplication" with three counterexamples still in the file would undercut the point of the change.

The _create_token_usage_schema one is the most instructive — it is call-site duplication rather than DDL-text duplication, which is why neither of my source guards catches it (both regex literal CREATE statements, not function calls). That's a genuine hole in the guards, and I'm noting it rather than papering over it: the two guards prove no statement is written twice, not that no schema function is called twice. A guard against the latter would have to reason about call graphs, which is more machinery than the risk warrants — the convergence test already catches any case where double-calling produced a different schema.

The other two were pure residue from removing the inline table blocks: two back-to-back conn.commit() calls with nothing between them, and a # Add indexes for PRD version chain queries comment left pointing at nothing after those indexes moved into _create_core_indexes in this same PR. A stale comment that describes code which is no longer there is the seed of the next drift bug.

No behaviour change — an IF NOT EXISTS schema call is idempotent and a SQLite commit with nothing pending is a no-op. 3792 core tests pass; full gate re-running on the final commit.

Five review rounds, every one finding something real. Thanks — the file is meaningfully better than the version I first pushed.

[claude]

Review

I read through the six prior review rounds in this thread (Claude bot x4, GLM x2, codex, and two prior "claude" passes) plus the final synced diff (e03d326). The consensus already established there still holds on re-check: the two original [P1]s (idx_prds_chain/idx_prds_depends_on before their ALTERs; the UNIQUE idx_tasks_external_url before dedupe) are correctly fixed by the _create_core_tables/_create_core_indexes split with indexes run last, the previously-flagged unguarded second idx_tasks_external_url attempt is gone (exactly one CREATE UNIQUE INDEX in the source, guarded in place at workspace.py:493-504), and the five duplicated CREATE INDEX statements plus the three later leftovers (double _create_token_usage_schema call, stale comment, extra bare commit()) called out in the last two rounds are all cleaned up. test_schema_convergence_1060.py is thorough and well-targeted, and SCHEMA_VERSION correctly stays at 5 since this is pure consolidation.

One thing none of the six prior rounds caught: the tasks-column ALTER block is now fully duplicated, not just the CREATE TABLE/INDEX statements.

_create_core_tables (workspace.py:213-240) runs 12 guarded ALTER TABLE tasks ADD COLUMN ... statements right after CREATE TABLE IF NOT EXISTS tasks. _ensure_schema_upgrades calls _create_core_tables(cursor) at :624 -- so by the time execution reaches the # Add new columns to tasks table if they don't exist block at :719-763, all 12 of those same columns (depends_on, estimated_hours, complexity_score, uncertainty_level, parent_id, lineage, is_leaf, hierarchical_id, requirement_ids, github_issue_number, external_url, auto_close_github_issue) have already been added. The PRAGMA table_info(tasks) re-check at :724 will always find them present, so every if column not in task_columns: guard in that block is now permanently false -- it's ~44 lines of dead code (grep -n "ADD COLUMN" codeframe/core/workspace.py shows each of the 12 statements appearing twice, at 218-240 and again at 727-761).

This is harmless at runtime (idempotent, guarded, gated behind the PRAGMA user_version check so it only ever executes once per real upgrade) -- same "no behavior change" category as the _create_token_usage_schema double-call and the five duplicate CREATE INDEX statements fixed in earlier rounds. But it's the same shape of issue at a larger scale (a whole 44-line block vs. a single call or index line), it directly contradicts this PR's own AC1 ("each table's DDL exists in exactly one place"), and -- worth noting for the test guards themselves -- neither test_no_create_table_statement_appears_twice nor test_no_create_index_statement_appears_twice catches it, since both regex CREATE TABLE/CREATE INDEX literals only, not ALTER TABLE ... ADD COLUMN. Given the PR's own pattern this cycle (each duplication category got its own guard once found), this would be the natural next one: a regex over ALTER TABLE (\w+) ADD COLUMN (\w+) catching repeated (table, column) pairs.

Suggested fix: delete :719-763 except the _dedupe_external_urls(conn, cursor) call and its surrounding cursor.execute("SELECT name FROM sqlite_master WHERE type='table' AND name='tasks'") / if cursor.fetchone(): guard, which is still needed -- _create_core_tables doesn't do dedup.

Two minor doc nits, non-blocking:

• _create_core_tables's docstring (:137) still says "Create every workspace table and index, idempotently" -- stale from before the table/index split that fixed the two P1s. The whole point of _create_core_indexes existing separately is that this function does not create indexes anymore; the docstring should drop "and index."
• The comment directly above the _create_token_usage_schema(cursor) call inside _create_core_tables (:458-459, "issue [P0.1] Create the token_usage table (with indexes) in the per-workspace schema #712 -- was never created here, so every save_token_usage() raised 'no such table'") reads oddly in its current location -- it plainly is created right there. It looks like historical context describing the old upgrade-path-only gap, now sitting on the shared function where it's created unconditionally; worth a reword or moving to _ensure_schema_upgrades where the history is more relevant.

Everything else matches what the prior rounds already established: ordering is load-bearing and correctly sequenced, the #943 UNIQUE-index guard is intact and tested (test_undedupable_duplicates_warn_instead_of_bricking), and the convergence/legacy-shape test suite is solid. No security concerns -- local SQLite DDL, no user-controlled input reaches these statements.

[github-actions]

Claude finished @frankbria's task in 2m 17s —— View job

---

Precision bug-hunting review — re-checked on sync (commit bb42225)

The new commit is a docstring fix (_create_core_tables no longer claims to create indexes) plus one new test (test_a_tasks_table_missing_columns_is_migrated_not_left_alone). Neither touches runtime behavior, so I re-traced the synced code for new concrete defects and re-confirmed the prior findings hold. Scoped to defects only (logic errors, data loss, security, races, breaking contracts); style/architecture/coverage are a separate review.

• Gather PR context (gh pr view, latest-commit diff, prior review findings)
• Read the changed regions of workspace.py + callers/callees
• Confirm each candidate defect against real code
• Post summary (no new findings)

✅ no new defects found

What I verified on this sync:

• The latest commit changes no runtime semantics. Only the _create_core_tables docstring text (137-140) and a new test in test_schema_convergence_1060.py moved. No DDL, no control-flow, no data path changed, so nothing new can be broken here.
• The new test is sound. It builds a fresh DB, resets tasks to a minimal legacy shape, forces user_version = 0, then asserts _ensure_schema_upgrades leaves every current column present. It deliberately does not pin which path adds them — and passes because both do (_create_core_tables ALTERs at 220-243 and the upgrade-path block at 729-766), so it would catch a regression in either. The PRAGMA user_version = 0 reset correctly defeats the current_version >= SCHEMA_VERSION early-return (615-617) so the upgrade body actually runs.

Prior findings still hold (no regression on this commit)

• [P1.25] Harden workspace DB integrity: foreign keys, safe UNIQUE index upgrade, single DDL, no control-plane contamination #943 UNIQUE-index guard intact. Exactly one CREATE UNIQUE INDEX ... idx_tasks_external_url statement exists (496-500), wrapped in try/except sqlite3.IntegrityError inside _create_core_indexes, so every caller inherits it. The upgrade path's only job for that index is _dedupe_external_urls (778); the index itself is created once, last, by the shared function.
• Ordering intact. _create_core_indexes runs at 787, after _create_core_tables (627), the batch_runs/workspace/prds/tasks ALTER migrations (635-766), and _dedupe_external_urls (778) — so idx_prds_chain/idx_prds_depends_on index columns that exist, and the UNIQUE index only fires on deduped data. This commit did not touch that sequence.

Candidate non-defects I checked and ruled out (listed only for transparency)

• The now-redundant tasks ALTER block (722-766), which the commit message itself flags as a probable follow-up deletion. _create_core_tables (called at 627) already runs the same 12 guarded ALTER TABLE tasks ADD COLUMN statements (220-243), so the PRAGMA table_info(tasks) re-check at 727-728 always finds all 12 present and every if … not in task_columns: guard is a no-op. I diffed the two column lists: identical columns, identical types/defaults (depends_on/lineage/requirement_ids → DEFAULT '[]', is_leaf → DEFAULT 1, auto_close_github_issue → DEFAULT 0), only reordered. Idempotent and guarded — no divergence, no data effect. This is the same class of harmless redundancy earlier rounds called cleanup-only, and the author has explicitly deferred it to its own change; not a runtime defect.
• Transaction state after the caught IntegrityError. SQLite rolls back only the failing statement, not the transaction, so the ~20 remaining CREATE INDEX statements after the guarded UNIQUE index still execute and the final conn.commit() (791) persists them. Re-confirmed.
• Bare conn.commit() at 720/779/781. No-ops with nothing pending; cosmetic, out of defect scope.

No Critical/major/minor defects to report on this sync. The docstring correction is accurate (the function creates tables only — indexes live in _create_core_indexes, which is the whole reason that function exists), and the legacy-tasks migration test closes the gap the prior round noted (no test previously pinned that a pre-existing tasks table ends up fully migrated).

[frankbria]

Two doc nits fixed. On the third — you were probably right and I was wrong, and the way I found out is worth recording.

Stale docstring — fixed. _create_core_tables said "every workspace table and index"; it creates tables only, which is the entire reason _create_core_indexes exists separately.

The :719-763 deletion suggestion. I initially disagreed, reasoning that CREATE TABLE IF NOT EXISTS cannot add columns to an existing table, so a legacy tasks would only get external_url etc. from those ALTERs. I wrote a test asserting exactly that — and then could not reproduce a failure when I removed the block. 12 tests passed either way.

The reason, checked directly rather than reasoned about:

_create_core_tables(cursor)  on a legacy tasks(id, title)
  -> external_url present? True
ALTER TABLE statements inside _create_core_tables: 15

_init_database already carried ALTERs, and extracting its body moved them into the shared function. So _create_core_tables is a full make-the-schema-current routine, not the CREATE-only helper its name implies — and your suggestion holds.

I have not deleted the block here. Removing ~45 lines of migration code as the last act of a five-round PR is how the earlier P1s happened; it deserves its own verification pass. Filed as #1104 [P3.1] with the evidence above and the open question of whether _create_core_tables should stop carrying ALTERs or be renamed to match what it does.

What I did change: my new test no longer claims the block is load-bearing. It asserts the outcome — a legacy tasks table comes out fully migrated — deliberately not asserting which path does it, so it stays valid whichever way #1104 goes. A test whose rationale is wrong is worse than no test, even when it's green.

Third nit (the #712 comment reading oddly now that the table is created unconditionally) folds into #1104 as well, since it's the same question about what that function is.

[claude]

Review — current head bb422255

I read the diff plus the full review thread (Claude bot x4, GLM x2, codex, two prior claude passes, and the author's replies). Seven rounds already ran on this, each catching something real, so I focused on: (a) re-verifying the fixes actually landed as described, and (b) looking for anything genuinely new.

The two original [P1]s and the follow-on unguarded-index bug are correctly fixed and still hold. Traced it myself: idx_prds_chain/idx_prds_depends_on are created in _create_core_indexes, called only at the end of _ensure_schema_upgrades (workspace.py:787), after the chain_id/depends_on ALTERs (:684-717) and after _dedupe_external_urls (:778). There is exactly one CREATE UNIQUE INDEX IF NOT EXISTS idx_tasks_external_url statement in the source, guarded in place inside _create_core_indexes (:496-507) — the earlier unguarded second attempt is gone. The five duplicated CREATE INDEX statements and the later leftovers (double _create_token_usage_schema call, stale PRD-index comment, extra bare commit()) are all cleaned up as described.

One small thing none of the prior rounds mentioned: a stale trailing comment. codeframe/core/workspace.py:465 — # Create indexes for common queries — sits at the end of _create_core_tables with nothing under it; the index statements it used to introduce moved into _create_core_indexes earlier in this same PR. Same category as the # Add indexes for PRD version chain queries comment fixed in commit e03d3266 ("a stale comment that describes code which is no longer there is the seed of the next drift bug," per the author's own comment on that fix) — this one just didn't get caught by the same pass. Trivial, but worth a one-line delete while the file's already being polished for exactly this.

Already-tracked, not re-raising as new: the _create_core_tables docstring still says "Column-level migrations (ALTER TABLE) stay in _ensure_schema_upgrades," but the function itself carries ~15 guarded ALTER TABLE statements for tasks and blockers, and the tasks-column ALTER block later in _ensure_schema_upgrades (:722-766) is now dead code as a result. The author verified this by direct experiment (removing the block, all tests stayed green) and filed #1104 rather than deleting ~45 lines of migration logic as the last act of an already five-round PR — that's the right call; a load-bearing-looking migration block deserves its own verification pass, not a rushed removal here.

Tests: test_schema_convergence_1060.py is well-targeted — the generic per-table-drop convergence test, the two "shapes a table-drop can't produce" ordering tests (missing chain_id, duplicate external_url data), the source-level no-duplicate-DDL guards for both CREATE TABLE and CREATE INDEX, and the outcome-only (not path-specific) legacy-tasks-migration test added in the last commit. Nothing to add there.

No new correctness, security, or performance concerns. Good to merge once the trailing comment is dropped (or even without it — it's cosmetic).

[frankbria]

Dropped in 92a0c2c — and fair that I missed it in the very pass where I said stale comments are "the seed of the next drift bug". Comment-only change, no code touched.

Also noting for the record: you're right that _create_core_tables' docstring still claims "Column-level migrations (ALTER TABLE) stay in _ensure_schema_upgrades" while the function carries ~15 of them. I'm leaving that wording to #1104 rather than patching it here, because the honest fix isn't a docstring edit — it's deciding whether that function should carry ALTERs at all or be renamed. Correcting the sentence now would make the file look consistent while the underlying question stays open, which is a worse state to leave it in than an obviously-stale claim that points at a filed issue.

[github-actions]

Claude finished @frankbria's task in 1m 12s —— View job

---

Precision bug-hunting review — re-checked on sync (commit 92a0c2c)

The new commit drops an orphaned trailing comment (# Create indexes for common queries) from the end of _create_core_tables — the statements it used to introduce moved into _create_core_indexes earlier in this PR. Pure documentation change, no DDL/control-flow/data path touched. Re-traced the synced code for new concrete defects and re-confirmed the prior findings hold. Scoped to defects only (logic errors, data loss, security, races, breaking contracts); style/architecture/coverage are a separate review.

• Gather PR context (gh pr view, latest-commit diff, prior review findings)
• Read the changed region of workspace.py
• Re-confirm prior findings (UNIQUE-index guard + ordering) against real code
• Post summary (no new findings)

✅ GLM review: no new defects found

What I verified on this sync:

• The commit changes no runtime semantics. Only the comment text at the tail of _create_core_tables (now ends cleanly with _create_token_usage_schema(cursor) at :463, two blank lines, then def _create_core_indexes). Nothing executable moved, so nothing new can break here.
• Source-level invariants still hold. Exactly one CREATE UNIQUE INDEX in the file (:496, the guarded idx_tasks_external_url); every one of the 26 index names appears exactly once; 17 CREATE TABLE IF NOT EXISTS literals. No statement written twice.

Prior findings still hold (no regression on this commit)

• [P1.25] Harden workspace DB integrity: foreign keys, safe UNIQUE index upgrade, single DDL, no control-plane contamination #943 UNIQUE-index guard intact. The single CREATE UNIQUE INDEX IF NOT EXISTS idx_tasks_external_url is wrapped in try/except sqlite3.IntegrityError inside _create_core_indexes (:494–505), so every caller inherits the recovery net. The upgrade path's only job for that index is _dedupe_external_urls (:776); the index is created once, last, by the shared function.
• Ordering intact. _create_core_indexes runs last (:785), after _create_core_tables (:625), the prds.chain_id/depends_on ALTERs (:682–684, :710–714), and _dedupe_external_urls (:776). So idx_prds_chain/idx_prds_depends_on only index columns that exist, and the UNIQUE index only fires on deduped data. This commit deleted a comment; it did not touch that sequence.

Candidate non-defects I checked and ruled out (listed only for transparency):

• Function boundary after the comment deletion. _create_core_tables now ends at _create_token_usage_schema(cursor) with two blank lines before the next def — valid and consistent with the file. No statement lost.
• Transaction state around the guarded UNIQUE index is unchanged from prior rounds: SQLite rolls back only the failing statement on IntegrityError, so the ~20 subsequent non-unique indexes in _create_core_indexes still build and the final conn.commit() (:789) persists them.

No Critical/major/minor defects to report on this sync. The orphaned-comment removal is correct and behavior-preserving.

[claude]

Review — current head 92a0c2c

I re-traced the full diff independently (not just skimming the prior 8 review rounds) against the checked-out merge commit, focused on the two invariants this PR exists to guarantee: no duplicated DDL, and correct create/migrate/index ordering on the upgrade path.

Confirmed correct, with line numbers from the current head:

• 18 distinct CREATE TABLE IF NOT EXISTS statements, each appearing exactly once (workspace.py:117-459), shared by both _init_database and _ensure_schema_upgrades via _create_core_tables.
• Every CREATE INDEX/CREATE UNIQUE INDEX statement also appears exactly once, inside _create_core_indexes (:483-525) — I grepped the whole file to confirm, the five previously-duplicated PRD/engine indexes are gone from _ensure_schema_upgrades.
• Ordering on the upgrade path is right: _create_core_indexes runs last (:785), after the prds.chain_id/depends_on ALTERs + backfill (:682-716) and after _dedupe_external_urls (:776). idx_prds_chain/idx_prds_depends_on can't hit "no such column," and the UNIQUE idx_tasks_external_url only ever indexes deduped data.
• The UNIQUE index has exactly one call site, guarded in place (:494-505) — the earlier unguarded-second-attempt bug (caught mid-PR) is gone.
• Test suite (test_schema_convergence_1060.py) matches what it claims: per-table-drop convergence, whole-schema-drop convergence, the two ordering-specific legacy shapes (missing chain_id, duplicate external_url rows) that a table-drop test structurally can't produce, and source-level no-duplicate-DDL guards for both CREATE TABLE and CREATE INDEX.

No new findings. This matches what the last full review round (11:56, commit bb42225) already concluded, and the one item it flagged (stale trailing comment at old :465) was dropped in 92a0c2c as stated.

One thing worth double-checking before merge, not a defect: _create_core_tables's docstring still says column migrations "stay in _ensure_schema_upgrades," but the function itself carries ~15 guarded ALTER TABLE statements for tasks and blockers (:216-243, :277-280) — this is already known and tracked as #1104, and I agree with the author's call to defer rather than patch the docstring cosmetically without resolving the underlying "should this function carry ALTERs" question. Flagging only so it isn't lost if #1104 doesn't get picked up promptly, since the dead-code block it points at (workspace.py:720-764, the redundant tasks column checks in _ensure_schema_upgrades) is ~45 lines that will keep confusing readers until it's resolved one way or the other.

Given the depth of review this PR has already been through (8 rounds across two bots and the author, catching and fixing two real P1s plus several duplication leftovers), and that my independent trace turned up nothing new: looks good to merge.