From a3f942812edf78b1253b1ced47ea77e6a26d850b Mon Sep 17 00:00:00 2001 From: mrsibe Date: Fri, 25 Sep 2026 20:16:17 +0800 Subject: [PATCH 1/2] docs(arch): record the seams v1.4 actually produced, and close the one reverse dependency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/architecture.md | 94 +++++++++++++++++++++++ src/main/index.ts | 8 +- src/main/protocol/documentProtocol.ts | 20 ++--- src/main/services/KnowledgeService.ts | 20 +++++ src/main/smokeTest.ts | 8 +- test/architectureBoundary.test.ts | 104 ++++++++++++++++++++++++++ 6 files changed, 239 insertions(+), 15 deletions(-) create mode 100644 test/architectureBoundary.test.ts diff --git a/docs/architecture.md b/docs/architecture.md index f8a4b01..d3771cd 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -4,6 +4,79 @@ Long-lived decisions that outlive a single feature. Feature-specific design live in the issue tracker and in `PRODUCT.md` / `DESIGN.md`; this file records the seams that other work is allowed to depend on. +## Module seams + +Three seams carry the v1.4 features. These are the names that **actually exist in the +code** — #60 guessed `DocumentParser` / `SourceStore` / `CitationService`, and none of +the three was needed. See [considered and rejected](#seams-considered-and-rejected) +for why, so the next reader does not re-litigate it. + +### Document + +| Role | What it is | +| ----------------------------- | ------------------------------------------------------------------------------ | +| Parsing contract | `IDocumentLoader` — `src/main/services/loaders/types.ts` | +| Format dispatch | `FileParserService` — picks a loader by extension / MIME | +| Implementations | `PdfLoader`, `DocxLoader`, `MarkdownLoader`, `PptLoader`, `WebLoader` | +| Persistence and orchestration | `KnowledgeService` — `documents`, `document_blocks`, chunks, vectors, re-index | +| Derived structure | `services/blocks/documentBlocks.ts`, `ChunkingService`, `chunkProvenance.ts` | + +Data invariants for this seam are in [Source provenance](#source-provenance). + +### Retrieval + +`Retriever` (interface) and `RetrievedEvidence` (result) in +`src/main/services/retrieval/`; `DenseRetriever` is the only implementation today. +Rules are in [Retrieval seam](#retrieval-seam). + +### Application + +**This seam is an architectural boundary, not a class.** There is deliberately no +`ApplicationService` and no `CitationService`: + +- **IPC handlers** (`src/main/ipc/`) translate a request into a service call and + translate the answer back. No handler calls `getDatabase()` itself. +- **Application services** — `ItemService`, `AnkiCardService`, `MindMapService`, + `QuizService` — own their tables and may call `getDatabase()`. +- **Shared pure functions** — `src/shared/utils/`: `citations.ts`, + `citationResolution.ts`, `answerSources.ts`, `sourceAnchor.ts`, `excerpt.ts` — hold + citation parsing/resolution and source-anchor mapping, because the main _and_ the + renderer process both need them and they carry no state. + +**Known legacy exception, not addressed here.** `noteHandlers.ts`, +`notebookHandlers.ts` and part of `chatHandlers.ts` call `db/queries` directly instead +of a service. That predates v1.4 and #60 deliberately leaves it alone: its non-goals +forbid removing a legacy path before a real feature exercises the replacement. + +### Call paths + +Concrete, checkable against the code: + +```text +src/main/ipc/itemHandlers.ts + → ItemService (Application) + → getDatabase() + → Drizzle / SQLite + +src/main/ipc/knowledgeHandlers.ts + → KnowledgeService (Document) + → getDatabase() + → Drizzle / SQLite + +knownote-doc:// +src/main/protocol/documentProtocol.ts + → KnowledgeService.getDocumentLocalFilePath(id) (Document, narrow query) + → getDatabase() + → documents.localFilePath +``` + +The protocol path is the one #60 changed. It used to run its own +`select().from(documents)` (introduced by #116), which made an Electron plumbing module +depend on the database from outside the Document/Application layers. It now receives +the narrow query as a parameter, and `test/architectureBoundary.test.ts` fails if +database access reappears under `src/main/protocol/`. The narrower query is deliberate: +the handler needs “a readable file for this id”, not a whole `Document` row. + ## Source provenance A document is an identity. Everything derived from it can be rebuilt: @@ -133,3 +206,24 @@ Rules: - **The text layer is real text.** pdfjs's `TextLayer` is rendered over the canvas so `getSelection()` can map a DOM selection back to a source location; selecting the canvas would give nothing. + +## Seams considered and rejected + +Recorded so they are not proposed again without a new reason. Each was considered +while building v1.4 and deliberately not introduced. + +- **`DocumentParser` — rejected.** `IDocumentLoader` plus `FileParserService` already + form the parsing seam, and all five format loaders implement it. A second interface + over the same boundary would be duplicate abstraction with no new consumer. +- **`SourceStore` — currently rejected.** There is no storage consumer independent + enough to justify a repository layer: the provenance helpers + (`chunkProvenance.ts`) take the database handle as an explicit parameter, and + `KnowledgeService` still owns Document persistence orchestration. Revisit only if a + second, genuinely independent storage consumer appears. +- **`CitationService` — rejected.** Citation parsing, resolution and source-anchor + mapping are stateless and shared between the main and renderer processes. Pure + functions in `src/shared/utils/` are the right shape; a service would add a + process-boundary round trip and a lifecycle for something that has no state. +- **A Model seam — not part of this work.** #44 already established it + (`ModelClient` + `protocols/*`, driven by `ConnectionManager`); it is not rebuilt or + re-documented here. diff --git a/src/main/index.ts b/src/main/index.ts index 3851a05..9ccdba3 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -77,7 +77,13 @@ app.whenReady().then(async () => { initDatabase() runMigrations() initVectorStore() - registerDocumentProtocolHandler() + // The resolver is lazy on purpose: `knowledgeService` is constructed further down, and + // the protocol only ever answers requests from a renderer that loads after startup. + // Reading it per request keeps the dependency direction protocol → Document layer → + // database without reordering startup (#60). + registerDocumentProtocolHandler( + (documentId) => knowledgeService?.getDocumentLocalFilePath(documentId) ?? null + ) Logger.info('Main', 'Database initialized') // Initialize electron-store diff --git a/src/main/protocol/documentProtocol.ts b/src/main/protocol/documentProtocol.ts index 30ba360..667779f 100644 --- a/src/main/protocol/documentProtocol.ts +++ b/src/main/protocol/documentProtocol.ts @@ -9,9 +9,6 @@ import { net, protocol } from 'electron' import { pathToFileURL } from 'url' -import { eq } from 'drizzle-orm' -import { getDatabase } from '../db' -import { documents } from '../db/schema' import { DOCUMENT_SCHEME, parseDocumentUrl } from '../../shared/utils/documentUrl' import Logger from '../../shared/utils/logger' @@ -45,23 +42,22 @@ export function registerDocumentScheme(): void { } /** 在 app ready 之后调用，安装只读的字节服务 handler。 */ -export function registerDocumentProtocolHandler(): void { +export function registerDocumentProtocolHandler( + resolveLocalFilePath: (documentId: string) => string | null +): void { protocol.handle(DOCUMENT_SCHEME, async (request) => { const documentId = parseDocumentUrl(request.url) if (!documentId) return new Response('Not found', { status: 404 }) - const document = getDatabase() - .select({ localFilePath: documents.localFilePath }) - .from(documents) - .where(eq(documents.id, documentId)) - .get() - - if (!document?.localFilePath) return new Response('Not found', { status: 404 }) + // 路径来自 Document 层，不来自这里，也不来自请求。handler 自己不碰数据库： + // 它只需要一个「可读文件」的答案，依赖方向保持 protocol → Document 层 → database（#60）。 + const localFilePath = resolveLocalFilePath(documentId) + if (!localFilePath) return new Response('Not found', { status: 404 }) try { // net.fetch handles file:// in the main process and streams the response, so // a large PDF is not read into memory to be served. - const response = await net.fetch(pathToFileURL(document.localFilePath).toString()) + const response = await net.fetch(pathToFileURL(localFilePath).toString()) const headers = new Headers(response.headers) const origin = request.headers.get('Origin') ?? 'null' if (ALLOWED_ORIGINS.has(origin)) headers.set('Access-Control-Allow-Origin', origin) diff --git a/src/main/services/KnowledgeService.ts b/src/main/services/KnowledgeService.ts index f6a60bf..d3d8692 100644 --- a/src/main/services/KnowledgeService.ts +++ b/src/main/services/KnowledgeService.ts @@ -815,6 +815,26 @@ export class KnowledgeService { return db.select().from(documents).where(eq(documents.id, documentId)).get() } + /** + * 只取一份来源的可读文件路径（#60）。 + * + * 刻意做成一个窄查询，而不是让 `knownote-doc://` 的协议 handler 调 `getDocument()`： + * 那个 handler 是 Electron plumbing，它只需要「按 id 找一个能读的文件」，把整行 + * `Document` 交给它会把依赖面撑得比实际需要宽。 + * + * 存在的意义是固定依赖方向：protocol → Document 层 → database，而不是 protocol 自己 + * 去 `getDatabase()`。不用为它另建 SourceStore 之类的 repository —— 一个方法就够。 + */ + getDocumentLocalFilePath(documentId: string): string | null { + const db = getDatabase() + const row = db + .select({ localFilePath: documents.localFilePath }) + .from(documents) + .where(eq(documents.id, documentId)) + .get() + return row?.localFilePath ?? null + } + /** * 获取文档的所有 chunks */ diff --git a/src/main/smokeTest.ts b/src/main/smokeTest.ts index 0e3131b..2f24cc4 100644 --- a/src/main/smokeTest.ts +++ b/src/main/smokeTest.ts @@ -247,7 +247,12 @@ async function runChecks(): Promise { Date.now() ) - registerDocumentProtocolHandler() + // Hoisted from the re-index section below so the protocol handler is wired to the + // real Document-layer query rather than a stand-in: this check is what proves + // `knownote-doc://` serves bytes *through* that path, and that it 404s when the query + // answers null (#60). + const knowledge = new KnowledgeService(fakeEmbeddingService()) + registerDocumentProtocolHandler((documentId) => knowledge.getDocumentLocalFilePath(documentId)) const served = await net.fetch(documentUrl(protocolDocumentId)) assert(served.ok, `an owned document did not serve (status ${served.status})`) assert((await served.text()) === protocolPayload, 'the document protocol served the wrong bytes') @@ -551,7 +556,6 @@ async function runChecks(): Promise { .prepare('INSERT INTO notebooks (id, title, created_at, updated_at) VALUES (?, ?, ?, ?)') .run(reindexNotebook, 'Smoke re-index notebook', reindexNow, reindexNow) - const knowledge = new KnowledgeService(fakeEmbeddingService()) const reindexDocId = await knowledge.addDocumentFromFile( reindexNotebook, join(fixtures, 'sample.pdf') diff --git a/test/architectureBoundary.test.ts b/test/architectureBoundary.test.ts new file mode 100644 index 0000000..1263a45 --- /dev/null +++ b/test/architectureBoundary.test.ts @@ -0,0 +1,104 @@ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { readFileSync, readdirSync, statSync } from 'node:fs' +import { join } from 'node:path' + +/** + * 守住 Electron plumbing → Document 层的依赖方向（#60）。 + * + * #60 的第三条 acceptance 不是文字要求，而是在约束依赖方向： + * + * no new service reaches directly into getDatabase() from outside the + * Document/Application layers + * + * `src/main/protocol/` 是 Electron plumbing —— 它既不是 Application 也不是 Document + * 层。`knownote-doc://` 曾经自己 `select().from(documents)` 取 `localFilePath`，是 v1.4 + * （#116）新增代码里唯一一处这类反向依赖。现在它改为接收一个窄查询 + * （`KnowledgeService.getDocumentLocalFilePath()`），方向变成： + * + * protocol → Document 层 → getDatabase() → Drizzle/SQLite + * + * 这个测试把那条方向固定下来：谁在 protocol 层重新引入数据库访问，CI 会在这里失败。 + * 失败时该怎么做：把需要的查询加到 Document 层并注入进来，不要只是把测试改绿。 + */ + +/** Electron plumbing：这里的模块只负责把请求翻译成对服务层的调用。 */ +const PLUMBING_DIR = join('src', 'main', 'protocol') + +const sourceFiles = (dir: string, files: string[] = []): string[] => { + for (const entry of readdirSync(dir)) { + const path = join(dir, entry) + if (statSync(path).isDirectory()) sourceFiles(path, files) + else if (/\.(ts|tsx|mts|cts)$/.test(path)) files.push(path) + } + return files +} + +const importSpecifiers = (source: string): string[] => { + const pattern = /(?:from\s*|import\s*\(\s*|require\s*\(\s*|import\s+)['"]([^'"\n]+)['"]/g + return [...source.matchAll(pattern)].map((match) => match[1]) +} + +/** 数据库层的入口：直接拿连接、Drizzle，或 Drizzle 的表定义。 */ +const isDatabaseAccess = (specifier: string): boolean => + specifier === 'drizzle-orm' || + specifier.startsWith('drizzle-orm/') || + /(^|\/)db$/.test(specifier) || + specifier.endsWith('/db/schema') || + specifier.endsWith('/db') + +test('the protocol layer does not reach into the database', () => { + const offenders: string[] = [] + + for (const file of sourceFiles(PLUMBING_DIR)) { + const source = readFileSync(file, 'utf8') + + for (const specifier of importSpecifiers(source)) { + if (isDatabaseAccess(specifier)) offenders.push(`${file} imports ${specifier}`) + } + + // Import form is not the only way in: a lazy `require('../../db')` or a direct + // call would be missed by the specifier scan, so check the call too. + if (/\bgetDatabase\s*\(/.test(source)) offenders.push(`${file} calls getDatabase()`) + } + + assert.deepEqual( + offenders, + [], + 'The protocol layer is Electron plumbing and must not query the database directly ' + + '(#60, acceptance 3). Add a narrow query to the Document layer ' + + '(KnowledgeService) and inject it via registerDocumentProtocolHandler() instead of ' + + 'reaching for getDatabase() here.' + ) +}) + +test('the protocol layer gets its data through an injected resolver', () => { + // The invariant is only meaningful if the handler really takes its answer from the + // caller: a module that had both the db import *and* the injection would pass the test + // above only until someone used the import. + const source = readFileSync(join(PLUMBING_DIR, 'documentProtocol.ts'), 'utf8') + + assert.match( + source, + /resolveLocalFilePath\s*:\s*\(/, + 'registerDocumentProtocolHandler must take the document-path resolver as a parameter' + ) +}) + +test('the guard actually scans the protocol layer', () => { + // A boundary test that silently scans nothing is worse than no test: it would report + // the direction as enforced while reading no files. + const files = sourceFiles(PLUMBING_DIR) + + assert.ok(files.length > 0, `expected to scan ${PLUMBING_DIR}, got ${files.length} files`) + assert.ok( + files.some((file) => file.endsWith('documentProtocol.ts')), + 'the document protocol handler must be inside the scanned set' + ) + + // And the extraction really finds imports. + assert.ok( + importSpecifiers(readFileSync(files[0], 'utf8')).includes('electron'), + 'import extraction found no specifiers in the protocol layer' + ) +}) From 618df19acbea2482d2ad1dc9f894c46df2592e4a Mon Sep 17 00:00:00 2001 From: mrsibe Date: Fri, 25 Sep 2026 20:26:16 +0800 Subject: [PATCH 2/2] 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. --- test/architectureBoundary.test.ts | 45 ++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/test/architectureBoundary.test.ts b/test/architectureBoundary.test.ts index 1263a45..3b96d49 100644 --- a/test/architectureBoundary.test.ts +++ b/test/architectureBoundary.test.ts @@ -39,13 +39,50 @@ const importSpecifiers = (source: string): string[] => { return [...source.matchAll(pattern)].map((match) => match[1]) } -/** 数据库层的入口：直接拿连接、Drizzle，或 Drizzle 的表定义。 */ +/** + * 数据库层的入口：直接拿连接、Drizzle，或 **`db` 目录下的任何东西**。 + * + * 这里必须把整个 db 子树当作边界，而不是只列 `db` 和 `db/schema`。只匹配目录名本身 + * 或固定后缀会让 `../db/queries` 漏过去 —— 而 `db/queries` 恰好就是 + * `docs/architecture.md` 里记为「数据库直达」的那条 legacy 路径。一个漏掉自己文档里 + * 点名情况的 guard，比没有这条规则更糟：它会让人相信一项未被执行的约束。 + */ const isDatabaseAccess = (specifier: string): boolean => specifier === 'drizzle-orm' || specifier.startsWith('drizzle-orm/') || - /(^|\/)db$/.test(specifier) || - specifier.endsWith('/db/schema') || - specifier.endsWith('/db') + /(^|\/)db(?:\/|$)/.test(specifier) + +test('the database matcher covers the whole db subtree, not just its exact name', () => { + // The false negative this pins: `../db/queries` used to pass the scan even though + // architecture.md records `db/queries` as a database-reaching legacy path. Anything + // under the db directory is database access. + for (const specifier of [ + '../db', + '../db/schema', + '../db/queries', + '../../db/foo', + 'db', + 'db/queries', + 'drizzle-orm', + 'drizzle-orm/sqlite-core' + ]) { + assert.equal(isDatabaseAccess(specifier), true, `should be flagged: ${specifier}`) + } + + // It must not swallow legitimate imports either: a guard that fires on ordinary code + // is one people route around, which is worse than a narrow one. + for (const specifier of [ + '../services/KnowledgeService', + '../../shared/utils/documentUrl', + '../../shared/utils/logger', + 'electron', + 'url', + 'db-utils', + '@shared/dbx' + ]) { + assert.equal(isDatabaseAccess(specifier), false, `should not be flagged: ${specifier}`) + } +}) test('the protocol layer does not reach into the database', () => { const offenders: string[] = []