Repository navigation
docs(arch): record the seams v1.4 actually produced, and close the one reverse dependency - #126
Merged
Merged
Conversation
…e 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.
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
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.
Closes #60.
This turned out to be an acceptance audit first: after #66–#73 the seams the issue planned had mostly already formed, so two of its four acceptance items were satisfied and the work is a documentation update plus one narrow code fix — not the architecture refactor the title suggests.
architecture.mdnames the three seams, their implementations, and the IPC → service → db call pathgetDatabase()from outside the Document/Application layersThe seams are named as they exist, not as the issue guessed
#60 proposed
DocumentParser,SourceStoreandCitationService. None of the three exists, and none was needed:DocumentParserIDocumentLoader(already an interface, five implementations) +FileParserServicefor dispatchSourceStoreKnowledgeServiceowns Document persistence orchestration; the provenance helpers take the DB handle explicitlyCitationServiceRetrieverApplication is written up as an architectural boundary, not a class, so nobody goes looking for the
ApplicationServicethis document would otherwise imply.The one code fix
src/main/protocol/documentProtocol.ts(added by #116) querieddocuments.localFilePathitself — the only v1.4 module reaching the database from outside both permitted layers.Recording that as a "known exception" would mean accepting a brand-new architectural exception at the moment #60 closes, and the fix is cheap, so it is fixed:
KnowledgeService.getDocumentLocalFilePath(documentId): string | null— deliberately narrow rather than reusinggetDocument(). The handler needs "a readable file for this id", not a wholeDocumentrow. NoSourceStore-style repository was invented for it — one method on the existing Document seam, no new class/interface/factory.registerDocumentProtocolHandler()now takes the resolver, so the protocol layer imports no database, no Drizzle and no schema at all:src/main/index.tsand the packaged smoke test inject it. The main-process wiring readsknowledgeServicelazily because it is constructed later in startup — noted at the call site rather than reordering startup for it.A guard, not a comment
test/architectureBoundary.test.tsfails if anything undersrc/main/protocol/imports the database, Drizzle or the schema, or callsgetDatabase(). It has a self-check so it cannot pass by scanning nothing, and I verified it by injecting the import back: it reportsdocumentProtocol.ts imports ../dband fails.I did not extend the design-token guard or build a general static analyser for this — one focused rule for the boundary this issue actually constrains.
Recorded honestly, not papered over
Three handlers (
noteHandlers,notebookHandlers, and part ofchatHandlers) still calldb/queriesinstead 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 the document says so rather than implying the boundary is uniformly clean. No handler callsgetDatabase()itself.Review fix: the guard had a false negative
Found in review, and it was a real hole rather than a style point. The matcher caught an exact
../dbplus the/dband/db/schemasuffixes, but not../db/queries— which is precisely the pattern this PR's own architecture document records as a database-reaching legacy path. Soimport { someQuery } from '../db/queries'in the protocol layer would have passed a guard whose stated job is to prevent it./(^|\/)db(?:\/|$)/now treats the wholedbsubtree as the boundary:../db,../db/schema,../db/queries,../../db/foo. It still does not fire ondb-utilsor@shared/dbx, and a table-driven test pins both directions so the rule cannot quietly become leaky or noisy.Verified by injecting the exact miss back, then restoring:
The seam work itself is unchanged: the narrow
KnowledgeServicequery, the injected resolver, the lazy wiring and the architecture document are as reviewed.Verification
npm run typecheck— all three projectsnpm test— 261 pass / 0 fail (3 new: the boundary guard)npm run build:unpack && npm run smoke:packaged— executed on the packaged app, 20/20 checks, includingknownote-doc:// serves an owned document and 404s an unknown idthrough the newly injected resolver. That is the regression that matters for this change, and it covers both branches (found → serves bytes, not found → 404)npm run check:design,npm run lint(0 errors, 107 warning baseline unchanged),npx prettier --checkdocs/architecture.mdis additive only: 94 insertions, 0 deletionsNot touched, per the issue's non-goals
KnowledgeServicestructure,SourceStore, aCitationService, directory layout, the Model seam (#44 owns it), and the legacydb/querieshandlers. No follow-up architecture round should be needed — the point of this PR is that reality is now written down and the one reverse dependency is gone.