Enforce module family import boundaries
This commit is contained in:
@@ -189,6 +189,11 @@ Seriatim registrars own their production leaf registrations. The D&D registrar
|
||||
owns D&D leaf registrations, the spell default-validator chain, and D&D
|
||||
prompt/schema asset collection.
|
||||
|
||||
Concrete implementation packages do not import generic implementation
|
||||
packages directly. A concrete family's `register` package is its composition
|
||||
point for specializing reusable generic implementations, while the generic
|
||||
registrar composes only generic children.
|
||||
|
||||
Framework packages must not import production extensions. Tests may compose
|
||||
registries and catalogs directly with fakes.
|
||||
|
||||
|
||||
@@ -41,69 +41,145 @@ func TestImportBoundaryFixtureIsRejected(t *testing.T) {
|
||||
fixture := filepath.Join(repositoryRoot, "internal", "modules", "generic", "testdata", "importboundaries", "imports_dnd.go")
|
||||
err := checkImportBoundaries(repositoryRoot, fixture)
|
||||
if err == nil {
|
||||
t.Fatal("fixture import was accepted, want generic-to-D&D violation")
|
||||
t.Fatal("fixture import was accepted, want generic-to-concrete violation")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "generic packages must not import D&D packages") {
|
||||
t.Fatalf("fixture error = %q, want generic-to-D&D violation", err)
|
||||
if !strings.Contains(err.Error(), "generic family must not import concrete family") {
|
||||
t.Fatalf("fixture error = %q, want generic-to-concrete violation", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestImportBoundaryRules(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
filename string
|
||||
importPath string
|
||||
wantError bool
|
||||
name string
|
||||
filename string
|
||||
sourcePackage string
|
||||
importPath string
|
||||
wantError bool
|
||||
}{
|
||||
{
|
||||
name: "D&D implementation cannot import Seriatim",
|
||||
filename: "internal/modules/dnd/extract/example/extractor.go",
|
||||
importPath: moduleImportPrefix + "seriatim/input/transcript",
|
||||
wantError: true,
|
||||
name: "concrete implementation cannot import peer concrete family",
|
||||
filename: "internal/modules/dnd/extract/example/extractor.go",
|
||||
sourcePackage: "example",
|
||||
importPath: moduleImportPrefix + "seriatim/input/transcript",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "Seriatim implementation cannot import D&D",
|
||||
filename: "internal/modules/seriatim/input/example/adapter.go",
|
||||
importPath: moduleImportPrefix + "dnd/shared",
|
||||
wantError: true,
|
||||
name: "future concrete family cannot import current concrete family",
|
||||
filename: "internal/modules/almanac/extract/example/extractor.go",
|
||||
sourcePackage: "example",
|
||||
importPath: moduleImportPrefix + "dnd/shared",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "generic implementation cannot import D&D",
|
||||
filename: "internal/modules/generic/merge/example/merger.go",
|
||||
importPath: moduleImportPrefix + "dnd/shared",
|
||||
wantError: true,
|
||||
name: "current concrete family cannot import future concrete family",
|
||||
filename: "internal/modules/seriatim/input/example/adapter.go",
|
||||
sourcePackage: "example",
|
||||
importPath: moduleImportPrefix + "almanac/shared",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "domain root cannot import child",
|
||||
filename: "internal/modules/dnd/types.go",
|
||||
importPath: moduleImportPrefix + "dnd/extract/spells",
|
||||
wantError: true,
|
||||
name: "generic implementation cannot import current concrete family",
|
||||
filename: "internal/modules/generic/merge/example/merger.go",
|
||||
sourcePackage: "example",
|
||||
importPath: moduleImportPrefix + "dnd/shared",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "D&D implementation may import generic implementation",
|
||||
filename: "internal/modules/dnd/extract/example/extractor.go",
|
||||
importPath: moduleImportPrefix + "generic/normalize/noop",
|
||||
name: "generic implementation cannot import future concrete family",
|
||||
filename: "internal/modules/generic/merge/example/merger.go",
|
||||
sourcePackage: "example",
|
||||
importPath: moduleImportPrefix + "almanac/shared",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "domain registrar may compose child packages",
|
||||
filename: "internal/modules/dnd/register/register.go",
|
||||
importPath: moduleImportPrefix + "dnd/extract/spells",
|
||||
name: "concrete implementation cannot import generic implementation",
|
||||
filename: "internal/modules/dnd/extract/example/extractor.go",
|
||||
sourcePackage: "example",
|
||||
importPath: moduleImportPrefix + "generic/normalize/noop",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "CLI may compose registrars",
|
||||
filename: "internal/cli/catalog.go",
|
||||
importPath: moduleImportPrefix + "dnd/register",
|
||||
name: "concrete white-box test follows production rules",
|
||||
filename: "internal/modules/dnd/extract/example/extractor_test.go",
|
||||
sourcePackage: "example",
|
||||
importPath: moduleImportPrefix + "generic/normalize/noop",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "external integration test may compose domains",
|
||||
filename: "internal/modules/integration/example_test.go",
|
||||
importPath: moduleImportPrefix + "dnd/extract/spells",
|
||||
name: "concrete registrar may compose generic implementation",
|
||||
filename: "internal/modules/almanac/register/register.go",
|
||||
sourcePackage: "register",
|
||||
importPath: moduleImportPrefix + "generic/normalize/noop",
|
||||
},
|
||||
{
|
||||
name: "family root cannot import child implementation",
|
||||
filename: "internal/modules/almanac/types.go",
|
||||
sourcePackage: "almanac",
|
||||
importPath: moduleImportPrefix + "almanac/extract/events",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "family root cannot import registrar",
|
||||
filename: "internal/modules/almanac/types.go",
|
||||
sourcePackage: "almanac",
|
||||
importPath: moduleImportPrefix + "almanac/register",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "child may import family root",
|
||||
filename: "internal/modules/almanac/extract/events/extractor.go",
|
||||
sourcePackage: "events",
|
||||
importPath: moduleImportPrefix + "almanac",
|
||||
},
|
||||
{
|
||||
name: "child may import same-family sibling",
|
||||
filename: "internal/modules/almanac/extract/events/extractor.go",
|
||||
sourcePackage: "events",
|
||||
importPath: moduleImportPrefix + "almanac/shared",
|
||||
},
|
||||
{
|
||||
name: "concrete registrar may compose own child",
|
||||
filename: "internal/modules/almanac/register/register.go",
|
||||
sourcePackage: "register",
|
||||
importPath: moduleImportPrefix + "almanac/extract/events",
|
||||
},
|
||||
{
|
||||
name: "generic registrar may compose generic child",
|
||||
filename: "internal/modules/generic/register/register.go",
|
||||
sourcePackage: "register",
|
||||
importPath: moduleImportPrefix + "generic/output/json",
|
||||
},
|
||||
{
|
||||
name: "application composition root may compose registrars",
|
||||
filename: "internal/cli/catalog.go",
|
||||
sourcePackage: "cli",
|
||||
importPath: moduleImportPrefix + "almanac/register",
|
||||
},
|
||||
{
|
||||
name: "black-box integration test may compose families",
|
||||
filename: "internal/modules/integration/example_test.go",
|
||||
sourcePackage: "integration_test",
|
||||
importPath: moduleImportPrefix + "almanac/extract/events",
|
||||
},
|
||||
{
|
||||
name: "white-box integration test is not exempt",
|
||||
filename: "internal/modules/integration/example_test.go",
|
||||
sourcePackage: "integration",
|
||||
importPath: moduleImportPrefix + "almanac/extract/events",
|
||||
wantError: true,
|
||||
},
|
||||
{
|
||||
name: "integration production file is not exempt",
|
||||
filename: "internal/modules/integration/example.go",
|
||||
sourcePackage: "integration",
|
||||
importPath: moduleImportPrefix + "almanac/extract/events",
|
||||
wantError: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
err := validateImport(tt.filename, tt.importPath)
|
||||
err := validateImport(tt.filename, tt.sourcePackage, tt.importPath)
|
||||
if tt.wantError && err == nil {
|
||||
t.Fatal("validateImport() error = nil, want boundary violation")
|
||||
}
|
||||
@@ -129,68 +205,77 @@ func checkImportBoundaries(repositoryRoot string, filename string) error {
|
||||
if err != nil {
|
||||
return fmt.Errorf("parse import in %s: %w", relative, err)
|
||||
}
|
||||
if err := validateImport(relative, importPath); err != nil {
|
||||
if err := validateImport(relative, parsed.Name.Name, importPath); err != nil {
|
||||
return fmt.Errorf("%s imports %s: %w", relative, importPath, err)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func validateImport(filename string, importPath string) error {
|
||||
if isExternalIntegrationTest(filename) {
|
||||
func validateImport(filename string, sourcePackage string, importPath string) error {
|
||||
if isBlackBoxIntegrationTest(filename, sourcePackage) {
|
||||
return nil
|
||||
}
|
||||
sourceDomain, sourceRoot := domainForFile(filename)
|
||||
targetDomain, targetChild := domainForImport(importPath)
|
||||
if sourceDomain == "" || targetDomain == "" {
|
||||
targetFamily, targetChild := moduleFamilyForImport(importPath)
|
||||
if targetFamily == "" {
|
||||
return nil
|
||||
}
|
||||
if sourceRoot && sourceDomain == targetDomain && targetChild {
|
||||
return fmt.Errorf("domain root packages must not import child implementations")
|
||||
if isIntegrationFile(filename) {
|
||||
return fmt.Errorf("module integration composition is allowed only in black-box tests")
|
||||
}
|
||||
if sourceDomain == "generic" && targetDomain == "dnd" {
|
||||
return fmt.Errorf("generic packages must not import D&D packages")
|
||||
sourceFamily, sourceRoot, sourceRegistrar := moduleFamilyForFile(filename)
|
||||
if sourceFamily == "" {
|
||||
return nil
|
||||
}
|
||||
if sourceDomain == "dnd" && targetDomain == "seriatim" {
|
||||
return fmt.Errorf("D&D packages must not import Seriatim packages")
|
||||
if sourceRoot && sourceFamily == targetFamily && targetChild {
|
||||
return fmt.Errorf("family root must not import child packages")
|
||||
}
|
||||
if sourceDomain == "seriatim" && targetDomain == "dnd" {
|
||||
return fmt.Errorf("Seriatim packages must not import D&D packages")
|
||||
if sourceFamily == targetFamily {
|
||||
return nil
|
||||
}
|
||||
return nil
|
||||
if sourceFamily == "generic" {
|
||||
return fmt.Errorf("generic family must not import concrete family %q", targetFamily)
|
||||
}
|
||||
if targetFamily == "generic" {
|
||||
if sourceRegistrar {
|
||||
return nil
|
||||
}
|
||||
return fmt.Errorf("concrete family %q may import generic implementations only from its registrar", sourceFamily)
|
||||
}
|
||||
return fmt.Errorf("concrete family %q must not import concrete family %q", sourceFamily, targetFamily)
|
||||
}
|
||||
|
||||
func domainForFile(filename string) (domain string, root bool) {
|
||||
func moduleFamilyForFile(filename string) (family string, root bool, registrar bool) {
|
||||
const prefix = "internal/modules/"
|
||||
if !strings.HasPrefix(filename, prefix) {
|
||||
return "", false
|
||||
return "", false, false
|
||||
}
|
||||
remainder := strings.TrimPrefix(filename, prefix)
|
||||
parts := strings.Split(remainder, "/")
|
||||
if len(parts) < 2 || !isDomain(parts[0]) {
|
||||
return "", false
|
||||
if len(parts) < 2 || parts[0] == "integration" {
|
||||
return "", false, false
|
||||
}
|
||||
return parts[0], len(parts) == 2
|
||||
return parts[0], len(parts) == 2, len(parts) > 2 && parts[1] == "register"
|
||||
}
|
||||
|
||||
func domainForImport(importPath string) (domain string, child bool) {
|
||||
func moduleFamilyForImport(importPath string) (family string, child bool) {
|
||||
if !strings.HasPrefix(importPath, moduleImportPrefix) {
|
||||
return "", false
|
||||
}
|
||||
remainder := strings.TrimPrefix(importPath, moduleImportPrefix)
|
||||
parts := strings.Split(remainder, "/")
|
||||
if len(parts) == 0 || !isDomain(parts[0]) {
|
||||
if len(parts) == 0 || parts[0] == "" || parts[0] == "integration" {
|
||||
return "", false
|
||||
}
|
||||
return parts[0], len(parts) > 1
|
||||
}
|
||||
|
||||
func isDomain(name string) bool {
|
||||
return name == "dnd" || name == "generic" || name == "seriatim"
|
||||
func isIntegrationFile(filename string) bool {
|
||||
return strings.HasPrefix(filename, "internal/modules/integration/")
|
||||
}
|
||||
|
||||
func isExternalIntegrationTest(filename string) bool {
|
||||
return strings.HasPrefix(filename, "internal/modules/integration/") && strings.HasSuffix(filename, "_test.go")
|
||||
func isBlackBoxIntegrationTest(filename string, sourcePackage string) bool {
|
||||
return isIntegrationFile(filename) && strings.HasSuffix(filename, "_test.go") && sourcePackage == "integration_test"
|
||||
}
|
||||
|
||||
func testRepositoryRoot(t *testing.T) string {
|
||||
|
||||
@@ -12,8 +12,6 @@ import (
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/core/source"
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/framework/contracts"
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline"
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/modules/generic/merge/appendorder"
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/modules/generic/normalize/noop"
|
||||
)
|
||||
|
||||
func TestPipelineConfigLoadsAndResolvesWithSeriatimInput(t *testing.T) {
|
||||
@@ -192,12 +190,7 @@ func mustRegisterExtractor(t *testing.T, registry *pipeline.ExtractorRegistry, s
|
||||
func mustRegisterMerger(t *testing.T, registry *pipeline.MergerRegistry, spec pipeline.ModuleSpec) {
|
||||
t.Helper()
|
||||
if err := pipeline.RegisterMerger[seriatimArtifact](registry, spec, func() (contracts.Merger[seriatimArtifact], error) {
|
||||
return appendorder.NewTyped(func(values []seriatimArtifact) (seriatimArtifact, error) {
|
||||
if len(values) == 0 {
|
||||
return seriatimArtifact{}, nil
|
||||
}
|
||||
return values[0], nil
|
||||
})
|
||||
return fakeMerger{}, nil
|
||||
}); err != nil {
|
||||
t.Fatalf("register merger: %v", err)
|
||||
}
|
||||
@@ -206,7 +199,7 @@ func mustRegisterMerger(t *testing.T, registry *pipeline.MergerRegistry, spec pi
|
||||
func mustRegisterNormalizer(t *testing.T, registry *pipeline.NormalizerRegistry, spec pipeline.ModuleSpec) {
|
||||
t.Helper()
|
||||
if err := pipeline.RegisterNormalizer[seriatimArtifact](registry, spec, func() (contracts.Normalizer[seriatimArtifact], error) {
|
||||
return noop.NewTyped[seriatimArtifact](), nil
|
||||
return fakeNormalizer{}, nil
|
||||
}); err != nil {
|
||||
t.Fatalf("register normalizer: %v", err)
|
||||
}
|
||||
@@ -241,6 +234,27 @@ func (fakeExtractor) Extract(ctx context.Context, req contracts.TypedExtractionR
|
||||
return contracts.TypedExtractionResult[seriatimArtifact]{}, nil
|
||||
}
|
||||
|
||||
type fakeMerger struct{}
|
||||
|
||||
func (fakeMerger) Key() string { return pipeline.DefaultMergeModule }
|
||||
|
||||
func (fakeMerger) Merge(ctx context.Context, req contracts.TypedMergeRequest[seriatimArtifact]) (contracts.TypedMergeResult[seriatimArtifact], error) {
|
||||
if len(req.ExtractOutputs) == 0 {
|
||||
return contracts.TypedMergeResult[seriatimArtifact]{}, nil
|
||||
}
|
||||
return contracts.TypedMergeResult[seriatimArtifact]{Value: req.ExtractOutputs[0].Value}, nil
|
||||
}
|
||||
|
||||
type fakeNormalizer struct{}
|
||||
|
||||
func (fakeNormalizer) Key() string { return pipeline.DefaultNormalizeModule }
|
||||
|
||||
func (fakeNormalizer) ReferenceSlots() []contracts.ReferenceSlot { return nil }
|
||||
|
||||
func (fakeNormalizer) Normalize(ctx context.Context, req contracts.TypedNormalizeRequest[seriatimArtifact]) (contracts.TypedNormalizeResult[seriatimArtifact], error) {
|
||||
return contracts.TypedNormalizeResult[seriatimArtifact]{Value: req.MergeOutput.Value}, nil
|
||||
}
|
||||
|
||||
type fakeOutput struct{}
|
||||
|
||||
func (fakeOutput) Key() string { return pipeline.DefaultOutputModule }
|
||||
|
||||
@@ -11,8 +11,6 @@ import (
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/core/source"
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/framework/contracts"
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline"
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/modules/generic/merge/appendorder"
|
||||
"gitea.maximumdirect.net/eric/notarius/internal/modules/generic/normalize/noop"
|
||||
)
|
||||
|
||||
func runPreparedPipeline(t *testing.T, registries pipeline.Registries, resolved pipeline.ResolvedPipeline, llmClient contracts.StructuredLLMClient, input pipeline.RunInput) (pipeline.RunOutput, error) {
|
||||
@@ -138,15 +136,14 @@ func seriatimRunnerRegistries(t *testing.T, extractor contracts.Extractor[seriat
|
||||
}); err != nil {
|
||||
t.Fatalf("register extractor: %v", err)
|
||||
}
|
||||
if err := appendorder.RegisterTyped(mergers, seriatimArtifactKind, func(values []seriatimArtifact) (seriatimArtifact, error) {
|
||||
if len(values) == 0 {
|
||||
return seriatimArtifact{}, nil
|
||||
}
|
||||
return values[0], nil
|
||||
if err := pipeline.RegisterMerger[seriatimArtifact](mergers, pipeline.ModuleSpec{Key: pipeline.DefaultMergeModule, Stage: pipeline.StageMerge, ArtifactKind: seriatimArtifactKind}, func() (contracts.Merger[seriatimArtifact], error) {
|
||||
return fakeMerger{}, nil
|
||||
}); err != nil {
|
||||
t.Fatalf("register merger: %v", err)
|
||||
}
|
||||
if err := noop.RegisterTyped[seriatimArtifact](normalizers, seriatimArtifactKind); err != nil {
|
||||
if err := pipeline.RegisterNormalizer[seriatimArtifact](normalizers, pipeline.ModuleSpec{Key: pipeline.DefaultNormalizeModule, Stage: pipeline.StageNormalize, ArtifactKind: seriatimArtifactKind}, func() (contracts.Normalizer[seriatimArtifact], error) {
|
||||
return fakeNormalizer{}, nil
|
||||
}); err != nil {
|
||||
t.Fatalf("register normalizer: %v", err)
|
||||
}
|
||||
if err := outputs.Register(pipeline.DefaultOutputModule, func() (contracts.OutputEncoder, error) {
|
||||
|
||||
Reference in New Issue
Block a user