Skip to content

PLA-51: give storage its own owner instead of the Sessions registry - #113

Merged
hristo2612 merged 2 commits into
mainfrom
simplify/PLA-51-storage-ownership
Aug 3, 2026
Merged

PLA-51: give storage its own owner instead of the Sessions registry#113
hristo2612 merged 2 commits into
mainfrom
simplify/PLA-51-storage-ownership

Conversation

@hristo2612

Copy link
Copy Markdown
Owner

Selected area

packages/jinn/src/sessions/registry.ts — the Sessions registry, which also owned the
process-wide SQLite bootstrap for every other module in the gateway.

Evidence of over-engineering / blended concerns (measured at base 23df25ed)

registry.ts was 3,985 lines and held three unrelated responsibilities:

  1. Session/message CRUD — its actual job.
  2. The process-wide storage bootstrap — the db singleton, initDb(), disk
    preflight, pre-migration backup/prune, DB-version sidecar, and the SQLite
    busy-retry helpers.
  3. Other modules' schema migrations — the single initialize immediate
    transaction created and migrated Sessions, Messages, messages_fts,
    queue_items, callback deliveries, Files and chat pins, and called
    migrateWorkItemsSchema (Todos) and migrateActivitySchema (Activity).

Consequences of the blend:

  • 102 files referenced initDb. Every non-session module — work-items (×9),
    activity (×2), gateway, cron, cli, knowledge — reached its database handle by
    importing from the Sessions module. Storage ownership was expressed as a
    dependency on an unrelated domain.
  • Four modules imported pure string sanitizers from the registry.
    knowledge/store.ts, mcp/knowledge-tools.ts, gateway/files.ts and
    gateway/api.ts pulled stripControlChars / hasControlBytes out of a
    3,985-line session-CRUD file.
  • Todo-primitive doctrine prose lived in a comment block inside the Sessions
    registry, documenting a domain the file does not own.
  • The codebase already had the right convention in two places and was not
    following it in the third: work-items/migrate.ts and activity/migrate.ts
    already exist, and workflows/repository-migrations.ts already exports
    openWorkflowDatabase() owning its own store.

Fixed constraint budget

Frozen in PLAN.md before implementation:

Field Budget
netLineDelta ≤ 0
maxFilesTouched 110
maxNewFiles 3
maxFileLines 7584

Measured budget output

Command run verbatim from PLAN.md at the pushed head:

netLineDelta=-4
filesTouched=109
newFiles=3
maxTouchedFileLines=7583

All four inside budget.

What was deleted or clarified

Three new files, each owning one concern:

File Lines Owns
packages/jinn/src/shared/db.ts 220 The storage bootstrap. Declares the db singleton and initDb(), plus disk preflight, pre-migration backup/prune, version sidecar and the busy-retry helpers. A composition root: it sequences each owner's migration but contains no DDL and no domain logic.
packages/jinn/src/sessions/migrate.ts 933 Sessions-owned DDL and migrations, moved verbatim — the CREATE_* constants and migrateMessagesSchema / migrateSessionsSchema / migrateFtsSchema / migrateQueueItemsSchema / migrateCallbackDeliveriesSchema.
packages/jinn/src/shared/sanitize.ts 22 stripControlChars and hasControlBytes, moved verbatim.

Removed from registry.ts: 1,174 lines deleted, 4 added — 3,985 → 2,815 lines.

  • initDb is now declared in exactly one place (shared/db.ts:136). registry.ts
    imports it like every other module (registry.ts:5) and does not re-export it,
    so nothing reaches storage through Sessions any more.
  • migrateWorkItemsSchema and migrateActivitySchema no longer appear in
    registry.ts; shared/db.ts sequences them from their owning modules'
    migrate.ts.
  • The Todo-primitive doctrine prose block is gone.
  • The ~100 initDb importers were re-pointed mechanically — same exported name,
    one import line changed per file.

Import-cycle hazard, resolved without a cycle. migrateCallbackDeliveriesSchema
called runImmediateMigrationWithRetry, which would have created a
shared/db.tssessions/migrate.ts cycle. The transaction runner is passed in
as a plain function parameter from the composition root instead. Verified: none of
sessions/migrate.ts, work-items/migrate.ts, activity/migrate.ts imports
shared/db.js or sessions/registry.js.

Deliberately not done (the constraint budget forbade it): no registration or
plugin framework, no registerMigration() indirection, no renamed public functions,
no new dependencies or config options. shared/db.ts calls the three owners directly.

Behaviour: pure code motion plus import re-pointing. No SQL text changed, no
migration-order changed — the initialize transaction runs the same steps in the
same sequence inside one runImmediateMigrationWithRetry call.

Acceptance greps at the pushed head

registry importers of initDb/sanitizers ....... NONE
registry declares or re-exports initDb ........ NONE  (declared only in shared/db.ts:136)
migrateWorkItemsSchema/migrateActivitySchema .. NONE  (in registry.ts)
Todo-doctrine prose in registry.ts ............ NONE
migrate.ts files importing db/registry ........ NONE  (no module-scope cycle)

Test results

Turbo replays cached logs across worktrees, so every gate below was run with the
cache bypassed (Cached: 0 cached) — these are real executions at the pushed head,
after the final commit.

pnpm typecheck --force

 Tasks:    2 successful, 2 total
Cached:    0 cached, 2 total
  Time:    4.433s

pnpm test --force

jinn-cli:test:  Test Files  309 passed (309)
jinn-cli:test:       Tests  3828 passed | 1 skipped (3829)
@jinn/web:test:  Test Files  122 passed (122)
@jinn/web:test:       Tests  1275 passed (1275)
 Tasks:    2 successful, 2 total
Cached:    0 cached, 2 total

pnpm build — the root script is turbo build && node scripts/sync-web-dist.mjs;
appending --force to pnpm build passes the flag to the second command, so the
chain was expanded to force the build itself:

 Tasks:    2 successful, 2 total
Cached:    0 cached, 2 total
  Time:    4.243s

===SYNC-WEB-DIST===
synced packages/web/out -> packages/jinn/dist/web

No test was deleted; tests were only re-pointed at the new import paths.


Todo PLA-51. Base 23df25ed62acfa162a39923638eb14dbbf1d8bea, head
491e160e137bd6e759b69c3358658c354db76b26. For operator review — not merged.

The process-wide SQLite bootstrap lived inside sessions/registry.ts, so
every non-session module reached its database handle through Sessions.

Pure code motion, no behaviour change:
- shared/db.ts owns the connection, upgrade safety and initDb(). It is a
  composition root: it sequences each module's migrations in the exact
  order the old initialize transaction used, and holds no DDL of its own.
- sessions/migrate.ts owns the Sessions DDL, its idempotent migrations and
  the callback_deliveries row model those migrations validate against.
- shared/sanitize.ts owns stripControlChars/hasControlBytes. Being
  dependency-free, it also retires the duplicate hasControlBytes that
  mcp/knowledge-tools.ts kept only to avoid importing better-sqlite3.
- Every initDb importer now imports from shared/db.js; registry.ts is one
  of them. The Todo-doctrine prose block is gone.

The busy-retry helpers stay with the migrations rather than being injected
into migrateCallbackDeliveriesSchema, which keeps every moved signature and
body unchanged and leaves no module-scope cycle among the migrate modules.

Verified: initDb() against a fresh throwaway home produces a byte-identical
sqlite_master dump at base and at HEAD.
Three test files landed on main after this branch's base and still reach
initDb()/__closeDbForTest() through sessions/registry.js, which no longer
re-exports them. git merges them clean, tsc does not.

- gateway/__tests__/budgets.test.ts (#116) — 2 call sites
- gateway/__tests__/dispatch-route.test.ts (Todo Dispatcher) — 1 call site
- sessions/__tests__/activity-schema-drop.test.ts (#112) — 2 call sites

Import-path only: every call site just opens and seeds the database, with
no mocking or spying on the registry module, so the behaviour under test is
unchanged.

Also carries #112's dropActivityLedgerSchema() into shared/db.ts, the new
home of the boot migration transaction it runs inside.
@hristo2612
hristo2612 force-pushed the simplify/PLA-51-storage-ownership branch from 491e160 to fe358e3 Compare August 3, 2026 09:36
@hristo2612
hristo2612 merged commit ebaac28 into main Aug 3, 2026
4 checks passed
hristo2612 added a commit that referenced this pull request Aug 3, 2026
#113 relocated stripControlChars/hasControlBytes out of sessions/registry.ts
into shared/sanitize.ts and left no re-export, so the absorbed knowledge code
in notes/store.ts must import from the new owner. Also retarget the stale
knowledge/store.ts reference in the mcp/knowledge-tools.ts header.
hristo2612 added a commit that referenced this pull request Aug 3, 2026
knowledge/store.ts is deleted and its behaviour absorbed into notes/store.ts, leaving one owner for reads under knowledge/ and docs/.

Path handling is not loosened. readKnowledgeFile keeps the whole battery: 300-char cap, control-byte reject rather than strip, POSIX and Win32 absolute-path reject, backslash reject, empty/dot/dotdot segment reject, realpath-into-realpath-home containment, isFile. The containment predicate moved to the Notes-side isRealpathContained, which differs from the old startsWith form only when candidate equals root, and that case is refused explicitly. Search is strictly stricter than before: walkMarkdown skips symlinks via lstat and openRegularFile uses O_NOFOLLOW, replacing a realpath-follows-then-check.

One behaviour change the original description did not declare, verified by running both implementations against the same fixture: the old search gated filenames through a charset regex, and the new walker does not. Files with spaces, non-ASCII names, or names longer than 250 characters are now searchable. This is a widening inside the same two allowlisted roots, those files were already reachable through list_notes and read_note via the same walker, and it arrives paired with the stronger symlink regime, so it is acceptable. It is stated here so it is a decision rather than a side effect. One cosmetic consequence: a deeply nested hit path can now exceed readKnowledgeFile's 300-char cap, making that hit un-readable.

Follow-up commit retargets stripControlChars and hasControlBytes to shared/sanitize.ts, where #113 moved them without leaving a re-export.

The packages/web suite was not green during gating, but bare main fails at the same rate with a barely-overlapping set of waitFor timeouts, and this PR changes zero web files. Tracked as #123.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant