From the PR description

Code quality follow-up: repo-wide audit fixes

A seven-area audit of the codebase (backend modules, backend lib/workers, remaining route files, frontend data layer, frontend components, Word add-in + shared UI, cross-cutting infra) surfaced ~100 findings. This PR lands the S/M-effort, high-confidence fixes - real bugs, silent-failure paths, drifted copy-paste forks, and missing safety infrastructure.

Diff size: 126 files, +2,857 / -2,604 (net +253). Ignoring whitespace it is +2,235 / -1,982 - the 620-line gap is almost entirely backend/src/routes/wordChat.ts, which reads +503/-486 raw but +55/-38 under git diff -w, because wrapping every handler in asyncRoute(...) re-indents its body. Per area: backend 74 files (+1,887/-1,712), frontend 30 (+706/-677), word-addin 10 (+79/-51), e2e 7 (+141/-149), .github 2 (+17/-3), docs 2 (+21/-12).

Stack mechanics (read this first)

This is the top PR of a three-PR architecture stack. Its base branch is olp-pr/service-layers (#295), not main, so the diff below contains only this PR's own 8 commits - the 14 commits of #294 and the 2 of #295 are not in it. (Earlier revisions of this description said "this PR's diff includes their commits until they land"; that is no longer true - the base was retargeted.)

• olp-pr/durable-queues and olp-pr/service-layers exist in this repo only as mirrors of the identically-named branches on the amal66/mike fork, so GitHub can compute a clean stacked diff. They are force-pushed together on every rebase; origin and fork tips are kept byte-identical (currently f59aee93 and b82bbbed). Nothing else should branch off them.
• Please do not delete olp-pr/service-layers on merging #295 until this PR has been retargeted, or this PR's base disappears from under it.
• Why stacked rather than based on main: most of these fixes live inside the module layout #295 creates. Basing on main would conflict wholesale with that motion. If #295 is not taken, I will re-port this onto main - the fixes are independent of the layout, the file paths are not.

The 8 commits:

What's fixed

Backend - error containment

Express 4 does not understand promises: an async handler that rejects is invisible to the router, so the request hangs until the client or proxy times out, and nothing is logged. Three route files had no containment at all - wordChat.ts (5 handlers), audit.ts (2), sourceDocuments.ts (1) - so 8 previously-unguarded handlers now go through a wrapper. Three other files (quickActions.ts 4, workflowAddons.ts 3, modules/workflows/workflows.routes.ts 21) each carried their own private copy of the same asyncRoute helper; all 28 now import the shared one. New backend/src/middleware/asyncRoute.ts provides asyncRoute + routerErrorHandler(tag), and app.ts gains a terminal error handler so anything a router re-raises stops at an opaque internal_error body instead of Express's default handler, which leaks stack traces outside production. Body-parser's own 400/413 keep their status and code, and a response that has already started streaming (SSE, file download) is handed on rather than rewritten.

Backend - silent failures and route correctness

• Document rename returned 200 on failure in two modules (projects.documents.ts, library.service.ts): the document_versions update's error was never destructured, and the response echoed the requested filename. Both now read back.
• Quick-action DELETE returned 204 without matching a row; tabular review/chat delete + rename ditto; sort_order validation was unreachable for non-integers (silently coerced to 0); DB errors surfaced as 404 "not found" in quick-actions/add-ons; the audit view silently narrowed to own-events on a failed project lookup; CSV export could never show display names.
• sourceDocuments returned raw upstream error bodies as 502 (now redacted via safeErrorMessage; credential failures map to 400).
• Folder-move cycle guard was missing in both the library and projects ancestor walks (an interleaved move could persist a cycle and spin a request forever). Both now walk with a visited set.
• History page learns the word surface label - the audit rows themselves come from #294, see the note below.

Backend - deduplication that had already caused bugs

• The project upload pipeline was a 183-line fork of the shared one (processProjectDocumentUpload, base lines 421-603) and had drifted: it omitted folder_id from its response and library_kind / library_folder_id from its INSERT. Deleted - projects.documents.ts drops 603 → 415 lines and delegates to createDocumentFromUpload with a surface parameter.
• countPdfPages existed twice (documents.shared.ts, projects.shared.ts) → one copy in backend/src/lib/pdfjs.ts.
• Suffix handling: 8 duplicated suffix-extraction implementations and 7 separate allowlist checks → parseAllowedSuffix / documentSuffix in lib/documentTypes.ts; ALLOWED_DOCUMENT_TYPES.has now appears exactly once.
• devLog existed 6 times → one copy in backend/src/lib/log.ts.

Backend lib - correctness

• Citation fallback merged positionally. Fallback citations are POSTed to CourtListener as one newline-joined blob, and it returns one row per citation it detects, not one per input line - so an unparseable input (0 rows) or a parallel cite (2 rows) shifted every later citation's verdict onto the wrong citation, and surplus rows were appended as phantom results. Now keyed by a normalized citation string (citationMatchKey(): lowercase, strip ./,, collapse whitespace) with a used-row set.
• scryptSync ran on every encrypt/decrypt - ~40ms of blocked event loop each. Derived keys are now memoized per (secret, salt) in two places: lib/userApiKeys.ts and lib/mcp/client.ts (the latter is hit on every MCP tool call).
• Chat-generated .docx was the only file type that never got a PDF rendition - generateDocx's 76-line drifted tail never wrote pdf_storage_path and never called docxToPdf. It now delegates to persistGeneratedFile. Same change fixes a buf.buffer as ArrayBuffer cast that ignored byteOffset.
• Raw LLM stream logging is hard-disabled in production (lib/llm/rawStreamLog.ts): these logs contain verbatim prompts, client documents and completions, and were previously gated only by an env flag. rawStreamLoggingAllowed() now returns false under NODE_ENV=production regardless of the flag, and .env.example labels both vars "NEVER ENABLE IN PRODUCTION".
• CLAUDE_API_KEY drift: userApiKeys.ts already accepted it, providers.ts did not - so Settings showed a green key and every request threw "not configured". ENVIRONMENT_KEY_ALIASES now maps ANTHROPIC_API_KEY → ["CLAUDE_API_KEY"].
• Shared-project email normalization drift: tabularReviewsOverview.ts and workflowsOverview.ts passed params.userEmail ?? null into a case-sensitive shared_with containment check, where projects already passed ?.trim().toLowerCase() || null. Now all three match.
• The stale-work sweep is bounded (MAX_GENERATING_CELLS_PER_SWEEP = 500) with batched job lookups, so a large backlog can't produce an unbounded query.
• backend/.env.example gains 72 lines documenting 31 previously-undocumented env vars - including 4 secrets (MCP_CONNECTORS_ENCRYPTION_SECRET, MCP_OAUTH_CLIENT_SECRET, ...), API_PUBLIC_URL, TRUST_PROXY_HOPS, the Ollama trio, 12 commented RATE_LIMIT_* defaults, and the 2 raw-LLM-log vars above.
• One dependency removed: fast-diff. Its only consumer, lib/docxTrackedChanges.ts, no longer needs it (+3/-41, which also drops a dead indexAll(), unused W_NS_ATTRS, and an unreferenced export const _internal test hatch).

Frontend

• Upload endpoints threw plain Errors instead of MikeApiError, so a 403 mfa_verification_required on upload never opened the MFA popup - it rendered raw JSON. All 7 hand-rolled FormData blocks in mikeApi.ts now go through one apiUploadRequest; zero hand-rolled multipart fetches remain. Regression test: "keeps the MFA code on upload failures so the popup can open".
• Three hand-rolled SSE parsers, two of which dropped the final frame. TRChatPanel.tsx:1205 and TabularReviewView.tsx:391 both did if (done) break; before flushing, and buffer = lines.pop() discarded the trailing partial - so a stream that closed without a final newline lost its last event. Replaced by one readSseFrames generator in frontend/src/app/lib/sse.ts, which flushes with decoder.decode() on done. 11 transport tests.
• Stream lifecycle: useAssistantChat and TRChatPanel had no unmount abort and no generation guard - two concurrent turns could interleave into one message, and the drip interval survived unmount. Both now carry a generation counter that the cleanup effect bumps, plus an identity-checked abortRef reset.
• PdfView render race: rapid zoom wiped the container while a RenderTask was still writing into it. The task is now cancelled before the wipe, with two additional staleness gates.
• Keyboard access: table rows were mouse-only. Fixed once in TablePrimitive - 19 <TableRow> sites inherit it, of which 8 are actually interactive (onClick) and therefore genuinely gain keyboard reachability; the other 11 are skeleton / interactive={false} rows.
• Re-render correctness: AuthContext and UserProfileContext rebuilt their context value on every render (and two more in (pages)/layout.tsx) - all four memoized.
• Sidebar persistence bug: localStorage.setItem("sidebarOpen", isSidebarOpen.toString()) wrote the effective value, while the restore path reads it back as the desktop preference - so collapsing the sidebar on mobile corrupted the desktop setting. Now writes isSidebarOpenDesktop.
• AskInputPopup double-submit under StrictMode: submit moved out of a dependency-array-less effect into the resolving handler, with a pendingSkipId parameter so skipping the last outstanding item still submits.
• NewTRModal empty picker: project documents were snapshotted into state when the modal opened, but the parent passes [] until the project loads - opening early left the picker permanently empty. Now derived rather than copied.
• Also: impure setTabs updater, double project fetch per chat-page mount, unhandled selectAllMatching rejections (×3 hooks), and a dead-code sweep of roughly 280-300 lines including three deleted files (ProjectPickerModal.tsx, useGenerateChatTitle.ts, and the add-in's button.tsx).

Word add-in

• Edits were reported as failed when they had actually applied: mutationApplied was set after the context.sync() that executes the mutation, so a throw at or after the sync produced "Word couldn't apply this change" for an edit Word had already made - inviting a duplicate re-apply. The assignment moved above the await.
• "View edit" permanently destroyed the bookmark anchor of an edit that had been accepted in Word's Review tab: revealPersistedTrackedEdit called deleteBookmark + removeWordEditAnchor whenever the range showed zero tracked changes. Reveal is now read-only. Pinned by a new e2e case appended to word-addin/e2e/tracked-edit-persistence.spec.ts: "View never deletes the bookmark of an edit resolved outside Mike", verified red-before / green-after.
• Privacy mode failed open: the .catch() on the OfficeRuntime.storage read had an empty body, so a failed settings read left the mode at its initial "cloud" value - silently reverting the user's "local, never persist" choice. Now fails closed to local.
• 4 design-token value mismatches vs the frontend, in src/shared/styles/tokens.css: --app-background #fafafa→#f9fafb, --app-surface-hover #f5f6f8→#f9fafb, --app-surface-active #eceef2→#eff0f3, --app-floating rgba(253,253,254,.82)→#fefefe.
• Dead byte-identical src/shared/ui/button.tsx fork deleted; composer regains a height cap (max-h-48) and real accessible names ("Stop response" / "Send message"); the hardcoded REACT_APP_DEFAULT_MODEL: "gemini-3-flash-preview" in webpack.config.js becomes process.env.REACT_APP_DEFAULT_MODEL || ""; e2e-live/ scripts are wired into npm scripts and into tsconfig.e2e.json so they type-check.

Tests & CI

• Backend tests were never type-checked - excluded from tsconfig.json, and vitest strips types without checking them. New backend/tsconfig.test.json + npm run typecheck:test + a CI step in the backend job. It surfaced ~69 latent type errors across 16 test files (measured: 71 → 2 under the new project, and the 2 survivors are a Cannot find module 'bullmq' artifact of the measurement environment, present identically before and after). The bulk: 21 implicit-any, 17 spread-arity, 10 tuple-index, 7 node16 .js specifier errors.
• Test mocks deduped: three near-duplicate Supabase query mocks (69 / 69 / 80 lines - the tabular one a superset with rpcCalls/operations recorders) → one 141-line __tests__/helpers/supabaseMock.ts; three of six identical auth mocks → __tests__/helpers/authMock.ts. Honest accounting: the suites shed ~190 lines and the helpers add ~180, so this is roughly line-neutral repo-wide - the win is one mock to fix, not fewer lines. Four inline makeQuery mocks remain (chat, projectChat, user, wordChat) and three auth copies remain; finishing them is a follow-up.
• Coverage scope widened from src/lib/** to src/** (with an explicit exclude for tests, since setting exclude at all replaces vitest's defaults). The old scope left routes/, modules/, workers/ and middleware/ out of the report entirely, so the ratchet could not see a regression there. Widening the denominator moves measured coverage from 52.72/46.31/53.24/54.11 to 47.56/40.86/49.78/49.20, so the floors move 52/46/53/54 → 45/38/47/47 - numerically down, over a much larger population. This also repairs the scope shrink #295 caused by moving lib/tabular/** (1,539 lines) out of the old measured set without adjusting the floors.
• e2e: shared e2e/helpers.ts, a default expect.timeout of 10s in playwright.config.ts, and 5 waitForLoadState("networkidle") waits replaced with element waits (one of them the .catch(() => {})-swallowed variant) - zero remain.
• The Word add-in CI workflow's path filter now includes the frontend shared-UI files it actually bundles (frontend/src/shared/**, frontend/public/icons/**), and its header comment is corrected from "fully self-contained" to "NOT self-contained". You can see this working on this PR: the "Typecheck and Playwright (chromium + webkit)" check runs on #356's head commit and does not run on #294 or #295 - those two touch the shared UI without triggering the add-in suite.

Base-case replication - see the problems on olp-pr/service-layers

1. Async route errors hang the socket. On the base, git grep -n "asyncRoute" backend/src/routes/wordChat.ts returns nothing. Make any await in a /word-chat handler throw before headers are sent (e.g. point SUPABASE_URL at an unroutable host): the request never completes, the client waits for its own timeout, and the process logs an UnhandledPromiseRejection with no request attribution. On this branch the same throw returns an internal_error body immediately and logs [word-chat] unhandled route error.
2. Citation verdicts shift onto the wrong citation. On the base, call the chat verify_citations tool with a list where one entry is a parallel cite (CourtListener's /citation-lookup/ returns two rows for it) or is unparseable (zero rows). Every citation after it gets the previous one's verdict, and a phantom extra result is appended. backend/src/lib/courtlistener.ts on the base does fallbackRows.shift() per slot - that is the bug in one line.
3. Rename appears to succeed when it fails. On the base, revoke UPDATE on document_versions (or point the version row at a missing id) and rename a project or library document: the API returns 200 with the new filename, the UI shows it, and a refresh shows the old name. Neither module read the update's error.
4. MFA popup never opens on upload. With MFA enrolled and an expired AAL2 session, upload a document from the web app: the 403 mfa_verification_required arrives as a plain Error, so the MFA popup never opens and raw JSON error text is rendered.
5. Word add-in "View" destroys the anchor. Apply an edit, accept it in Word's Review tab, then click View on the card: the bookmark is deleted and the card demotes to historical, permanently.
6. A privacy setting that fails open. In the add-in, set storage to "local, never persist", then make the OfficeRuntime.storage read fail (offline / cleared storage): on the base the mode silently reverts to cloud and the next turn is persisted server-side.

PR replication - see them fixed

Pick any of these three; each is observable without reading the diff.

A. SSE last-frame loss (frontend, no infrastructure needed).

cd frontend && npx vitest run src/app/lib/sse.test.ts

11 cases pass, including "swallows a failing reader.cancel() so the frames still surface" (the tip commit b32e32d5) - a stream whose reader.cancel() rejects during cleanup still yields every frame the consumer already received. To see the old behaviour, run the same file against the base: sse.ts does not exist there, and the two buggy inline parsers in TRChatPanel.tsx / TabularReviewView.tsx drop the final frame when a stream closes without a trailing newline.

B. MFA-on-upload (frontend).

cd frontend && npx vitest run src/app/lib/mikeApi.test.ts -t "MFA"

The regression test asserts the thrown value is a MikeApiError carrying mfa_verification_required, which is what the popup keys off. End-to-end: enroll MFA, let the AAL2 session expire, upload - the popup opens instead of raw JSON.

C. Word add-in "View" is read-only (real Word host).

cd word-addin && npm run test:e2e -- tracked-edit-persistence

The case "View never deletes the bookmark of an edit resolved outside Mike" fails on the base and passes here. By hand: apply an edit → accept it in Word's Review tab → click View → the bookmark survives and the card stays live.

D. Route-surface parity (proves this PR changes no endpoints).

for r in fork/olp-pr/service-layers fork/olp-pr/code-quality; do
  git ls-tree -r --name-only "$r" backend/src | grep -E '\.ts$' | grep -vE '__tests__|\.test\.ts$' \
  | while read -r f; do git show "$r:$f"; done \
  | perl -0777 -ne 'print "\U$2\E $3\n" while /\b(\w*[Rr]outer)\s*\.\s*(get|post|put|patch|delete|all)\s*\(\s*"([^"]*)"/g' \
  | sort > "/tmp/$(basename "$r").routes"
done
diff /tmp/service-layers.routes /tmp/code-quality.routes && echo IDENTICAL; wc -l /tmp/*.routes

→ 144 / 144, IDENTICAL. Resolved through app.ts's mount table (note userRouter mounts at both /user and /users) that is 174 mounted method+path pairs, also identical on both sides. (An earlier revision of this description claimed "181 method+path routes"; that figure is not reproducible from either tree - 144 registrations / 174 mounted are the real numbers.)

Tradeoffs & design decisions (flagged)

• Stacked on #295, not main - the fixes live in the module layout. Basing on main would conflict wholesale with the motion; I'll re-port if #295 is not taken.
• The terminal error handler honors body-parser's 4xx statuses rather than blanket-500ing, so malformed JSON stays 400 and an oversized body stays 413. A response that has already started streaming is handed to Express to destroy - its status line is gone, so there is no honest alternative.
• user.shared.errorMessage keeps PostgREST detail text (now redacted) rather than flattening to a generic message - its detail strings are load-bearing in tests and UI.
• Coverage floors went numerically down (52/46/53/54 → 45/38/47/47) while the measured population roughly doubled. A reviewer who reads only the numbers will read it as a weakening. The alternative - keeping the old floors against the new scope - would have failed CI immediately and told us nothing. Known drift shipped by this PR: docs/testing-coverage.md:121 says "35 / 29 / 37 / 36" and the config says 45/38/47/47. The doc line is wrong and should be corrected; flagging it rather than quietly fixing it in a force-push.
• Mock dedup is roughly line-neutral repo-wide (-190 in suites, +180 in helpers) and only covers 3 of 7 Supabase mocks and 3 of 6 auth mocks. The value is one place to fix, not a line count.
• The scryptSync memo Map is unbounded and keyed on the raw secret. In practice it holds one entry; a runtime secret rotation would leak the old one, and the plaintext secret stays heap-resident as a Map key. Accepted for a ~40ms-per-call stall on the request path; an LRU would be ceremony for N=1.
• The citation-merge fix is unpinned. No test file for backend/src/lib/courtlistener.ts exists anywhere on the branch. It is verified by the base-case replication above, not by a test - call that out if it blocks the merge and I will add one.
• The generated-.docx rendition fix is also unpinned. documentOps.generatedRendition.test.ts covers generatePpt, not generateDocx. Related side effect to be aware of: lib/__tests__/documentGeneration.test.ts mocks ../storage and ../downloadTokens but not ../convert, so generateDocx now enters the real docxToPdf path and will shell out to soffice on a runner that has LibreOffice. It is inside a try/catch so it cannot fail the test, but it is real I/O in a unit test.
• word-addin/src/shared/styles/tokens.css also gains --color-azure: 0, 136, 255;, which is not one of the 4 mismatch fixes - the name exists nowhere else in the repo and has no consumer (the frontend spells the same colour --color-blue). Happy to drop it.
• backend/src/lib/supabase.ts now exports type Db, but 22 files still redeclare it locally. Half-finished on purpose (the migration is mechanical churn); say the word and I'll finish it or drop the export.
• CI ordering nit: in ci.yml, npm test --if-present runs before npm run build and npm run typecheck:test, so a type-broken mock still gets to run its tests first. Harmless, but the new gate fires later in the job than its commit message implies.
• Deferred L-effort items (each its own PR): DocTable / TRChatPanel / FileDirectory splits, toolDispatcher handler registry, packages/contracts shared types (the SSE event contract is authored 3× with real drift), finishing modules/tabular decomposition (tabular.routes.ts is still 1,806 lines), wordChat modularization, ChatView unification, usePaginatedList extraction, zod env config at boot, backend/add-in ESLint, and the remaining mock/Db-type dedup above.

Testing evidence

All CI checks are green on b32e32d5: CodeQL, gitleaks (full history), backend build and tests, frontend build and tests, playwright, Supabase stack integration tests, eval harness, the "Fresh install vs upgraded deployment" schema-drift job, and - new to this stack - the Word add-in "Typecheck and Playwright (chromium + webkit)" job, which now triggers because this PR fixed its path filter.

Suite sizes at the tip (static counts of it(/test( on b32e32d5; a live run may differ slightly through .each expansion):

(Earlier revisions of this description cited 658 backend / "351+" frontend / 114 word-addin. Those were snapshots taken before several of the commits above; the word-addin figure in particular ignored that playwright.config.ts defines two projects. The table above is the current tree.)

Type gates: tsc clean, plus the new tsc -p tsconfig.test.json gate over the test sources. Word add-in: both tsconfigs clean, including e2e-live/**/*.mjs, which was previously unchecked.

New tests added by this PR: 11 SSE transport cases (frontend/src/app/lib/sse.test.ts, one of them the tip commit), the MFA-upload regression in mikeApi.test.ts, and the tracked-edit "View never deletes the bookmark" e2e case.

Corrections to earlier revisions of this description

Kept here rather than silently edited away, because several of them are claims a reviewer may have already read:

• "96 async route handlers had no error containment" → the real figure is 8 newly guarded handlers in 3 files, plus 28 migrated off three private copies of the same helper onto the shared one.
• "Word chat had no audit trail - now audits with surface word" → that work is #294's (commit a1eaab4b; surface: "word" is already present at this PR's base). This PR only adds the word label to the History page's surface filter. The wordChat.ts audit lines in this diff are re-indentation under asyncRoute, not new code.
• "Ollama abort threw the wrong error shape; OpenAI/Ollama stream readers leaked sockets" → both targets were deleted by the Vercel AI SDK migration (#368), which is already an ancestor of this base. Neither fix is in this diff.
• "useSelectedModel allowlist now covers settings-tier model IDs" → already on the base (isAllowedModelId); not in this range.
• "A failed profile fetch fabricated a fake profile (999999 credits)" → still present at UserProfileContext.tsx:203 and untouched here. What this PR actually changed in that file is memoizing the context value.
• "Storage cleanup ×3 consolidated" → deliberately not done; all three blocks are byte-identical before and after, because merging them would reintroduce the leak #294 fixed. They already route through enqueueStorageCleanup.
• "Word add-in model catalog drift (3 models missing)", "anchor registry deleted", "byte-identical mike-icon fork replaced with an @mike/* alias" → none of the three are in this range; those files are byte-identical at both refs (the alias already existed).
• "CI: dead landing/ drift-check path removed" → no landing reference exists in .github/ at either ref.
• "~600 lines of dead code incl. two whole files" → roughly 280-300 lines, across three deleted files.
• "Ratchet floors raised 23/17/23/23 → 35/29/37/36" → the real move is 52/46/53/54 → 45/38/47/47, downward in number and upward in scope; see the tradeoff note.
• "Endpoint parity: 181 method+path routes" → 144 registrations / 174 mounted; both identical between base and head.
• "Net -904 lines (+2,350/-3,254 across 123 files)" → +2,857/-2,604 across 126 files, net +253. It changed because the base was retargeted onto #295 (which took b82bbbed and the wholesale routes/* deletions with it) and because the tip gained b32e32d5.

🤖 Generated with Claude Code