Repository navigation
Refactor core to DB-injected services with runtime bootstrap - #6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors @stash/core to use DB-injected feature/service functions and introduces a runtime bootstrap (createCoreRuntime) that owns migrations + DB lifecycle, then updates the CLI and API app composition to use that runtime-managed lifecycle.
Changes:
- Replace
OperationContext/per-call DB bootstrapping with injectedDb/StashDbacross core feature modules andcreateCoreServices. - Add
createCoreRuntime({ dbPath, migrationsDir })and wire API server + CLI commands to create/close a runtime deterministically. - Update tests and docs to match the new wiring and local “pre-PR gate” guidance.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/test/tts-jobs.test.ts | Update tests to seed and exercise TTS job logic using an injected StashDb. |
| packages/core/test/core-services.test.ts | Update service-factory delegation test to assert calls are bound to db instead of OperationContext. |
| packages/core/test/auto-tags.test.ts | Remove OperationContext usage and replace with an inline { dbPath, migrationsDir } shape in tests. |
| packages/core/src/types.ts | Remove OperationContext type. |
| packages/core/src/services/index.ts | Export create-core-runtime from services barrel. |
| packages/core/src/services/create-core-services.ts | Change service factory to accept { db: StashDb } and bind feature calls to the injected DB. |
| packages/core/src/services/create-core-runtime.ts | Add runtime bootstrap that ensures DB dir, runs migrations, opens DB, exposes { services, close }. |
| packages/core/src/features/tts/service.ts | Refactor TTS generation to accept injected Db and remove withReadyDb usage. |
| packages/core/src/features/tts/jobs.ts | Refactor TTS job operations/worker to accept injected Db and remove withReadyDb usage. |
| packages/core/src/features/tags/service.ts | Refactor tags service functions to accept injected Db instead of OperationContext. |
| packages/core/src/features/status/service.ts | Refactor status functions to accept injected Db instead of OperationContext. |
| packages/core/src/features/items/service.ts | Refactor item save/list/get flows to operate directly on injected Db (no per-call open/close). |
| packages/core/src/features/extract/service.ts | Refactor extract flow to accept injected Db (no per-call open/close). |
| packages/core/src/features/common/db.ts | Remove DB bootstrap helpers (withReadyDb*, etc.); keep shared DB utilities and types. |
| package.json | Update check script to run staged local gates (format, lint, test, web build). |
| apps/cli/src/cli.ts | Switch CLI commands to use createCoreRuntime via withCoreRuntime(...) helper. |
| apps/api/test/integration.test.ts | Update integration test to create/close a core runtime and inject services into API app. |
| apps/api/src/features/options.ts | Remove old API route options type that included dbPath/migrationsDir. |
| apps/api/src/app/server.ts | Create one runtime per API server instance, start worker off runtime services, close on shutdown. |
| apps/api/src/app/create-api-app.ts | Change API app creation to accept injected CoreServices + audioDir (no internal core bootstrapping). |
| README.md | Update local code quality guidance to emphasize pnpm run check as pre-PR gate. |
| AGENTS.md | Add/extend contributor guidance, including preference for explicit named types. |
| .agents/notes.md | Record agent note reinforcing preference for explicit named types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| const result = action(runtime.services) | ||
| if (result instanceof Promise) { | ||
| return result.finally(() => { | ||
| runtime.close() | ||
| }) | ||
| } | ||
| runtime.close() | ||
| return result | ||
| } catch (error) { | ||
| runtime.close() |
There was a problem hiding this comment.
withCoreRuntime uses result instanceof Promise to decide whether to defer runtime.close(). This is unreliable for thenables / Promise-like values (and can also be false across realms), which can cause the runtime/DB to be closed before the async action completes. Consider always wrapping with Promise.resolve(action(...)) and closing in a single .finally(...) path (while still handling sync returns).
| try { | |
| const result = action(runtime.services) | |
| if (result instanceof Promise) { | |
| return result.finally(() => { | |
| runtime.close() | |
| }) | |
| } | |
| runtime.close() | |
| return result | |
| } catch (error) { | |
| runtime.close() | |
| let closed = false | |
| const closeRuntime = (): void => { | |
| if (!closed) { | |
| closed = true | |
| runtime.close() | |
| } | |
| } | |
| try { | |
| const result = action(runtime.services) | |
| const resultPromise = Promise.resolve(result).finally(() => { | |
| closeRuntime() | |
| }) | |
| if ( | |
| result !== null && | |
| (typeof result === "object" || typeof result === "function") && | |
| typeof (result as { then?: unknown }).then === "function" | |
| ) { | |
| // Thenable / Promise-like result: defer closing the runtime until it settles. | |
| return resultPromise | |
| } | |
| // Synchronous result: close immediately, as before, and return the value. | |
| closeRuntime() | |
| return result | |
| } catch (error) { | |
| closeRuntime() |
| ensureDbDirectory(options.dbPath) | ||
| runMigrations(options.dbPath, options.migrationsDir) | ||
|
|
||
| const { db, sqlite } = openDb(options.dbPath) |
There was a problem hiding this comment.
createCoreRuntime currently runs migrations via runMigrations(...) and then calls openDb(...), but runMigrations itself opens/closes its own DB connection. This means runtime startup still opens the SQLite database at least twice, which conflicts with the stated goal of "run migrations once, open DB once" and adds overhead/lock churn. Consider refactoring migrations to run against the already-opened sqlite/db instance so runtime can truly open the DB once.
| } catch (error) { | ||
| await server.close() | ||
| await ttsWorker.stop() | ||
| runtime.close() | ||
| throw error |
There was a problem hiding this comment.
In the failure path for listenApiServer, the cleanup sequence (await server.close(); await ttsWorker.stop(); runtime.close()) can short-circuit if server.close() throws, leaving the worker/runtime running and the SQLite connection open. Consider using nested try/finally (or Promise.allSettled) so ttsWorker.stop() and runtime.close() are always attempted regardless of which close step fails.
| close: async () => { | ||
| await ttsWorker.stop() | ||
| await server.close() | ||
| runtime.close() | ||
| }, |
There was a problem hiding this comment.
The returned close function awaits ttsWorker.stop() and server.close() before calling runtime.close(), but if either awaited call throws, runtime.close() won't run and the SQLite connection may remain open. Consider wrapping shutdown steps in try/finally to guarantee runtime.close() (and ideally attempt both worker+server shutdown even if one fails).
Summary
Notable updates
Validation