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..3b96d49 --- /dev/null +++ b/test/architectureBoundary.test.ts @@ -0,0 +1,141 @@ +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,或 **`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) + +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[] = [] + + 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' + ) +})