feat(desktop,store,packtool): C3 Node PackManager lifecycle on SQLite with cross-backend parity (issue #70) - #115
Merged
Merged
Conversation
… with cross-backend parity (issue #70) Schema v3 (contracts/store.schema.sql): packs becomes per-version — PK (id, version) + active/install_path — because C2's supersede/rollback semantics need multiple installed versions per pack id; supersedes and docs.pack_id FKs dropped (composite-PK parent; links.pack_id precedent). Both migrate ladders extended (migrate.ts + store-interop/migrate.py with --selftest), packtool STORE_SCHEMA_VERSION reconciled, writePackIndex/ installPackRows insert active=1 explicitly. New desktop/main/backend/store/pack-manager.ts ports pack_manager.py's lifecycle (install/supersede/rollback/remove/listInstalled) onto SQLite: ADR-0004 content-hash chunk ids, fixed-words chunker (NOT TextChunker), Ajv-2020 validation against the authoritative pack.schema.json, single BEGIN IMMEDIATE per operation, D4 link helpers composed in-transaction. Wired in the backend host start path and attached to the engine (attachPackManager) — b3 duck-type pin respected. Cross-backend parity: contracts/tests/test_pack_parity.py installs versioned-a-1.0.0 through BOTH the Python and Node PackManagers and asserts identical chunk-id sets (verified: 199b3af6... on both sides). Acceptance: C1-C6 frozen NEW-SURFACE checks all RED at base da5a9d4 and GREEN at head; desktop vitest 400 passed, packtool 95 passed, test_pack_manager + test_pack_schema 64 passed, migrate selftest OK.
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a production store schema migration (v2→v3) that runs on every store open plus a large new cross-runtime lifecycle module, which warrants final human review despite the changes otherwise looking correct.
Pull request overview
This PR implements C3 (issue #70): a Node/SQLite PackManager lifecycle (install / supersede / rollback / remove / listInstalled) that mirrors the Python C2 reference (pack_manager.py) with byte-identical chunk-id semantics, proven by a cross-backend parity test. To support multiple installed versions per pack id, it reshapes the shared store's packs table (schema v2→v3) and extends both migration ladders. It fits into Workstream C (Knowledge packs) and depends on B5's frozen store schema and C1/C2's chunk-id formula.
Changes:
- Schema v3:
packsbecomes per-version (PK(id, version), newactive/install_path),packs.supersedes/docs.pack_idFKs dropped;meta.schema_versionbumped to3with both migrate ladders (migrate.ts+migrate.py) extended using the FK-off schema-change dance. - New
pack-manager.ts(~880 lines): content-hash identity, fixed-words chunker, semver precedence, delete-before-reingest, Ajv validation againstcontracts/pack.schema.json, AC4 schema-version refusal, D4 link maintenance — wired into the host start path and both engines via an optionalattachPackManagerseam. - Tests + pins: new C3 tests (manager, wiring, migration, parity) plus the Python dual-runtime parity driver; version-pin updates (2→3) across pre-existing tests, packtool (
STORE_SCHEMA_VERSION,index-writer), and ADR-0004.
File summaries
| File | Description |
|---|---|
| contracts/store.schema.sql | Reshapes packs to per-version v3; drops inert FKs; bumps schema_version to 3 |
| desktop/main/backend/store/migrate.ts | Adds v2→v3 ladder (FK-off restructure, supersedes JSON-wrap, active=1 mapping) |
| contracts/tests/store-interop/migrate.py | Mirror v2→v3 ladder + --selftest CLI |
| desktop/main/backend/store/pack-manager.ts | New Node PackManager lifecycle port of pack_manager.py |
| desktop/main/backend/index.ts | Constructs PackManager on host start and attaches to engine |
| desktop/main/backend/engine.ts / inference/llama-engine.ts | Add attachPackManager/attachedPackManager seam |
| desktop/package.json / package-lock.json | Adds ajv@^8 + ajv-formats@^3 as runtime deps |
| packtool/build/pack-json.ts / index-writer.ts | STORE_SCHEMA_VERSION=3; explicit active=1, install_path=NULL inserts |
| docs/adr/0004-knowledge-packs.md | Notes schema_version 3 since C3 |
| contracts/tests/test_pack_parity.py | New dual-runtime chunk-id parity driver |
| desktop/src/tests/c3-*.test.ts | New C3 acceptance tests (manager, wiring, migration, parity) |
| desktop/src/tests/{b5,b6,d4}-*.test.ts, packtool tests, ac8 | Version-pin updates 2→3 |
I verified: chunk-id normalization/formula, semver comparison, and fixed-words chunking are byte-faithful to the Python reference; both migration ladders agree; transaction atomicity keeps async embedding outside BEGIN IMMEDIATE; and the AC4 mismatch refusal is correct. The supersede/rollback deactivate-then-activate ordering deliberately (and correctly) diverges from Python to keep shared-content chunk ids live. The only concrete issue found is a stale inline test comment.
Review details
Files not reviewed (1)
- desktop/package-lock.json: Generated file
- Files reviewed: 25/26 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
313
to
+316
| // The explicit ladder is idempotent at v2. | ||
| const again = openStore({ dbPath, dims: 8, repoRoot: REPO_ROOT }); | ||
| try { | ||
| expect(migrate(again.db)).toBe(2); | ||
| expect(migrate(again.db)).toBe(3); |
Collaborator
Author
Swarm PR review (run pr115-20260917-033230) — REQUEST_CHANGESStructured review: 6 base explorer lanes + 11 mandatory micro risk families (8 matched/dispatched, 3 not-triggered with absence evidence) → 18 normalized candidates → 2 independent reviewer shards → critic challenge on HIGH/CRITICAL. Verified findings (16):
Rejected (transparency): PRR-015 (installPackRows scalar-supersedes: unreachable — no v2-era producer of non-NULL supersedes exists, exhaustive git grep); plus by-design candidates (old-code-can't-open-v3 fail-loud, asymmetric backups, AC4 mismatch hard-block mandated by the issue, frozen chunks schema, ajv promotion, zero-word chunk C2-parity, one-way schema). Fixes incoming on this branch via the feedback phase. |
#115) PRR-001/010 (CI blocker): explicit 120s per-test timeouts on the Python-mirror migration test and the parity vitest leg — vitest's default 5s wrapper fired before the child timeout on cold CI runners (the observed 'Electron shell' job failure). PRR-002: cross-backend parity driver wired into CI's store-interop job (pip chromadb + python contracts/tests/test_pack_parity.py) so the AC1 proof executes on every run instead of only on dev machines. PRR-003: PackManager rebinds across clear-cache/recovery store swaps (rebindStore + host set-accessor hook, mirroring the document-surface accessor contract) instead of holding a closed handle. PRR-005: public lifecycle operations serialize behind a promise queue — the C2 registry-lock (pack_manager.py:82) parity for cross-surface races. PRR-006: normalize the two packtool storyline test files back to LF (blob-level churn from an earlier edit; settled via git cat-file). PRR-007/008: CHANGELOG entry for C3/#70; desktop/README schema v1 text corrected to v3 with a Pack lifecycle section; packtool/README links-table note clarified. PRR-009: five C2-parity tests added — zip refusal, semver pre-release ordering (unit + install-level downgrade), supersedes absent-target warning, rows persisting across store reopen, index.schema_version newer than store refusal. PRR-014: unreadable-doc error keeps the fs detail; readPackRows wraps raw sqlite errors (C2 'pack registry read failed' parity). PRR-016: host stop() detaches the pack manager like the other surfaces. PRR-004/011/012/013/018: dispositioned (retry-idempotent / C8-owned deferred / C2-parity-faithful / acceptable given the b3 pin) — closure ledger in the PR comment. Desktop vitest 405 passed (5 new), packtool 95, C2 pytest 64, parity driver OK, checkpoint verify 6/6 OK (frozen checks untouched).
Collaborator
Author
Closure ledger — swarm-pr-feedback (fix commit 401c855)Every finding from the review above, with its outcome. Original IDs preserved; provenance chain: explorer lanes → reviewer shards (MiniMax-M3) → critic challenge (GLM) → fix → fresh swarm-reviewer APPROVE (per-item FIXED-VERIFIED, frozen checks re-run 6/6 PASS) → final critic (items below addressed pre-push).
Accounting correction from the final critic: 11 findings fixed in code, 5 dispositioned (004, 011, 012, 013 + 018), 1 rejected (015), 1 sub-disposition inside 014 — all 16 verified findings + the PRR-015 rejection accounted for above. Post-fix verification: desktop vitest 405 passed / 0 failed, packtool 95 passed, C2 pytest 64 passed, parity driver OK, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #70
Root Cause
Issue #70 (C3) is a feature-slot issue: the desktop Node backend had no
pack lifecycle at all. The Python reference (
pack_manager.py, C2/#69)implements install/supersede/rollback/remove/list_installed against
ChromaDB + a JSON registry, but nothing on the Node/SQLite side could
install, supersede, roll back, remove, or list Knowledge Packs — the
packstable (contracts/store.schema.sql, v2) had no writers and, moredeeply, a lone
idPRIMARY KEY that cannot represent C2's semantics,which require multiple installed versions per pack id (supersede needs
both rows; rollback reactivates a retained inactive row).
Fix
contracts/store.schema.sql):packsre-shaped per-version —PK
(id, version), newactive INTEGER NOT NULL DEFAULT 0andinstall_path TEXT;supersedesbecomes a JSON-array TEXT (FK dropped:composite-PK parent),
docs.pack_idFK dropped (links.pack_id precedent).meta.schema_version bumped to '3' per the schema's own
SCHEMA-BUMP-CONTRACT.
desktop/main/backend/store/migrate.ts(v2→v3 with the official FK-off schema-change dance, supersedes scalar
wrap, active=1 mapping) and the mirror
contracts/tests/store-interop/ migrate.py(plus a--selftestCLI).desktop/main/backend/store/pack-manager.ts(~700 lines): faithfulport of
pack_manager.py— ADR-0004 content-hash chunk ids(
sha256(${docSha}:${index}:${normalized})), the fixed-words chunker(deliberately NOT TextChunker, which would break parity), semver-2.0.0
precedence, delete-before-reingest keyed on path drop OR sha change,
deactivate/activate asymmetry, single BEGIN IMMEDIATE per operation, Ajv
2020-12 validation against the authoritative
contracts/pack.schema.json(new runtime deps: ajv@^8 + ajv-formats@^3), AC4 index.schema_version
mismatch refusal, D4 link helpers composed in-transaction.
desktop/main/backend/index.ts)as an instance field + optional
engine.attachPackManager(implemented inboth
engine.tsStubEngine andinference/llama-engine.ts) — b3duck-type pin respected.
STORE_SCHEMA_VERSION = 3;writePackIndex/installPackRowsinsertactive=1, install_path=NULLexplicitly;schema-version pins updated in 9 pre-existing test files and ADR-0004.
Recurrence Prevention (defect class)
(chunk-id formula, chunking algorithm, lifecycle semantics, schema shape)
drift apart when only one copy is edited.
(08a-recurrence-sweep.md): 12 FIXED-BY-THIS-PR / INTENDED (new pinned
copies), the rest pre-existing and already guarded.
PackManager and the REAL Node PackManager over the same fixture and fails
on any chunk-id divergence; C6 pins the two migration ladders to each
other. Both demonstrated RED at base and GREEN at head (frozen checks).
Tests
python contracts/tests/test_pack_parity.py→ exit 0,identical chunk-id sets (
199b3af6…) from both runtimesvitest runin desktop/ → 400 passed / 0 failed(repro/logs/desktop-final-green.log);
vitest runin packtool/ → 95passed;
pytest tests/test_pack_manager.py contracts/tests/test_pack_schema.py→ 64 passed;
python contracts/tests/store-interop/migrate.py --selftest→ OK
tsc --noEmitclean; black + flake8 clean onchanged Python files
Regression Protection
desktop/src/__tests__/c3-pack-manager.test.ts(10 scenarios:stale-chunk, cross-id supersede, rollback id-set restore, symmetry, 5
refusal classes, embedder mismatch, scoped/bulk remove, listInstalled
shape),
c3-pack-wiring.test.ts(host-attach identity + lifecycle throughthe attached instance),
c3-schema-migration.test.ts(v2→v3 on a realseeded store + DDL parity + Python mirror),
c3-pack-parity.test.ts(frozen Node parity leg),
contracts/tests/test_pack_parity.py(dual-runtime driver).
sha tampering, unsupported mime, over-size non-fixed-words strategy,
overlap ≥ size, embedder length mismatch — all refused before any write.
updates (2→3) and literal-templating only — no deleted assertions
(reviewer-verified).
Acceptance Criteria -> Evidence
python contracts/tests/test_pack_parity.py→[parity] OK; frozen check C1 RED→GREEN (repro/C1.head.log)Coordination with B5 (#63)
B5 is closed; the coordination mechanism its schema mandates is the
SCHEMA-BUMP-CONTRACT (
contracts/store.schema.sql:19-24): bump the metaseed, extend BOTH migrate ladders (
desktop/main/backend/store/migrate.tscontracts/tests/store-interop/migrate.py), which this PR does; the B5lineage PRs are [Workstream B] PR 5 of 9: Prove Node/Python sqlite-vec interoperability and freeze the store schema #63's store freeze and feat(api): C2 Python PackManager lifecycle (install/supersede/rollback/remove) with content-hash chunk ids and update-in-place (issue #69) #114 (C2). ADR-0004's schema-version
note is updated (
2at freeze time,3since this PR).Invariant Audit
The repo has no checked-in invariant doc (
docs/engineering-invariants.mddoes not exist here); audited against the in-process contracts instead:
SCHEMA-BUMP-CONTRACT (honored — see AC6), ADR-0004 chunk identity
(byte-matched, parity-proven), the b3 host prototype pin (respected —
instance field + engine attach only), the C1 manifest schema (validated
against the authoritative file, never copied).
Risk and Rollback
open, v1→v2→v3 ladder preserves rows; back/restore range check extends
automatically)
a store already migrated to v3 would need the v3 file restored before
reopen — no data loss (ladder is additive/lossless)
open) flips the desktop backend from Node to a Python sidecar, the
schema and semantics survive but
pack-manager.tsspecifically isinvalidated; the C2 Python implementation remains the reference either way
pytest tests/has ~99-100 pre-existing environmentfailures on this Windows box at BOTH base and head (flaky API/integration
timing; e.g. test_bm25_index_search fails at base because rank-bm25 is
installed locally); every failure is present at base or passes in
isolation at head — no deterministic regression
Waivers (or none)
none
Merge status
Awaiting explicit user approval; not merged.
PR head: 401c855
Issue Closure
Closes #70