Skip to content

[code-quality-shared-pure-modules-direction] Retire the server-mirror copies in favour of the client→server import the codebase already uses #6364

Description

@atomantic

Slice audited: the pure modules shared between server/lib/ and client/src/lib/ (the "server mirror" convention) and the two reverse-direction imports. Audit date: 2026-09-06.

Problem

The same need — one pure module used by both runtimes — is served three different ways today:

  1. Copy + parity test. client/src/lib/README.md:10-12 declares that "several modules here are server mirrors — kept byte-for-byte in sync". 25 declared pairs (2,669 client lines), 34 *.mirror.test.js / *.parity.test.js / *.contract.test.js files, the server/lib/mirrorParity.js helper, and server/lib/mirrorCoverage.test.js policing README rows. The recorded reason is that the browser cannot import server/lib (Give the client a textUtils mirror so escapeRegExp stops being re-inlined there #5790) / "server and client don't share a build step" (client/src/lib/isSafeHref.js:10, server/lib/isSafeHref.js:10).
  2. Client imports the server leaf. client/src/components/Layout.jsx:134 (NAV_COMMANDS, in production since promote NAV_COMMANDS to the structural source of truth for the sidebar, leaving Layout.jsx presentation-only #5053), client/src/components/apps/constants.js:5 (instanceFeatureRegistry), client/src/components/ui/SectionTabsHeader.jsx:2, client/src/components/settings/SettingsTabsHeader.jsx:2, client/src/components/models/ModelsTabsHeader.jsx:1, and 12 FableLoom files (client/src/components/fableloom/*, client/src/pages/FableLoom.jsx:30, client/src/pages/FableLoomStory.jsx:54) import server/lib/* directly. CI builds this (.github/workflows/ci.yml:381, npm run build --prefix client) and Layout.test.jsx renders it, so the reason recorded for (1) is not true.
  3. Server imports the client leaf. server/lib/personaTraitBlend.js:21 takes clamp from client/src/utils/formatters.js, and server/routes/cosStatusRoutes.js:18 takes AVATAR_STYLE_IDS from client/src/lib/avatarStyles.js, citing the former as the pattern (client/src/lib/avatarStyles.js:6-8). That reverses dependency direction: the server process loads client source at boot, and a client-only dependency added to either file breaks the server CI job — the failure PR Pin a reasoning-effort tier per code reviewer #3614 hit when a server test imported client/src/components/cos/constants.js.

The mirrors are also not byte-for-byte: 13 of the 15 same-named pairs differ (canonPrompt 142 diff lines, catalogTypes 784, scenePrompt 72, issueLength 161, editorial/shotContinuity 254, …) in comments, imports and subset exports. The README states a contract the tests do not enforce; the real contract is per-declaration text equality with comments stripped.

Impact

Every change to shared logic is a two-file edit plus a parity test that stays red until a human copies the change. History shows the copies drifting anyway: #6303 (server accepted https:foo, client rejected it — an unguarded pair), #5673 (effort ladders, tool-use regex and gateway table in three copies; a row added on one side only 400s a run or mis-badges a working model), server/services/stageRunner.mirror.test.js ("a model bump that added a row to only one copy left BOTH suites green"). And per #6363, the parity tests are unreachable from either side on a scoped PR, so the drift they guard reaches main.

Fix

One direction: the client imports server pure leaves; the server never imports client source. Import the leaf, never server/lib/index.js.

  1. Convert each declared mirror in client/src/lib/ into a re-export shim of the server original — named re-exports (export { a, b } from '../../../server/lib/x.js'), not export *, so server-only extras such as canonPrompt's flatten* do not enter the client barrel's collision detector. Keep the file, its README row and its index.js entry so no client import path changes. Convertible as-is (no Node-only transitive import): appIdentity, assetProvenance, bareUrl, canonPrompt, scenePrompt, seasonStructure, grokVideoClip, reactorVideoClip, isSafeHref, issueLength, goalFeatureMap, loraEffect, loraTriggers, musicDuration, postRotation, repoUrl, shotGrammar, slashdoCatalog, textUtils, tribeCadence, videoReferenceModes, extensionErrors, editorial/shotContinuity, editorial/letteringDensity, and personaTraitBlend after step 3.
  2. Three originals need a pure core split first: server/lib/catalogTypes.js:33 imports storyBible.js, which pulls crypto and fileUtils (storyBible.js:12-14) — move BIBLE_LIMITS into a new pure server/lib/bibleLimits.js (the client already carries client/src/lib/bibleLimits.js mirroring exactly that constant) and import it from both; server/lib/youtubeUrl.js:23 imports ServerError — keep assertYoutubeVideoUrl in a thin server wrapper and share only the regexes/parsers; server/lib/ports.js:37-39 reads process.env at module top — share only the PORTS map.
  3. Flip the two reverse edges: inline clamp in server/lib/personaTraitBlend.js (or add it to server/lib/objects.js); move AVATAR_STYLES to server/lib/avatarStyles.js and make client/src/lib/avatarStyles.js a shim.
  4. Delete the parity tests whose only assertion is declaration equality; keep behaviour tests as plain unit tests of the one module (e.g. the shared input matrix in client/src/lib/isSafeHref.mirror.test.js). Retire server/lib/mirrorParity.js and mirrorCoverage.test.js once no copy remains; a pair that must stay a copy keeps its parity test and says why in its README row.
  5. Replace the README "server mirrors" paragraph with the rule and add two guards, both registered in ALWAYS_RUN_TESTS per scripts/repo-scan-guards.test.js: (a) walk the import graph of every server/lib module the client imports (found by git grep for server/lib/ under client/src) and fail on a Node built-in or an import outside server/lib; (b) fail if any file under server/ imports from client/.

Rejected: a shared workspace package (#5673 deferred it as a larger architectural change; the direct-import convention already runs in production and needs no new build plumbing). Rejected: widening the parity machinery further — it guards copies that no longer need to exist.

Acceptance criteria

  • git grep -l "from '../../client/\|from '../client/" server/ is empty.
  • Every file in client/src/lib/ listed in step 1 is a re-export of the server module; the diff-based / compareDeclaration-based tests for those pairs are gone.
  • client/src/lib/bibleLimits.js, catalogTypes.js, youtubeUrl.js and ports.js re-export from pure server leaves that import no Node built-in.
  • The two new guards exist, are in ALWAYS_RUN_TESTS, and scripts/repo-scan-guards.test.js passes.
  • client/src/lib/README.md no longer says "byte-for-byte"; each converted row says it re-exports the server leaf.
  • cd client && npm run build, cd client && npm test, cd server && npm test all pass.

Scope: large

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

code-qualityProposed from a code-quality auditeffort:highDispatch reasoning effort: highmodel:mediumModel size: mediumplanTracked by /do:replanplanner:fable-5-1Plan authored by the fable-5-1 model

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions