Skip to content

Widen the import-cycle guard from one cluster to all of server/services with a shrinking baseline #5693

Description

@atomantic

Problemserver/services/agentImportCycles.test.js already builds the complete static import graph of server/services (1805 files scanned) but deliberately fails only on cycles touching an 11-module allowlist: "A cycle anywhere in server/services will be reported, but only a cycle TOUCHING one of these fails the assertion — unrelated pre-existing cycles elsewhere are out of scope for this guard." As a result 12 static cycles currently live in server/services with nothing stopping the 13th, and each one is a latent boot-order TDZ hazard in a codebase where new services land weekly. The scanning machinery is already written and already correct; only the assertion scope is missing.

Evidence — the guard's scope declaration and the graph builder it already has:

// server/services/agentImportCycles.test.js:28-42
// The modules the #2837 audit named, plus the leaf modules extracted to break
// the cycle. A cycle anywhere in `server/services` will be reported, but only a
// cycle TOUCHING one of these fails the assertion — unrelated pre-existing
// cycles elsewhere are out of scope for this guard.
const CLUSTER = [ 'agentLifecycle.js', 'agentCliSpawning.js', ... ];
// server/services/agentImportCycles.test.js:66
function buildStaticGraph() {   // walks every non-test .js under SERVICES_DIR, recursing into subdirs

Running the same scan with the cluster filter removed reports 12 live static cycles (import() excluded, comments stripped):

taskLearning/metrics.js -> taskLearning/routing.js -> taskLearning/metrics.js
meatspacePost.js -> meatspacePostStats.js -> meatspacePost.js
meatspacePost.js -> meatspacePostRecommendations.js -> meatspacePost.js
pipeline/manuscriptReview.js -> pipeline/manuscriptFix.js -> pipeline/manuscriptReview.js
sharing/peerSync.js -> sharing/peerSyncReceive.js -> sharing/peerSync.js
loraDatasetGenerate.js -> loraDatasetCaption.js -> loraDatasetGenerate.js
taste-questionnaire.js -> digital-twin.js -> digital-twin-export.js -> taste-questionnaire.js
identity.js -> identity/status.js -> taste-questionnaire.js -> digital-twin.js -> digital-twin-export.js -> identity.js
identity.js -> identity/status.js -> taste-questionnaire.js -> digital-twin.js -> digital-twin-avatar-bio.js -> identity.js
cos.js -> persistentMindAdapter.js -> cosToolRegistry.js -> voice/tools.js -> voice/tools/ask.js -> askService.js -> memoryEmbeddings.js -> cos.js
cos.js -> persistentMindAdapter.js -> cosToolRegistry.js -> voice/tools.js -> voice/tools/ask.js -> askService.js -> character.js -> cos.js
creativeDirector/completionHook.js -> creativeDirector/agentBridge.js -> creative/toolRegistry.js -> creative/tools/pipeline.js -> pipeline/seriesAutopilot.js -> pipeline/seriesAutopilot/session.js -> pipeline/autoRunner.js -> pipeline/episodeVideo.js -> creativeDirector/completionHook.js

(client/src scanned the same way has zero cycles, so this is a server-only gap.)

Plan

  1. Add server/services/serviceImportCycles.test.js. Move listServiceFiles() and buildStaticGraph() out of agentImportCycles.test.js into a new non-test helper server/lib/serviceImportGraph.js exporting buildServiceImportGraph(dir) and findStaticCycles(graph) (Tarjan or a colored DFS returning canonical cycles), and have both suites import it — one parser, per the rationale already written into server/lib/staticImportGraph.js.
  2. In the new suite, declare const KNOWN_CYCLES = [...] holding the canonical (sorted-member-key) form of each remaining cycle, with a one-line comment per entry naming the issue that will remove it. Assert two things: every detected cycle is in KNOWN_CYCLES (a NEW cycle fails), and every entry in KNOWN_CYCLES is still detected (a FIXED cycle must be deleted from the baseline, so the list can only shrink). Decision: a shrinking baseline rather than a hard zero, because fixing all 12 in one PR would be an un-reviewable change; the ratchet still blocks every new cycle from today.
  3. Seed KNOWN_CYCLES with the 12 above, minus any already removed by architecture-01/02/03 when this lands.
  4. Leave agentImportCycles.test.js's cluster-specific assertions (facade re-export rules, deferred-import ban) exactly as they are — they check properties the general guard does not.

Tests

  • The new server/services/serviceImportCycles.test.js IS the deliverable. It uniquely catches "a new service closes a static cycle", which today no test detects outside the 11-module agent cluster, and its second assertion uniquely catches "a cycle was fixed but the baseline entry was left behind", which is how a ratchet list rots.
  • Add one negative-control case that feeds findStaticCycles a hand-built 3-node cyclic graph and asserts it is reported, so the detector itself cannot silently return [].

Acceptance criteria

  • server/lib/serviceImportGraph.js exists, is re-exported from server/lib/index.js, and has a row in server/lib/README.md (Module Organization maintenance rule).
  • agentImportCycles.test.js and the new suite share one graph builder; neither has its own copy.
  • Adding a deliberate new static cycle between two services fails cd server && npm test.
  • Removing a cycle without deleting its KNOWN_CYCLES entry also fails.

Out of scope — fixing any of the 12 baseline cycles (each has, or gets, its own issue); extending the guard to server/lib or client/src.


Filed by a /do:better --scan-only --issues audit (2026-09-01). Category: architecture · Severity: medium · Files: server/services/agentImportCycles.test.js:28, server/services/agentImportCycles.test.js:66, server/lib/staticImportGraph.js:36

All labels already exist in the repo; do NOT create labels. Never add planner:* labels.

Activity

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

Metadata

Metadata

Assignees

Labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions