Guard every static import cycle in server/services with a shrinking baseline - #5921
Merged
Conversation
…king baseline (#5693) The two existing cycle guards (agentImportCycles #2837/#3450, twinImportCycles #5687) each fail only on a ring touching their own cluster, so the rest of a 1000-module directory had nothing stopping the next one. A static ESM cycle is a boot-order hazard: whichever member evaluates first sees `undefined` for the others' bindings, and no behavior test notices until an unrelated import-order change surfaces it. serviceImportCycles.test.js now covers the whole directory against a baseline of the five components still live, each recorded against the issue that removes it (#5916-#5920). Both directions are asserted: an unlisted component fails, and a listed one that is no longer detected fails too, so the list can only shrink and a fixed cycle cannot leave a stale entry behind. The baseline is keyed on strongly-connected components, not on the rings findImportCycles renders. That walk reports whichever rings it closes from wherever it enters a component, and it enters wherever readdirSync put the first file - filesystem order, not alphabetical - so the same untouched graph yields a different list on another machine. Components are a property of the edges alone. They are also the truer picture: the DFS walk names 8 modules in the autopilot ring; the component is 22. findImportCycleComponents joins the shared parser in lib/staticImportGraph.js rather than getting its own module, for the reason that file's header already gives - two copies of a structural matcher is how a guard rots.
The comparator returned 1 for equal elements, which is not a strict weak ordering. SCCs partition the nodes so a tie cannot arise today, but a sort that lies about equality is not something a determinism-critical helper should carry. Compared by code point rather than localeCompare — locale-dependent order is the machine-to-machine variation this function exists to keep out of a baseline.
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.
Summary
server/services/serviceImportCycles.test.jsguards the wholeserver/servicesdirectory against new static ESM import cycles. The two existing guards (agentImportCycles.test.js,twinImportCycles.test.js) each fail only on a ring touching their own cluster, so ~1000 modules had nothing stopping the next one. A static cycle is a boot-order hazard: whichever member evaluates first seesundefinedfor the others' bindings, and no behavior test notices until an unrelated import-order change surfaces it.KNOWN_CYCLIC_COMPONENTSfails, and a listed component that is no longer detected fails too — so the list can only shrink, and a fixed cycle cannot leave a stale entry behind.findImportCycleComponentsjoins the shared parser inserver/lib/staticImportGraph.jsrather than getting a module of its own — that file already exists for exactly this reason, and two copies of a structural matcher is how a guard rots. README row and barrel entry updated.Why components rather than rendered rings
findImportCyclesis depth-first, so which rings it names depends on where the walk enters a component — and it enters whereverreaddirSyncput the first file, i.e. filesystem order, not alphabetical. That is fine for an "is this empty?" assertion and unusable as a baseline: the same untouched graph would produce a different list on another machine, so CI would go red over a change nobody made. Strongly-connected components are a property of the edges alone. They are also the truer picture — the DFS walk names 8 modules in the autopilot ring; the component is 22.Test plan
cd server && npm test— the new suite passes (5 tests);agentImportCycles,twinImportCycles,lib/index.test.jsandlib/generatedManifests.test.jsstay green.import './cosForgeSpawnGate.js'toagentState.js, closing a two-module ring. The guard failed and named both the component and the ring that closed it. Reverted.taskLearning/routing.js→metrics.jsback-edge without touching the baseline. The other direction failed, naming#5916and the entry to delete. Reverted.mainwithout this branch — pre-existing local-environment failures, not introduced here.Closes #5693