Repository navigation
refactor(models): replace provider registry with protocol-based model connections - #44
Merged
Merged
Conversation
… connections KnowNote no longer knows any vendor. A model is now a connection: API Protocol + Base URL + API Key + Model ID, with the capability (chat / embedding) declared explicitly by the user instead of guessed. Main process: - add src/main/models/: protocol adapters for openai-completions, openai-responses, anthropic-messages and google-generative-ai, plus ModelClient (chat streaming + embeddings) and ConnectionManager (resolves chat/embedding connections, tests them, lists models). - persist connections in electron-store with the apiKey encrypted via safeStorage (ConnectionConfigManager). - one-time migration from the legacy providers store; the pure mapping lives in legacyConnectionMapping.ts and reports warnings instead of silently degrading when a config cannot be mapped. - delete src/main/providers, the builtin provider registry, the builtin model catalog, modelClassifier and the provider IPC handlers. Renderer: - Settings shows "Chat Model" and "Embedding Model" forms (Protocol / Base URL / API Key / Model ID + Test connection + Fetch models) instead of a provider list; quick setups only prefill the form. - GeneralSettings drops the default-model pickers; ProcessPanel and SourcePanel gate on whether a chat/embedding connection exists. Tooling: - add a Node built-in test suite (no new runtime dependency) covering the migration for the 7 legacy builtin providers, plus a typecheck project for test/**.
- README / README_CN: replace "supported providers" wording with "any OpenAI / Anthropic / Google-compatible endpoint, or a local deployment", and point the tree at src/main/models. - CONTRIBUTING: document `npm test`, the new test layout, and update the lint baseline after the provider modules were removed. - verify workflow: run `npm test` alongside typecheck.
`npm run lint` now exits 0 (was 4 errors). The remaining 118 warnings are the pre-existing @typescript-eslint/no-explicit-any baseline. - RenameDialog / SourcePanel / ItemList / use-mobile: replace the setState-in-effect pattern with a render-time state adjustment (React's documented alternative), so no cascading render. - QuizResultView: submit the quiz session once via a ref guard and give the effect its real dependencies. - ResizableLayout: drop an eslint-disable directive that no longer suppresses anything. - FileParserService: apply the one prettier fix it needed. - CONTRIBUTING / verify.yml: update the lint baseline notes.
This was referenced Sep 24, 2026
Closed
mrsibe
added a commit
that referenced
this pull request
Sep 24, 2026
`impeccable context` reports NO_PRODUCT_MD / BUILD_INIT_REQUIRED: DESIGN.md records the visual system, but nothing records product truth, so any new-surface or redesign request is blocked until it exists. The maintainer asked for this to be written directly rather than through the init interview, so nothing in it is an interview answer: - Facts are sourced to the artifact that states them (README, #82, #65, #44, DESIGN.md, CONTRIBUTING.md, package.json). - Everything read out of the code rather than stated is marked `[inferred]`, with a provenance note at the top of the file saying so. - Two contradictions are recorded as open questions instead of being resolved by guessing: the README calls quiz/transcription/slide generation unreleased while quiz and Anki surfaces are implemented, and no accessibility standard is named anywhere. - No image generation is available in this tool surface, so init records no `buildPath` preference. That is a working state, not a gap. The positioning section records what #82 establishes: a citation resolves to a source location and must survive re-chunking, which is why source blocks are persisted and chunks reference them. It also carries the maintainer's own correction — the README's traceability claim ran ahead of the code — as a standing commitment rather than a footnote.
7 tasks done
mrsibe
added a commit
that referenced
this pull request
Sep 24, 2026
`impeccable context` reports NO_PRODUCT_MD / BUILD_INIT_REQUIRED: DESIGN.md records the visual system, but nothing records product truth, so any new-surface or redesign request is blocked until it exists. The maintainer asked for this to be written directly rather than through the init interview, so nothing in it is an interview answer: - Facts are sourced to the artifact that states them (README, #82, #65, #44, DESIGN.md, CONTRIBUTING.md, package.json). - Everything read out of the code rather than stated is marked `[inferred]`, with a provenance note at the top of the file saying so. - Two contradictions are recorded as open questions instead of being resolved by guessing: the README calls quiz/transcription/slide generation unreleased while quiz and Anki surfaces are implemented, and no accessibility standard is named anywhere. - No image generation is available in this tool surface, so init records no `buildPath` preference. That is a working state, not a gap. The positioning section records what #82 establishes: a citation resolves to a source location and must survive re-chunking, which is why source blocks are persisted and chunks reference them. It also carries the maintainer's own correction — the README's traceability claim ran ahead of the code — as a standing commitment rather than a footnote.
mrsibe
added a commit
that referenced
this pull request
Sep 25, 2026
…e reverse dependency (#126) * docs(arch): record the seams v1.4 actually produced, and close the one reverse dependency Closes #60. This was an acceptance audit before it was anything else: after #66–#73 the seams the issue planned had mostly already formed, and two of its four acceptance items were satisfied. The work is therefore a documentation update plus **one narrow code fix** — not the architecture refactor the title suggests. **Acceptance outcome** | Item | Before | | --- | --- | | `architecture.md` names the three seams, their implementations, and the IPC → service → db call path | **Partial** — the Retrieval seam was documented, the Document and Application seams were never named, and no call path appeared anywhere | | Every v1.4 module sits behind one of the seams | **Met**, now recorded | | No new service reaches `getDatabase()` from outside the Document/Application layers | **One instance** — fixed below | | The document lists the seams considered and rejected | **Not met** — added | **The seams are named as they exist, not as the issue guessed.** #60 proposed `DocumentParser`, `SourceStore` and `CitationService`. None of the three exists, and none was needed: - parsing is `IDocumentLoader` (already an interface, five implementations) plus `FileParserService` for dispatch, so `DocumentParser` would be duplicate abstraction; - there is no storage consumer independent enough for `SourceStore` — the provenance helpers take the database handle explicitly and `KnowledgeService` owns Document persistence orchestration; - `CitationService` was never needed because citation parsing, resolution and source-anchor mapping are stateless and shared by main *and* renderer; pure functions in `src/shared/utils/` are the right shape. Application is documented as an **architectural boundary, not a class**, so nobody goes looking for the `ApplicationService` that this document would otherwise imply. **The one code fix.** `src/main/protocol/documentProtocol.ts` (added by #116) queried `documents.localFilePath` itself, which made an Electron plumbing module depend on the database from outside both permitted layers — the only such instance in v1.4 code. Writing it off as a "known exception" would mean accepting a fresh architectural exception at the moment this issue closes, and the fix is cheap, so it is fixed: - `KnowledgeService.getDocumentLocalFilePath(documentId): string | null` — a narrow query rather than reusing `getDocument()`. The handler needs "a readable file for this id", not a whole `Document` row, so handing it the row would widen the dependency surface for nothing. No `SourceStore`-style repository was invented for it; it is one method on the existing Document seam. - `registerDocumentProtocolHandler()` now takes the resolver, so the protocol layer imports no database at all. The dependency direction is now `protocol → Document layer → getDatabase() → Drizzle/SQLite`. - `src/main/index.ts` and the packaged smoke test both inject it. The main-process wiring reads `knowledgeService` lazily because it is constructed later in startup; that is noted at the call site rather than reordering startup for it. **A guard, not a comment.** `test/architectureBoundary.test.ts` fails if any file under `src/main/protocol/` imports the database, Drizzle, or the schema, or calls `getDatabase()` — with a self-check so it cannot pass by scanning nothing. Verified by injecting the import back: the guard reports `documentProtocol.ts imports ../db` and fails. **Accuracy note recorded in the document.** Three handlers (`noteHandlers`, `notebookHandlers`, part of `chatHandlers`) still call `db/queries` instead of a service. That predates v1.4 and #60's non-goals forbid removing a legacy path before a real feature exercises the replacement, so it is documented as a known legacy exception rather than silently implied to be clean. No handler calls `getDatabase()` itself. Verification: `npm run typecheck` (all three projects), `npm test` (261 pass, 3 new for the boundary guard), `npm run check:design`, `npm run lint` (0 errors, baseline unchanged), `npx prettier --check`, and `npm run build:unpack && npm run smoke:packaged` **executed** — 20/20 checks, including `knownote-doc:// serves an owned document and 404s an unknown id` through the newly injected resolver, which is the regression that matters for this change. Not touched, per the issue's non-goals: `KnowledgeService`'s structure, `SourceStore`, a `CitationService`, directory layout, the Model seam (#44 owns it), and the legacy `db/queries` handlers. * test(arch): match the whole db subtree in the protocol boundary guard The matcher only caught an exact `../db` plus the `/db` and `/db/schema` suffixes, so `../db/queries` passed the scan - even though `docs/architecture.md` records `db/queries` as exactly the kind of database-reaching path this guard exists to catch. A guard that misses a case its own documentation names is worse than no guard: it reports an invariant as enforced while leaving it open. `/(^|\/)db(?:\/|$)/` treats the whole db subtree as the boundary, covering `../db`, `../db/schema`, `../db/queries` and `../../db/foo`. It still does not fire on `db-utils` or `@shared/dbx`, and a table-driven test pins both directions so the rule cannot quietly become either leaky or noisy. Verified by injecting the exact miss back: `import { getNotesByNotebook } from '../db/queries'` in documentProtocol.ts now fails with `documentProtocol.ts imports ../db/queries`, and passes again once restored. The seam work itself is unchanged: the narrow KnowledgeService query, the injected resolver and the architecture document are as reviewed.
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
1 participant
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.
What does this PR do?
Replaces the "global model provider database" with a vendor-agnostic Model Connection model:
The capability (
chat/embedding) is now declared explicitly by the user, so the code no longer guesses whether a model is a chat or embedding model — and there is nounknownstate anymore.Concretely, KnowNote no longer knows any vendor. A compatible service is just an OpenAI-compatible endpoint. Reachable protocols:
openai-completions(OpenAI, DeepSeek, Qwen, Kimi, SiliconFlow, 智谱, Ollama, LM Studio, vLLM, SGLang, OpenRouter, …)openai-responsesanthropic-messagesgoogle-generative-aiWhy?
The provider registry + per-vendor model catalogs + keyword-based model classifier had become the heaviest subsystem in the repo, while adding nothing for a local-first / BYO-API tool. The classification path also caused real bugs: #30 (
gemma3:12bmisclassified asunknown, with no manual override), and #14 / #31 / #33 / #35 all happened in the ambiguous "which provider does the system think it is talking to" zone.After this change, the 7 builtin provider implementations and their 62 hardcoded model entries are gone.
Related issue
Fixes #42
What changed?
src/main/models/protocols/): one adapter per API protocol; business code dispatches onprotocol, not on a vendor name.ModelClient(src/main/models/ModelClient.ts): chat streaming + embeddings on top of an adapter. Replaces the provider-name-branchingAISDKProvider.ConnectionManager(src/main/models/ConnectionManager.ts): resolves the chat/embedding connections, tests them, and lists available models.ConnectionConfigManager): stored inelectron-storeunderconnections,apiKeystill encrypted viasafeStorage.providersstore. The pure mapping lives inlegacyConnectionMapping.ts(Electron-free, unit tested) and reports explicit warnings instead of silently degrading when a config cannot be mapped.GeneralSettingsdrops the default-model selectors;ProcessPanel/SourcePanelnow gate on whether a chat/embedding connection exists.src/main/providers/,src/shared/config/models/,src/shared/utils/modelClassifier.ts, theProviderRegistry/ProviderDescriptorlayer, the 4 provider Settings components, and the now-unused@ai-sdk/alibaba,@ai-sdk/deepseek,ollama-ai-provider-v2dependencies. Added@ai-sdk/anthropic,@ai-sdk/google(devDependencies, bundled).npm test, no new test framework dependency) and atypecheck:testproject.npm testruns in the verify workflow.How was this tested?
The migration test covers the typical legacy configuration of all 7 builtin providers, plus: model-id preservation for a user-declared chat model (
gemma3:12b), Ollama/api→/v1normalization, fallback to selected models when no default model was set, warning instead of a fabricated connection, and injected decryption.Screenshots / recordings
Not attached — this environment is headless, so the Settings UI was verified via typecheck/build only. The two new forms are in
ModelConnectionForm/ModelsSettings; visual review of that screen is the main thing worth a human look.Checklist
npm run typecheckpasses.npm run buildpasses.Desktop / build changes
npm run build:unpackpasses.npm run smoke:packagedpasses.