diff --git a/workspace-server/internal/db/architecture_test.go b/workspace-server/internal/db/architecture_test.go new file mode 100644 index 00000000..d585db62 --- /dev/null +++ b/workspace-server/internal/db/architecture_test.go @@ -0,0 +1,63 @@ +package db_test + +// Architecture test (#2344): db is a leaf — DB pool + migrations + raw +// SQL helpers, no business-logic dependencies. The DB layer must be +// testable with sqlmock in isolation. If db starts importing handlers +// or provisioner, every db unit test would need to bring up that +// subsystem, and the layering becomes circular. +// +// If this test fails: you put business logic in the db package. Move +// it to a higher-tier package that imports db, not the reverse. + +import ( + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +const moduleInternalPrefix = "github.com/Molecule-AI/molecule-monorepo/platform/internal/" + +func TestDBHasNoInternalDependencies(t *testing.T) { + t.Parallel() + for path, file := range listImports(t, ".") { + if strings.HasPrefix(path, moduleInternalPrefix) { + t.Errorf( + "db must not import other internal packages "+ + "(found %q in %s) — db is the foundation layer and a "+ + "reverse dep creates a cycle (everything imports db). "+ + "See workspace-server/internal/db/architecture_test.go.", + path, file, + ) + } + } +} + +func listImports(t *testing.T, dir string) map[string]string { + t.Helper() + fset := token.NewFileSet() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + out := make(map[string]string) + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + f, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + for _, imp := range f.Imports { + path := strings.Trim(imp.Path.Value, "\"") + if _, seen := out[path]; !seen { + out[path] = name + } + } + } + return out +} diff --git a/workspace-server/internal/models/architecture_test.go b/workspace-server/internal/models/architecture_test.go new file mode 100644 index 00000000..40b65ba6 --- /dev/null +++ b/workspace-server/internal/models/architecture_test.go @@ -0,0 +1,63 @@ +package models_test + +// Architecture test (#2344): models is a leaf — it carries pure type +// definitions and must not import any other internal/* package. Almost +// every package in workspace-server depends on models; if models grew a +// reverse dep, the import graph would cycle. +// +// If this test fails: you put behavior inside models. Move the behavior +// to whichever package actually owns it (handlers, provisioner, db, …) +// and have *that* package import models, not the reverse. + +import ( + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +const moduleInternalPrefix = "github.com/Molecule-AI/molecule-monorepo/platform/internal/" + +func TestModelsHasNoInternalDependencies(t *testing.T) { + t.Parallel() + for path, file := range listImports(t, ".") { + if strings.HasPrefix(path, moduleInternalPrefix) { + t.Errorf( + "models must not import other internal packages "+ + "(found %q in %s) — models is the pure-types leaf and any "+ + "reverse dep creates an import cycle since most packages "+ + "depend on models. See workspace-server/internal/models/architecture_test.go.", + path, file, + ) + } + } +} + +func listImports(t *testing.T, dir string) map[string]string { + t.Helper() + fset := token.NewFileSet() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + out := make(map[string]string) + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + f, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + for _, imp := range f.Imports { + path := strings.Trim(imp.Path.Value, "\"") + if _, seen := out[path]; !seen { + out[path] = name + } + } + } + return out +} diff --git a/workspace-server/internal/provisioner/architecture_test.go b/workspace-server/internal/provisioner/architecture_test.go new file mode 100644 index 00000000..c7455e52 --- /dev/null +++ b/workspace-server/internal/provisioner/architecture_test.go @@ -0,0 +1,80 @@ +package provisioner_test + +// Architecture test (#2344): provisioner is below handlers/router in +// the layer hierarchy. handlers wires provisioner into HTTP routes; +// the reverse direction (provisioner reaching back into handlers or +// the router) creates a cycle and tangles infra-orchestration with +// transport. +// +// Note: provisioner CURRENTLY imports db (for the runtime-image +// lookup). That's a known coupling — see PR #2276 review thread on +// where image resolution should live. The narrower rule we enforce +// here is "no upward import to handlers/router," which is the harder +// rule to keep clean. +// +// If this test fails: you reached "up" the stack. Pass whatever you +// need from handlers down through a constructor parameter or a +// function-typed callback instead of importing the package directly. + +import ( + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +const moduleInternalPrefix = "github.com/Molecule-AI/molecule-monorepo/platform/internal/" + +var provisionerForbiddenImports = []string{ + moduleInternalPrefix + "handlers", + moduleInternalPrefix + "router", +} + +func TestProvisionerDoesNotImportUpstreamLayers(t *testing.T) { + t.Parallel() + imports := listImports(t, ".") + for path, file := range imports { + for _, forbidden := range provisionerForbiddenImports { + if path == forbidden || strings.HasPrefix(path, forbidden+"/") { + t.Errorf( + "provisioner must not import %q (found in %s) — "+ + "provisioner sits below handlers/router in the layer "+ + "hierarchy and a reverse dep creates a cycle. Pass "+ + "what you need down via constructor params or "+ + "function-typed callbacks. See workspace-server/internal/"+ + "provisioner/architecture_test.go.", + path, file, + ) + } + } + } +} + +func listImports(t *testing.T, dir string) map[string]string { + t.Helper() + fset := token.NewFileSet() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + out := make(map[string]string) + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + f, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + for _, imp := range f.Imports { + path := strings.Trim(imp.Path.Value, "\"") + if _, seen := out[path]; !seen { + out[path] = name + } + } + } + return out +} diff --git a/workspace-server/internal/wsauth/architecture_test.go b/workspace-server/internal/wsauth/architecture_test.go new file mode 100644 index 00000000..c61e5b7e --- /dev/null +++ b/workspace-server/internal/wsauth/architecture_test.go @@ -0,0 +1,68 @@ +package wsauth_test + +// Architecture test (#2344): wsauth is a leaf package — it must not import +// any other internal/* package. The auth layer is below business logic; +// importing handlers, db, or any cousin package would force every wsauth +// test to spin up that subsystem, defeating the unit-test boundary that +// makes the auth code reviewable. +// +// If this test fails: you added an import that crosses a layer. Either +// move the dependency the other direction (consumer wires wsauth into +// itself), accept the boundary by inlining what you need, or — if the +// new coupling is genuinely correct — explicitly update this test with +// the new allowed import + a comment explaining why. + +import ( + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +const moduleInternalPrefix = "github.com/Molecule-AI/molecule-monorepo/platform/internal/" + +func TestWsauthHasNoInternalDependencies(t *testing.T) { + t.Parallel() + for path, file := range listImports(t, ".") { + if strings.HasPrefix(path, moduleInternalPrefix) { + t.Errorf( + "wsauth must not import other internal packages "+ + "(found %q in %s) — wsauth is the auth leaf and must stay "+ + "unit-testable without spinning up other subsystems. "+ + "See workspace-server/internal/wsauth/architecture_test.go for context.", + path, file, + ) + } + } +} + +// listImports returns import-path → first-file-where-seen for non-test +// .go files in dir. Used by every architecture_test.go in this tree. +func listImports(t *testing.T, dir string) map[string]string { + t.Helper() + fset := token.NewFileSet() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + out := make(map[string]string) + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + f, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + for _, imp := range f.Imports { + path := strings.Trim(imp.Path.Value, "\"") + if _, seen := out[path]; !seen { + out[path] = name + } + } + } + return out +}