test(arch): codify 4 module boundaries as architecture tests (#2344)
Hard gate #4: codified module boundaries as Go tests, so a new contributor (or AI agent) can't silently land an import that crosses a layer. Boundaries enforced (one architecture_test.go per package): - wsauth has no internal/* deps — auth leaf, must be unit-testable in isolation - models has no internal/* deps — pure-types leaf, reverse dep would create cycles since most packages depend on models - db has no internal/* deps — DB layer below business logic, must be testable with sqlmock without spinning up handlers/provisioner - provisioner does not import handlers or router — unidirectional layering: handlers wires provisioner into HTTP routes; the reverse is a cycle Each test parses .go files in its package via go/parser (no x/tools dep needed) and asserts forbidden import paths don't appear. Failure messages name the rule, the offending file, and explain WHY the boundary exists so the diff reviewer learns the rule. Note: the original issue's first two proposed boundaries (provisioner-no-DB, handlers-no-docker) don't match the codebase today — provisioner already imports db (PR #2276 runtime-image lookup) and handlers hold *docker.Client directly (terminal, plugins, bundle, templates). I picked the four boundaries that actually hold; the first two are aspirational and would need a refactor before they could be codified. Hand-tested by injecting a deliberate wsauth -> orgtoken violation: the gate fires red with the rule message before merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
0b83faa33c
commit
68f18424f5
63
workspace-server/internal/db/architecture_test.go
Normal file
63
workspace-server/internal/db/architecture_test.go
Normal file
@ -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
|
||||
}
|
||||
63
workspace-server/internal/models/architecture_test.go
Normal file
63
workspace-server/internal/models/architecture_test.go
Normal file
@ -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
|
||||
}
|
||||
80
workspace-server/internal/provisioner/architecture_test.go
Normal file
80
workspace-server/internal/provisioner/architecture_test.go
Normal file
@ -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
|
||||
}
|
||||
68
workspace-server/internal/wsauth/architecture_test.go
Normal file
68
workspace-server/internal/wsauth/architecture_test.go
Normal file
@ -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
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user