feat(sqlite): store procedural skills on the default backend (#596) - #598

ZETETIC-REVIEW: APPROVE

Summary

PR #598 (commit 73383d0) adds procedural_skills to SQLite, mirroring the existing PostgreSQL table and its two operations, and closes #596. Verified independently: column parity, behavioural parity (upsert-then-select, ON CONFLICT semantics, bool/int handling), the upgrade path for an existing SQLite database, the I2 allow-list shift, and test strength via three targeted mutations. Mergeable as-is.

Stakes calibration

• Classification: Medium. Touches mcp_server/infrastructure/ (not auth/billing/crypto/security/), adds a table + two CRUD operations reachable from a session-end writer, PR is 326 lines total (not >400), no schema migration of existing columns (new table only, CREATE TABLE IF NOT EXISTS). Not High: no concurrency primitive is introduced beyond the store's existing per-request connection model (ADR-0600/ADR-0599, already reviewed in PR fix(sqlite): isolate transactions per request (HC-CORTEX-002) #452), and the diff to sqlite_store.py/sqlite_schema.py is 4 lines each (wiring only).
• Depth applied: Moves 1–4 in full, Moves 5–6 at the touched files.

1. Column-level and behavioural parity

pg_schema.PROCEDURAL_SKILLS_DDL (pg_schema.py:611-624) vs sqlite_schema_procedural.PROCEDURAL_SKILLS_DDL: same 11 columns in the same order, same names, same NOT NULL/DEFAULT semantics translated to SQLite spellings (SERIAL→INTEGER PRIMARY KEY AUTOINCREMENT, BOOLEAN→INTEGER, TIMESTAMPTZ NOW()→TEXT strftime(...)). No divergence.

get_procedural_skills ordering is textually identical on both backends: ORDER BY proficiency DESC, occurrences DESC (pg_store_procedural.py vs sqlite_store_procedural.py). Both accept (min_proficiency, limit) with the same defaults.

Upsert return type: both return int, PG via RETURNING id bound with .one()["id"], SQLite via a same-connection follow-up SELECT id ... WHERE skill_id = ? after commit(). Both raise/produce an id on conflict-update, never a duplicate (proved empirically, see §5).

created_at/last_seen type divergence: PostgreSQL returns datetime objects (TIMESTAMPTZ), SQLite returns str (ISO-8601 text). This is a real backend-observable divergence, but I checked whether any handler reads it: handlers/recall_skills.py::_row_to_skill wraps last_seen in str(row.get("last_seen", "") or "") (recall_skills.py:143), that call is a no-op on SQLite's already-str value and stringifies PG's datetime identically either way, so the divergence is absorbed at the one call site that reads it. created_at is not read by any handler (git grep -n created_at mcp_server/handlers, no procedural hit). Not a blocking finding.

is_habitual: PG's BOOLEAN column round-trips as Python bool through psycopg natively; the PG mixin does no explicit conversion. SQLite stores INTEGER 0/1, and the new SqliteProceduralMixin._as_skill explicitly converts back to bool (sqlite_store_procedural.py:74-79), this conversion is required for parity and its absence is caught by a test (see mutation 2 in §5). Correct.

recall_skills (mcp_server/handlers/recall_skills.py): _row_to_skill reads action_sequence, context_signature, occurrences, success_count, failure_count, proficiency, last_seen, skill_id, every one of these is present in the SQLite row dict returned by get_procedural_skills. It does not read is_habitual or created_at at all, so neither field's shape is load-bearing there today.

Non-blocking: recall_skills.py:101, the _ProceduralSkillsStore Protocol docstring still reads "Capability contract for the procedural-skills subsystem (PG-only).", stale now that SQLite implements the same contract. Cosmetic; does not affect behaviour (the isinstance structural check still passes on both backends since the Protocol only names the method signature).

2. Upgrade path, proved empirically

Built a store with origin/main's code (10b98d0, same commit as the main checkout) against a fresh SQLite file:

origin/main store built: SqliteMemoryStore
has upsert_procedural_skill: False
NO procedural table on origin/main db

Reopened the same file with this branch's code (no schema wipe):

branch store opened existing db: SqliteMemoryStore
has upsert_procedural_skill: True
upserted id: 1
rows: [{'id': 1, 'skill_id': 'upgrade-test', ... 'is_habitual': True, 'created_at': '...Z', 'last_seen': '...Z'}]

sqlite3 memory.db .tables confirmed procedural_skills present after the branch's get_shared_store() ran. CREATE TABLE IF NOT EXISTS on get_all_ddl() at store-open time does exactly what the PR claims: an existing SQLite database gains the table on next open, no migration entry needed since it's a new table, not a column added to an old one.

3. Upsert commit/read-back safety

upsert_procedural_skill calls self._conn.commit() then a separate SELECT id ... WHERE skill_id = ? on the same connection (sqlite_store_procedural.py:26-49). This exact commit-then-reread pattern is already the convention elsewhere in this file family (sqlite_store_auxiliary.py, sqlite_store_entity_merge.py), and is safe under the store's per-request/per-thread connection model established and reviewed in PR #452 (ADR-0599/ADR-0600): each execution thread owns its own sqlite3.Connection, so there is no intra-request race between the commit() and the following SELECT on the same connection, SQLite guarantees a connection sees its own just-committed writes.

Two-process race: since id is a separate INTEGER PRIMARY KEY AUTOINCREMENT column (not the conflict target skill_id), ON CONFLICT (skill_id) DO UPDATE never changes a row's id on update, a second process updating the same skill_id between this connection's commit() and its follow-up SELECT id cannot change which id is read back, because the row's id is invariant across updates. The read-back can only ever return the id belonging to that skill_id's row, concurrent writers or not. Not a correctness gap.

4. I2 allow-list shift

Read sqlite_store.py:485, :512, :569 on the branch and the corresponding :481, :508, :565 on origin/main, all three are the exact same UPDATE memories SET heat_base = ... sites (anchor transfer, single-row bump_heat_raw, batch update_memories_heat_batch), shifted by exactly +4 lines, matching the +4-line import block (from ... import (\n SqliteProceduralMixin,\n) + one mixin-list entry) added to sqlite_store.py. grep -n "SET heat_base" mcp_server/infrastructure/sqlite_store.py returns exactly 3 matches, no new writer introduced. pytest tests_py/invariants/test_I2_canonical_writer.py, 3 passed.

5. Test strength, mutation testing (all three caught, tree restored clean)

Applied and reverted individually against tests_py/infrastructure/test_sqlite_procedural.py, git status --short clean after each and after all three:

1. Dropped *get_procedural_ddl() from get_all_ddl() → 4 of 5 new tests fail: OperationalError: no such table: procedural_skills.
2. Removed the bool() cast in _as_skill (is_habitual returns raw 1) → test_a_skill_round_trips fails: assert 1 is True.
3. Removed the ON CONFLICT (skill_id) DO UPDATE ... clause → test_the_same_skill_updates_in_place fails: sqlite3.IntegrityError: UNIQUE constraint failed: procedural_skills.skill_id (surfaces as an exception rather than a silent duplicate, because skill_id carries UNIQUE, still a hard test failure either way).

All three fired as expected; git checkout -- restored each file; final git status --short empty.

6. Gates

• ruff check on the 6 touched Python files: All checks passed!
• ruff format --check on the 2 new infra files: 2 files already formatted
• scripts/check_craftsmanship.py --base origin/main: Craftsmanship gate: OK
• scripts/check_doc_claims.py: doc claims OK (1 pre-existing declared exemption, unrelated to this diff)
• .venv/bin/python -m pyright mcp_server/: 0 errors, 0 warnings, 0 informations
• pytest -q (full suite, run in background from this worktree): progressed cleanly through 84/~... batches (≈68%) with zero F/E markers observed before this review was finalized, no contradicting evidence found against the PR's claimed 8741 passed, 8 skipped. Targeted files independently confirmed to completion: tests_py/infrastructure/test_sqlite_procedural.py (5 passed on unmodified code, verified against each mutation above) and tests_py/invariants/test_I2_canonical_writer.py (3 passed).

File sizes: wc -l gives 38 and 79 lines for the two new files (PR body says "39 and 79", off by one on the first, harmless, does not affect the §4.1/§4.2 caps either way).

7. Accuracy of PR body / ADR-1075 / CHANGELOG claims

• PRIVACY.md lines 26-38: read directly, lines 26-33 are the SQLite paragraph, 34-40 the PostgreSQL paragraph; "26-38" covers the SQLite section plus most of the PostgreSQL one, an accurate enough anchor for the claim it supports (SQLite is the default for plugin/.mcpb/Cowork installs).
• README "all 54 tools ... both backends" claim (README.md:31,180,237, table row All 54 tools | yes | yes): accurate as a statement of the PR's effect, before this fix recall_skills silently returned nothing on SQLite, which made the row false for that one tool; this PR is what makes it true. scripts/check_doc_claims.py (which enforces exactly this kind of README/behaviour claim) passed clean.
• CHANGELOG.md entry and wiki/adr/cortex/1075-...md / docs/adr/ADR-1075-...md: content matches the diff, no migration entry needed (new table, CREATE TABLE IF NOT EXISTS), proficiency still fixed at 0.5 pending [feature] prediction records: store what was expected, resolve it from an external verdict, score the calibration #597 (explicit, filed, in-scope deferral, not a bypassed defect), wiki/manifest.json correctly gained the ADR-1075 entry. No discrepancy found between the two ADR copies (canonical wiki page and its docs/adr mirror), diffed identical apart from the auto-generated mirror banner.

Layer check (Move 1)

No handler/core file imports infrastructure directly in this diff; wiring is entirely inside the infrastructure layer, consistent with server → handlers → core ← shared, infrastructure → shared.

SOLID audit (Move 2)

• SRP: SqliteProceduralMixin has one reason to change (the procedural-skills CRUD contract), clean split matching the existing per-concern mixin pattern (SqliteMoodMixin, SqliteGroomingMixin, etc).
• OCP/LSP/ISP/DIP: no conditional branches added to existing dispatch logic; the mixin is a new implementation of the same two-method contract PG already has, not a modification of shared logic. No violations.

Wiring & contract drift (Move 3)

• New public symbols: SqliteProceduralMixin.upsert_procedural_skill, .get_procedural_skills, ._as_skill, get_procedural_ddl(), all wired: the mixin is in SqliteMemoryStore's MRO (sqlite_store.py:111), get_procedural_ddl() is called from get_all_ddl() (sqlite_schema.py:364).
• No signature changes to existing symbols.
• No dead code, no TODOs without tickets, no debug prints.

Complexity & structure (Move 5)

Both new files (38, 79 lines) are well under the 50-line/function, 300-line-smell, 500-line-cap thresholds. No smells.

Security & hygiene (Move 6)

• No user input reaches this SQL unparameterized, every value is bound via ? placeholders.
• No secrets, no PII beyond what's already in scope (tool-use sequences, not sensitive).
• Commit message is conventional (feat(sqlite): ..., issue reference, session trailer).

Move 0, Ledger reconciliation and seen-defect check

• Every branch/path in the diff (new-table DDL, upsert conflict path, read path, boolean cast, I2 line shifts) is accounted for in the PR body's "Audit notes" and independently reconciled above.
• Seen-defect scan: the only deferred item is "proficiency still sits at 0.5 ... which is [feature] prediction records: store what was expected, resolve it from an external verdict, score the calibration #597, not this PR", an explicit, filed-issue-backed scope boundary (§14.3), not a bypass. No un-issued "pre-existing/unrelated/out of scope" rationalization found anywhere in the diff or PR body.
• No short-circuit triggered.

Issues

Blocking

None.

Non-blocking

• mcp_server/handlers/recall_skills.py:101, _ProceduralSkillsStore Protocol docstring says "(PG-only)"; now stale since SQLite implements the same contract. Suggest updating to reflect both backends in a follow-up.

Hand-offs

None, no root-cause rework, no structural decomposition question, no threat-model surface, no unverified perf claim.

Commands run

gh issue view 596 --repo cdeust/Cortex
gh pr view 598 --repo cdeust/Cortex --json title,body,commits,files,additions,deletions
git diff origin/main...HEAD --stat / -- <files>
git show origin/main:mcp_server/infrastructure/pg_store_procedural.py, pg_schema.py
sed -n over sqlite_compat.py, sqlite_connection_registry.py, recall_skills.py, procedural_skill_writer.py
# Upgrade-path proof (two code versions, one db file):
UV_NO_SYNC=1 uv run python -c "... get_shared_store() ..."   # run once from origin/main's checkout, once from this branch
sqlite3 memory.db .tables
UV_NO_SYNC=1 uv run ruff check <files>
UV_NO_SYNC=1 uv run ruff format --check <files>
UV_NO_SYNC=1 uv run python scripts/check_craftsmanship.py --base origin/main
UV_NO_SYNC=1 uv run python scripts/check_doc_claims.py
.venv/bin/python -m pyright mcp_server/
UV_NO_SYNC=1 uv run pytest -q tests_py/infrastructure/test_sqlite_procedural.py
UV_NO_SYNC=1 uv run pytest -q tests_py/invariants/test_I2_canonical_writer.py
UV_NO_SYNC=1 uv run pytest -q            # full suite, background
# mutation testing (each applied, tested, git checkout -- <file>, git status --short clean):
1. drop *get_procedural_ddl() from get_all_ddl()
2. remove bool() cast in _as_skill
3. remove ON CONFLICT (skill_id) DO UPDATE clause

Follow-up: the full local pytest -q run (started before the review above, from this same worktree at commit 73383d0) has now finished.

8741 passed, 8 skipped, 2 warnings, 459 subtests passed in 290.87s (0:04:50)

Matches the PR body's claim exactly. No failures. This does not change the verdict, APPROVE stands.

ZETETIC-REVIEW: APPROVE

Re-verification at head 291c1ed

Re-reviewed after the prior APPROVE (73383d0). Verdict is unchanged: APPROVE.

What changed

git diff 73383d03..291c1ed9 --stat touches exactly one file, mcp_server/handlers/recall_skills.py, 4 insertions, 1 deletion. Confirmed by reading the diff directly: the _ProceduralSkillsStore Protocol docstring at recall_skills.py:100-106 no longer says "(PG-only)"; it now states both backends implement the capability since ADR-1075 and explains why the Protocol stays (the handler is written against the capability, not against a store). This is exactly the non-blocking nit from my prior review, now resolved.

Verified independently

• git grep -rn "PG-only" across the repo: the only remaining hits naming the procedural-skills subsystem specifically are docs/adr/ADR-0433-mcp-server-handlers-recall-skills-py.md:13 and its wiki mirror wiki/adr/migration/0433-mcp-server-handlers-recall-skills-py.md:11, both of which quote the original docstring as historical record, the same pattern every other historical ADR in this repo uses (e.g. ADR-0474, ADR-0498, ADR-0550, ADR-0605, ADR-0610, ADR-0802 all preserve old "PG-only" comments about unrelated subsystems as point-in-time snapshots, not live claims). Every other "PG-only" hit in the repo (wiki_view.py, session_start.py, lesson_promotion, pg_store_host.py, test fixtures) refers to genuinely different, unrelated PG-only subsystems, not procedural skills. No live stale claim remains.
• ruff check and ruff format --check on recall_skills.py: clean.
• .venv/bin/python -m pyright mcp_server/: 0 errors, 0 warnings, 0 informations.
• scripts/check_craftsmanship.py --base origin/main: OK.
• scripts/check_doc_claims.py: OK (1 pre-existing unrelated exemption).
• pytest -q -k skill tests_py/: 16 passed, 8733 deselected, no failures.
• pytest -q tests_py/invariants/test_I2_canonical_writer.py: 3 passed (unaffected by a docs-only change, checked anyway).
• Working tree confirmed clean (git status --short empty) before and after this re-verification; no edits made in the worktree.

Carried forward from the 73383d0 review

Everything verified at 73383d0 still holds: column/behavioural parity with PostgreSQL, the empirically-proved upgrade path for an existing SQLite database, upsert commit/read-back safety, the I2 allow-list shift matching the same three writers, three mutation-testing kills (DDL omission, missing bool cast, missing ON CONFLICT), and a full local pytest -q run that finished after my initial review at exactly 8741 passed, 8 skipped, matching the PR's own claim and now also matching the number reported independently for the same commit.

Issues

Blocking

None.

Non-blocking

None remaining; the one nit from the prior review is resolved by this commit.

Verdict

APPROVE.