[Architecture] refactor: per-domain modules with service layers, enforced by an architecture test - pure motion, zero new deps

🟢 open · #295 · open-legal-products/mike ← amal66/mike · opened 1mo ago by amal66 · +21,560-13,501 across 163 files · ↗ on GitHub

From the PR description

Row amal66#42 of the reference index (#205) - the backend reorganization the index says to take last. Rebuilt on 2026-09-03 on top of main (its former base #294 has merged), and finished: the previous revision stated conventions the tree did not meet; this one meets them and enforces them.

Summary

The Express backend becomes domain modules over a shared kernel. Every HTTP surface lives in backend/src/modules/<domain>/ as a thin *.routes.ts (parse, call, map status) over a service layer (explicit db: Db, typed results, no req/res), reachable from outside only through a named-re-export facade. backend/src/routes/ is gone. A fitness test makes the layering rules fail CI when crossed. Zero new dependencies; 222 endpoints byte-identical to main including registration order.

main this PR
HTTP handler files 17 in routes/, 14,826 lines 18 *.routes.ts in 16 modules, 6,105 lines (HTTP layer only)
Largest routes file routes/tabular.ts 2,389 tabular.routes.ts 779
Inline DB queries in routes files everywhere 0 (enforced)
Modules with a facade + service layer 0 16 / 16
type Db = redeclarations 30 0 (one export in lib/supabase.ts)
Backend test files 105 115 (+ ~60 service-level unit tests, fake db, no server)
Coverage floors (stmts/branches/fns/lines) 52/46/53/54 over lib/ only 55/47/57/57 over lib/ + modules/
Layering enforcement none src/__tests__/architecture.test.ts, 7 rules

Start with docs/backend-architecture.md - it is the reference for everything below.

What changed

1. One Db type, one result contract (lib/supabase.ts, lib/serviceResult.ts). Db is exported once and every service takes it first. ServiceResult<T> / ServiceFailure name why an operation failed (validation→400, forbidden→403, not_found→404, conflict→409, unavailable→503, error→500 through sendInternalError, so raw errors are logged with the request id and never sent). sendServiceFailure(res, f) is the single status-code policy. Modules that predate it keep local kinds where a response needs a status the table does not name (422, 502).

2. The motion, completed. Sixteen modules: audit, auth, chat, documents, downloads, library, models, project-chat, projects, quick-actions, source-documents, tabular, uploads, user, word-chat, workflows. Compared with the previous revision: the six "not yet modularized" route files are modularized; uploadSessions.ts (new on main) became modules/uploads together with lib/uploadProcessing + lib/uploadSessions; models gained the facade it lacked; tabular.routes.ts went from 2,035 lines / 46 inline queries to 779 / 0; chat and project-chat routes are at zero too. Domain logic that lived in lib/ moved into its module when nothing that stays in lib/ imports it (userSettings, userApiKeys, projectsOverview, workflowsOverview, workflowName, tabularReviewsOverview, chatTitle, the upload pair). SSE loops stay in routes files; tabular.generateStream.ts is the one sanctioned service file that takes res, and its header says so.

3. The rules are executable. backend/src/__tests__/architecture.test.ts walks every import under src/ and fails on: lib/modules/ (one allowlisted edge), any cross-module import that is not the facade (except app.ts mounting *.routes), express or res.* outside *.routes.ts (one allowlisted exception), any .from(/.rpc( in a routes file (ratchet, baseline empty), export * in a facade, middleware/modules/, and the existence of src/routes/. Each allowlist entry carries the reason. No lint plugin, no dependency-cruiser; it runs with npm test.

4. Coverage ratchet widened and raised. Include is src/lib/** + src/modules/**; the new service tests lifted the measurement, so floors go up to 55/47/57/57.

5. Docs. docs/backend-architecture.md (module anatomy, service contract with example, the seven rules, what stays in lib/ and why, named follow-ups, how to add a domain); AGENTS.md Backend Structure rewritten; docs index and testing-coverage updated.

Why

The monoliths were where changes went to be risky, and the earlier revision of this PR proved that a convention without enforcement does not hold: it documented "routes never query the database" while shipping 46 inline queries in one file. A codebase is only as modular as its worst module and only as consistent as its CI makes it. This revision closes both gaps in the same change so the next contributor inherits a layout that the test suite defends.

Replication - proving it is pure motion

No user-visible behavior changes; the thing to verify is that only the layout changed.

  1. Endpoint parity, runtime proof. From backend/ on each ref, with SUPABASE_URL/SUPABASE_ANON_KEY/SUPABASE_SERVICE_ROLE_KEY set to dummies and DB_JOBS_ENABLED=false, import src/app.ts under tsx, walk app._router.stack recursively, print METHOD path per layer. main and this tip both print 222 lines, diff empty (order included - /create must beat /:chatId).
  2. Rebase fidelity. Every hunk main applied to the moved files since this branch's previous base was re-homed: the direct-upload session protocol replacing multipart uploads (documents, projects, library, workflows), workflow reference files → document-backed assets, transparent_tables in the profile cascade, workflow_id + canEdit gates on document access, the streaming download-zip with folder selection. main's integration tests for each (documentsUpload, uploadSessions*, workflows, workflowAddons, user) pass unchanged.
  3. Build + tests. npx tsc --noEmit clean at every commit; npm test --prefix backend1,209 passed | 34 skipped; npm run test:coverage --prefix backend passes the raised floors (55.54 / 47.47 / 57.94 / 57.94 measured).
  4. Architecture test. npm test --prefix backend -- src/__tests__/architecture.test.ts → 8/8. To see it bite, add db.from("chats") to any routes file, or import "../modules/chat/chat.title" from a lib/ file: the failure names the rule and the file.

The five commits

Each compiles on its own and each message is a self-contained explainer of its concept (result contracts, service layers, fitness functions, ratchets).

  1. refactor(backend): one Db type and one service-result contract for every module
  2. refactor(backend): decompose route monoliths into per-domain modules with service layers
  3. test(architecture): make the module layering rules executable
  4. test(coverage): widen the ratchet to src/modules and re-floor to the measured tree
  5. docs: the module layout is the backend convention

Tradeoffs & design decisions (flagged)

  • One big motion commit rather than a commit per module. A per-module series would not compile in between (cross-module facades) and would multiply the review surface without adding reviewable meaning. The commit message and docs/backend-architecture.md carry the map.
  • lib/ still holds domain-flavored code (access, audit, documentTypes, documentVersions, modelSelection, routerModels, userLookup, workflowCatalog*, sourceDocuments, lib/chat/, lib/dbq/handlers.ts). It stays because the durable-job handler registry and the assistant engine import it and rule 1 forbids lib/modules/. Moving them means first moving the job handlers and lib/chat into modules - named follow-ups, kept out of this PR because several open PRs edit lib/chat heavily.
  • Two result vocabularies coexist. New services use lib/serviceResult.ts; modules that predate it keep local kinds with identical mappings. Migrating them is mechanical and safer module by module than in one sweep.
  • The parity proof covers method + path + order, not status codes or bodies. That boundary is unchanged from the previous revision; the seam is covered by the untouched integration suites and by reviewer inspection. One deliberate byte-level detail: the shared mapper emits code before detail, the order the pre-existing handlers used.
  • app.ts imports *.routes directly, not the facade. A facade must stay HTTP-agnostic (rule 3); the routes file is a module's second, HTTP-only entry point and only app.ts may use it (rule 2 encodes exactly that).
  • Known pre-existing bug, deliberately not fixed here: the project/library delete paths do not enqueue the extracted-text cache object for cleanup (the documents path does). Verified present on main; needs its own PR.

Sequencing note

Open route-touching PRs will need re-porting onto the module layout once this lands: #247, #267/#268/#363, #346, #385, #401, #341, and #356 (which was stacked on the previous revision of this branch and must be re-stacked). The handler bodies they touch are unchanged, so each re-port is a file move plus import re-rooting; git rename detection handles most of it. I'm happy to do all of them within 48h of merge.

Maintenance

Same commitment as the other index rows: I maintain what I land - 48h triage on anything this breaks; if I go dark for 60 days, revert freely.

🤖 Generated with Claude Code

https://claude.ai/code/session_015rqMYqb5Tj7z1DdGJ5VcQC

Our analysis

Split route monoliths into domain service layers — read the full analysis →

Think the analysis missed something the PR description covers?

Capture this PR into my fork

Download a Markdown prompt that tells Claude how to port every commit in this PR into your working tree. Run it via claude -p < capture-pull-295.md from inside the repo you want the changes in.

⬇ Download capture-pull-295.md