From edc4732572742d83faf4935d0b2895a9774f70cd Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 14:03:36 -0700 Subject: [PATCH 01/22] feat(schema): add scope and imports to the v1 format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A model may declare a kebab-case `scope:` slug and list `imports:` — paths to peer model files, relative to itself — so an item defined elsewhere can be referenced as `slug.Name` (ADR-0010). Both are optional; the slug is declared by the imported model and never restated by the importer. Schema and structs move together, per TestSchemaStructSync. Signed-off-by: Joe Beda --- internal/model/model.go | 24 ++++++++++++++++-------- internal/schema/v1/modelith.schema.json | 13 +++++++++++++ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/internal/model/model.go b/internal/model/model.go index 9c1e925..64d389a 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -18,14 +18,22 @@ import ( // Model is the top-level domain model document. type Model struct { - Kind string `json:"kind"` - Version string `json:"version"` - Title string `json:"title,omitempty"` - Description string `json:"description,omitempty"` - Glossary map[string]string `json:"glossary,omitempty"` - Enums map[string]Enum `json:"enums,omitempty"` - Entities map[string]Entity `json:"entities,omitempty"` - Scenarios []Scenario `json:"scenarios,omitempty"` + Kind string `json:"kind"` + Version string `json:"version"` + Title string `json:"title,omitempty"` + Description string `json:"description,omitempty"` + // Scope is the slug another model writes before an item's name + // ("payments.PaymentMethod") when it imports this one. It is declared here + // and nowhere restated, so nothing can disagree about the slug. + Scope string `json:"scope,omitempty"` + // Imports are paths to other model files, relative to this one, whose items + // this model may reference. Resolution does not recurse: an imported model's + // own imports are not reachable from here (ADR-0010). + Imports []string `json:"imports,omitempty"` + Glossary map[string]string `json:"glossary,omitempty"` + Enums map[string]Enum `json:"enums,omitempty"` + Entities map[string]Entity `json:"entities,omitempty"` + Scenarios []Scenario `json:"scenarios,omitempty"` // Invariants are model-level rules that span several entities and have no // single owner. They share the per-entity invariant shape, and their ids // share one namespace with entity invariants (unique across the model). diff --git a/internal/schema/v1/modelith.schema.json b/internal/schema/v1/modelith.schema.json index dae7599..623ec5b 100644 --- a/internal/schema/v1/modelith.schema.json +++ b/internal/schema/v1/modelith.schema.json @@ -25,6 +25,19 @@ "description": "Optional one-paragraph summary of what this model covers.", "type": "string" }, + "scope": { + "description": "Kebab-case slug naming this model where another model references its items as \"scope.Name\". Optional — a model nobody imports never needs one — but required in order to be imported. Declared here and nowhere restated: an importing model lists a path, not a slug.", + "type": "string", + "pattern": "^[a-z][a-z0-9-]+$" + }, + "imports": { + "description": "Paths to other model files whose items this model references, each relative to this file (\"..\" is fine; an absolute path is not portable across checkouts and is rejected). Each listed file declares its own \"scope\". Resolution does not recurse: only items defined directly in a listed file are reachable.", + "type": "array", + "items": { + "type": "string", + "minLength": 1 + } + }, "glossary": { "description": "Ubiquitous-language terms that are NOT entities — roles, states of being, domain nouns — mapped to their definitions. Any term used as a relationship role or scenario actor should be defined here so the vocabulary is explicit and checkable. Keys are the term in PascalCase (e.g. Owner, Member).", "type": "object", From 2546612f20f588bcef79346e1bf8bd7cedc0fca6 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 14:07:33 -0700 Subject: [PATCH 02/22] feat(lint): resolve local imports and qualified attribute types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lint.Run now takes the model's path and a one-method FileReader seam so it can read the peer models an `imports:` list names. fs.FS was rejected: fs.ValidPath forbids "..", which peer models in a sibling directory require. Resolution is non-recursive (ADR-0010): only items defined directly in a listed file are reachable, so mutual imports terminate with no cycle detection. An unresolvable qualified type is an error, where an unqualified PascalCase type that names no enum stays a warning — it may be a primitive the author invented, while scope.Name can only be a cross-model reference. A qualified value in an entity position (relationship.entity, subtypeOf) gets a friendly error ahead of the schema's opaque pattern violation, which is then suppressed so one mistake reads as one finding. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 2 +- internal/lint/imports.go | 210 ++++++++++++++++++ internal/lint/imports_test.go | 386 ++++++++++++++++++++++++++++++++++ internal/lint/lint.go | 44 +++- internal/lint/lint_test.go | 96 +++++---- 5 files changed, 681 insertions(+), 57 deletions(-) create mode 100644 internal/lint/imports.go create mode 100644 internal/lint/imports_test.go diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index a6fcff6..51da14d 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -127,7 +127,7 @@ func lintCmd() *cobra.Command { if err != nil { return fmt.Errorf("%s: %w", path, err) } - res, err := lint.Run(data) + res, err := lint.Run(path, data, lint.OSFiles{}) if err != nil { return fmt.Errorf("%s: %w", path, err) } diff --git a/internal/lint/imports.go b/internal/lint/imports.go new file mode 100644 index 0000000..9834192 --- /dev/null +++ b/internal/lint/imports.go @@ -0,0 +1,210 @@ +package lint + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + + "github.com/stacklok/modelith/internal/model" +) + +// FileReader reads an imported model file by path. It is the seam that keeps +// lint's file access substitutable in tests. +// +// It is not an fs.FS: fs.ValidPath rejects "..", and peer models commonly sit +// in sibling directories, so "../payments/payments.modelith.yaml" has to work. +type FileReader interface { + ReadFile(path string) ([]byte, error) +} + +// OSFiles reads imported models from the local filesystem. +type OSFiles struct{} + +// ReadFile reads the named file from the local filesystem. +func (OSFiles) ReadFile(path string) ([]byte, error) { return os.ReadFile(path) } + +// qualifiedRefRE matches a cross-model reference, "scope.Name". The scope side +// mirrors the schema's scope pattern; the name side stays loose so a reference +// to something that isn't a defined enum is reported as a broken reference +// rather than silently read as a primitive type. +var qualifiedRefRE = regexp.MustCompile(`^([a-z][a-z0-9-]*)\.([A-Za-z0-9]+)$`) + +// importedModel is one successfully resolved entry of a model's imports list. +type importedModel struct { + index int // position in the importing model's imports list, for the finding path + path string // the path as written in imports + model *model.Model +} + +// runImports resolves the model's imports, checks every qualified attribute +// type against them, and reports an import nothing references. +// +// modelPath is the path of the model being linted; imports resolve relative to +// its directory. +func runImports(modelPath string, m *model.Model, fr FileReader, res *Result) { + byScope := loadImports(modelPath, m, fr, res) + used := checkQualifiedTypes(m, byScope, res) + for _, scope := range sortedMapKeys(byScope) { + if used[scope] { + continue + } + imp := byScope[scope] + res.Findings = append(res.Findings, Finding{ + Severity: SeverityWarning, + Category: CategorySemantic, + Path: fmt.Sprintf("/imports/%d", imp.index), + Message: fmt.Sprintf( + "import %q (scope %q) is never referenced — drop it, or reference one of its enums as %s.Name in an attribute type", + imp.path, scope, scope, + ), + }) + } +} + +// loadImports reads each declared import and returns the ones that resolved, +// keyed by the scope they declare. Every rejection is reported as an error: an +// import that cannot be resolved is a broken reference, not a gap. +func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) map[string]importedModel { + byScope := map[string]importedModel{} + dir := filepath.Dir(modelPath) + for i, imp := range m.Imports { + reject := func(format string, args ...any) { + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategorySemantic, + Path: fmt.Sprintf("/imports/%d", i), + Message: fmt.Sprintf(format, args...), + }) + } + if imp == "" { + continue // the schema's minLength reports the empty path + } + if filepath.IsAbs(imp) || imp[0] == '/' { + reject("import %q is an absolute path — imports are relative to this model so they resolve in any checkout", imp) + continue + } + data, err := fr.ReadFile(filepath.Join(dir, imp)) + if err != nil { + reject("import %q cannot be read: %v", imp, err) + continue + } + im, err := model.Parse(data) + if err != nil || im.Kind != "DomainModel" { + reject("import %q is not a domain model — lint it on its own with `modelith lint` to see why", imp) + continue + } + if im.Scope == "" { + reject("import %q declares no `scope:` — a model needs a scope slug to be imported, since that slug is how this model names its items", imp) + continue + } + if prev, ok := byScope[im.Scope]; ok { + reject("import %q declares scope %q, which import %q already declares — rename one model's scope so %s.Name resolves unambiguously", + imp, im.Scope, prev.path, im.Scope) + continue + } + byScope[im.Scope] = importedModel{index: i, path: imp, model: im} + } + return byScope +} + +// checkQualifiedTypes resolves every qualified attribute type against the +// imports and returns the set of scopes that were referenced. +// +// A qualified type that does not resolve is an error, where an unqualified +// PascalCase type that names no enum is only a warning (runSemantic). The +// asymmetry is deliberate: an unqualified name may be a primitive the author +// invented, while "scope.Name" can only be a cross-model reference. +func checkQualifiedTypes(m *model.Model, byScope map[string]importedModel, res *Result) map[string]bool { + used := map[string]bool{} + for _, name := range m.EntityNames() { + for i, attr := range m.Entities[name].Attributes { + match := qualifiedRefRE.FindStringSubmatch(attr.Type) + if match == nil { + continue + } + scope, item := match[1], match[2] + path := fmt.Sprintf("/entities/%s/attributes/%d/type", name, i) + broken := func(format string, args ...any) { + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategorySemantic, + Path: path, + Message: fmt.Sprintf(format, args...), + }) + } + imp, ok := byScope[scope] + if !ok { + broken("attribute type %q references scope %q, which this model does not import — add the model that declares that scope to `imports:`", attr.Type, scope) + continue + } + // The import is referenced even when the item does not resolve, so + // the same mistake never reads as an unused import too. + used[scope] = true + if _, ok := imp.model.Enums[item]; ok { + continue + } + if _, ok := imp.model.Entities[item]; ok { + broken("attribute type %q resolves to the entity %q in %q — only an enum can be referenced across models", attr.Type, item, imp.path) + continue + } + broken("attribute type %q names no enum %q in %q — an imported model's own imports are not reachable from here", attr.Type, item, imp.path) + } + } + return used +} + +// reportQualifiedEntityRefs reports a cross-model reference in an entity +// position — relationship.entity or subtypeOf — and returns the instance paths +// it reported, so the schema's own finding for the same value is suppressed. +// +// Both fields carry pattern ^[A-Z][A-Za-z0-9]+$, so "payments.Card" already +// fails validation with a message about a pattern. This says what is actually +// wrong, in the spirit of the unsupported-version check. Cross-model entity +// references are deferred, not planned against: ADR-0010 records why. +func reportQualifiedEntityRefs(inst any, res *Result) map[string]bool { + reported := map[string]bool{} + doc, ok := inst.(map[string]any) + if !ok { + return reported + } + entities, ok := doc["entities"].(map[string]any) + if !ok { + return reported + } + report := func(path, value string) { + reported[path] = true + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategoryStructural, + Path: path, + Message: fmt.Sprintf( + "%q is a cross-model reference, which is not supported in an entity position — only an attribute `type` can be qualified as scope.Name", + value, + ), + }) + } + for _, name := range sortedMapKeys(entities) { + ent, ok := entities[name].(map[string]any) + if !ok { + continue + } + if parent, ok := ent["subtypeOf"].(string); ok && qualifiedRefRE.MatchString(parent) { + report(fmt.Sprintf("/entities/%s/subtypeOf", name), parent) + } + rels, ok := ent["relationships"].([]any) + if !ok { + continue + } + for i, r := range rels { + rel, ok := r.(map[string]any) + if !ok { + continue + } + if target, ok := rel["entity"].(string); ok && qualifiedRefRE.MatchString(target) { + report(fmt.Sprintf("/entities/%s/relationships/%d/entity", name, i), target) + } + } + } + return reported +} diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go new file mode 100644 index 0000000..9c53dc3 --- /dev/null +++ b/internal/lint/imports_test.go @@ -0,0 +1,386 @@ +package lint + +import ( + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" +) + +// fakeFiles is a map-backed FileReader keyed by cleaned, slash-separated path. +// A hand-written fake rather than a mock: it behaves like the filesystem, +// including reporting a missing file the way os.ReadFile does. +type fakeFiles map[string]string + +func (f fakeFiles) ReadFile(path string) ([]byte, error) { + src, ok := f[filepath.ToSlash(filepath.Clean(path))] + if !ok { + return nil, fmt.Errorf("open %s: %w", path, fs.ErrNotExist) + } + return []byte(src), nil +} + +// importerPath is where the model under test lives; its imports resolve +// relative to the "docs" directory. +const importerPath = "docs/garage.modelith.yaml" + +const paymentsModel = `kind: DomainModel +version: v1 +scope: payments +title: Payments +enums: + PaymentMethod: + description: How a bill is settled. + values: + - name: card + - name: transfer +entities: + Invoice: + definition: A request for payment. +` + +// importer builds a model that lists the given imports and types one attribute +// with the given type. +func importer(imports []string, attrType string) string { + var b strings.Builder + b.WriteString("kind: DomainModel\nversion: v1\nscope: garage\n") + if len(imports) > 0 { + b.WriteString("imports:\n") + for _, imp := range imports { + fmt.Fprintf(&b, " - %q\n", imp) + } + } + fmt.Fprintf(&b, `entities: + Visit: + definition: One car's stay in the garage. + attributes: + - name: settledWith + type: %s +`, attrType) + return b.String() +} + +type wantFinding struct { + severity Severity + category Category + path string + contains string +} + +// importFindings narrows a result to the surfaces these tests drive: the +// imports list and the reference sites that resolve against it. Completeness +// gaps in the deliberately thin fixtures are not what is under test. +func importFindings(all []Finding) []Finding { + var out []Finding + for _, f := range all { + switch { + case strings.HasPrefix(f.Path, "/imports/"), + strings.HasSuffix(f.Path, "/type"), + strings.HasSuffix(f.Path, "/entity"), + strings.HasSuffix(f.Path, "/subtypeOf"): + out = append(out, f) + } + } + return out +} + +func assertFindings(t *testing.T, got []Finding, want []wantFinding) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("expected %d finding(s), got %d: %+v", len(want), len(got), got) + } + for i, w := range want { + g := got[i] + if g.Severity != w.severity || g.Category != w.category || g.Path != w.path { + t.Errorf("finding %d: got %s/%s at %q, want %s/%s at %q", + i, g.Severity, g.Category, g.Path, w.severity, w.category, w.path) + } + if !strings.Contains(g.Message, w.contains) { + t.Errorf("finding %d: message %q does not contain %q", i, g.Message, w.contains) + } + } +} + +func TestImports_Resolution(t *testing.T) { + t.Parallel() + + files := fakeFiles{ + "docs/payments.modelith.yaml": paymentsModel, + "payments/payments.modelith.yaml": paymentsModel, + "docs/billing.modelith.yaml": strings.Replace(paymentsModel, + "title: Payments", "title: Billing", 1), + "docs/anonymous.modelith.yaml": strings.Replace(paymentsModel, + "scope: payments\n", "", 1), + "docs/not-a-model.yaml": "kind: SomethingElse\nversion: v1\n", + } + + const typePath = "/entities/Visit/attributes/0/type" + + cases := []struct { + name string + imports []string + attrType string + want []wantFinding + }{ + { + name: "peer import resolves", + imports: []string{"./payments.modelith.yaml"}, + attrType: "payments.PaymentMethod", + }, + { + name: "parent-relative import resolves", + imports: []string{"../payments/payments.modelith.yaml"}, + attrType: "payments.PaymentMethod", + }, + { + name: "qualified type with no imports at all", + attrType: "payments.PaymentMethod", + want: []wantFinding{{ + SeverityError, CategorySemantic, typePath, + `references scope "payments", which this model does not import`, + }}, + }, + { + name: "qualified type naming an unimported scope", + imports: []string{"./payments.modelith.yaml"}, + attrType: "shipping.Carrier", + want: []wantFinding{ + {SeverityError, CategorySemantic, typePath, + `references scope "shipping", which this model does not import`}, + {SeverityWarning, CategorySemantic, "/imports/0", "is never referenced"}, + }, + }, + { + name: "qualified type naming no enum in the imported model", + imports: []string{"./payments.modelith.yaml"}, + attrType: "payments.Nonexistent", + want: []wantFinding{{ + SeverityError, CategorySemantic, typePath, + `names no enum "Nonexistent"`, + }}, + }, + { + name: "qualified type resolving to an entity, not an enum", + imports: []string{"./payments.modelith.yaml"}, + attrType: "payments.Invoice", + want: []wantFinding{{ + SeverityError, CategorySemantic, typePath, + `resolves to the entity "Invoice"`, + }}, + }, + { + name: "two imports declaring the same scope", + imports: []string{"./payments.modelith.yaml", "./billing.modelith.yaml"}, + attrType: "payments.PaymentMethod", + want: []wantFinding{{ + SeverityError, CategorySemantic, "/imports/1", + `declares scope "payments", which import "./payments.modelith.yaml" already declares`, + }}, + }, + { + name: "import declaring no scope", + imports: []string{"./anonymous.modelith.yaml"}, + attrType: "string", + want: []wantFinding{{ + SeverityError, CategorySemantic, "/imports/0", + "declares no `scope:`", + }}, + }, + { + name: "absolute import path", + imports: []string{"/abs/payments.modelith.yaml"}, + attrType: "string", + want: []wantFinding{{ + SeverityError, CategorySemantic, "/imports/0", + "is an absolute path", + }}, + }, + { + name: "unreadable import", + imports: []string{"./missing.modelith.yaml"}, + attrType: "string", + want: []wantFinding{{ + SeverityError, CategorySemantic, "/imports/0", + "cannot be read", + }}, + }, + { + name: "import that is not a domain model", + imports: []string{"./not-a-model.yaml"}, + attrType: "string", + want: []wantFinding{{ + SeverityError, CategorySemantic, "/imports/0", + "is not a domain model", + }}, + }, + { + name: "import nothing references", + imports: []string{"./payments.modelith.yaml"}, + attrType: "string", + want: []wantFinding{{ + SeverityWarning, CategorySemantic, "/imports/0", + "is never referenced", + }}, + }, + { + name: "unqualified PascalCase type is still only a warning", + attrType: "PaymentMethod", + want: []wantFinding{{ + SeverityWarning, CategorySemantic, typePath, + "looks like an enum reference", + }}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + res, err := Run(importerPath, []byte(importer(tc.imports, tc.attrType)), files) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), tc.want) + }) + } +} + +// TestADR_0010_NonTransitiveResolution pins that resolution does not recurse: +// an item defined in a model that an *imported* model imports is unreachable, +// and the file it lives in is never read. +func TestADR_0010_NonTransitiveResolution(t *testing.T) { + t.Parallel() + + const middle = `kind: DomainModel +version: v1 +scope: payments +imports: + - "./shipping.modelith.yaml" +entities: + Invoice: + definition: A request for payment. +` + const leaf = `kind: DomainModel +version: v1 +scope: shipping +enums: + Carrier: + values: + - name: ground +` + read := map[string]int{} + files := countingFiles{ + files: fakeFiles{ + "docs/payments.modelith.yaml": middle, + "docs/shipping.modelith.yaml": leaf, + }, + read: read, + } + + res, err := Run(importerPath, []byte(importer([]string{"./payments.modelith.yaml"}, "shipping.Carrier")), files) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), []wantFinding{ + {SeverityError, CategorySemantic, "/entities/Visit/attributes/0/type", + `references scope "shipping", which this model does not import`}, + {SeverityWarning, CategorySemantic, "/imports/0", "is never referenced"}, + }) + if n := read["docs/shipping.modelith.yaml"]; n != 0 { + t.Errorf("an imported model's own imports must not be read, but shipping was read %d time(s)", n) + } +} + +// countingFiles records how many times each path was read, so a test can assert +// a file was never opened. +type countingFiles struct { + files fakeFiles + read map[string]int +} + +func (c countingFiles) ReadFile(path string) ([]byte, error) { + c.read[filepath.ToSlash(filepath.Clean(path))]++ + return c.files.ReadFile(path) +} + +// TestRun_QualifiedEntityReferenceIsDeferred checks the friendly error for a +// cross-model reference in an entity position, and that it replaces — rather +// than joins — the schema's pattern violation and the undefined-entity finding. +func TestRun_QualifiedEntityReferenceIsDeferred(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + src string + path string + }{ + { + name: "relationship target", + path: "/entities/Visit/relationships/0/entity", + src: `kind: DomainModel +version: v1 +entities: + Visit: + definition: One car's stay in the garage. + relationships: + - entity: payments.Invoice + cardinality: "n:1" +`, + }, + { + name: "subtypeOf", + path: "/entities/Visit/subtypeOf", + src: `kind: DomainModel +version: v1 +entities: + Visit: + definition: One car's stay in the garage. + subtypeOf: payments.Invoice +`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + res, err := Run(importerPath, []byte(tc.src), fakeFiles{}) + if err != nil { + t.Fatal(err) + } + var at []Finding + for _, f := range res.Findings { + if f.Path == tc.path { + at = append(at, f) + } + } + assertFindings(t, at, []wantFinding{{ + SeverityError, CategoryStructural, tc.path, + "is a cross-model reference, which is not supported in an entity position", + }}) + if !res.HasBlocking(false) { + t.Error("an unsupported cross-model entity reference must block") + } + if findingWithMessage(res.Findings, "undefined entity") { + t.Errorf("one mistake reported twice: %+v", res.Findings) + } + }) + } +} + +// TestRun_NilFileReaderReadsFromDisk covers the documented default: a nil +// FileReader resolves imports against the local filesystem. +func TestRun_NilFileReaderReadsFromDisk(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "payments.modelith.yaml"), []byte(paymentsModel), 0o600); err != nil { + t.Fatal(err) + } + src := importer([]string{"./payments.modelith.yaml"}, "payments.PaymentMethod") + res, err := Run(filepath.Join(dir, "garage.modelith.yaml"), []byte(src), nil) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), nil) +} diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 4a7a385..33ccb48 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -77,17 +77,22 @@ var ( printer = message.NewPrinter(language.English) ) -// Run validates the given YAML bytes and returns all findings. -func Run(data []byte) (*Result, error) { +// Run validates the model at path and returns all findings. src is the model's +// own bytes; fr reads the files its `imports:` name, resolved relative to +// path's directory. A nil fr reads them from the local filesystem. +func Run(path string, src []byte, fr FileReader) (*Result, error) { res := &Result{} + if fr == nil { + fr = OSFiles{} + } // Layer 1: structural validation against the JSON Schema. - structuralOK := runStructural(data, res) + structuralOK := runStructural(src, res) // If it does not even parse into our typed model, stop — semantic and // completeness checks need a model to work with. The structural layer has // already reported why. - m, err := model.Parse(data) + m, err := model.Parse(src) if err != nil { if structuralOK { // Schema passed but typed parsing failed; surface it so we never @@ -104,6 +109,7 @@ func Run(data []byte) (*Result, error) { } runSemantic(m, res) + runImports(path, m, fr, res) runRelationshipShape(m, res) runSubtypes(m, res) runReciprocity(m, res) @@ -182,10 +188,15 @@ func runStructural(data []byte, res *Result) bool { return false } + before := len(res.Findings) + // Say what a cross-model entity reference actually is before the schema + // reports it as a pattern violation, and suppress its opaque message so one + // mistake reads as one finding. + qualified := reportQualifiedEntityRefs(inst, res) + if err := sch.Validate(inst); err != nil { if ve, ok := err.(*jsonschema.ValidationError); ok { - before := len(res.Findings) - collectLeaves(ve, res) + collectLeaves(ve, res, qualified) return len(res.Findings) == before } res.Findings = append(res.Findings, Finding{ @@ -195,15 +206,18 @@ func runStructural(data []byte, res *Result) bool { }) return false } - return true + return len(res.Findings) == before } -func collectLeaves(e *jsonschema.ValidationError, res *Result) { +func collectLeaves(e *jsonschema.ValidationError, res *Result, skip map[string]bool) { if len(e.Causes) == 0 { ptr := "/" + strings.Join(e.InstanceLocation, "/") if ptr == "/" { ptr = "" } + if skip[ptr] { + return + } msg := e.Error() if e.ErrorKind != nil { msg = e.ErrorKind.LocalizedString(printer) @@ -217,7 +231,7 @@ func collectLeaves(e *jsonschema.ValidationError, res *Result) { return } for _, c := range e.Causes { - collectLeaves(c, res) + collectLeaves(c, res, skip) } } @@ -286,14 +300,19 @@ func runSemantic(m *model.Model, res *Result) { for _, name := range m.EntityNames() { ent := m.Entities[name] for i, rel := range ent.Relationships { - if !entitySet[rel.Entity] { + switch { + case qualifiedRefRE.MatchString(rel.Entity): + // Already reported as an unsupported cross-model reference by + // reportQualifiedEntityRefs; calling it an undefined entity too + // would report one mistake twice. + case !entitySet[rel.Entity]: res.Findings = append(res.Findings, Finding{ Severity: SeverityError, Category: CategorySemantic, Path: fmt.Sprintf("/entities/%s/relationships/%d/entity", name, i), Message: fmt.Sprintf("relationship targets undefined entity %q", rel.Entity), }) - } else if rel.Ownership == "owned" && m.Entities[rel.Entity].Derived { + case rel.Ownership == "owned" && m.Entities[rel.Entity].Derived: res.Findings = append(res.Findings, Finding{ Severity: SeverityWarning, Category: CategorySemantic, @@ -536,6 +555,9 @@ func runSubtypes(m *model.Model, res *Result) { if parent == "" { continue } + if qualifiedRefRE.MatchString(parent) { + continue // reported as an unsupported cross-model reference + } if _, ok := m.Entities[parent]; !ok { res.Findings = append(res.Findings, Finding{ Severity: SeverityError, diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go index d5a5491..56eba57 100644 --- a/internal/lint/lint_test.go +++ b/internal/lint/lint_test.go @@ -7,6 +7,11 @@ import ( "testing" ) +// testModelPath stands in for the linted file's path. Only a model with +// imports cares what it is (they resolve relative to its directory), so the +// inline fixtures here pass it and a nil FileReader. +const testModelPath = "model.modelith.yaml" + func countBy(fs []Finding, sev Severity, cat Category) int { n := 0 for _, f := range fs { @@ -18,11 +23,12 @@ func countBy(fs []Finding, sev Severity, cat Category) int { } func TestExampleIsClean(t *testing.T) { - data, err := os.ReadFile("../../examples/example.modelith.yaml") + const path = "../../examples/example.modelith.yaml" + data, err := os.ReadFile(path) if err != nil { t.Fatal(err) } - res, err := Run(data) + res, err := Run(path, data, nil) if err != nil { t.Fatal(err) } @@ -53,7 +59,7 @@ entities: } for name, src := range cases { t.Run(name, func(t *testing.T) { - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -70,7 +76,7 @@ entities: func TestMalformedYAMLIsStructuralError(t *testing.T) { // Unbalanced brackets — not parseable as YAML at all. src := "kind: DomainModel\nentities: {Project: [unterminated\n" - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -92,7 +98,7 @@ func TestNonObjectDocumentIsStructuralError(t *testing.T) { // A bare scalar is valid YAML/JSON but not an object: the schema rejects it // and lint should still produce a blocking structural finding rather than // proceeding to typed parsing. - res, err := Run([]byte(`"just a string"`)) + res, err := Run(testModelPath, []byte(`"just a string"`), nil) if err != nil { t.Fatal(err) } @@ -109,7 +115,7 @@ entities: Project: definition: A container. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -138,7 +144,7 @@ entities: - entity: Ghost cardinality: "1:n" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -162,7 +168,7 @@ scenarios: steps: - "Use the ` + "`Project`" + `" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -197,7 +203,7 @@ entities: - id: always-valid statement: "Always valid" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -229,7 +235,7 @@ scenarios: invariants_touched: - has-an-owner ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -283,7 +289,7 @@ entities: - entity: Project cardinality: "1:1" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -313,7 +319,7 @@ entities: - entity: Project cardinality: "n:1" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -345,7 +351,7 @@ entities: - entity: Project cardinality: "1:1" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -362,7 +368,7 @@ entities: Lonely: definition: An entity with no invariants and no scenario. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -394,7 +400,7 @@ scenarios: steps: ["the ` + "`Project`" + ` does a thing"] invariants_touched: [no-such-rule] ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -422,7 +428,7 @@ entities: - id: dup statement: "Second" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -447,7 +453,7 @@ invariants: - id: dup statement: "Model-level rule for the ` + "`Project`" + `" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -482,7 +488,7 @@ scenarios: steps: ["the ` + "`Project`" + ` is archived"] invariants_touched: [cross-entity-rule] ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -511,7 +517,7 @@ scenarios: steps: ["the ` + "`Project`" + ` is used"] invariants_touched: [ghost-model-rule] ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -537,7 +543,7 @@ entities: User: definition: A principal. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -569,7 +575,7 @@ scenarios: actors: [Maintainer] steps: ["a ` + "`Maintainer`" + ` touches the ` + "`Project`" + ` and the ` + "`User`" + `"] ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -616,7 +622,7 @@ entities: User: definition: A principal. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -664,7 +670,7 @@ entities: User: definition: A principal. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -752,7 +758,7 @@ entities: - entity: Alpha cardinality: "n:1"` + field("ownership", tc.bOwn) + field("role", tc.bRole) + ` ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -822,7 +828,7 @@ scenarios: actors: [Alpha] steps: ["an ` + "`Alpha`" + ` gains a ` + "`Beta`" + `"] ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -978,7 +984,7 @@ func TestADR_0008_AmbiguousPairingIsWarning(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - res, err := Run([]byte("kind: DomainModel\nversion: v1\nentities:" + tc.rels)) + res, err := Run(testModelPath, []byte("kind: DomainModel\nversion: v1\nentities:"+tc.rels), nil) if err != nil { t.Fatal(err) } @@ -1024,7 +1030,7 @@ entities: - id: real-rule statement: "Has a rule" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1044,7 +1050,7 @@ entities: - name: status type: ProjectStatus ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1067,7 +1073,7 @@ entities: Project: definition: A container. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1094,7 +1100,7 @@ entities: type: integer derived: true ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1117,7 +1123,7 @@ entities: type: integer derivation: "counts something" ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1137,7 +1143,7 @@ entities: definition: A computed summary. No derivation string given. derived: true ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1157,7 +1163,7 @@ entities: definition: A computed summary. derivation: "Computed from other state." ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1182,7 +1188,7 @@ entities: derived: true derivation: Recomputed on every query from the resolved geometry. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1223,7 +1229,7 @@ entities: derived: true derivation: Recomputed on every query from the resolved geometry. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1254,7 +1260,7 @@ scenarios: steps: ["the ` + "`Project`" + ` is used"] invariants_touched: [rule] ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1287,7 +1293,7 @@ entities: symmetric: true role: the unordered pair ` - res, err := Run([]byte(valid)) + res, err := Run(testModelPath, []byte(valid), nil) if err != nil { t.Fatal(err) } @@ -1309,7 +1315,7 @@ entities: Person: definition: A person. ` - res, err = Run([]byte(invalid)) + res, err = Run(testModelPath, []byte(invalid), nil) if err != nil { t.Fatal(err) } @@ -1333,7 +1339,7 @@ entities: B: definition: Another entity. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1363,7 +1369,7 @@ entities: cardinality: "0..n:1" role: owns ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1389,7 +1395,7 @@ entities: B: definition: Another. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1412,7 +1418,7 @@ entities: definition: A card that claims to be a kind of an undefined thing. subtypeOf: PaymentMethod ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1440,7 +1446,7 @@ entities: Cash: definition: A standalone entity with no rule and no parent. ` - res, err := Run([]byte(src)) + res, err := Run(testModelPath, []byte(src), nil) if err != nil { t.Fatal(err) } @@ -1463,7 +1469,7 @@ entities: definition: An entity that is a kind of itself. subtypeOf: A ` - res, err := Run([]byte(self)) + res, err := Run(testModelPath, []byte(self), nil) if err != nil { t.Fatal(err) } @@ -1482,7 +1488,7 @@ entities: definition: A kind of Alpha. subtypeOf: Alpha ` - res, err = Run([]byte(mutual)) + res, err = Run(testModelPath, []byte(mutual), nil) if err != nil { t.Fatal(err) } From 840a9e1d4d3cd8d79f42019edc328176df342674 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 14:08:43 -0700 Subject: [PATCH 03/22] docs: document scope, imports, and cross-model references Adds an Imports section to the schema reference covering the two new fields, the non-recursive resolution rule, the entity-position deferral, and why an unresolvable qualified type is an error while an unqualified PascalCase one is only a warning. Amends the bounded-contexts omission, which claimed the format had no cross-context reference at all. Signed-off-by: Joe Beda --- docs/06-schema-reference.md | 93 +++++++++++++++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 5 deletions(-) diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index de9c3ef..0ec0789 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -31,6 +31,8 @@ the schema). Print the schema any time with `modelith schema`. | `version` | string | yes | Schema revision. Currently `v1`. | | `title` | string | no | Heading used when rendering. | | `description` | string | no | One-paragraph summary. | +| `scope` | string | no | Kebab-case slug naming this model where another model references its items. Needed only to *be* imported. See [Imports](#imports). | +| `imports` | list | no | Paths to other model files whose items this one references, relative to this file. See [Imports](#imports). | | `glossary` | map | no | Ubiquitous-language terms that aren't entities. See [Glossary](#glossary). | | `enums` | map | no | First-class enumerated types. See [Enum](#enum). | | `entities` | map | no | Keyed by canonical PascalCase entity name. If present, must contain at least one entity. | @@ -256,6 +258,78 @@ A scenario is a diagnostic, not a backlog item: it tests whether the entities and actions actually hang together. If writing one reveals an invariant that can't be satisfied — or that doesn't exist yet — fix the model, not the scenario. +## Imports + +Once a system outgrows one model, two models end up needing the same concept — +typically a shared enum. Defining it in both and asserting in prose that they +match leaves nothing to check, and they drift. Instead, one model owns the +definition and the other **references** it. + +The referenced model declares a slug: + +```yaml +# payments.modelith.yaml +kind: DomainModel +version: v1 +scope: payments +enums: + PaymentMethod: + values: + - name: card + - name: transfer +``` + +The referencing model lists a path to it and writes `scope.Name` at the +reference site: + +```yaml +# garage.modelith.yaml +kind: DomainModel +version: v1 +imports: + - ./payments.modelith.yaml +entities: + Visit: + definition: One car's stay in the garage. + attributes: + - name: settledWith + type: payments.PaymentMethod +``` + +| Field | Type | Required | Notes | +|---|---|---|---| +| `scope` | string | no | Lowercase kebab-case slug (`^[a-z][a-z0-9-]+$`). Optional — a model nobody imports never needs one — but required in order to be imported. | +| `imports` | list of string | no | Paths to model files, each relative to *this* file. `..` is fine; an absolute path is an error, since it wouldn't resolve in another checkout. | + +Four rules are worth knowing before you use this: + +- **The slug is declared once, by the imported model.** An importing model + lists a path, never a slug, so there is nothing for the two files to disagree + about. Two imports declaring the same `scope` is an error — rename one. +- **Resolution does not recurse.** Only items defined *directly* in a listed + file are reachable. If `garage` imports `payments` and `payments` imports + `shipping`, `shipping.Carrier` is not available in `garage` — import it + there too. Mutual imports (`a` lists `b`, `b` lists `a`) are therefore legal + and terminate. +- **Only an attribute `type` may be qualified.** Cross-model references in + `relationship.entity` and `subtypeOf` are not supported; the linter says so + plainly rather than letting the name-pattern rejection speak for it. Whether + the ER diagram should draw a foreign entity — and how reciprocity would work + across a boundary — has no answer yet, and no live model needs one. +- **Nothing is fetched.** `imports` names files that are already in your + repository; `lint` and `render` never touch the network + ([ADR-0011](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0011-network-boundary.md)). + +The linter reports a qualified type that doesn't resolve as an **error**, while +an *unqualified* PascalCase type that names no enum is only a **warning**. The +asymmetry is deliberate: `PaymentMethod` might be a primitive the author +invented, so the linter can only suggest; `payments.PaymentMethod` can be +nothing but a cross-model reference, so failing to resolve it is a broken +reference. + +The full rationale, including where this is heading, is +[ADR-0010](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0010-cross-model-references-by-vendoring.md). + ## What this format deliberately leaves out modelith is a light, agent-authored subset of domain-driven design, not a full @@ -277,9 +351,11 @@ is *not* here is as useful as knowing what is. while invariants govern the legal transitions between them. Deliberate, and consistent with why enums carry no transition edges. - **Bounded contexts and context maps.** One model is one context. There is no - construct for relating multiple contexts or mapping shared concepts across - them. Compose several `*.modelith.yaml` models at the repository level instead - of expressing context boundaries inside one file. + construct for declaring a context boundary or mapping a concept from one + context onto another's. What there is, is a narrow reference: + [imports](#imports) let one model name an item another defines, so shared + vocabulary has one definition. Everything else about how two contexts relate + stays outside the format. These omissions keep the format small enough for an agent to author reliably and for a human to read in one sitting. Any of them can become a roadmap item if a @@ -299,7 +375,12 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: - a duplicate invariant `id` (across entity-level *and* model-level invariants — they share one namespace); - a scenario `invariants_touched` or an action `preserves` that references an - invariant id no entity or model-level invariant declares. + invariant id no entity or model-level invariant declares; + - an [import](#imports) that is absolute, unreadable, not a domain model, + declares no `scope`, or declares a `scope` another import already + declared; + - a qualified attribute `type` whose scope isn't imported, or that names + no enum in the model it resolves to. - **Warnings** (likely-but-not-certainly wrong): - a backticked term in freeform text that resolves to no entity, glossary term, role, or actor; @@ -312,7 +393,9 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: the other declares it back, so which is the reciprocal of which can't be determined — the diagram draws every declaration as its own line; - an attribute `type` that looks like an enum reference (PascalCase) but - names no defined enum; + names no defined enum — a *warning* where the qualified `scope.Name` form + is an error, for the reason given under [Imports](#imports); + - an [import](#imports) nothing references; - an action `actor` that is neither a defined entity nor a glossary term. - **Completeness** checks (advisory warnings): entities with no invariants; entities no scenario exercises; a glossary term nothing references; an enum no From 9399bdd90bec6abfd8703e01f84dc6e37b1c8301 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 14:36:17 -0700 Subject: [PATCH 04/22] feat(schema)!: bind an import's scope in the importer, not the imported file Supersedes the binding introduced in 04d34be. A model no longer declares a top-level `scope:`; each `imports:` entry binds one instead, defaulting to the file's basename with .modelith.yaml stripped, with an explicit {scope, path} object for a filename that is not a usable slug or is already bound. The union follows the precedent `actions` set. The cause was the renderer. Its links into an imported model's Markdown need to pair a scope with a path, and it may not open imported files to learn that pairing, so a scope declared by the imported file was unresolvable where it was needed most. Moving the binding to the importer makes it local, and the renderer needs no filename convention and no guessing. Falls out of it: the imported-file-declares-no-scope error is gone, and the unreferenced-import finding becomes a completeness one, alongside the unused enum and glossary term whose promotion under --completeness error it shares. Recorded as ADR-0012. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 12 ++-- internal/lint/imports.go | 60 ++++++++++------ internal/lint/imports_test.go | 72 +++++++++---------- internal/model/model.go | 77 +++++++++++++++++--- internal/model/model_test.go | 85 +++++++++++++++++++++++ internal/model/sync_test.go | 1 + internal/render/markdown/markdown.go | 58 +++++++++++++++- internal/render/markdown/markdown_test.go | 48 +++++++++++++ internal/schema/v1/modelith.schema.json | 41 ++++++++--- 9 files changed, 370 insertions(+), 84 deletions(-) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 51da14d..b5bd2d3 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "os" - "path/filepath" "runtime/debug" "strings" @@ -263,13 +262,10 @@ func renderCmd() *cobra.Command { return cmd } -func defaultOut(in string) string { - ext := filepath.Ext(in) - if ext == ".yaml" || ext == ".yml" { - return strings.TrimSuffix(in, ext) + ".md" - } - return in + ".md" -} +// defaultOut is where render writes when no -o is given. It is the same +// mapping the renderer uses for its links into an imported model's Markdown, so +// the two cannot disagree about where a model's .md lives. +func defaultOut(in string) string { return model.RenderedPath(in) } // ---- schema ---- diff --git a/internal/lint/imports.go b/internal/lint/imports.go index 9834192..b62a6de 100644 --- a/internal/lint/imports.go +++ b/internal/lint/imports.go @@ -24,11 +24,16 @@ type OSFiles struct{} // ReadFile reads the named file from the local filesystem. func (OSFiles) ReadFile(path string) ([]byte, error) { return os.ReadFile(path) } -// qualifiedRefRE matches a cross-model reference, "scope.Name". The scope side -// mirrors the schema's scope pattern; the name side stays loose so a reference -// to something that isn't a defined enum is reported as a broken reference -// rather than silently read as a primitive type. -var qualifiedRefRE = regexp.MustCompile(`^([a-z][a-z0-9-]*)\.([A-Za-z0-9]+)$`) +var ( + // qualifiedRefRE matches a cross-model reference, "scope.Name". The scope + // side mirrors the schema's scope pattern; the name side stays loose so a + // reference to something that isn't a defined enum is reported as a broken + // reference rather than silently read as a primitive type. + qualifiedRefRE = regexp.MustCompile(`^([a-z][a-z0-9-]*)\.([A-Za-z0-9]+)$`) + // scopeRE is the slug a bare import's filename has to yield. An explicitly + // written scope is held to the same pattern by the schema. + scopeRE = regexp.MustCompile(`^[a-z][a-z0-9-]+$`) +) // importedModel is one successfully resolved entry of a model's imports list. type importedModel struct { @@ -45,6 +50,10 @@ type importedModel struct { func runImports(modelPath string, m *model.Model, fr FileReader, res *Result) { byScope := loadImports(modelPath, m, fr, res) used := checkQualifiedTypes(m, byScope, res) + // An unreferenced import is a completeness finding, alongside the unused + // enum and the unused glossary term: vocabulary the model declares and + // nothing uses. Sharing their category means sharing their promotion under + // --completeness error. for _, scope := range sortedMapKeys(byScope) { if used[scope] { continue @@ -52,7 +61,7 @@ func runImports(modelPath string, m *model.Model, fr FileReader, res *Result) { imp := byScope[scope] res.Findings = append(res.Findings, Finding{ Severity: SeverityWarning, - Category: CategorySemantic, + Category: CategoryCompleteness, Path: fmt.Sprintf("/imports/%d", imp.index), Message: fmt.Sprintf( "import %q (scope %q) is never referenced — drop it, or reference one of its enums as %s.Name in an attribute type", @@ -63,8 +72,9 @@ func runImports(modelPath string, m *model.Model, fr FileReader, res *Result) { } // loadImports reads each declared import and returns the ones that resolved, -// keyed by the scope they declare. Every rejection is reported as an error: an -// import that cannot be resolved is a broken reference, not a gap. +// keyed by the scope the importing model binds them to. Every rejection is +// reported as an error: an import that cannot be resolved is a broken +// reference, not a gap. func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) map[string]importedModel { byScope := map[string]importedModel{} dir := filepath.Dir(modelPath) @@ -77,33 +87,37 @@ func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) m Message: fmt.Sprintf(format, args...), }) } - if imp == "" { + if imp.Path == "" { continue // the schema's minLength reports the empty path } - if filepath.IsAbs(imp) || imp[0] == '/' { - reject("import %q is an absolute path — imports are relative to this model so they resolve in any checkout", imp) + if filepath.IsAbs(imp.Path) || imp.Path[0] == '/' { + reject("import %q is an absolute path — imports are relative to this model so they resolve in any checkout", imp.Path) + continue + } + // A written scope is held to the pattern by the schema; a derived one the + // schema never sees, and a filename that yields no usable slug is the + // case the explicit form exists for. + if imp.ScopeFromPath && !scopeRE.MatchString(imp.Scope) { + reject("import %q binds the scope %q taken from its filename, which is not a valid slug (lowercase kebab-case) — name the scope explicitly instead: `- {scope: , path: %s}`", + imp.Path, imp.Scope, imp.Path) continue } - data, err := fr.ReadFile(filepath.Join(dir, imp)) + data, err := fr.ReadFile(filepath.Join(dir, imp.Path)) if err != nil { - reject("import %q cannot be read: %v", imp, err) + reject("import %q cannot be read: %v", imp.Path, err) continue } im, err := model.Parse(data) if err != nil || im.Kind != "DomainModel" { - reject("import %q is not a domain model — lint it on its own with `modelith lint` to see why", imp) - continue - } - if im.Scope == "" { - reject("import %q declares no `scope:` — a model needs a scope slug to be imported, since that slug is how this model names its items", imp) + reject("import %q is not a domain model — lint it on its own with `modelith lint` to see why", imp.Path) continue } - if prev, ok := byScope[im.Scope]; ok { - reject("import %q declares scope %q, which import %q already declares — rename one model's scope so %s.Name resolves unambiguously", - imp, im.Scope, prev.path, im.Scope) + if prev, ok := byScope[imp.Scope]; ok { + reject("import %q binds scope %q, which import %q already binds — give one of them an explicit, different scope so %s.Name resolves unambiguously", + imp.Path, imp.Scope, prev.path, imp.Scope) continue } - byScope[im.Scope] = importedModel{index: i, path: imp, model: im} + byScope[imp.Scope] = importedModel{index: i, path: imp.Path, model: im} } return byScope } @@ -135,7 +149,7 @@ func checkQualifiedTypes(m *model.Model, byScope map[string]importedModel, res * } imp, ok := byScope[scope] if !ok { - broken("attribute type %q references scope %q, which this model does not import — add the model that declares that scope to `imports:`", attr.Type, scope) + broken("attribute type %q references the scope %q, which no import binds — add the model that defines %s to `imports:`", attr.Type, scope, item) continue } // The import is referenced even when the item does not resolve, so diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index 9c53dc3..c6c837b 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -28,7 +28,6 @@ const importerPath = "docs/garage.modelith.yaml" const paymentsModel = `kind: DomainModel version: v1 -scope: payments title: Payments enums: PaymentMethod: @@ -42,14 +41,15 @@ entities: ` // importer builds a model that lists the given imports and types one attribute -// with the given type. +// with the given type. Each entry is written verbatim as a YAML sequence item, +// so a case can use either the bare-path form or the explicit {scope, path} one. func importer(imports []string, attrType string) string { var b strings.Builder - b.WriteString("kind: DomainModel\nversion: v1\nscope: garage\n") + b.WriteString("kind: DomainModel\nversion: v1\n") if len(imports) > 0 { b.WriteString("imports:\n") for _, imp := range imports { - fmt.Fprintf(&b, " - %q\n", imp) + fmt.Fprintf(&b, " - %s\n", imp) } } fmt.Fprintf(&b, `entities: @@ -107,13 +107,10 @@ func TestImports_Resolution(t *testing.T) { t.Parallel() files := fakeFiles{ - "docs/payments.modelith.yaml": paymentsModel, - "payments/payments.modelith.yaml": paymentsModel, - "docs/billing.modelith.yaml": strings.Replace(paymentsModel, - "title: Payments", "title: Billing", 1), - "docs/anonymous.modelith.yaml": strings.Replace(paymentsModel, - "scope: payments\n", "", 1), - "docs/not-a-model.yaml": "kind: SomethingElse\nversion: v1\n", + "docs/payments.modelith.yaml": paymentsModel, + "payments/payments.modelith.yaml": paymentsModel, + "docs/legacy/pay-v2.modelith.yaml": paymentsModel, + "docs/not-a-model.yaml": "kind: SomethingElse\nversion: v1\n", } const typePath = "/entities/Visit/attributes/0/type" @@ -126,12 +123,12 @@ func TestImports_Resolution(t *testing.T) { }{ { name: "peer import resolves", - imports: []string{"./payments.modelith.yaml"}, + imports: []string{`"./payments.modelith.yaml"`}, attrType: "payments.PaymentMethod", }, { name: "parent-relative import resolves", - imports: []string{"../payments/payments.modelith.yaml"}, + imports: []string{`"../payments/payments.modelith.yaml"`}, attrType: "payments.PaymentMethod", }, { @@ -139,22 +136,22 @@ func TestImports_Resolution(t *testing.T) { attrType: "payments.PaymentMethod", want: []wantFinding{{ SeverityError, CategorySemantic, typePath, - `references scope "payments", which this model does not import`, + `references the scope "payments", which no import binds`, }}, }, { name: "qualified type naming an unimported scope", - imports: []string{"./payments.modelith.yaml"}, + imports: []string{`"./payments.modelith.yaml"`}, attrType: "shipping.Carrier", want: []wantFinding{ {SeverityError, CategorySemantic, typePath, - `references scope "shipping", which this model does not import`}, - {SeverityWarning, CategorySemantic, "/imports/0", "is never referenced"}, + `references the scope "shipping", which no import binds`}, + {SeverityWarning, CategoryCompleteness, "/imports/0", "is never referenced"}, }, }, { name: "qualified type naming no enum in the imported model", - imports: []string{"./payments.modelith.yaml"}, + imports: []string{`"./payments.modelith.yaml"`}, attrType: "payments.Nonexistent", want: []wantFinding{{ SeverityError, CategorySemantic, typePath, @@ -163,7 +160,7 @@ func TestImports_Resolution(t *testing.T) { }, { name: "qualified type resolving to an entity, not an enum", - imports: []string{"./payments.modelith.yaml"}, + imports: []string{`"./payments.modelith.yaml"`}, attrType: "payments.Invoice", want: []wantFinding{{ SeverityError, CategorySemantic, typePath, @@ -171,26 +168,31 @@ func TestImports_Resolution(t *testing.T) { }}, }, { - name: "two imports declaring the same scope", - imports: []string{"./payments.modelith.yaml", "./billing.modelith.yaml"}, + name: "explicit scope overrides the filename", + imports: []string{"{scope: billing, path: ./legacy/pay-v2.modelith.yaml}"}, + attrType: "billing.PaymentMethod", + }, + { + name: "two imports binding the same scope", + imports: []string{`"./payments.modelith.yaml"`, `"../payments/payments.modelith.yaml"`}, attrType: "payments.PaymentMethod", want: []wantFinding{{ SeverityError, CategorySemantic, "/imports/1", - `declares scope "payments", which import "./payments.modelith.yaml" already declares`, + `binds scope "payments", which import "./payments.modelith.yaml" already binds`, }}, }, { - name: "import declaring no scope", - imports: []string{"./anonymous.modelith.yaml"}, + name: "bare import whose filename is not a usable slug", + imports: []string{`"./PayMents.modelith.yaml"`}, attrType: "string", want: []wantFinding{{ SeverityError, CategorySemantic, "/imports/0", - "declares no `scope:`", + "which is not a valid slug", }}, }, { name: "absolute import path", - imports: []string{"/abs/payments.modelith.yaml"}, + imports: []string{`"/abs/payments.modelith.yaml"`}, attrType: "string", want: []wantFinding{{ SeverityError, CategorySemantic, "/imports/0", @@ -199,7 +201,7 @@ func TestImports_Resolution(t *testing.T) { }, { name: "unreadable import", - imports: []string{"./missing.modelith.yaml"}, + imports: []string{`"./missing.modelith.yaml"`}, attrType: "string", want: []wantFinding{{ SeverityError, CategorySemantic, "/imports/0", @@ -208,7 +210,7 @@ func TestImports_Resolution(t *testing.T) { }, { name: "import that is not a domain model", - imports: []string{"./not-a-model.yaml"}, + imports: []string{`"./not-a-model.yaml"`}, attrType: "string", want: []wantFinding{{ SeverityError, CategorySemantic, "/imports/0", @@ -217,10 +219,10 @@ func TestImports_Resolution(t *testing.T) { }, { name: "import nothing references", - imports: []string{"./payments.modelith.yaml"}, + imports: []string{`"./payments.modelith.yaml"`}, attrType: "string", want: []wantFinding{{ - SeverityWarning, CategorySemantic, "/imports/0", + SeverityWarning, CategoryCompleteness, "/imports/0", "is never referenced", }}, }, @@ -254,7 +256,6 @@ func TestADR_0010_NonTransitiveResolution(t *testing.T) { const middle = `kind: DomainModel version: v1 -scope: payments imports: - "./shipping.modelith.yaml" entities: @@ -263,7 +264,6 @@ entities: ` const leaf = `kind: DomainModel version: v1 -scope: shipping enums: Carrier: values: @@ -278,14 +278,14 @@ enums: read: read, } - res, err := Run(importerPath, []byte(importer([]string{"./payments.modelith.yaml"}, "shipping.Carrier")), files) + res, err := Run(importerPath, []byte(importer([]string{`"./payments.modelith.yaml"`}, "shipping.Carrier")), files) if err != nil { t.Fatal(err) } assertFindings(t, importFindings(res.Findings), []wantFinding{ {SeverityError, CategorySemantic, "/entities/Visit/attributes/0/type", - `references scope "shipping", which this model does not import`}, - {SeverityWarning, CategorySemantic, "/imports/0", "is never referenced"}, + `references the scope "shipping", which no import binds`}, + {SeverityWarning, CategoryCompleteness, "/imports/0", "is never referenced"}, }) if n := read["docs/shipping.modelith.yaml"]; n != 0 { t.Errorf("an imported model's own imports must not be read, but shipping was read %d time(s)", n) @@ -377,7 +377,7 @@ func TestRun_NilFileReaderReadsFromDisk(t *testing.T) { if err := os.WriteFile(filepath.Join(dir, "payments.modelith.yaml"), []byte(paymentsModel), 0o600); err != nil { t.Fatal(err) } - src := importer([]string{"./payments.modelith.yaml"}, "payments.PaymentMethod") + src := importer([]string{`"./payments.modelith.yaml"`}, "payments.PaymentMethod") res, err := Run(filepath.Join(dir, "garage.modelith.yaml"), []byte(src), nil) if err != nil { t.Fatal(err) diff --git a/internal/model/model.go b/internal/model/model.go index 64d389a..9a18323 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -9,6 +9,7 @@ import ( "bytes" "encoding/json" "fmt" + "path" "sort" "strconv" "strings" @@ -22,14 +23,11 @@ type Model struct { Version string `json:"version"` Title string `json:"title,omitempty"` Description string `json:"description,omitempty"` - // Scope is the slug another model writes before an item's name - // ("payments.PaymentMethod") when it imports this one. It is declared here - // and nowhere restated, so nothing can disagree about the slug. - Scope string `json:"scope,omitempty"` - // Imports are paths to other model files, relative to this one, whose items - // this model may reference. Resolution does not recurse: an imported model's - // own imports are not reachable from here (ADR-0010). - Imports []string `json:"imports,omitempty"` + // Imports are the other model files this one references, each bound to the + // scope written at its reference sites. Resolution does not recurse: an + // imported model's own imports are not reachable from here (ADR-0010, + // ADR-0012). + Imports []Import `json:"imports,omitempty"` Glossary map[string]string `json:"glossary,omitempty"` Enums map[string]Enum `json:"enums,omitempty"` Entities map[string]Entity `json:"entities,omitempty"` @@ -40,6 +38,69 @@ type Model struct { Invariants []Invariant `json:"invariants,omitempty"` } +// Import is another model file this one references, bound to the scope written +// before an imported item's name ("payments.PaymentMethod"). The binding is the +// importer's: the imported file says nothing about how it is named here, so two +// models may bind the same file to different scopes (ADR-0012). +type Import struct { + Scope string `json:"scope"` + Path string `json:"path"` + // ScopeFromPath records that Scope came from Path's basename rather than + // being written out, so the linter can point at the explicit form when a + // filename yields an unusable slug. The schema validates an explicit scope + // itself; a derived one it never sees. + ScopeFromPath bool `json:"-"` +} + +// UnmarshalJSON lets an import be written as a bare path ("./payments.modelith.yaml", +// whose basename gives the scope) or as an object naming the scope explicitly +// ({scope: billing, path: ./legacy/pay-v2.modelith.yaml}). +func (i *Import) UnmarshalJSON(data []byte) error { + trimmed := bytes.TrimSpace(data) + if len(trimmed) == 0 || bytes.Equal(trimmed, []byte("null")) { + return fmt.Errorf("import must be a path string or an object, not null") + } + if trimmed[0] == '"' { + var s string + if err := json.Unmarshal(trimmed, &s); err != nil { + return err + } + *i = Import{Scope: ScopeFromPath(s), Path: s, ScopeFromPath: true} + return nil + } + type rawImport Import + var r rawImport + dec := json.NewDecoder(bytes.NewReader(trimmed)) + dec.DisallowUnknownFields() + if err := dec.Decode(&r); err != nil { + return err + } + *i = Import(r) + return nil +} + +// ScopeFromPath derives the scope a bare import path binds: the basename with +// ".modelith.yaml" — or failing that, the final extension — stripped. The +// result is not guaranteed to be a valid slug; the linter reports one that +// isn't, since only an explicitly written scope passes through the schema. +func ScopeFromPath(p string) string { + base := path.Base(p) + if trimmed, ok := strings.CutSuffix(base, ".modelith.yaml"); ok { + return trimmed + } + return strings.TrimSuffix(base, path.Ext(base)) +} + +// RenderedPath returns where `modelith render` writes a model file's Markdown: +// the same path with a .yaml/.yml extension replaced by .md. +func RenderedPath(p string) string { + ext := path.Ext(p) + if ext == ".yaml" || ext == ".yml" { + return strings.TrimSuffix(p, ext) + ".md" + } + return p + ".md" +} + // Enum is a named, first-class set of allowed values for an attribute. Defining // it once (rather than inline in a "type" string) makes the values // referenceable and checkable. diff --git a/internal/model/model_test.go b/internal/model/model_test.go index 3de22bd..74d0735 100644 --- a/internal/model/model_test.go +++ b/internal/model/model_test.go @@ -162,3 +162,88 @@ func TestParseCardinality(t *testing.T) { t.Error("ParseCardinality with an inverted range should not be ok") } } + +// TestParseImportForms covers the imports union: a bare path binds the scope its +// filename yields, an object names the scope outright, and a derived scope is +// marked so the linter can tell the two apart. +func TestParseImportForms(t *testing.T) { + t.Parallel() + + src := ` +kind: DomainModel +version: v1 +imports: + - ./payments.modelith.yaml + - scope: billing + path: ./legacy/pay-v2.modelith.yaml +entities: + Project: + definition: A thing. +` + m, err := Parse([]byte(src)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []Import{ + {Scope: "payments", Path: "./payments.modelith.yaml", ScopeFromPath: true}, + {Scope: "billing", Path: "./legacy/pay-v2.modelith.yaml"}, + } + if len(m.Imports) != len(want) { + t.Fatalf("expected %d imports, got %+v", len(want), m.Imports) + } + for i, w := range want { + if m.Imports[i] != w { + t.Errorf("import %d: got %+v, want %+v", i, m.Imports[i], w) + } + } +} + +func TestParseRejectsMalformedImport(t *testing.T) { + t.Parallel() + + cases := map[string]string{ + "null entry": "imports:\n - ~\n", + "unknown field": "imports:\n - {scope: a, path: b, extra: c}\n", + "wrong item type": "imports:\n - [./a.modelith.yaml]\n", + } + for name, block := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + if _, err := Parse([]byte("kind: DomainModel\nversion: v1\n" + block)); err == nil { + t.Fatal("expected an error, got nil") + } + }) + } +} + +func TestScopeFromPath(t *testing.T) { + t.Parallel() + + cases := map[string]string{ + "./payments.modelith.yaml": "payments", + "../shared/billing.modelith.yaml": "billing", + "./legacy/pay-v2.yaml": "pay-v2", + "./Payments.modelith.yaml": "Payments", + "noextension": "noextension", + } + for in, want := range cases { + if got := ScopeFromPath(in); got != want { + t.Errorf("ScopeFromPath(%q) = %q, want %q", in, got, want) + } + } +} + +func TestRenderedPath(t *testing.T) { + t.Parallel() + + cases := map[string]string{ + "./payments.modelith.yaml": "./payments.modelith.md", + "../a/b.yml": "../a/b.md", + "weird": "weird.md", + } + for in, want := range cases { + if got := RenderedPath(in); got != want { + t.Errorf("RenderedPath(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/model/sync_test.go b/internal/model/sync_test.go index adf1e9d..93e4181 100644 --- a/internal/model/sync_test.go +++ b/internal/model/sync_test.go @@ -37,6 +37,7 @@ func TestSchemaStructSync(t *testing.T) { {"Enum", []string{"$defs", "enum"}, reflect.TypeOf(Enum{})}, {"EnumValue", []string{"$defs", "enumValue"}, reflect.TypeOf(EnumValue{})}, {"Action", []string{"$defs", "actionObject"}, reflect.TypeOf(Action{})}, + {"Import", []string{"$defs", "importObject"}, reflect.TypeOf(Import{})}, {"Invariant", []string{"$defs", "invariant"}, reflect.TypeOf(Invariant{})}, } diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index e9f963c..7d15a53 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -5,6 +5,7 @@ package markdown import ( "fmt" + "regexp" "sort" "strings" @@ -16,6 +17,11 @@ import ( // are consumed by Docusaurus v3, whose MDX parser rejects HTML comments. const generatedBanner = "{/* Generated by `modelith render`. Do not edit by hand; edit the .modelith.yaml source and re-render. */}" +// qualifiedTypeRE matches an attribute type that references an imported item, +// "scope.Name". It mirrors the linter's pattern; the renderer only decides +// whether to link, never whether the reference resolves. +var qualifiedTypeRE = regexp.MustCompile(`^([a-z][a-z0-9-]*)\.([A-Za-z0-9]+)$`) + // Render produces the full Markdown document for the model. func Render(m *model.Model) string { var b strings.Builder @@ -34,6 +40,7 @@ func Render(m *model.Model) string { b.WriteString("\n\n") } + renderImports(&b, m) renderGlossary(&b, m) renderEnums(&b, m) renderEntities(&b, m) @@ -59,6 +66,54 @@ func renderInvariants(b *strings.Builder, m *model.Model) { b.WriteString("\n") } +// renderImports names the models this one references and where they live, +// linking each to its rendered Markdown. It never opens them: the imported +// definitions are not restated here, only pointed at, so this document stays a +// pure function of its own source (ADR-0010). +func renderImports(b *strings.Builder, m *model.Model) { + if len(m.Imports) == 0 { + return + } + b.WriteString("## Imports\n\n") + b.WriteString("Items defined in these models are referenced below as `scope.Name`.\n\n") + for _, imp := range m.Imports { + fmt.Fprintf(b, "- **`%s`** — [`%s`](%s)\n", imp.Scope, imp.Path, model.RenderedPath(imp.Path)) + } + b.WriteString("\n") +} + +// importAnchors maps each bound scope to the rendered Markdown of the model it +// names, for linking a qualified type. The first binding of a scope wins; a +// second one is a lint error, and the renderer stays deterministic either way. +func importAnchors(m *model.Model) map[string]string { + if len(m.Imports) == 0 { + return nil + } + out := make(map[string]string, len(m.Imports)) + for _, imp := range m.Imports { + if _, seen := out[imp.Scope]; !seen { + out[imp.Scope] = model.RenderedPath(imp.Path) + } + } + return out +} + +// linkQualifiedType renders an attribute type, linking a "scope.Name" reference +// into the imported model's Markdown. Headings render as "### `Name`", so the +// anchor is the item's name lowercased. A scope no import binds — a lint error +// — renders as written rather than as a link to nowhere. +func linkQualifiedType(typ string, targets map[string]string) string { + match := qualifiedTypeRE.FindStringSubmatch(typ) + if match == nil { + return typ + } + target, ok := targets[match[1]] + if !ok { + return typ + } + return fmt.Sprintf("[%s](%s#%s)", typ, target, strings.ToLower(match[2])) +} + func renderGlossary(b *strings.Builder, m *model.Model) { if len(m.Glossary) == 0 { return @@ -115,6 +170,7 @@ func renderEntities(b *strings.Builder, m *model.Model) { subtypes[p] = append(subtypes[p], n) } } + importTargets := importAnchors(m) b.WriteString("## Entities\n\n") for _, name := range names { ent := m.Entities[name] @@ -183,7 +239,7 @@ func renderEntities(b *strings.Builder, m *model.Model) { desc = d } } - fmt.Fprintf(b, "| `%s` | %s | %s |\n", attr.Name, mdCell(attr.Type), mdCell(desc)) + fmt.Fprintf(b, "| `%s` | %s | %s |\n", attr.Name, mdCell(linkQualifiedType(attr.Type, importTargets)), mdCell(desc)) } b.WriteString("\n") } diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index 84dfdd5..0639b97 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -152,6 +152,54 @@ func TestGoldenExample(t *testing.T) { } } +// TestRenderImports_SectionAndLinkedTypes checks the imports section and the +// deep link on a qualified attribute type. The renderer never opens an imported +// file: the scope comes from the import that bound it and the anchor from the +// heading format enums render with, so the output stays a pure function of this +// model (ADR-0010, ADR-0012). +func TestRenderImports_SectionAndLinkedTypes(t *testing.T) { + t.Parallel() + + m := &model.Model{ + Imports: []model.Import{ + {Scope: "payments", Path: "../payments/payments.modelith.yaml", ScopeFromPath: true}, + {Scope: "billing", Path: "./legacy/pay-v2.modelith.yaml"}, + }, + Entities: map[string]model.Entity{ + "Ticket": { + Definition: "A temporary entry credential.", + Attributes: []model.Attribute{ + {Name: "paidWith", Type: "payments.PaymentMethod"}, + {Name: "plan", Type: "billing.Plan"}, + {Name: "unbound", Type: "shipping.Carrier"}, + {Name: "issuedAt", Type: "timestamp"}, + }, + }, + }, + } + got := Render(m) + + for _, want := range []string{ + "## Imports\n", + "- **`payments`** — [`../payments/payments.modelith.yaml`](../payments/payments.modelith.md)\n", + "- **`billing`** — [`./legacy/pay-v2.modelith.yaml`](./legacy/pay-v2.modelith.md)\n", + "| `paidWith` | [payments.PaymentMethod](../payments/payments.modelith.md#paymentmethod) | |\n", + "| `plan` | [billing.Plan](./legacy/pay-v2.modelith.md#plan) | |\n", + // A scope no import binds is a lint error; the renderer states it rather + // than linking to a file it has no reason to believe exists. + "| `unbound` | shipping.Carrier | |\n", + "| `issuedAt` | timestamp | |\n", + } { + if !strings.Contains(got, want) { + t.Errorf("expected %q in:\n%s", want, got) + } + } + + if strings.Contains(Render(&model.Model{Entities: m.Entities}), "## Imports") { + t.Error("did not expect an Imports section when there are no imports") + } +} + // TestRenderEntity_SubtypeHierarchy checks that a child names its supertype and // a parent lists its subtypes. func TestRenderEntity_SubtypeHierarchy(t *testing.T) { diff --git a/internal/schema/v1/modelith.schema.json b/internal/schema/v1/modelith.schema.json index 623ec5b..a5cd57b 100644 --- a/internal/schema/v1/modelith.schema.json +++ b/internal/schema/v1/modelith.schema.json @@ -25,17 +25,11 @@ "description": "Optional one-paragraph summary of what this model covers.", "type": "string" }, - "scope": { - "description": "Kebab-case slug naming this model where another model references its items as \"scope.Name\". Optional — a model nobody imports never needs one — but required in order to be imported. Declared here and nowhere restated: an importing model lists a path, not a slug.", - "type": "string", - "pattern": "^[a-z][a-z0-9-]+$" - }, "imports": { - "description": "Paths to other model files whose items this model references, each relative to this file (\"..\" is fine; an absolute path is not portable across checkouts and is rejected). Each listed file declares its own \"scope\". Resolution does not recurse: only items defined directly in a listed file are reachable.", + "description": "Other model files whose items this model references. Each entry is a path relative to this file — \"..\" is fine; an absolute path is not portable across checkouts and is rejected — and binds a scope this model writes before an imported item's name (\"payments.PaymentMethod\"). Resolution does not recurse: only items defined directly in a listed file are reachable.", "type": "array", "items": { - "type": "string", - "minLength": 1 + "$ref": "#/$defs/import" } }, "glossary": { @@ -77,6 +71,37 @@ } }, "$defs": { + "import": { + "title": "Import", + "description": "Another model this one references: either a bare path (the scope is the file's basename with its extension stripped) or an object naming the scope explicitly.", + "oneOf": [ + { + "description": "A path to a model file, relative to this one. Its basename with \".modelith.yaml\" (or the final extension) stripped is the scope.", + "type": "string", + "minLength": 1 + }, + { "$ref": "#/$defs/importObject" } + ] + }, + "importObject": { + "title": "Import (explicit scope)", + "description": "An import that names its scope, for a file whose basename is not a usable slug or is already bound by another import.", + "type": "object", + "required": ["scope", "path"], + "additionalProperties": false, + "properties": { + "scope": { + "description": "The slug this model writes before an item's name to reference it (\"payments\" in \"payments.PaymentMethod\"). Lowercase kebab-case. Local to this model: the imported file does not declare it, and another model may bind the same file to a different scope.", + "type": "string", + "pattern": "^[a-z][a-z0-9-]+$" + }, + "path": { + "description": "Path to the model file, relative to this one.", + "type": "string", + "minLength": 1 + } + } + }, "enum": { "title": "Enum", "type": "object", From 3dfba319c055f0c8b8f966987fc77599621f83e6 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 14:37:55 -0700 Subject: [PATCH 05/22] docs(adr): record ADR-0012 and align the schema reference ADR-0012 supersedes ADR-0010's identity decision and the part of its resolution decision that made the imported file the source of truth for the slug; both originals are left untouched. The schema reference now documents the union form, the filename default, and the local nature of the binding. Signed-off-by: Joe Beda --- docs/06-schema-reference.md | 63 +++++++++++-------- internal/lint/imports_test.go | 35 +++++++++++ .../adr/0012-imports-bind-the-scope.md | 58 +++++++++++++++++ 3 files changed, 129 insertions(+), 27 deletions(-) create mode 100644 project-docs/adr/0012-imports-bind-the-scope.md diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 0ec0789..dad9450 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -31,8 +31,7 @@ the schema). Print the schema any time with `modelith schema`. | `version` | string | yes | Schema revision. Currently `v1`. | | `title` | string | no | Heading used when rendering. | | `description` | string | no | One-paragraph summary. | -| `scope` | string | no | Kebab-case slug naming this model where another model references its items. Needed only to *be* imported. See [Imports](#imports). | -| `imports` | list | no | Paths to other model files whose items this one references, relative to this file. See [Imports](#imports). | +| `imports` | list | no | Other model files whose items this one references, each a path relative to this file. See [Imports](#imports). | | `glossary` | map | no | Ubiquitous-language terms that aren't entities. See [Glossary](#glossary). | | `enums` | map | no | First-class enumerated types. See [Enum](#enum). | | `entities` | map | no | Keyed by canonical PascalCase entity name. If present, must contain at least one entity. | @@ -265,22 +264,7 @@ typically a shared enum. Defining it in both and asserting in prose that they match leaves nothing to check, and they drift. Instead, one model owns the definition and the other **references** it. -The referenced model declares a slug: - -```yaml -# payments.modelith.yaml -kind: DomainModel -version: v1 -scope: payments -enums: - PaymentMethod: - values: - - name: card - - name: transfer -``` - -The referencing model lists a path to it and writes `scope.Name` at the -reference site: +List the other model's file, and write `scope.Name` where you reference it: ```yaml # garage.modelith.yaml @@ -296,16 +280,37 @@ entities: type: payments.PaymentMethod ``` +The imported file needs no cooperation at all — it is an ordinary model that +happens to define a `PaymentMethod` enum. **The scope is bound by the model +doing the importing**, and defaults to the file's basename with +`.modelith.yaml` stripped: `./payments.modelith.yaml` binds `payments`. + +Name it explicitly when the filename yields no usable slug, or when the obvious +one is already taken: + +```yaml +imports: + - ./payments.modelith.yaml # binds "payments" + - scope: billing # binds "billing" + path: ./legacy/pay-v2.modelith.yaml +``` + | Field | Type | Required | Notes | |---|---|---|---| -| `scope` | string | no | Lowercase kebab-case slug (`^[a-z][a-z0-9-]+$`). Optional — a model nobody imports never needs one — but required in order to be imported. | -| `imports` | list of string | no | Paths to model files, each relative to *this* file. `..` is fine; an absolute path is an error, since it wouldn't resolve in another checkout. | +| `scope` | string | yes (explicit form) | The slug written before an item's name. Lowercase kebab-case (`^[a-z][a-z0-9-]+$`). | +| `path` | string | yes (explicit form) | Path to the model file, relative to *this* one. | + +A bare string is the same thing with the scope derived — `./x.modelith.yaml` is +exactly `{scope: x, path: ./x.modelith.yaml}`. If the filename doesn't yield a +valid slug (`./Pay Ments.yaml`), the linter says so and points at the explicit +form. Four rules are worth knowing before you use this: -- **The slug is declared once, by the imported model.** An importing model - lists a path, never a slug, so there is nothing for the two files to disagree - about. Two imports declaring the same `scope` is an error — rename one. +- **The binding is local.** Two models may import the same file under different + scopes, and neither one's choice is visible to the other. Two imports binding + the *same* scope in one model is an error — give one of them an explicit, + different scope. - **Resolution does not recurse.** Only items defined *directly* in a listed file are reachable. If `garage` imports `payments` and `payments` imports `shipping`, `shipping.Carrier` is not available in `garage` — import it @@ -320,6 +325,11 @@ Four rules are worth knowing before you use this: repository; `lint` and `render` never touch the network ([ADR-0011](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0011-network-boundary.md)). +Rendered Markdown names each import and links it to that model's rendered `.md`, +and a qualified type links straight to the item's heading there. The renderer +never opens an imported file, so a link points at where the Markdown *would* be: +render the imported model too, or the link dangles. + The linter reports a qualified type that doesn't resolve as an **error**, while an *unqualified* PascalCase type that names no enum is only a **warning**. The asymmetry is deliberate: `PaymentMethod` might be a primitive the author @@ -377,8 +387,8 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: - a scenario `invariants_touched` or an action `preserves` that references an invariant id no entity or model-level invariant declares; - an [import](#imports) that is absolute, unreadable, not a domain model, - declares no `scope`, or declares a `scope` another import already - declared; + binds a scope another import already bound, or is a bare path whose + filename yields no valid slug; - a qualified attribute `type` whose scope isn't imported, or that names no enum in the model it resolves to. - **Warnings** (likely-but-not-certainly wrong): @@ -395,11 +405,10 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: - an attribute `type` that looks like an enum reference (PascalCase) but names no defined enum — a *warning* where the qualified `scope.Name` form is an error, for the reason given under [Imports](#imports); - - an [import](#imports) nothing references; - an action `actor` that is neither a defined entity nor a glossary term. - **Completeness** checks (advisory warnings): entities with no invariants; entities no scenario exercises; a glossary term nothing references; an enum no - attribute uses. + attribute uses; an [import](#imports) nothing references. These are advisory on purpose. An entity that genuinely has no rule to state is fine — leave its invariants empty rather than inventing a filler rule that diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index c6c837b..55ee973 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -248,6 +248,41 @@ func TestImports_Resolution(t *testing.T) { } } +// TestADR_0012_ImporterBindsScope pins that the scope belongs to the importing +// model: the same file resolves under whatever scope each importer binds it to, +// and a model has no say in — and no field for — what it is called elsewhere. +func TestADR_0012_ImporterBindsScope(t *testing.T) { + t.Parallel() + + files := fakeFiles{"docs/payments.modelith.yaml": paymentsModel} + + t.Run("same file, different bindings", func(t *testing.T) { + t.Parallel() + for _, binding := range []struct{ entry, scope string }{ + {`"./payments.modelith.yaml"`, "payments"}, + {"{scope: money, path: ./payments.modelith.yaml}", "money"}, + } { + res, err := Run(importerPath, []byte(importer([]string{binding.entry}, binding.scope+".PaymentMethod")), files) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), nil) + } + }) + + t.Run("a model cannot name itself", func(t *testing.T) { + t.Parallel() + src := "kind: DomainModel\nversion: v1\nscope: payments\nentities:\n Visit:\n definition: One stay.\n" + res, err := Run(importerPath, []byte(src), files) + if err != nil { + t.Fatal(err) + } + if countBy(res.Findings, SeverityError, CategoryStructural) == 0 { + t.Fatalf("expected a top-level `scope:` to be rejected, got: %+v", res.Findings) + } + }) +} + // TestADR_0010_NonTransitiveResolution pins that resolution does not recurse: // an item defined in a model that an *imported* model imports is unreachable, // and the file it lives in is never read. diff --git a/project-docs/adr/0012-imports-bind-the-scope.md b/project-docs/adr/0012-imports-bind-the-scope.md new file mode 100644 index 0000000..78047ad --- /dev/null +++ b/project-docs/adr/0012-imports-bind-the-scope.md @@ -0,0 +1,58 @@ +# The importer binds an import's scope + +An `imports:` entry names the scope it binds, defaulting to the imported file's +basename with `.modelith.yaml` stripped. A model does not declare a name for +itself: the top-level `scope:` of ADR-0010 is gone. Supersedes ADR-0010's +**Identity** decision and the part of **Resolution** that made the imported file +the source of truth for the slug. The rest of ADR-0010 stands. + +## Context + +ADR-0010 put the slug in the imported file, so nothing was restated and nothing +could disagree. It also said the renderer documents each dependency with a deep +link into the imported model's rendered Markdown, and that it must never open an +imported file, so its output stays a pure function of the model it is rendering. + +Those two cannot both hold. A link needs a scope *and* a path. Rendering a model +alone, the scopes are visible at the reference sites and the paths in `imports:`, +but the pairing between them lived in a file the renderer may not read. The gap +surfaced during implementation, not review: the lint layer was built and the +renderer had nothing to render. + +The workarounds all made the tool guess. Matching a scope against an import's +filename, or building the link from the scope alone, each assumes a file is named +after its slug — an assumption nothing in the format enforces, whose failure mode +is a wrong link or a silently missing one. + +## Decision + +The binding moves to the importer, and `scope:` leaves the schema. `imports:` is +a union in the shape `actions` already established — a bare path, or +`{scope, path}` when the filename yields no usable slug or the obvious one is +taken: + +```yaml +imports: + - ./payments.modelith.yaml # binds "payments" + - scope: billing + path: ./legacy/pay-v2.modelith.yaml # binds "billing" +``` + +This is deliberate aliasing, reversing issue #25's "no xmlns-style mapping" line. +Two models may bind the same file to different scopes, and the imported file has +no say in either. What it buys is that every consumer — linter, renderer, reader +— resolves a reference using only the file in front of it. + +## Consequences + +- The filename is the de facto identity, since the default comes from it. + Consistent naming across repositories is conventional, not enforced. Accepted: + the explicit form is the escape hatch, and a bare import whose filename is not + a valid slug is a lint error that points at it. +- Two diagnostics from ADR-0010 are gone: an imported file that declares no + scope, and a comparison between a declared scope and anything else. Duplicate + bindings remain an error, now between two entries in one list rather than two + files. +- Pinned by `TestADR_0012_ImporterBindsScope`: one file bound to different + scopes by different importers resolves in both, and a top-level `scope:` is + rejected. From e779cb1bb38d44965f8f0919420cc66d83bc12f8 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 14:39:37 -0700 Subject: [PATCH 06/22] docs(example): reference a peer payments model from the parking garage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds docs/05-parking-garage/payments.modelith.yaml — a small payments context owning the PaymentMethod enum — and has the garage import it for Ticket.paidWith rather than keeping a second copy of the same list. Both lint clean under --completeness error and both .md files are committed; the directory is globbed by path, so CI picks the new model up with no change to Taskfile.yml or ci.yml. The imports material sits at the end of index.md, marked advanced: most readers will not need it for a long time and the teaching flow above it is the point of the page. Signed-off-by: Joe Beda --- docs/05-parking-garage/garage.modelith.md | 7 + docs/05-parking-garage/garage.modelith.yaml | 8 ++ docs/05-parking-garage/index.md | 57 ++++++++ docs/05-parking-garage/payments.modelith.md | 112 ++++++++++++++++ docs/05-parking-garage/payments.modelith.yaml | 123 ++++++++++++++++++ internal/model/selfmodel_test.go | 5 +- 6 files changed, 310 insertions(+), 2 deletions(-) create mode 100644 docs/05-parking-garage/payments.modelith.md create mode 100644 docs/05-parking-garage/payments.modelith.yaml diff --git a/docs/05-parking-garage/garage.modelith.md b/docs/05-parking-garage/garage.modelith.md index 4559d09..e6bac1e 100644 --- a/docs/05-parking-garage/garage.modelith.md +++ b/docs/05-parking-garage/garage.modelith.md @@ -4,6 +4,12 @@ Tracks the state of a single parking garage: its spots, the cars that occupy them, and the two ways a car enters — a monthly parker's keycard or a temporary visitor's pay-on-exit ticket. +## Imports + +Items defined in these models are referenced below as `scope.Name`. + +- **`payments`** — [`./payments.modelith.yaml`](./payments.modelith.md) + ## Glossary - **`Driver`** — A person who enters and exits using a monthly `Account`'s `Keycard`. The human behind a `Car`; not modeled as an entity. @@ -178,6 +184,7 @@ A temporary entry credential issued at the gate to a visitor with no `Account`. | `issuedAt` | timestamp | | | `amountDue` | integer | Fee owed, in the smallest currency unit. _Derived:_ Computed at payment time from the parking duration (issue time to now) and the `Garage`'s rate schedule. | | `paidAt` | timestamp | When the `Ticket` was paid; unset until then. | +| `paidWith` | [payments.PaymentMethod](./payments.modelith.md#paymentmethod) | How the fee was settled at the `Kiosk`; unset until paid. The garage does not define what a payment method is — the payments model does. | | `paid` | boolean | _Derived:_ True once `paidAt` is set. | **Actions** diff --git a/docs/05-parking-garage/garage.modelith.yaml b/docs/05-parking-garage/garage.modelith.yaml index 500cc29..b20cf13 100644 --- a/docs/05-parking-garage/garage.modelith.yaml +++ b/docs/05-parking-garage/garage.modelith.yaml @@ -7,6 +7,9 @@ description: >- them, and the two ways a car enters — a monthly parker's keycard or a temporary visitor's pay-on-exit ticket. +imports: + - ./payments.modelith.yaml + glossary: Operator: >- Garage staff or management who open `Account`s, issue `Keycard`s, register @@ -245,6 +248,11 @@ entities: - name: paidAt type: timestamp description: When the `Ticket` was paid; unset until then. + - name: paidWith + type: payments.PaymentMethod + description: >- + How the fee was settled at the `Kiosk`; unset until paid. The garage + does not define what a payment method is — the payments model does. - name: paid type: boolean derived: true diff --git a/docs/05-parking-garage/index.md b/docs/05-parking-garage/index.md index 9e99ece..5700744 100644 --- a/docs/05-parking-garage/index.md +++ b/docs/05-parking-garage/index.md @@ -231,3 +231,60 @@ the source. - **[View the full source YAML →](https://github.com/stacklok/modelith/blob/main/docs/05-parking-garage/garage.modelith.yaml)** Want to try it yourself? See [Authoring with Agents](../02-getting-started.md). + +## Advanced: borrowing a concept from another model + +:::note[Advanced] + +Skip this until you have two models. One model is the normal case, and +everything above works without it. + +::: + +Late in the session, one more concept turned up — and it did **not** belong to +the garage: + +> **You:** The kiosk takes card or cash. Add a payment method to the ticket. +> +> **Agent:** Happy to, but a payment method isn't a parking concept — it's the +> same list your billing system already has, and the moment both models define +> it, they start to drift. Is there another model that owns money? + +> **You:** There's a small payments model, yes. + +Rather than a second copy of the same list, the garage **references** the one in +the payments model. The payments model needs no changes at all — it doesn't know +it is being referenced: + +```yaml +# payments.modelith.yaml — an ordinary model that happens to define the enum +enums: + PaymentMethod: + values: + - name: card + - name: cash + - name: account +``` + +The garage lists the file and writes `scope.Name` at the reference site. The +scope comes from the filename, so `./payments.modelith.yaml` is written +`payments.`: + +```yaml +# garage.modelith.yaml +imports: + - ./payments.modelith.yaml + +# ... in Ticket: + - name: paidWith + type: payments.PaymentMethod +``` + +The linter now resolves that reference — a typo like `payments.PaymentMode` is +an error, not a shrug — and the [rendered model](./garage.modelith.md) links +`paidWith` straight to the enum in the +[payments model](./payments.modelith.md). One definition, in the context that +owns it. + +Full rules, including how to name a scope explicitly when the filename won't do: +[Imports](../06-schema-reference.md#imports). diff --git a/docs/05-parking-garage/payments.modelith.md b/docs/05-parking-garage/payments.modelith.md new file mode 100644 index 0000000..3f4b363 --- /dev/null +++ b/docs/05-parking-garage/payments.modelith.md @@ -0,0 +1,112 @@ +{/* Generated by `modelith render`. Do not edit by hand; edit the .modelith.yaml source and re-render. */} + +# Payments + +How money is taken and given back. A deliberately small context that owns the vocabulary of settling a charge — nothing in it knows what was bought, which is why the parking-garage model can reference its `PaymentMethod` instead of keeping a second copy of the same list. + +## Glossary + +- **`Merchant`** — Whoever took the money and can give it back. Issues a `Refund`; a role, not modeled as an entity. +- **`Payer`** — Whoever hands over the money for a `Payment` — a person at a terminal, or a standing arrangement charged automatically. A role, not modeled as an entity. + +## Enums + +### `PaymentMethod` + +How a `Payment` was tendered. + +| Value | Definition | +| --- | --- | +| `card` | A payment card, tapped or inserted at a terminal. | +| `cash` | Notes or coins, accepted by a machine or by a person. | +| `account` | Charged to a standing arrangement and settled later, so the `Payer` is billed rather than paying at the moment of the `Payment`. | + +## Entities + +### `Payment` + +One completed transfer of money for one charge: an amount, the method it was tendered with, and the moment it succeeded. A `Payment` records a transfer that settled — a declined card produces no `Payment` — and its amount never changes afterwards; money given back is a `Refund`. + +**Relationships** + +- `Refund` — 1:n — owned — Money returned from this `Payment`. There may be several partial ones, and none at all is the ordinary case. + +**Attributes** + +| Name | Type | Description | +| --- | --- | --- | +| `amount` | integer | What was taken, in the smallest currency unit. | +| `method` | PaymentMethod | | +| `settledAt` | timestamp | When the transfer succeeded. | +| `refundedTotal` | integer | _Derived:_ The sum of the amounts of this `Payment`'s `Refund`s. | + +**Actions** + +- `take` — actor `Payer`; preserves payment-amount-positive — Tender the amount and, on success, record the `Payment`. + +**Invariants** + +- **payment-amount-positive** — A `Payment`'s amount is greater than zero. +- **payment-method-fixed** — A `Payment`'s method is fixed once it has settled; paying a different way is a new `Payment`. + +### `Refund` + +Money returned from one `Payment`, in whole or in part. A `Refund` belongs to exactly one `Payment` and is always given back by the method that `Payment` used, so it carries no method of its own. + +**Attributes** + +| Name | Type | Description | +| --- | --- | --- | +| `amount` | integer | What was returned, in the smallest currency unit. | +| `issuedAt` | timestamp | | + +**Actions** + +- `issue` — actor `Merchant`; preserves refund-within-payment, refund-after-settlement + +**Invariants** + +- **refund-within-payment** — The amounts of a `Payment`'s `Refund`s never total more than that `Payment`'s amount. +- **refund-after-settlement** — A `Refund` is issued at or after its `Payment` settled. + +## Relationships + +```mermaid +erDiagram + Payment {} + Refund {} + Payment ||--o{ Refund : "" +``` + +## Scenarios + +### A charge is settled by card + +**Actors:** Payer, Payment + +**Steps** + +1. A `Payer` is asked for an amount and taps a card at the terminal. +2. The card is accepted, so a `Payment` records the amount, the method `card`, and the moment it settled. +3. Asked later to pay the same charge by cash instead, the system declines: the settled `Payment`'s method does not change. + +**Invariants touched** + +- **payment-amount-positive** — A `Payment`'s amount is greater than zero. +- **payment-method-fixed** — A `Payment`'s method is fixed once it has settled; paying a different way is a new `Payment`. + +### Part of a charge is given back + +**Actors:** Merchant, Payment, Refund + +**Steps** + +1. A settled `Payment` of 500 exists, with nothing returned from it yet. +2. The `Merchant` issues a `Refund` of 200 against it; the `Payment`'s refunded total becomes 200. +3. A second `Refund` of 400 is attempted; the system refuses it, because the two together would exceed the `Payment`. + +**Invariants touched** + +- **refund-within-payment** — The amounts of a `Payment`'s `Refund`s never total more than that `Payment`'s amount. +- **refund-after-settlement** — A `Refund` is issued at or after its `Payment` settled. + diff --git a/docs/05-parking-garage/payments.modelith.yaml b/docs/05-parking-garage/payments.modelith.yaml new file mode 100644 index 0000000..393ee33 --- /dev/null +++ b/docs/05-parking-garage/payments.modelith.yaml @@ -0,0 +1,123 @@ +# yaml-language-server: $schema=https://modelith.sh/schema/domain-model/v1.json +kind: DomainModel +version: v1 +title: Payments +description: >- + How money is taken and given back. A deliberately small context that owns the + vocabulary of settling a charge — nothing in it knows what was bought, which + is why the parking-garage model can reference its `PaymentMethod` instead of + keeping a second copy of the same list. + +glossary: + Payer: >- + Whoever hands over the money for a `Payment` — a person at a terminal, or a + standing arrangement charged automatically. A role, not modeled as an + entity. + Merchant: >- + Whoever took the money and can give it back. Issues a `Refund`; a role, not + modeled as an entity. + +enums: + PaymentMethod: + description: How a `Payment` was tendered. + values: + - name: card + definition: A payment card, tapped or inserted at a terminal. + - name: cash + definition: Notes or coins, accepted by a machine or by a person. + - name: account + definition: >- + Charged to a standing arrangement and settled later, so the `Payer` is + billed rather than paying at the moment of the `Payment`. + +entities: + Payment: + definition: >- + One completed transfer of money for one charge: an amount, the method it + was tendered with, and the moment it succeeded. A `Payment` records a + transfer that settled — a declined card produces no `Payment` — and its + amount never changes afterwards; money given back is a `Refund`. + relationships: + - entity: Refund + cardinality: 1:n + ownership: owned + note: >- + Money returned from this `Payment`. There may be several partial ones, + and none at all is the ordinary case. + attributes: + - name: amount + type: integer + description: What was taken, in the smallest currency unit. + - name: method + type: PaymentMethod + - name: settledAt + type: timestamp + description: When the transfer succeeded. + - name: refundedTotal + type: integer + derived: true + derivation: The sum of the amounts of this `Payment`'s `Refund`s. + actions: + - name: take + actor: Payer + preserves: [payment-amount-positive] + description: Tender the amount and, on success, record the `Payment`. + invariants: + - id: payment-amount-positive + statement: A `Payment`'s amount is greater than zero. + - id: payment-method-fixed + statement: >- + A `Payment`'s method is fixed once it has settled; paying a different + way is a new `Payment`. + + Refund: + definition: >- + Money returned from one `Payment`, in whole or in part. A `Refund` belongs + to exactly one `Payment` and is always given back by the method that + `Payment` used, so it carries no method of its own. + attributes: + - name: amount + type: integer + description: What was returned, in the smallest currency unit. + - name: issuedAt + type: timestamp + actions: + - name: issue + actor: Merchant + preserves: [refund-within-payment, refund-after-settlement] + invariants: + - id: refund-within-payment + statement: >- + The amounts of a `Payment`'s `Refund`s never total more than that + `Payment`'s amount. + - id: refund-after-settlement + statement: A `Refund` is issued at or after its `Payment` settled. + +scenarios: + - name: A charge is settled by card + actors: [Payer, Payment] + steps: + - A `Payer` is asked for an amount and taps a card at the terminal. + - >- + The card is accepted, so a `Payment` records the amount, the method + `card`, and the moment it settled. + - >- + Asked later to pay the same charge by cash instead, the system declines: + the settled `Payment`'s method does not change. + invariants_touched: + - payment-amount-positive + - payment-method-fixed + + - name: Part of a charge is given back + actors: [Merchant, Payment, Refund] + steps: + - A settled `Payment` of 500 exists, with nothing returned from it yet. + - >- + The `Merchant` issues a `Refund` of 200 against it; the `Payment`'s + refunded total becomes 200. + - >- + A second `Refund` of 400 is attempted; the system refuses it, because + the two together would exceed the `Payment`. + invariants_touched: + - refund-within-payment + - refund-after-settlement diff --git a/internal/model/selfmodel_test.go b/internal/model/selfmodel_test.go index d503570..5e30050 100644 --- a/internal/model/selfmodel_test.go +++ b/internal/model/selfmodel_test.go @@ -20,8 +20,9 @@ func TestADR_0009_NoSelfModel(t *testing.T) { t.Parallel() allowed := map[string]string{ - "examples/example.modelith.yaml": "the worked example (golden fixture)", - "docs/05-parking-garage/garage.modelith.yaml": "the docs example (golden fixture)", + "examples/example.modelith.yaml": "the worked example (golden fixture)", + "docs/05-parking-garage/garage.modelith.yaml": "the docs example (golden fixture)", + "docs/05-parking-garage/payments.modelith.yaml": "the docs example's imported peer model (golden fixture)", } root := repoRoot(t) From df2616e7eb17e2c5ff8637f868faa077e14fa19a Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 15:14:38 -0700 Subject: [PATCH 07/22] fix(imports): escape import paths and tighten the reference diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An import path reaches a published Markdown page unescaped, in both the link label and the link destination, while every other string in the renderer goes through mdCell/oneLine. A path closing the destination and opening a tag renders a live element and a second link from a model that lints clean; a path holding a newline injects a heading into the Imports list and splits a lint diagnostic across two lines. Escape each value for the position it lands in. A link destination is percent-encoded outside the URL unreserved set, so parentheses, whitespace, angle brackets and control characters cannot end it. A link label is backslash-escaped plain text rather than a code span: a code span cannot escape the "]" that ends the label, which would leave its inertness resting on the rule that code spans bind more tightly than link brackets — the spec says so and widely used renderers disagree. A code span, where one is still used, is fenced past the longest backtick run inside it. oneLine now replaces every control character, not only the line breaks. The parts are escaped before the link is assembled; escaping the finished link is what left a pipe inside a URL as "\|". Alongside, the lint layer: - an import path holding a control character is an error, and every import diagnostic quotes the path with %q so an embedded newline survives as text instead of a line break; - an attribute type containing a dot must be exactly scope.Name, and a malformed one says which way it is malformed rather than passing as a primitive; - imports resolve only against a document the schema accepted, so a rejected scope no longer binds and then advises impossible syntax; - an imported model declaring an unsupported schema version is an error naming the file and the version; - references to one unbound scope are reported once with a count, and a scope whose import failed to load is not blamed at every reference site — a single typo was producing 201 errors; - a duplicate scope binding is settled before the file is opened, so an unreadable second binder no longer masks it. model.ScopeSlug becomes the one definition of a scope slug, consumed by the linter, the renderer and the schema, which disagreed on the minimum length: a single-character scope is now legal. The only golden change is the imports bullet, whose path label loses its backticks. Signed-off-by: Joe Beda --- docs/05-parking-garage/garage.modelith.md | 2 +- docs/06-schema-reference.md | 11 +- internal/lint/imports.go | 160 ++++++++++++++---- internal/lint/imports_test.go | 187 ++++++++++++++++++++++ internal/lint/lint.go | 8 +- internal/model/model.go | 6 + internal/model/sync_test.go | 21 +++ internal/render/markdown/markdown.go | 136 +++++++++++++--- internal/render/markdown/markdown_test.go | 92 ++++++++++- internal/schema/v1/modelith.schema.json | 2 +- 10 files changed, 563 insertions(+), 62 deletions(-) diff --git a/docs/05-parking-garage/garage.modelith.md b/docs/05-parking-garage/garage.modelith.md index e6bac1e..20e4c2e 100644 --- a/docs/05-parking-garage/garage.modelith.md +++ b/docs/05-parking-garage/garage.modelith.md @@ -8,7 +8,7 @@ Tracks the state of a single parking garage: its spots, the cars that occupy the Items defined in these models are referenced below as `scope.Name`. -- **`payments`** — [`./payments.modelith.yaml`](./payments.modelith.md) +- **`payments`** — [./payments.modelith.yaml](./payments.modelith.md) ## Glossary diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index dad9450..6902ac7 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -297,16 +297,21 @@ imports: | Field | Type | Required | Notes | |---|---|---|---| -| `scope` | string | yes (explicit form) | The slug written before an item's name. Lowercase kebab-case (`^[a-z][a-z0-9-]+$`). | -| `path` | string | yes (explicit form) | Path to the model file, relative to *this* one. | +| `scope` | string | yes (explicit form) | The slug written before an item's name. Lowercase kebab-case (`^[a-z][a-z0-9-]*$`). | +| `path` | string | yes (explicit form) | Path to the model file, relative to *this* one. Relative, and free of control characters. | A bare string is the same thing with the scope derived — `./x.modelith.yaml` is exactly `{scope: x, path: ./x.modelith.yaml}`. If the filename doesn't yield a valid slug (`./Pay Ments.yaml`), the linter says so and points at the explicit form. -Four rules are worth knowing before you use this: +Five rules are worth knowing before you use this: +- **A dot means a cross-model reference.** An attribute `type` containing one + has to be exactly `scope.Name` — one dot, a slug before it, a PascalCase item + name after. `payments.v2.PaymentMethod`, `.PaymentMethod` and `payments.` are + errors that say which way they are malformed, not types quietly read as + primitives. - **The binding is local.** Two models may import the same file under different scopes, and neither one's choice is visible to the other. Two imports binding the *same* scope in one model is an error — give one of them an explicit, diff --git a/internal/lint/imports.go b/internal/lint/imports.go index b62a6de..ae73067 100644 --- a/internal/lint/imports.go +++ b/internal/lint/imports.go @@ -5,8 +5,11 @@ import ( "os" "path/filepath" "regexp" + "strings" + "unicode" "github.com/stacklok/modelith/internal/model" + "github.com/stacklok/modelith/internal/schema" ) // FileReader reads an imported model file by path. It is the seam that keeps @@ -25,14 +28,14 @@ type OSFiles struct{} func (OSFiles) ReadFile(path string) ([]byte, error) { return os.ReadFile(path) } var ( - // qualifiedRefRE matches a cross-model reference, "scope.Name". The scope - // side mirrors the schema's scope pattern; the name side stays loose so a - // reference to something that isn't a defined enum is reported as a broken - // reference rather than silently read as a primitive type. - qualifiedRefRE = regexp.MustCompile(`^([a-z][a-z0-9-]*)\.([A-Za-z0-9]+)$`) + // qualifiedRefRE matches a well-formed cross-model reference, "scope.Name": + // exactly one dot, a scope slug before it, a PascalCase item name after. + // Anything else containing a dot is a malformed reference, not a primitive + // type — malformedRefReason says which way it is malformed. + qualifiedRefRE = regexp.MustCompile(`^(` + model.ScopeSlug + `)\.([A-Z][A-Za-z0-9]*)$`) // scopeRE is the slug a bare import's filename has to yield. An explicitly // written scope is held to the same pattern by the schema. - scopeRE = regexp.MustCompile(`^[a-z][a-z0-9-]+$`) + scopeRE = regexp.MustCompile(`^` + model.ScopeSlug + `$`) ) // importedModel is one successfully resolved entry of a model's imports list. @@ -48,8 +51,8 @@ type importedModel struct { // modelPath is the path of the model being linted; imports resolve relative to // its directory. func runImports(modelPath string, m *model.Model, fr FileReader, res *Result) { - byScope := loadImports(modelPath, m, fr, res) - used := checkQualifiedTypes(m, byScope, res) + byScope, claimed := loadImports(modelPath, m, fr, res) + used := checkQualifiedTypes(m, byScope, claimed, res) // An unreferenced import is a completeness finding, alongside the unused // enum and the unused glossary term: vocabulary the model declares and // nothing uses. Sharing their category means sharing their promotion under @@ -72,11 +75,13 @@ func runImports(modelPath string, m *model.Model, fr FileReader, res *Result) { } // loadImports reads each declared import and returns the ones that resolved, -// keyed by the scope the importing model binds them to. Every rejection is -// reported as an error: an import that cannot be resolved is a broken -// reference, not a gap. -func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) map[string]importedModel { - byScope := map[string]importedModel{} +// keyed by the scope the importing model binds them to, plus every scope the +// list claimed at all — including entries that then failed to load — mapped to +// the path of the first entry that claimed it. Every rejection is reported as an +// error: an import that cannot be resolved is a broken reference, not a gap. +func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) (byScope map[string]importedModel, claimed map[string]string) { + byScope = map[string]importedModel{} + claimed = map[string]string{} dir := filepath.Dir(modelPath) for i, imp := range m.Imports { reject := func(format string, args ...any) { @@ -90,18 +95,42 @@ func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) m if imp.Path == "" { continue // the schema's minLength reports the empty path } - if filepath.IsAbs(imp.Path) || imp.Path[0] == '/' { - reject("import %q is an absolute path — imports are relative to this model so they resolve in any checkout", imp.Path) + // The scope is settled before the path is judged, and claimed before the + // file is opened. Reference sites depend on the scope alone, so an entry + // that names a usable one holds it whatever else is wrong with the entry: + // a duplicate binding is then reported on its own terms instead of being + // pre-empted by the second entry's unrelated trouble, and a broken path + // does not also make every "scope.Name" that names it look like a + // reference to nothing. A written scope is held to the pattern by the + // schema; a derived one the schema never sees. + scopeOK := !imp.ScopeFromPath || scopeRE.MatchString(imp.Scope) + if scopeOK { + if prev, dup := claimed[imp.Scope]; dup { + reject("import %q binds scope %q, which import %q already binds — give one of them an explicit, different scope so %s.Name resolves unambiguously", + imp.Path, imp.Scope, prev, imp.Scope) + continue + } + claimed[imp.Scope] = imp.Path + } + // A control character is never part of a filename, and one that reached + // here would be carried into this diagnostic, the shell, and the rendered + // Markdown. Every message here quotes the path with %q, so an escape + // survives as text rather than as a line break in the output. + if at := strings.IndexFunc(imp.Path, unicode.IsControl); at >= 0 { + reject("import path %q contains a control character at byte %d — a filename cannot contain one; check for a stray newline or escape in the YAML", imp.Path, at) continue } - // A written scope is held to the pattern by the schema; a derived one the - // schema never sees, and a filename that yields no usable slug is the - // case the explicit form exists for. - if imp.ScopeFromPath && !scopeRE.MatchString(imp.Scope) { - reject("import %q binds the scope %q taken from its filename, which is not a valid slug (lowercase kebab-case) — name the scope explicitly instead: `- {scope: , path: %s}`", + if !scopeOK { + // A filename that yields no usable slug is the case the explicit form + // exists for. + reject("import %q binds the scope %q taken from its filename, which is not a valid slug (lowercase kebab-case) — name the scope explicitly instead: `- {scope: , path: %q}`", imp.Path, imp.Scope, imp.Path) continue } + if filepath.IsAbs(imp.Path) || imp.Path[0] == '/' { + reject("import %q is an absolute path — imports are relative to this model so they resolve in any checkout", imp.Path) + continue + } data, err := fr.ReadFile(filepath.Join(dir, imp.Path)) if err != nil { reject("import %q cannot be read: %v", imp.Path, err) @@ -112,14 +141,14 @@ func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) m reject("import %q is not a domain model — lint it on its own with `modelith lint` to see why", imp.Path) continue } - if prev, ok := byScope[imp.Scope]; ok { - reject("import %q binds scope %q, which import %q already binds — give one of them an explicit, different scope so %s.Name resolves unambiguously", - imp.Path, imp.Scope, prev.path, imp.Scope) + if !schema.Supported(im.Version) { + reject("import %q declares schema version %q, which this modelith does not support: %s (upgrade modelith, or move that model to a supported version)", + imp.Path, im.Version, strings.Join(schema.SupportedVersions(), ", ")) continue } byScope[imp.Scope] = importedModel{index: i, path: imp.Path, model: im} } - return byScope + return byScope, claimed } // checkQualifiedTypes resolves every qualified attribute type against the @@ -129,15 +158,18 @@ func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) m // PascalCase type that names no enum is only a warning (runSemantic). The // asymmetry is deliberate: an unqualified name may be a primitive the author // invented, while "scope.Name" can only be a cross-model reference. -func checkQualifiedTypes(m *model.Model, byScope map[string]importedModel, res *Result) map[string]bool { +func checkQualifiedTypes(m *model.Model, byScope map[string]importedModel, claimed map[string]string, res *Result) map[string]bool { used := map[string]bool{} + unbound := map[string]*unboundScope{} for _, name := range m.EntityNames() { for i, attr := range m.Entities[name].Attributes { - match := qualifiedRefRE.FindStringSubmatch(attr.Type) - if match == nil { + // A dot in a type can only be a cross-model reference: primitives are + // lowercase words and enum names are PascalCase, neither of which + // carries one. So a dotted type that is not well formed is a typo to + // report, not a type to pass over. + if !strings.Contains(attr.Type, ".") { continue } - scope, item := match[1], match[2] path := fmt.Sprintf("/entities/%s/attributes/%d/type", name, i) broken := func(format string, args ...any) { res.Findings = append(res.Findings, Finding{ @@ -147,9 +179,34 @@ func checkQualifiedTypes(m *model.Model, byScope map[string]importedModel, res * Message: fmt.Sprintf(format, args...), }) } + match := qualifiedRefRE.FindStringSubmatch(attr.Type) + if match == nil { + broken("attribute type %q is a malformed cross-model reference (%s) — write it as scope.Name: one dot, a lowercase kebab-case scope, a PascalCase item name", + attr.Type, malformedRefReason(attr.Type)) + // The scope the author was reaching for is still referenced, so a + // typo in the item name does not also read as an unused import. + if head, _, _ := strings.Cut(attr.Type, "."); byScope[head].model != nil { + used[head] = true + } + continue + } + scope, item := match[1], match[2] imp, ok := byScope[scope] if !ok { - broken("attribute type %q references the scope %q, which no import binds — add the model that defines %s to `imports:`", attr.Type, scope, item) + if _, listed := claimed[scope]; listed { + // An import claims this scope but failed to load, and that + // failure is already reported against the imports list. It is + // the one thing to fix; saying the scope is unbound here would + // report the same mistake again at every reference site. + used[scope] = true + continue + } + u, seen := unbound[scope] + if !seen { + unbound[scope] = &unboundScope{path: path, typ: attr.Type, item: item, count: 1} + continue + } + u.count++ continue } // The import is referenced even when the item does not resolve, so @@ -165,9 +222,54 @@ func checkQualifiedTypes(m *model.Model, byScope map[string]importedModel, res * broken("attribute type %q names no enum %q in %q — an imported model's own imports are not reachable from here", attr.Type, item, imp.path) } } + reportUnboundScopes(unbound, res) return used } +// unboundScope accumulates the references to one scope no import binds, so a +// single missing import is reported once rather than once per reference site. +type unboundScope struct { + path string // where the first reference is + typ string // the first reference, verbatim + item string + count int +} + +func reportUnboundScopes(unbound map[string]*unboundScope, res *Result) { + for _, scope := range sortedMapKeys(unbound) { + u := unbound[scope] + msg := fmt.Sprintf("attribute type %q references the scope %q, which no import binds — add the model that defines %s to `imports:`", u.typ, scope, u.item) + if u.count > 1 { + msg += fmt.Sprintf(" (%d attribute types reference this scope; this is the first)", u.count) + } + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategorySemantic, + Path: u.path, + Message: msg, + }) + } +} + +// malformedRefReason names what is wrong with a type that contains a dot but is +// not a well-formed "scope.Name", so the diagnostic points at the fix instead of +// only restating the shape. +func malformedRefReason(typ string) string { + scope, item, _ := strings.Cut(typ, ".") + switch { + case scope == "": + return "nothing before the dot" + case item == "": + return "nothing after the dot" + case strings.Contains(item, "."): + return "more than one dot" + case !scopeRE.MatchString(scope): + return fmt.Sprintf("the scope %q is not lowercase kebab-case", scope) + default: + return fmt.Sprintf("the item name %q is not PascalCase", item) + } +} + // reportQualifiedEntityRefs reports a cross-model reference in an entity // position — relationship.entity or subtypeOf — and returns the instance paths // it reported, so the schema's own finding for the same value is suppressed. diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index 55ee973..a9d174a 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -248,6 +248,193 @@ func TestImports_Resolution(t *testing.T) { } } +// TestImports_MalformedQualifiedType covers a type that contains a dot but is +// not a well-formed "scope.Name". Each shape names the way it is malformed +// rather than passing silently as a primitive. +func TestImports_MalformedQualifiedType(t *testing.T) { + t.Parallel() + + files := fakeFiles{"docs/payments.modelith.yaml": paymentsModel} + const typePath = "/entities/Visit/attributes/0/type" + + cases := []struct { + name string + attrType string + reason string + // unusedImport is set where the malformed type names no bound scope, so + // the import genuinely goes unreferenced. + unusedImport bool + }{ + {name: "two dots", attrType: "payments.v2.PaymentMethod", reason: "more than one dot"}, + {name: "leading dot", attrType: ".PaymentMethod", reason: "nothing before the dot", unusedImport: true}, + {name: "trailing dot", attrType: "payments.", reason: "nothing after the dot"}, + {name: "scope is not a slug", attrType: "Payments.PaymentMethod", reason: `the scope "Payments" is not lowercase kebab-case`, unusedImport: true}, + {name: "item is not PascalCase", attrType: "payments.paymentMethod", reason: `the item name "paymentMethod" is not PascalCase`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + want := []wantFinding{{ + SeverityError, CategorySemantic, typePath, + fmt.Sprintf("attribute type %q is a malformed cross-model reference (%s)", tc.attrType, tc.reason), + }} + if tc.unusedImport { + want = append(want, wantFinding{ + SeverityWarning, CategoryCompleteness, "/imports/0", "is never referenced", + }) + } + src := importer([]string{`"./payments.modelith.yaml"`}, tc.attrType) + res, err := Run(importerPath, []byte(src), files) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), want) + }) + } +} + +// TestImports_PathRejectsControlCharacters checks the lint-side half of the +// injection defence: a path holding a newline or any other control character is +// not a path, and saying so keeps it out of the renderer and out of this +// diagnostic's own line-wise output. +func TestImports_PathRejectsControlCharacters(t *testing.T) { + t.Parallel() + + src := importer([]string{`{scope: payments, path: "./pay\n## Injected\nments.modelith.yaml"}`}, "payments.PaymentMethod") + res, err := Run(importerPath, []byte(src), fakeFiles{}) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), []wantFinding{{ + SeverityError, CategorySemantic, "/imports/0", + `import path "./pay\n## Injected\nments.modelith.yaml" contains a control character at byte 5`, + }}) + for _, f := range res.Findings { + if strings.ContainsAny(f.Message, "\n\r") { + t.Errorf("a diagnostic must stay on one line, got %q", f.Message) + } + } +} + +// TestImports_UnresolvedScopeReportedOnce checks that one missing import is one +// finding however many attributes reference it, and that an import which is +// listed but failed to load does not also make every reference site look like a +// reference to nothing. +func TestImports_UnresolvedScopeReportedOnce(t *testing.T) { + t.Parallel() + + // Six attributes, all naming the same unbound scope. + var b strings.Builder + b.WriteString("kind: DomainModel\nversion: v1\nentities:\n Visit:\n definition: One stay.\n attributes:\n") + for i := 0; i < 6; i++ { + fmt.Fprintf(&b, " - name: a%d\n type: shipping.Carrier\n", i) + } + res, err := Run(importerPath, []byte(b.String()), fakeFiles{}) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), []wantFinding{{ + SeverityError, CategorySemantic, "/entities/Visit/attributes/0/type", + `references the scope "shipping", which no import binds — add the model that defines Carrier to ` + + "`imports:` (6 attribute types reference this scope; this is the first)", + }}) + + t.Run("an import that failed to load speaks for its own scope", func(t *testing.T) { + t.Parallel() + src := importer([]string{`"./typo.modelith.yaml"`}, "typo.PaymentMethod") + res, err := Run(importerPath, []byte(src), fakeFiles{}) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), []wantFinding{{ + SeverityError, CategorySemantic, "/imports/0", "cannot be read", + }}) + }) +} + +// TestImports_DuplicateScopeSurvivesAnUnreadableBinder checks that the second +// entry's own trouble does not mask the duplicate binding: fixing the path would +// otherwise surface a second, unrelated failure. +func TestImports_DuplicateScopeSurvivesAnUnreadableBinder(t *testing.T) { + t.Parallel() + + files := fakeFiles{"docs/payments.modelith.yaml": paymentsModel} + src := importer( + []string{`"./payments.modelith.yaml"`, "{scope: payments, path: ./gone.modelith.yaml}"}, + "payments.PaymentMethod", + ) + res, err := Run(importerPath, []byte(src), files) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), []wantFinding{{ + SeverityError, CategorySemantic, "/imports/1", + `binds scope "payments", which import "./payments.modelith.yaml" already binds`, + }}) +} + +// TestImports_UnsupportedVersionInAnImportedModel checks that an imported file +// is held to the versions this binary understands, not just to its kind. +func TestImports_UnsupportedVersionInAnImportedModel(t *testing.T) { + t.Parallel() + + files := fakeFiles{ + "docs/payments.modelith.yaml": strings.Replace(paymentsModel, "version: v1", "version: v99", 1), + } + src := importer([]string{`"./payments.modelith.yaml"`}, "payments.PaymentMethod") + res, err := Run(importerPath, []byte(src), files) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), []wantFinding{{ + SeverityError, CategorySemantic, "/imports/0", + `import "./payments.modelith.yaml" declares schema version "v99", which this modelith does not support: v1`, + }}) +} + +// TestImports_SkippedWhenTheSchemaRejectedTheDocument checks that a scope the +// schema already rejected never binds. Advising a fix in terms of a scope that +// cannot exist — "reference it as .Name" — is worse than saying nothing. +func TestImports_SkippedWhenTheSchemaRejectedTheDocument(t *testing.T) { + t.Parallel() + + files := fakeFiles{"docs/payments.modelith.yaml": paymentsModel} + for _, scope := range []string{`""`, "Payments", "pay.ments"} { + t.Run(scope, func(t *testing.T) { + t.Parallel() + src := importer([]string{"{scope: " + scope + ", path: ./payments.modelith.yaml}"}, "payments.PaymentMethod") + res, err := Run(importerPath, []byte(src), files) + if err != nil { + t.Fatal(err) + } + if !findingWithMessage(res.Findings, "does not match pattern") { + t.Fatalf("expected the schema to reject the scope, got: %+v", res.Findings) + } + for _, f := range res.Findings { + if f.Category != CategoryStructural && strings.HasPrefix(f.Path, "/imports/") { + t.Errorf("imports must not resolve against a rejected document, got %+v", f) + } + } + }) + } +} + +// TestImports_SingleCharacterScope pins the one definition of a scope slug: a +// one-character scope is legal, so "a.modelith.yaml" can be imported bare. The +// schema, the linter and the reference pattern all read it from +// model.ScopeSlug, so they cannot disagree about the minimum length. +func TestImports_SingleCharacterScope(t *testing.T) { + t.Parallel() + + files := fakeFiles{"docs/a.modelith.yaml": paymentsModel} + res, err := Run(importerPath, []byte(importer([]string{`"./a.modelith.yaml"`}, "a.PaymentMethod")), files) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), nil) +} + // TestADR_0012_ImporterBindsScope pins that the scope belongs to the importing // model: the same file resolves under whatever scope each importer binds it to, // and a model has no say in — and no field for — what it is called elsewhere. diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 33ccb48..5c1703b 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -109,7 +109,13 @@ func Run(path string, src []byte, fr FileReader) (*Result, error) { } runSemantic(m, res) - runImports(path, m, fr, res) + // Imports resolve only against a document the schema accepted. A scope the + // schema already rejected would otherwise bind anyway, and the advice that + // follows would tell the author to write syntax that cannot work — the same + // reason the version check gates schema validation. + if structuralOK { + runImports(path, m, fr, res) + } runRelationshipShape(m, res) runSubtypes(m, res) runReciprocity(m, res) diff --git a/internal/model/model.go b/internal/model/model.go index 9a18323..b5b0eaa 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -38,6 +38,12 @@ type Model struct { Invariants []Invariant `json:"invariants,omitempty"` } +// ScopeSlug is the one definition of a valid import scope, as an unanchored +// regexp source so callers can embed it in a larger pattern ("scope.Name"). +// The schema's `scope` pattern is this anchored, and +// TestInvariant_ScopeSlugMatchesSchema guards the two against drift. +const ScopeSlug = `[a-z][a-z0-9-]*` + // Import is another model file this one references, bound to the scope written // before an imported item's name ("payments.PaymentMethod"). The binding is the // importer's: the imported file says nothing about how it is named here, so two diff --git a/internal/model/sync_test.go b/internal/model/sync_test.go index 93e4181..2aeea9b 100644 --- a/internal/model/sync_test.go +++ b/internal/model/sync_test.go @@ -95,3 +95,24 @@ func sortedKeys(m map[string]bool) []string { sort.Strings(out) return out } + +// TestInvariant_ScopeSlugMatchesSchema guards the one definition of an import +// scope. Three places used to carry their own pattern and disagreed on the +// minimum length, so a one-character filename could not be imported bare. The +// linter and the renderer build theirs from ScopeSlug; the schema's copy is +// hand-written JSON, so it is checked here. +func TestInvariant_ScopeSlugMatchesSchema(t *testing.T) { + var root map[string]any + if err := json.Unmarshal(schema.JSON(), &root); err != nil { + t.Fatal(err) + } + defs, _ := root["$defs"].(map[string]any) + obj, _ := defs["importObject"].(map[string]any) + props, _ := obj["properties"].(map[string]any) + scope, _ := props["scope"].(map[string]any) + got, _ := scope["pattern"].(string) + + if want := "^" + ScopeSlug + "$"; got != want { + t.Errorf("schema importObject.scope pattern is %q, want %q (from model.ScopeSlug)", got, want) + } +} diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index 7d15a53..1d97cc0 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -8,6 +8,7 @@ import ( "regexp" "sort" "strings" + "unicode" "github.com/stacklok/modelith/internal/model" "github.com/stacklok/modelith/internal/render/mermaid" @@ -17,10 +18,14 @@ import ( // are consumed by Docusaurus v3, whose MDX parser rejects HTML comments. const generatedBanner = "{/* Generated by `modelith render`. Do not edit by hand; edit the .modelith.yaml source and re-render. */}" -// qualifiedTypeRE matches an attribute type that references an imported item, -// "scope.Name". It mirrors the linter's pattern; the renderer only decides -// whether to link, never whether the reference resolves. -var qualifiedTypeRE = regexp.MustCompile(`^([a-z][a-z0-9-]*)\.([A-Za-z0-9]+)$`) +var ( + // qualifiedTypeRE matches an attribute type that references an imported item, + // "scope.Name". It mirrors the linter's pattern; the renderer only decides + // whether to link, never whether the reference resolves. + qualifiedTypeRE = regexp.MustCompile(`^(` + model.ScopeSlug + `)\.([A-Z][A-Za-z0-9]*)$`) + // backtickRunRE finds the backtick runs a code span's fence has to clear. + backtickRunRE = regexp.MustCompile("`+") +) // Render produces the full Markdown document for the model. func Render(m *model.Model) string { @@ -76,8 +81,11 @@ func renderImports(b *strings.Builder, m *model.Model) { } b.WriteString("## Imports\n\n") b.WriteString("Items defined in these models are referenced below as `scope.Name`.\n\n") + // A path — and the scope a bare import derives from it — is author-supplied + // text that reaches a published page, so both go through the same escaping + // as every other string here, and the link destination through its own. for _, imp := range m.Imports { - fmt.Fprintf(b, "- **`%s`** — [`%s`](%s)\n", imp.Scope, imp.Path, model.RenderedPath(imp.Path)) + fmt.Fprintf(b, "- **%s** — [%s](%s)\n", codeSpan(imp.Scope), mdLinkText(imp.Path), linkTarget(model.RenderedPath(imp.Path))) } b.WriteString("\n") } @@ -98,20 +106,26 @@ func importAnchors(m *model.Model) map[string]string { return out } -// linkQualifiedType renders an attribute type, linking a "scope.Name" reference -// into the imported model's Markdown. Headings render as "### `Name`", so the -// anchor is the item's name lowercased. A scope no import binds — a lint error -// — renders as written rather than as a link to nowhere. -func linkQualifiedType(typ string, targets map[string]string) string { +// typeCell renders an attribute type for a table cell, linking a "scope.Name" +// reference into the imported model's Markdown. Headings render as "### `Name`", +// so the anchor is the item's name lowercased. A scope no import binds — a lint +// error — renders as written rather than as a link to nowhere. +// +// Each part is escaped for the position it lands in, before the link is +// assembled. Escaping the finished link instead would treat a URL as cell text +// and corrupt the destination. +func typeCell(typ string, targets map[string]string) string { match := qualifiedTypeRE.FindStringSubmatch(typ) if match == nil { - return typ + return mdCell(typ) } target, ok := targets[match[1]] if !ok { - return typ + return mdCell(typ) } - return fmt.Sprintf("[%s](%s#%s)", typ, target, strings.ToLower(match[2])) + // typ matched qualifiedTypeRE, so the label holds nothing a cell or a link + // label reacts to. + return fmt.Sprintf("[%s](%s#%s)", typ, linkTarget(target), strings.ToLower(match[2])) } func renderGlossary(b *strings.Builder, m *model.Model) { @@ -239,7 +253,7 @@ func renderEntities(b *strings.Builder, m *model.Model) { desc = d } } - fmt.Fprintf(b, "| `%s` | %s | %s |\n", attr.Name, mdCell(linkQualifiedType(attr.Type, importTargets)), mdCell(desc)) + fmt.Fprintf(b, "| `%s` | %s | %s |\n", attr.Name, typeCell(attr.Type, importTargets), mdCell(desc)) } b.WriteString("\n") } @@ -364,18 +378,90 @@ func renderScenarios(b *strings.Builder, m *model.Model) { // mdCell escapes a value for use inside a Markdown table cell. func mdCell(s string) string { - s = strings.ReplaceAll(s, "|", "\\|") - s = strings.ReplaceAll(s, "\n", " ") - return strings.TrimSpace(s) + return strings.ReplaceAll(oneLine(s), "|", "\\|") } -// oneLine collapses a value onto a single line for use in a heading, where a -// stray newline would break the document structure. Entity names are -// PascalCase-constrained by the schema; titles and scenario names are freeform, -// so they get collapsed here. +// oneLine collapses a value onto a single line for use in a heading, a list +// item, or a table cell, where a stray line break would break the document +// structure. Entity names are PascalCase-constrained by the schema; titles, +// scenario names and import paths are freeform, so they get collapsed here. +// Every control character goes, not just the line breaks: the rest are +// invisible on the rendered page and belong in none of these positions. func oneLine(s string) string { - s = strings.ReplaceAll(s, "\r\n", " ") - s = strings.ReplaceAll(s, "\n", " ") - s = strings.ReplaceAll(s, "\r", " ") - return strings.TrimSpace(s) + return strings.TrimSpace(strings.Map(func(r rune) rune { + if unicode.IsControl(r) { + return ' ' + } + return r + }, s)) +} + +// codeSpan renders s as a Markdown code span. The scope a bare import derives +// from its filename is arbitrary author-supplied text, and a backtick inside it +// would close the span early and let the remainder render as Markdown or raw +// HTML. CommonMark's answer is a fence one backtick longer than the longest run +// inside, which keeps the value intact instead of mangling it. +func codeSpan(s string) string { + s = oneLine(s) + if s == "" { + return "` `" + } + fence := "`" + for _, run := range backtickRunRE.FindAllString(s, -1) { + if len(run) >= len(fence) { + fence = strings.Repeat("`", len(run)+1) + } + } + // CommonMark strips one leading and one trailing space from a span's + // content, so this padding keeps a value that begins or ends with a + // backtick from fusing with the fence. + if strings.HasPrefix(s, "`") || strings.HasSuffix(s, "`") { + s = " " + s + " " + } + return fence + s + fence +} + +// mdLinkText escapes a value for use as a Markdown link label. A code span +// would be the natural way to show a path, but a code span cannot escape the +// "]" that ends the label: its inertness would rest on the rule that code spans +// bind more tightly than link brackets, which the CommonMark spec states and +// widely used renderers get wrong. A backslash escape holds everywhere, so the +// label is plain text with every character that carries inline meaning escaped. +// An ordinary relative path holds none of them and passes through unchanged. +func mdLinkText(s string) string { + var b strings.Builder + for _, r := range oneLine(s) { + if strings.ContainsRune("\\`*_[]()<>&|~!#", r) { + b.WriteByte('\\') + } + b.WriteRune(r) + } + return b.String() +} + +// linkTarget percent-encodes a path for use as a Markdown link destination. +// Escaping a value for a table cell is not escaping it for a URL: a closing +// parenthesis ends the destination early, whitespace ends it, and an angle +// bracket opens raw HTML in whatever follows. Everything outside the URL +// unreserved set and the path separator is encoded, so an ordinary relative +// path passes through unchanged and a hostile one cannot break out. +func linkTarget(p string) string { + var b strings.Builder + for i := 0; i < len(p); i++ { + if c := p[i]; unreservedPathByte(c) { + b.WriteByte(c) + } else { + fmt.Fprintf(&b, "%%%02X", c) + } + } + return b.String() +} + +func unreservedPathByte(c byte) bool { + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9': + return true + default: + return strings.IndexByte("-._~/", c) >= 0 + } } diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index 0639b97..f3390c6 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -181,8 +181,8 @@ func TestRenderImports_SectionAndLinkedTypes(t *testing.T) { for _, want := range []string{ "## Imports\n", - "- **`payments`** — [`../payments/payments.modelith.yaml`](../payments/payments.modelith.md)\n", - "- **`billing`** — [`./legacy/pay-v2.modelith.yaml`](./legacy/pay-v2.modelith.md)\n", + "- **`payments`** — [../payments/payments.modelith.yaml](../payments/payments.modelith.md)\n", + "- **`billing`** — [./legacy/pay-v2.modelith.yaml](./legacy/pay-v2.modelith.md)\n", "| `paidWith` | [payments.PaymentMethod](../payments/payments.modelith.md#paymentmethod) | |\n", "| `plan` | [billing.Plan](./legacy/pay-v2.modelith.md#plan) | |\n", // A scope no import binds is a lint error; the renderer states it rather @@ -200,6 +200,94 @@ func TestRenderImports_SectionAndLinkedTypes(t *testing.T) { } } +// TestRenderImports_HostilePathCannotEscapeItsMarkup pins that an import path — +// author-supplied text that reaches a published page — is escaped for the exact +// position it lands in. A path is not validated by the schema beyond a minimum +// length, so the renderer has to be safe on its own; the linter's rejection of +// control characters is defence in depth, not the barrier. +func TestRenderImports_HostilePathCannotEscapeItsMarkup(t *testing.T) { + t.Parallel() + + // Closes the link destination, then opens an image and a second link. + const breakout = `./p.modelith.yaml) [click me](#` + + m := &model.Model{ + Imports: []model.Import{ + {Scope: "payments", Path: breakout}, + {Scope: "piped", Path: "./a|b.modelith.yaml"}, + {Scope: "spaced", Path: "./a b.modelith.yaml"}, + // A newline would inject a heading into the Imports list; the derived + // scope carries it too, since it comes from the same string. + {Scope: "line\nbreak", Path: "./a\n## Injected\n\nb.modelith.yaml", ScopeFromPath: true}, + {Scope: "ticked", Path: "./a`b``c.modelith.yaml"}, + }, + Entities: map[string]model.Entity{ + "Ticket": { + Definition: "A stub.", + Attributes: []model.Attribute{{Name: "paidWith", Type: "payments.PaymentMethod"}}, + }, + }, + } + got := Render(m) + + for _, want := range []string{ + // The label is backslash-escaped plain text, so nothing in it opens a + // tag or closes the label early. + `- **` + "`payments`" + `** — [./p.modelith.yaml\) \ \[click me\]\(\#](./p.modelith.yaml%29%20%3Cimg%20src%3Dx%20onerror%3Dalert%281%29%3E%20%5Bclick%20me%5D%28%23.md)` + "\n", + // A pipe survives in the destination as %7C rather than being escaped + // for a table cell it is not in. + "[./a\\|b.modelith.yaml](./a%7Cb.modelith.md)\n", + "[./a b.modelith.yaml](./a%20b.modelith.md)\n", + // Every control character is replaced before the value is written. + "- **`line break`** — [./a \\#\\# Injected b.modelith.yaml](./a%0A%23%23%20Injected%0A%0Ab.modelith.md)\n", + "[./a\\`b\\`\\`c.modelith.yaml](./a%60b%60%60c.modelith.md)\n", + // The deep link is assembled from escaped parts: escaping the finished + // link instead would put a backslash inside the destination. + "| `paidWith` | [payments.PaymentMethod](./p.modelith.yaml%29%20%3Cimg%20src%3Dx%20onerror%3Dalert%281%29%3E%20%5Bclick%20me%5D%28%23.md#paymentmethod) | |\n", + } { + if !strings.Contains(got, want) { + t.Errorf("expected %q in:\n%s", want, got) + } + } + + // Every occurrence of a breakout token is a backslash-escaped one. + for _, tok := range []string{" Date: Sun, 26 Jul 2026 15:25:27 -0700 Subject: [PATCH 08/22] feat(lint)!: confine import resolution to the enclosing repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An import path could walk anywhere. "../../../../etc/passwd" was read, and the four outcomes a read produces — resolves, missing, unreadable, holds no model — are distinguishable, so they form an existence and permission oracle over the whole filesystem. modelith ships an Action that lints models in CI, so a *.modelith.yaml from a fork could probe the runner and report what it found through diagnostic text landing in a pull-request check. Resolution is now bounded by the nearest ancestor of the linted model holding a .git entry, or by the model's own directory when there is no repository above it, since outside one the tool cannot know how far the project extends. Containment is decided before the file is opened, because opening it is the leak. The three previously distinct answers on the oracle fixtures collapse to one diagnostic whose only variable is the path the author wrote. Three details decide whether this works at all, each pinned by a case that fails when the behaviour is removed: - .git is tested for existence, not for being a directory. In a linked worktree and in a submodule it is a regular file holding a gitdir pointer, and requiring a directory would collapse the root to a single directory in every such checkout — including the worktree this was written in. - Symlinks resolve before the containment test, including a dangling one, which is followed via its recorded target. A link is judged by where it points, not by where the link file sits. Leaving a dangling link unresolved would keep the oracle alive, since "unreadable" is one of the four answers. - Containment is filepath.Rel, not strings.HasPrefix, which would place "/repo-evil" inside "/repo". No flag names the root, so no diagnostic advises one; a test asserts the string never appears. Recorded as ADR-0013, with the boundary and the no-repository fallback documented in the schema reference. The oracle is not closed within the repository, and that is stated rather than engineered against: anyone who can commit a file to your repository has better options than a lint diagnostic. Signed-off-by: Joe Beda --- docs/06-schema-reference.md | 21 +- internal/lint/imports.go | 22 +- internal/lint/root.go | 92 +++++++ internal/lint/root_test.go | 248 ++++++++++++++++++ ...0013-imports-confined-to-the-repository.md | 80 ++++++ 5 files changed, 460 insertions(+), 3 deletions(-) create mode 100644 internal/lint/root.go create mode 100644 internal/lint/root_test.go create mode 100644 project-docs/adr/0013-imports-confined-to-the-repository.md diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 6902ac7..44bdb95 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -305,7 +305,7 @@ exactly `{scope: x, path: ./x.modelith.yaml}`. If the filename doesn't yield a valid slug (`./Pay Ments.yaml`), the linter says so and points at the explicit form. -Five rules are worth knowing before you use this: +Six rules are worth knowing before you use this: - **A dot means a cross-model reference.** An attribute `type` containing one has to be exactly `scope.Name` — one dot, a slug before it, a PascalCase item @@ -329,6 +329,14 @@ Five rules are worth knowing before you use this: - **Nothing is fetched.** `imports` names files that are already in your repository; `lint` and `render` never touch the network ([ADR-0011](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0011-network-boundary.md)). +- **An import cannot leave the repository.** `..` is fine, but only as far as + the repository holding the model — the nearest directory above it with a + `.git` entry. Past that it is an error naming where the path resolved to and + what the root is. Symlinks are followed first, so a link pointing out is + caught too. **With no repository anywhere above the model, the root is the + model's own directory** — outside a repository the tool cannot tell how far + your project extends, so it assumes the least, and even a sibling directory + is out of reach. Rendered Markdown names each import and links it to that model's rendered `.md`, and a qualified type links straight to the item's heading there. The renderer @@ -342,8 +350,17 @@ invented, so the linter can only suggest; `payments.PaymentMethod` can be nothing but a cross-model reference, so failing to resolve it is a broken reference. +The repository boundary keeps a model from an untrusted source probing the +filesystem of whatever machine lints it — the four answers an import can produce +(resolves, missing, unreadable, not a model) are otherwise a usable oracle. It +does not stop the same probing *within* the repository, and that is accepted: +anyone who can already commit a file to your repository has better options than +a lint diagnostic. + The full rationale, including where this is heading, is -[ADR-0010](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0010-cross-model-references-by-vendoring.md). +[ADR-0010](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0010-cross-model-references-by-vendoring.md), +and the boundary itself is +[ADR-0013](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0013-imports-confined-to-the-repository.md). ## What this format deliberately leaves out diff --git a/internal/lint/imports.go b/internal/lint/imports.go index ae73067..5263317 100644 --- a/internal/lint/imports.go +++ b/internal/lint/imports.go @@ -82,7 +82,11 @@ func runImports(modelPath string, m *model.Model, fr FileReader, res *Result) { func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) (byScope map[string]importedModel, claimed map[string]string) { byScope = map[string]importedModel{} claimed = map[string]string{} + if len(m.Imports) == 0 { + return byScope, claimed + } dir := filepath.Dir(modelPath) + root, inRepo := resolutionRoot(modelPath) for i, imp := range m.Imports { reject := func(format string, args ...any) { res.Findings = append(res.Findings, Finding{ @@ -131,7 +135,23 @@ func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) ( reject("import %q is an absolute path — imports are relative to this model so they resolve in any checkout", imp.Path) continue } - data, err := fr.ReadFile(filepath.Join(dir, imp.Path)) + // Containment is decided before the file is opened, because opening it + // is the leak: whether a path resolves, is missing, is unreadable, or + // holds no model are four distinct diagnostics, and together they let a + // model from an untrusted source probe the filesystem of whatever runner + // lints it (ADR-0013). + joined := filepath.Join(dir, imp.Path) + if resolved := realPath(absolute(joined)); !withinRoot(root, resolved) { + if inRepo { + reject("import %q resolves to %q, outside %q — that directory is the repository holding this model (the nearest ancestor with a .git entry), and an import may not name a file beyond it", + imp.Path, resolved, root) + } else { + reject("import %q resolves to %q, outside %q — this model is in no repository, so resolution is confined to the directory holding it; move the imported model into that directory or below it", + imp.Path, resolved, root) + } + continue + } + data, err := fr.ReadFile(joined) if err != nil { reject("import %q cannot be read: %v", imp.Path, err) continue diff --git a/internal/lint/root.go b/internal/lint/root.go new file mode 100644 index 0000000..9cd0799 --- /dev/null +++ b/internal/lint/root.go @@ -0,0 +1,92 @@ +package lint + +import ( + "os" + "path/filepath" + "strings" +) + +// maxSymlinkHops bounds the link-following in realPath. filepath.EvalSymlinks +// does its own bounding; this covers the links it refuses to follow because +// their target does not exist, where a loop would otherwise spin forever. +const maxSymlinkHops = 40 + +// resolutionRoot returns the directory an import may not resolve outside of, +// and whether an enclosing repository defined it. The root is the nearest +// ancestor of the model holding a `.git` entry; failing that, the model's own +// directory, because outside a repository the tool cannot know the project's +// extent and assumes the smallest safe thing (ADR-0013). +// +// The `.git` entry is tested for existence, not for being a directory: in a +// linked worktree and in a submodule it is a regular file holding a gitdir +// pointer, and reading that as "no repository" would confine resolution to a +// single directory in every such checkout. +func resolutionRoot(modelPath string) (root string, inRepo bool) { + dir := absolute(filepath.Dir(modelPath)) + for cur := dir; ; { + if _, err := os.Lstat(filepath.Join(cur, ".git")); err == nil { + return realPath(cur), true + } + parent := filepath.Dir(cur) + if parent == cur { + return realPath(dir), false + } + cur = parent + } +} + +// withinRoot reports whether candidate sits at or below root. Both must already +// be absolute, cleaned and symlink-resolved. filepath.Rel decides it rather +// than a string prefix, which would place "/repo-evil" inside "/repo". +func withinRoot(root, candidate string) bool { + rel, err := filepath.Rel(root, candidate) + if err != nil { + return false + } + return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))) +} + +// realPath resolves p as far as the filesystem allows. A component that does +// not exist is kept as written, because a missing import is reported as +// unreadable rather than as an escape. A dangling symlink is followed via its +// recorded target, so a link is judged by where it points and not by where the +// link file sits — otherwise a link committed inside a repository aims anywhere +// and the boundary is decorative. +func realPath(p string) string { + p = filepath.Clean(p) + var tail []string + for hops := 0; hops <= maxSymlinkHops; { + if resolved, err := filepath.EvalSymlinks(p); err == nil { + return filepath.Join(resolved, filepath.Join(tail...)) + } + // EvalSymlinks gives up on a link whose target is missing. Follow it by + // hand so the target, not the link's own location, is what gets judged. + if target, err := os.Readlink(p); err == nil { + hops++ + if filepath.IsAbs(target) { + p = filepath.Clean(target) + } else { + p = filepath.Clean(filepath.Join(filepath.Dir(p), target)) + } + continue + } + parent := filepath.Dir(p) + if parent == p { + break + } + tail = append([]string{filepath.Base(p)}, tail...) + p = parent + } + return filepath.Join(p, filepath.Join(tail...)) +} + +// absolute makes p absolute against the working directory, falling back to the +// cleaned path in the one case filepath.Abs fails — the working directory being +// unavailable, where there is nothing better to say. +func absolute(p string) string { + abs, err := filepath.Abs(p) + if err != nil { + return filepath.Clean(p) + } + return abs +} diff --git a/internal/lint/root_test.go b/internal/lint/root_test.go new file mode 100644 index 0000000..4d39e3e --- /dev/null +++ b/internal/lint/root_test.go @@ -0,0 +1,248 @@ +package lint + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +// m5Tree builds the layout the confinement cases share and returns its base, +// with symlinks already resolved so a test can predict the paths a diagnostic +// will name (a temp directory is reached through a symlink on macOS). +// +// /repo/ .git as a directory +// /repo/docs/ link -> /outside/p…, dangle -> a missing file +// /repo-evil/ a sibling sharing "repo" as a string prefix +// /outside/ +// /wt/ .git as a regular file, the linked-worktree shape +// /loose/{a,ab,b}/ no repository anywhere above +func m5Tree(t *testing.T) string { + t.Helper() + + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + // The no-repository cases only mean anything if nothing above the temp + // directory is a repository. + for cur := base; ; { + parent := filepath.Dir(cur) + if parent == cur { + break + } + if _, err := os.Lstat(filepath.Join(cur, ".git")); err == nil { + t.Skipf("temp directory %s sits inside a repository (%s), so the no-repository cases cannot be built", base, cur) + } + cur = parent + } + + for _, dir := range []string{"repo/docs", "repo-evil", "outside", "wt/docs", "loose/a", "loose/ab", "loose/b"} { + if err := os.MkdirAll(filepath.Join(base, dir), 0o750); err != nil { + t.Fatal(err) + } + } + // A repository marked by a directory, and one marked by the regular file a + // linked worktree and a submodule leave behind. + if err := os.Mkdir(filepath.Join(base, "repo", ".git"), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(base, "wt", ".git"), []byte("gitdir: /elsewhere/.git/worktrees/wt\n"), 0o600); err != nil { + t.Fatal(err) + } + for _, dir := range []string{"repo", "repo/docs", "repo-evil", "outside", "wt", "loose/a", "loose/ab", "loose/b"} { + if err := os.WriteFile(filepath.Join(base, dir, "p.modelith.yaml"), []byte(paymentsModel), 0o600); err != nil { + t.Fatal(err) + } + } + if err := os.Symlink(filepath.Join(base, "outside", "p.modelith.yaml"), filepath.Join(base, "repo/docs", "link.modelith.yaml")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(base, "outside", "gone.modelith.yaml"), filepath.Join(base, "repo/docs", "dangle.modelith.yaml")); err != nil { + t.Fatal(err) + } + return base +} + +// TestADR_0013_ImportsConfinedToTheRepository pins the resolution boundary: an +// import may not name a file outside the repository enclosing the model, and +// outside a repository it may not leave the model's own directory. Reading a +// file is what leaks, so containment is decided before the read — the four +// outcomes a read produces are the oracle ADR-0013 closes. +func TestADR_0013_ImportsConfinedToTheRepository(t *testing.T) { + t.Parallel() + + base := m5Tree(t) + + cases := []struct { + name string + dir string // where the model under test is written, relative to base + entry string // the imports entry, written verbatim + // resolved and root are relative to base. Empty resolved means the + // import is expected to be allowed. + resolved string + root string + inRepo bool + }{ + { + name: "peer inside the repository", + dir: "repo/docs", + entry: "{scope: p, path: ./p.modelith.yaml}", + }, + { + name: "parent-relative inside the repository", + dir: "repo/docs", + entry: "{scope: p, path: ../p.modelith.yaml}", + }, + { + name: "model sitting at the repository root", + dir: "repo", + entry: "{scope: p, path: ./p.modelith.yaml}", + }, + { + name: "linked worktree, where .git is a file", + dir: "wt/docs", + entry: "{scope: p, path: ../p.modelith.yaml}", + }, + { + name: "escaping the repository root", + dir: "repo/docs", + entry: "{scope: p, path: ../../outside/p.modelith.yaml}", + resolved: "outside/p.modelith.yaml", + root: "repo", + inRepo: true, + }, + { + // Judged by where the link points, not by where the link file sits. + name: "symlink inside the repository pointing outside", + dir: "repo/docs", + entry: "{scope: p, path: ./link.modelith.yaml}", + resolved: "outside/p.modelith.yaml", + root: "repo", + inRepo: true, + }, + { + // A dangling link would otherwise pass containment and be reported + // as unreadable, which is the oracle answer the boundary removes. + name: "dangling symlink pointing outside", + dir: "repo/docs", + entry: "{scope: p, path: ./dangle.modelith.yaml}", + resolved: "outside/gone.modelith.yaml", + root: "repo", + inRepo: true, + }, + { + // A string prefix would read "…/repo-evil" as inside "…/repo". + name: "sibling whose name shares a prefix with the root", + dir: "repo/docs", + entry: "{scope: p, path: ../../repo-evil/p.modelith.yaml}", + resolved: "repo-evil/p.modelith.yaml", + root: "repo", + inRepo: true, + }, + { + name: "no repository, same directory", + dir: "loose/a", + entry: "{scope: p, path: ./p.modelith.yaml}", + }, + { + name: "no repository, sibling directory", + dir: "loose/a", + entry: "{scope: p, path: ../b/p.modelith.yaml}", + resolved: "loose/b/p.modelith.yaml", + root: "loose/a", + }, + { + name: "no repository, sibling sharing a prefix", + dir: "loose/a", + entry: "{scope: p, path: ../ab/p.modelith.yaml}", + resolved: "loose/ab/p.modelith.yaml", + root: "loose/a", + }, + } + + for i, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + modelPath := filepath.Join(base, tc.dir, fmt.Sprintf("g%d.modelith.yaml", i)) + src := importer([]string{tc.entry}, "p.PaymentMethod") + if err := os.WriteFile(modelPath, []byte(src), 0o600); err != nil { + t.Fatal(err) + } + res, err := Run(modelPath, []byte(src), nil) + if err != nil { + t.Fatal(err) + } + + if tc.resolved == "" { + assertFindings(t, importFindings(res.Findings), nil) + return + } + assertFindings(t, importFindings(res.Findings), []wantFinding{{ + SeverityError, CategorySemantic, "/imports/0", + fmt.Sprintf("resolves to %q, outside %q", + filepath.Join(base, tc.resolved), filepath.Join(base, tc.root)), + }}) + + // The message says where the root came from; the tighter no-repository + // root is the surprising one and has to explain itself. + origin := "this model is in no repository, so resolution is confined to the directory holding it" + if tc.inRepo { + origin = "the nearest ancestor with a .git entry" + } + if got := res.Findings[0].Message; !strings.Contains(got, origin) { + t.Errorf("message %q does not say where the root came from (%q)", got, origin) + } + // No flag names the root, so nothing may advise passing one. + if strings.Contains(res.Findings[0].Message, "--root") { + t.Errorf("message advises a flag that does not exist: %q", res.Findings[0].Message) + } + }) + } +} + +// TestWithinRoot checks the containment test on its own, including the pair a +// string prefix gets wrong. +func TestWithinRoot(t *testing.T) { + t.Parallel() + + cases := []struct { + root, candidate string + want bool + }{ + {"/repo", "/repo/docs/p.modelith.yaml", true}, + {"/repo", "/repo", true}, + {"/repo", "/repo-evil/p.modelith.yaml", false}, + {"/repo", "/repository/p.modelith.yaml", false}, + {"/repo", "/etc/passwd", false}, + {"/repo", "/", false}, + {"/repo/docs", "/repo/p.modelith.yaml", false}, + } + for _, tc := range cases { + root, candidate := filepath.FromSlash(tc.root), filepath.FromSlash(tc.candidate) + if got := withinRoot(root, candidate); got != tc.want { + t.Errorf("withinRoot(%q, %q) = %v, want %v", root, candidate, got, tc.want) + } + } +} + +// TestRealPath_SymlinkLoopTerminates checks the bound on the link-following +// filepath.EvalSymlinks refuses to do: a cycle has to return, not spin. +func TestRealPath_SymlinkLoopTerminates(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + a, b := filepath.Join(dir, "a"), filepath.Join(dir, "b") + if err := os.Symlink(b, a); err != nil { + t.Fatal(err) + } + if err := os.Symlink(a, b); err != nil { + t.Fatal(err) + } + // The value is unimportant — that the call returns at all is the point. + if got := realPath(a); got == "" { + t.Error("realPath returned an empty path for a symlink cycle") + } +} diff --git a/project-docs/adr/0013-imports-confined-to-the-repository.md b/project-docs/adr/0013-imports-confined-to-the-repository.md new file mode 100644 index 0000000..baf38f2 --- /dev/null +++ b/project-docs/adr/0013-imports-confined-to-the-repository.md @@ -0,0 +1,80 @@ +# Import resolution is confined to the enclosing repository + +An `imports:` entry may only name a file inside the repository holding the model +being linted — the nearest ancestor with a `.git` entry. Outside a repository +the root is the model's own directory. An import resolving beyond the root is an +error, decided before the file is opened. + +## Context + +Paths in `imports:` are relative, and `..` has to work: peer models commonly sit +in sibling directories, and the worked example relies on it (ADR-0010). Nothing +bounded how far `..` could walk, so `../../../../etc/passwd` was read. + +Reading is the leak. An import produces four distinguishable diagnostics — it +resolves, it is missing, it is unreadable, it holds no model — and that is an +existence-and-permission oracle over the whole filesystem. modelith ships a +GitHub Action that lints models in CI, so a `*.modelith.yaml` from a fork can +probe the runner and report what it finds back through diagnostic text, which +lands in a pull-request check anyone can read. + +## Decision + +**The resolution root is the nearest ancestor of the linted model containing a +`.git` entry.** An import that resolves outside it is a semantic error naming +the offending path, where it resolved to, and the root. + +**With no `.git` anywhere above, the root is the directory holding the model.** +Outside a repository the tool cannot know the project's extent, so it assumes the +smallest safe thing. This is the surprising case, so its diagnostic says why the +root is so tight. + +**The root is computed per model, from that model's own location.** Linting two +models in different repositories in one invocation gives each its own root. Only +the direct imports of the model being linted are checked, because resolution does +not recurse (ADR-0010); a glob that lints an imported file directly gives that +file its own root. + +Three details decide whether this works at all, and each is pinned by a case in +`TestADR_0013_ImportsConfinedToTheRepository`: + +- **`.git` is tested for existence, not for being a directory.** In a linked + worktree and in a submodule it is a regular file holding a `gitdir:` pointer. + Requiring a directory would collapse the root to a single directory in every + such checkout. +- **Symlinks are resolved before the containment test**, including a dangling + one, which is followed via its recorded target. A link is judged by where it + points, not by where the link file sits; otherwise a link committed inside the + repository aims anywhere and the boundary is decorative. A dangling link left + unresolved would also keep the oracle alive, since "unreadable" is one of the + four answers. +- **Containment is `filepath.Rel`, not `strings.HasPrefix`.** A string prefix + places `/repo-evil` inside `/repo`. + +## Considered options + +**A flag naming the root.** Rejected for now, not forever: no real case needs one +yet, and the fallback covers the loose-file case. Because it does not exist, no +diagnostic may advise passing one — an error that recommends a flag the binary +does not have is worse than the confinement it explains. + +**Refusing `..` outright.** Simple and airtight, but it breaks the peer-directory +layout the format is designed around and the worked example already uses. +Bounding the walk keeps the layout and removes the oracle. + +## Consequences + +- A repository that vendors models above its own root can no longer import them. + That is the intended loss; vendoring inside the repository is what ADR-0010 + describes. +- The oracle is closed across the repository boundary, not within it. An attacker + who can already place a file in your repository has better options than a lint + diagnostic, so probing inside the root is accepted rather than engineered + against. The docs say so. +- The root walk touches the real filesystem, so it does not go through lint's + `FileReader` seam. Tests that need a real tree use `t.TempDir()`; the seam + still covers the reads. +- The error text names absolute paths — the resolved candidate and the root. + This is the same disclosure the pre-existing "cannot be read" diagnostic + already makes, and without it the message cannot explain a `..` chain or a + symlink. From 8f629207dcd77627cf2fe7e0f0ba6f41620677b9 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 16:04:08 -0700 Subject: [PATCH 09/22] fix(lint): take the resolution root from the model's real directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolutionRoot climbed the ancestor chain of the path as written while every candidate import was judged after symlink resolution, so a model reached through a symlinked directory was assigned a root from a different tree: a peer in the same directory was refused, and the diagnostic named a repository that does not hold the model. Seed the walk with the resolved directory instead. The two cases added to TestADR_0013_ImportsConfinedToTheRepository — a peer import through a link into another repository, and an escape from that same location — both fail without the change. Signed-off-by: Joe Beda --- internal/lint/root.go | 12 +++++++++--- internal/lint/root_test.go | 36 ++++++++++++++++++++++++++++++++---- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/internal/lint/root.go b/internal/lint/root.go index 9cd0799..96101f0 100644 --- a/internal/lint/root.go +++ b/internal/lint/root.go @@ -21,15 +21,21 @@ const maxSymlinkHops = 40 // linked worktree and in a submodule it is a regular file holding a gitdir // pointer, and reading that as "no repository" would confine resolution to a // single directory in every such checkout. +// +// The walk climbs the resolved directory, not the one as written. A model +// reached through a symlinked directory belongs to the tree it really sits in, +// and every candidate import is judged after the same resolution, so seeding +// the walk with the unresolved path would compare the two against different +// trees. func resolutionRoot(modelPath string) (root string, inRepo bool) { - dir := absolute(filepath.Dir(modelPath)) + dir := realPath(absolute(filepath.Dir(modelPath))) for cur := dir; ; { if _, err := os.Lstat(filepath.Join(cur, ".git")); err == nil { - return realPath(cur), true + return cur, true } parent := filepath.Dir(cur) if parent == cur { - return realPath(dir), false + return dir, false } cur = parent } diff --git a/internal/lint/root_test.go b/internal/lint/root_test.go index 4d39e3e..70f4f88 100644 --- a/internal/lint/root_test.go +++ b/internal/lint/root_test.go @@ -18,6 +18,9 @@ import ( // /outside/ // /wt/ .git as a regular file, the linked-worktree shape // /loose/{a,ab,b}/ no repository anywhere above +// /outer/ .git as a directory +// /outer/work link -> /inner/models, a directory in another repository +// /inner/ .git as a directory func m5Tree(t *testing.T) string { t.Helper() @@ -38,24 +41,31 @@ func m5Tree(t *testing.T) string { cur = parent } - for _, dir := range []string{"repo/docs", "repo-evil", "outside", "wt/docs", "loose/a", "loose/ab", "loose/b"} { + for _, dir := range []string{"repo/docs", "repo-evil", "outside", "wt/docs", "loose/a", "loose/ab", "loose/b", "outer", "inner/models"} { if err := os.MkdirAll(filepath.Join(base, dir), 0o750); err != nil { t.Fatal(err) } } // A repository marked by a directory, and one marked by the regular file a // linked worktree and a submodule leave behind. - if err := os.Mkdir(filepath.Join(base, "repo", ".git"), 0o750); err != nil { - t.Fatal(err) + for _, dir := range []string{"repo", "outer", "inner"} { + if err := os.Mkdir(filepath.Join(base, dir, ".git"), 0o750); err != nil { + t.Fatal(err) + } } if err := os.WriteFile(filepath.Join(base, "wt", ".git"), []byte("gitdir: /elsewhere/.git/worktrees/wt\n"), 0o600); err != nil { t.Fatal(err) } - for _, dir := range []string{"repo", "repo/docs", "repo-evil", "outside", "wt", "loose/a", "loose/ab", "loose/b"} { + for _, dir := range []string{"repo", "repo/docs", "repo-evil", "outside", "wt", "loose/a", "loose/ab", "loose/b", "inner/models"} { if err := os.WriteFile(filepath.Join(base, dir, "p.modelith.yaml"), []byte(paymentsModel), 0o600); err != nil { t.Fatal(err) } } + // A directory in one repository reached through a link in another, so a + // root taken from the path as written comes from the wrong tree. + if err := os.Symlink(filepath.Join(base, "inner", "models"), filepath.Join(base, "outer", "work")); err != nil { + t.Fatal(err) + } if err := os.Symlink(filepath.Join(base, "outside", "p.modelith.yaml"), filepath.Join(base, "repo/docs", "link.modelith.yaml")); err != nil { t.Fatal(err) } @@ -141,6 +151,24 @@ func TestADR_0013_ImportsConfinedToTheRepository(t *testing.T) { root: "repo", inRepo: true, }, + { + // The root comes from the tree the model really sits in. Climbing + // the path as written finds /outer, which holds neither the + // model nor its peer, and refuses a file in the same directory. + name: "model reached through a symlinked directory", + dir: "outer/work", + entry: "{scope: p, path: ./p.modelith.yaml}", + }, + { + // The boundary still holds from there, and names the real tree's + // root rather than the one the link was written under. + name: "escaping from a symlinked directory", + dir: "outer/work", + entry: "{scope: p, path: ../../outside/p.modelith.yaml}", + resolved: "outside/p.modelith.yaml", + root: "inner", + inRepo: true, + }, { name: "no repository, same directory", dir: "loose/a", From 8c5e729c59b68b912c51d2481e60907dab466048 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 16:06:22 -0700 Subject: [PATCH 10/22] refactor(lint): put import containment behind the file seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The FileReader seam covered reads only: resolutionRoot, realPath and absolute called os.Lstat, filepath.EvalSymlinks and os.Getwd directly, so a caller that supplied its own reader still had containment judged against the local disk. The test suite inherited that. TestImports_Resolution/parent-relative_import_resolves passed only because internal/lint happens to sit under the repository's real .git; the same tests fail in a copy of the tree without one — a source tarball, a vendored module, a checkout CI exported rather than cloned. Widen the seam to the three questions import resolution asks: read the file, where is resolution confined, and where does this path land. FileReader becomes Files; OSFiles answers all three from the local filesystem, and the test fake answers them from its own map, with a ".git" key marking a repository just as the entry does on disk. TestImports_ContainmentUsesTheFileSeam pins the behaviour that used to depend on the ambient tree: with a marker a sibling directory is reachable, without one resolution stops at the model's own directory. `go test ./...` now passes in a copy of the tree with .git removed. Signed-off-by: Joe Beda --- internal/lint/imports.go | 34 +++++++++---- internal/lint/imports_test.go | 92 ++++++++++++++++++++++++++++++++--- internal/lint/lint.go | 13 ++--- internal/lint/root.go | 8 ++- 4 files changed, 121 insertions(+), 26 deletions(-) diff --git a/internal/lint/imports.go b/internal/lint/imports.go index 5263317..8b374cb 100644 --- a/internal/lint/imports.go +++ b/internal/lint/imports.go @@ -12,16 +12,30 @@ import ( "github.com/stacklok/modelith/internal/schema" ) -// FileReader reads an imported model file by path. It is the seam that keeps -// lint's file access substitutable in tests. +// Files is the filesystem seam import resolution runs on: reading an imported +// model, and the two path questions that decide whether it may be read at all. +// Reading and containment sit behind one interface deliberately — judging the +// boundary against the local disk while reading from somewhere else would +// confine imports on a filesystem the reads never touch. // // It is not an fs.FS: fs.ValidPath rejects "..", and peer models commonly sit // in sibling directories, so "../payments/payments.modelith.yaml" has to work. -type FileReader interface { +type Files interface { + // ReadFile reads the named file. ReadFile(path string) ([]byte, error) + // ResolutionRoot returns the directory an import of the model at modelPath + // may not resolve outside of, and whether an enclosing repository defined + // it (ADR-0013). + ResolutionRoot(modelPath string) (root string, inRepo bool) + // Resolve returns path as this filesystem sees it — absolute, cleaned, and + // symlink-resolved as far as it can be — so containment judges where a path + // lands rather than how it was written. It must agree with ResolutionRoot: + // the two results are compared against each other. + Resolve(path string) string } -// OSFiles reads imported models from the local filesystem. +// OSFiles resolves imported models against the local filesystem. Its path +// methods are in root.go, alongside the containment rules they implement. type OSFiles struct{} // ReadFile reads the named file from the local filesystem. @@ -50,8 +64,8 @@ type importedModel struct { // // modelPath is the path of the model being linted; imports resolve relative to // its directory. -func runImports(modelPath string, m *model.Model, fr FileReader, res *Result) { - byScope, claimed := loadImports(modelPath, m, fr, res) +func runImports(modelPath string, m *model.Model, files Files, res *Result) { + byScope, claimed := loadImports(modelPath, m, files, res) used := checkQualifiedTypes(m, byScope, claimed, res) // An unreferenced import is a completeness finding, alongside the unused // enum and the unused glossary term: vocabulary the model declares and @@ -79,14 +93,14 @@ func runImports(modelPath string, m *model.Model, fr FileReader, res *Result) { // list claimed at all — including entries that then failed to load — mapped to // the path of the first entry that claimed it. Every rejection is reported as an // error: an import that cannot be resolved is a broken reference, not a gap. -func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) (byScope map[string]importedModel, claimed map[string]string) { +func loadImports(modelPath string, m *model.Model, files Files, res *Result) (byScope map[string]importedModel, claimed map[string]string) { byScope = map[string]importedModel{} claimed = map[string]string{} if len(m.Imports) == 0 { return byScope, claimed } dir := filepath.Dir(modelPath) - root, inRepo := resolutionRoot(modelPath) + root, inRepo := files.ResolutionRoot(modelPath) for i, imp := range m.Imports { reject := func(format string, args ...any) { res.Findings = append(res.Findings, Finding{ @@ -141,7 +155,7 @@ func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) ( // model from an untrusted source probe the filesystem of whatever runner // lints it (ADR-0013). joined := filepath.Join(dir, imp.Path) - if resolved := realPath(absolute(joined)); !withinRoot(root, resolved) { + if resolved := files.Resolve(joined); !withinRoot(root, resolved) { if inRepo { reject("import %q resolves to %q, outside %q — that directory is the repository holding this model (the nearest ancestor with a .git entry), and an import may not name a file beyond it", imp.Path, resolved, root) @@ -151,7 +165,7 @@ func loadImports(modelPath string, m *model.Model, fr FileReader, res *Result) ( } continue } - data, err := fr.ReadFile(joined) + data, err := files.ReadFile(joined) if err != nil { reject("import %q cannot be read: %v", imp.Path, err) continue diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index a9d174a..f505138 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -9,9 +9,15 @@ import ( "testing" ) -// fakeFiles is a map-backed FileReader keyed by cleaned, slash-separated path. -// A hand-written fake rather than a mock: it behaves like the filesystem, -// including reporting a missing file the way os.ReadFile does. +// fakeFiles is a map-backed Files keyed by cleaned, slash-separated path. A +// hand-written fake rather than a mock: it behaves like a filesystem, reporting +// a missing file the way os.ReadFile does and answering containment from its own +// tree. Containment is part of the fake because deciding it on the real disk +// while reading from a map would make these cases depend on where the source +// tree happens to sit, and on whether it is a git checkout at all. +// +// A ".git" key marks a repository, exactly as the entry does on disk. Paths in +// the map are relative, so the whole tree hangs off ".". type fakeFiles map[string]string func (f fakeFiles) ReadFile(path string) ([]byte, error) { @@ -22,6 +28,27 @@ func (f fakeFiles) ReadFile(path string) ([]byte, error) { return []byte(src), nil } +// Resolve cleans the path and stops there: the fake's keys are the whole world +// and hold no symlinks, so there is nothing further to follow. +func (f fakeFiles) Resolve(path string) string { return filepath.Clean(path) } + +// ResolutionRoot mirrors OSFiles.ResolutionRoot over the map: the nearest +// ancestor holding a ".git" key, or the model's own directory when there is +// none. +func (f fakeFiles) ResolutionRoot(modelPath string) (string, bool) { + dir := filepath.Dir(filepath.Clean(modelPath)) + for cur := dir; ; { + if _, ok := f[filepath.ToSlash(filepath.Join(cur, ".git"))]; ok { + return cur, true + } + parent := filepath.Dir(cur) + if parent == cur { + return dir, false + } + cur = parent + } +} + // importerPath is where the model under test lives; its imports resolve // relative to the "docs" directory. const importerPath = "docs/garage.modelith.yaml" @@ -107,6 +134,10 @@ func TestImports_Resolution(t *testing.T) { t.Parallel() files := fakeFiles{ + // The repository marker at the top of the fake tree is what lets an + // import reach a sibling directory; TestImports_ContainmentUsesTheFileSeam + // covers the tree without one. + ".git": "", "docs/payments.modelith.yaml": paymentsModel, "payments/payments.modelith.yaml": paymentsModel, "docs/legacy/pay-v2.modelith.yaml": paymentsModel, @@ -248,6 +279,51 @@ func TestImports_Resolution(t *testing.T) { } } +// TestImports_ContainmentUsesTheFileSeam pins that the boundary is judged +// against the filesystem the reads go to, not against whatever tree the tests +// happen to run in. The same model and the same import resolve or are refused +// purely by whether the fake tree carries a repository marker, so neither case +// depends on the source tree being a git checkout — copy it without .git (a +// source tarball, a vendored module, a shallow CI export) and both still hold. +func TestImports_ContainmentUsesTheFileSeam(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + files fakeFiles + want []wantFinding + }{ + { + name: "a repository above the model admits a sibling directory", + files: fakeFiles{ + ".git": "", + "payments/payments.modelith.yaml": paymentsModel, + }, + }, + { + name: "no repository confines resolution to the model's directory", + files: fakeFiles{"payments/payments.modelith.yaml": paymentsModel}, + want: []wantFinding{{ + SeverityError, CategorySemantic, "/imports/0", + `import "../payments/payments.modelith.yaml" resolves to ` + + `"payments/payments.modelith.yaml", outside "docs" — this model is in no repository`, + }}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + src := importer([]string{`"../payments/payments.modelith.yaml"`}, "payments.PaymentMethod") + res, err := Run(importerPath, []byte(src), tc.files) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), tc.want) + }) + } +} + // TestImports_MalformedQualifiedType covers a type that contains a dot but is // not a well-formed "scope.Name". Each shape names the way it is malformed // rather than passing silently as a primitive. @@ -493,7 +569,7 @@ enums: ` read := map[string]int{} files := countingFiles{ - files: fakeFiles{ + fakeFiles: fakeFiles{ "docs/payments.modelith.yaml": middle, "docs/shipping.modelith.yaml": leaf, }, @@ -515,15 +591,15 @@ enums: } // countingFiles records how many times each path was read, so a test can assert -// a file was never opened. +// a file was never opened. Everything else about the filesystem it inherits. type countingFiles struct { - files fakeFiles - read map[string]int + fakeFiles + read map[string]int } func (c countingFiles) ReadFile(path string) ([]byte, error) { c.read[filepath.ToSlash(filepath.Clean(path))]++ - return c.files.ReadFile(path) + return c.fakeFiles.ReadFile(path) } // TestRun_QualifiedEntityReferenceIsDeferred checks the friendly error for a diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 5c1703b..366d4f7 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -78,12 +78,13 @@ var ( ) // Run validates the model at path and returns all findings. src is the model's -// own bytes; fr reads the files its `imports:` name, resolved relative to -// path's directory. A nil fr reads them from the local filesystem. -func Run(path string, src []byte, fr FileReader) (*Result, error) { +// own bytes; files reads the ones its `imports:` name, resolved relative to +// path's directory, and answers where that resolution is confined. A nil files +// uses the local filesystem. +func Run(path string, src []byte, files Files) (*Result, error) { res := &Result{} - if fr == nil { - fr = OSFiles{} + if files == nil { + files = OSFiles{} } // Layer 1: structural validation against the JSON Schema. @@ -114,7 +115,7 @@ func Run(path string, src []byte, fr FileReader) (*Result, error) { // follows would tell the author to write syntax that cannot work — the same // reason the version check gates schema validation. if structuralOK { - runImports(path, m, fr, res) + runImports(path, m, files, res) } runRelationshipShape(m, res) runSubtypes(m, res) diff --git a/internal/lint/root.go b/internal/lint/root.go index 96101f0..5222001 100644 --- a/internal/lint/root.go +++ b/internal/lint/root.go @@ -11,7 +11,7 @@ import ( // their target does not exist, where a loop would otherwise spin forever. const maxSymlinkHops = 40 -// resolutionRoot returns the directory an import may not resolve outside of, +// ResolutionRoot returns the directory an import may not resolve outside of, // and whether an enclosing repository defined it. The root is the nearest // ancestor of the model holding a `.git` entry; failing that, the model's own // directory, because outside a repository the tool cannot know the project's @@ -27,7 +27,7 @@ const maxSymlinkHops = 40 // and every candidate import is judged after the same resolution, so seeding // the walk with the unresolved path would compare the two against different // trees. -func resolutionRoot(modelPath string) (root string, inRepo bool) { +func (OSFiles) ResolutionRoot(modelPath string) (root string, inRepo bool) { dir := realPath(absolute(filepath.Dir(modelPath))) for cur := dir; ; { if _, err := os.Lstat(filepath.Join(cur, ".git")); err == nil { @@ -41,6 +41,10 @@ func resolutionRoot(modelPath string) (root string, inRepo bool) { } } +// Resolve returns path as the local filesystem sees it: absolute against the +// working directory, then symlink-resolved as far as the filesystem allows. +func (OSFiles) Resolve(path string) string { return realPath(absolute(path)) } + // withinRoot reports whether candidate sits at or below root. Both must already // be absolute, cleaned and symlink-resolved. filepath.Rel decides it rather // than a string prefix, which would place "/repo-evil" inside "/repo". From ffcd1fdc233c180c40761ba960202395b9d4eecd Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 16:07:18 -0700 Subject: [PATCH 11/22] fix(lint): report the imports layer alongside a qualified entity reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reportQualifiedEntityRefs writes its findings before schema validation, and runStructural counted them when deciding whether the document was structurally valid. A model carrying both `subtypeOf: payments.Payment` and a broken import therefore reported only the former: the nonexistent import, and every unreferenced-import finding with it, stayed hidden until the unrelated mistake was fixed, at which point a second round of errors appeared unannounced. Judge structural validity by what the schema said, and let the cross-model entity reference — a supported-feature limit, not a shape the imports list depends on — stand as its own finding. Both layers now surface in one run. Signed-off-by: Joe Beda --- internal/lint/imports_test.go | 35 ++++++++++++++++++++++++++++++++--- internal/lint/lint.go | 19 +++++++++++++------ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index f505138..b1b5b7c 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -666,9 +666,38 @@ entities: } } -// TestRun_NilFileReaderReadsFromDisk covers the documented default: a nil -// FileReader resolves imports against the local filesystem. -func TestRun_NilFileReaderReadsFromDisk(t *testing.T) { +// TestRun_QualifiedEntityReferenceDoesNotGateTheImportsLayer pins that two +// unrelated mistakes are reported in one run. The cross-model entity reference +// used to count as a structural failure, which skipped the imports layer +// entirely: the broken import below stayed invisible until the subtypeOf was +// fixed, and fixing it produced a second, unannounced round of errors. +func TestRun_QualifiedEntityReferenceDoesNotGateTheImportsLayer(t *testing.T) { + t.Parallel() + + src := `kind: DomainModel +version: v1 +imports: + - "./gone.modelith.yaml" +entities: + Visit: + definition: One car's stay in the garage. + subtypeOf: payments.Payment +` + res, err := Run(importerPath, []byte(src), fakeFiles{}) + if err != nil { + t.Fatal(err) + } + assertFindings(t, importFindings(res.Findings), []wantFinding{ + {SeverityError, CategoryStructural, "/entities/Visit/subtypeOf", + "is a cross-model reference, which is not supported in an entity position"}, + {SeverityError, CategorySemantic, "/imports/0", + `import "./gone.modelith.yaml" cannot be read`}, + }) +} + +// TestRun_NilFilesReadsFromDisk covers the documented default: a nil Files +// resolves imports against the local filesystem. +func TestRun_NilFilesReadsFromDisk(t *testing.T) { t.Parallel() dir := t.TempDir() diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 366d4f7..7964900 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -113,7 +113,9 @@ func Run(path string, src []byte, files Files) (*Result, error) { // Imports resolve only against a document the schema accepted. A scope the // schema already rejected would otherwise bind anyway, and the advice that // follows would tell the author to write syntax that cannot work — the same - // reason the version check gates schema validation. + // reason the version check gates schema validation. A cross-model reference + // in an entity position is not one of those rejections (see runStructural), + // so it does not take the imports layer down with it. if structuralOK { runImports(path, m, files, res) } @@ -139,8 +141,10 @@ func Structural(data []byte) []Finding { return res.Findings } -// runStructural validates against the JSON Schema. Returns true if the document -// is structurally valid. +// runStructural validates against the JSON Schema. Returns true if the schema +// accepted the document — which the cross-model entity references reported here +// do not affect, since they are a supported-feature limit rather than a shape +// the imports list depends on. func runStructural(data []byte, res *Result) bool { jsonBytes, err := yaml.YAMLToJSON(data) if err != nil { @@ -195,12 +199,15 @@ func runStructural(data []byte, res *Result) bool { return false } - before := len(res.Findings) // Say what a cross-model entity reference actually is before the schema // reports it as a pattern violation, and suppress its opaque message so one - // mistake reads as one finding. + // mistake reads as one finding. It is reported on its own terms and is not + // counted against the document's structural validity: a broken import in the + // same file is an unrelated mistake, and holding the imports layer back + // until this one is fixed would hide it. qualified := reportQualifiedEntityRefs(inst, res) + before := len(res.Findings) if err := sch.Validate(inst); err != nil { if ve, ok := err.(*jsonschema.ValidationError); ok { collectLeaves(ve, res, qualified) @@ -213,7 +220,7 @@ func runStructural(data []byte, res *Result) bool { }) return false } - return len(res.Findings) == before + return true } func collectLeaves(e *jsonschema.ValidationError, res *Result, skip map[string]bool) { From 7c42c459182a995d71fa3b7e8d1a5ed3682ad4b9 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 16:08:08 -0700 Subject: [PATCH 12/22] fix(model): derive an import scope from a .modelith.yml file too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ScopeFromPath stripped ".modelith.yaml" whole but fell back to trimming only the final extension, so "./payments.modelith.yml" bound the scope "payments.modelith" — not a valid slug. RenderedPath and the render command's default output path both treat .yml as a model file, so the bare import form was the one place the spelling was not first-class, and it failed twice: an invalid-slug error on the import, then an unbound-scope error at every reference site, since an invalid slug never claims its scope. Strip both spellings whole. Covered at the unit level in TestScopeFromPath and end to end by a resolution case importing a .modelith.yml peer. Signed-off-by: Joe Beda --- docs/06-schema-reference.md | 3 ++- internal/lint/imports_test.go | 9 +++++++++ internal/model/model.go | 14 +++++++++----- internal/model/model_test.go | 9 ++++++--- internal/schema/v1/modelith.schema.json | 2 +- 5 files changed, 27 insertions(+), 10 deletions(-) diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 44bdb95..81cf1c6 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -283,7 +283,8 @@ entities: The imported file needs no cooperation at all — it is an ordinary model that happens to define a `PaymentMethod` enum. **The scope is bound by the model doing the importing**, and defaults to the file's basename with -`.modelith.yaml` stripped: `./payments.modelith.yaml` binds `payments`. +`.modelith.yaml` — or `.modelith.yml` — stripped: `./payments.modelith.yaml` +binds `payments`. Name it explicitly when the filename yields no usable slug, or when the obvious one is already taken: diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index b1b5b7c..92f9f0c 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -139,6 +139,7 @@ func TestImports_Resolution(t *testing.T) { // covers the tree without one. ".git": "", "docs/payments.modelith.yaml": paymentsModel, + "docs/shipping.modelith.yml": paymentsModel, "payments/payments.modelith.yaml": paymentsModel, "docs/legacy/pay-v2.modelith.yaml": paymentsModel, "docs/not-a-model.yaml": "kind: SomethingElse\nversion: v1\n", @@ -162,6 +163,14 @@ func TestImports_Resolution(t *testing.T) { imports: []string{`"../payments/payments.modelith.yaml"`}, attrType: "payments.PaymentMethod", }, + { + // A .yml model file is a model file: the derived scope strips the + // whole ".modelith.yml", not just the extension, or the bare form + // would bind "shipping.modelith" and resolve nothing. + name: "bare import of a .modelith.yml file resolves", + imports: []string{`"./shipping.modelith.yml"`}, + attrType: "shipping.PaymentMethod", + }, { name: "qualified type with no imports at all", attrType: "payments.PaymentMethod", diff --git a/internal/model/model.go b/internal/model/model.go index b5b0eaa..cb5f91a 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -86,13 +86,17 @@ func (i *Import) UnmarshalJSON(data []byte) error { } // ScopeFromPath derives the scope a bare import path binds: the basename with -// ".modelith.yaml" — or failing that, the final extension — stripped. The -// result is not guaranteed to be a valid slug; the linter reports one that -// isn't, since only an explicitly written scope passes through the schema. +// ".modelith.yaml" or ".modelith.yml" — or failing that, the final extension — +// stripped. Both spellings are stripped whole, because trimming ".yml" alone +// leaves "payments.modelith", which is no slug at all. The result is not +// guaranteed to be a valid slug; the linter reports one that isn't, since only +// an explicitly written scope passes through the schema. func ScopeFromPath(p string) string { base := path.Base(p) - if trimmed, ok := strings.CutSuffix(base, ".modelith.yaml"); ok { - return trimmed + for _, suffix := range []string{".modelith.yaml", ".modelith.yml"} { + if trimmed, ok := strings.CutSuffix(base, suffix); ok { + return trimmed + } } return strings.TrimSuffix(base, path.Ext(base)) } diff --git a/internal/model/model_test.go b/internal/model/model_test.go index 74d0735..5308147 100644 --- a/internal/model/model_test.go +++ b/internal/model/model_test.go @@ -222,9 +222,12 @@ func TestScopeFromPath(t *testing.T) { cases := map[string]string{ "./payments.modelith.yaml": "payments", "../shared/billing.modelith.yaml": "billing", - "./legacy/pay-v2.yaml": "pay-v2", - "./Payments.modelith.yaml": "Payments", - "noextension": "noextension", + // RenderedPath treats .yml as a model file, so the scope has to come out + // of one too: trimming only the extension would leave "payments.modelith". + "./payments.modelith.yml": "payments", + "./legacy/pay-v2.yaml": "pay-v2", + "./Payments.modelith.yaml": "Payments", + "noextension": "noextension", } for in, want := range cases { if got := ScopeFromPath(in); got != want { diff --git a/internal/schema/v1/modelith.schema.json b/internal/schema/v1/modelith.schema.json index 528f27a..3d9786f 100644 --- a/internal/schema/v1/modelith.schema.json +++ b/internal/schema/v1/modelith.schema.json @@ -76,7 +76,7 @@ "description": "Another model this one references: either a bare path (the scope is the file's basename with its extension stripped) or an object naming the scope explicitly.", "oneOf": [ { - "description": "A path to a model file, relative to this one. Its basename with \".modelith.yaml\" (or the final extension) stripped is the scope.", + "description": "A path to a model file, relative to this one. Its basename with \".modelith.yaml\" or \".modelith.yml\" (or, failing either, the final extension) stripped is the scope.", "type": "string", "minLength": 1 }, From 4935a3ae2a7155eede1a02535a68b5ab0ea51fdf Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 16:11:09 -0700 Subject: [PATCH 13/22] feat(schema): mark a model shared so its vocabulary may be unused locally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A model that exists to be imported cannot pass --completeness error. The references that justify its enums and glossary terms live in the files that import it, which it cannot see, so every enum it publishes collects "defined but no attribute uses it" and CI fails. The docs recommend factoring exactly such a model out; docs/05-parking-garage/payments.modelith.yaml escaped only because Payment.method happens to use the enum locally. Add a top-level `shared: true`. It is the counterpart of `imports` — one model declares what it reaches for, the other declares that it is reached for — and relaxes exactly the checks about a definition nothing in this file uses: the unused enum and the unused glossary term. It relaxes nothing about content: an entity with no invariants, or one no scenario exercises, is a gap in the model itself, which being imported does not fill. Schema property, struct field (TestSchemaStructSync guards the pair) and the schema reference land together, and the parking-garage payments model now declares what it already is. Signed-off-by: Joe Beda --- docs/05-parking-garage/payments.modelith.yaml | 1 + docs/06-schema-reference.md | 35 ++++++- internal/lint/lint.go | 11 +++ internal/lint/lint_test.go | 94 +++++++++++++++++++ internal/model/model.go | 6 ++ internal/schema/v1/modelith.schema.json | 4 + 6 files changed, 150 insertions(+), 1 deletion(-) diff --git a/docs/05-parking-garage/payments.modelith.yaml b/docs/05-parking-garage/payments.modelith.yaml index 393ee33..a7b9573 100644 --- a/docs/05-parking-garage/payments.modelith.yaml +++ b/docs/05-parking-garage/payments.modelith.yaml @@ -2,6 +2,7 @@ kind: DomainModel version: v1 title: Payments +shared: true description: >- How money is taken and given back. A deliberately small context that owns the vocabulary of settling a charge — nothing in it knows what was bought, which diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 81cf1c6..3035664 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -31,6 +31,7 @@ the schema). Print the schema any time with `modelith schema`. | `version` | string | yes | Schema revision. Currently `v1`. | | `title` | string | no | Heading used when rendering. | | `description` | string | no | One-paragraph summary. | +| `shared` | boolean | no | `true` if other models import this one. Relaxes the completeness checks that flag a definition nothing in *this* file uses. See [Imports](#imports). | | `imports` | list | no | Other model files whose items this one references, each a path relative to this file. See [Imports](#imports). | | `glossary` | map | no | Ubiquitous-language terms that aren't entities. See [Glossary](#glossary). | | `enums` | map | no | First-class enumerated types. See [Enum](#enum). | @@ -363,6 +364,36 @@ The full rationale, including where this is heading, is and the boundary itself is [ADR-0013](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0013-imports-confined-to-the-repository.md). +### The model on the other end: `shared: true` + +A model that exists to be imported has a problem the completeness checks would +otherwise punish it for: the references that justify its enums and glossary +terms are in files it cannot see. A pure vocabulary model — one that owns a +`PaymentMethod` and nothing else — collects a "defined but no attribute uses it" +warning for every enum in it, and under +[`--completeness error`](./07-cli.md) that is a failed build. + +Say so at the top level: + +```yaml +# payments.modelith.yaml +kind: DomainModel +version: v1 +shared: true +enums: + PaymentMethod: + values: + - name: card +``` + +`shared: true` is the counterpart of `imports`: one model declares what it +reaches for, the other declares that it is reached for. It relaxes exactly the +checks about a definition nothing *here* uses — the unused enum and the unused +glossary term. It relaxes nothing about content: an entity with no invariants, +and an entity no scenario exercises, are gaps in the model itself, and being +imported does not fill them. Nothing verifies the claim, and nothing needs to: +overstating it costs two advisory warnings, not correctness. + ## What this format deliberately leaves out modelith is a light, agent-authored subset of domain-driven design, not a full @@ -431,7 +462,9 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: - an action `actor` that is neither a defined entity nor a glossary term. - **Completeness** checks (advisory warnings): entities with no invariants; entities no scenario exercises; a glossary term nothing references; an enum no - attribute uses; an [import](#imports) nothing references. + attribute uses; an [import](#imports) nothing references. The two + nothing-references-it checks don't apply to a model marked + [`shared: true`](#the-model-on-the-other-end-shared-true). These are advisory on purpose. An entity that genuinely has no rule to state is fine — leave its invariants empty rather than inventing a filler rule that diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 7964900..4a1bcc4 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -811,6 +811,17 @@ func runCompleteness(m *model.Model, res *Result) { } // Defined-but-unused glossary terms and enums — vocabulary nothing references. + // + // A shared model is imported by others, and an incoming reference is + // invisible from here: the vocabulary such a model exists to publish would + // otherwise read as vocabulary nothing uses, and a pure vocabulary model + // could never pass --completeness error. Only these two checks are relaxed; + // a missing invariant or an unexercised entity is a gap in the model itself, + // which being imported does not fill. + if m.Shared { + return + } + usedTerm := map[string]bool{} scan := func(text string) { for _, b := range entityRefs(text) { diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go index 56eba57..349e9c9 100644 --- a/internal/lint/lint_test.go +++ b/internal/lint/lint_test.go @@ -1088,6 +1088,100 @@ entities: } } +// TestCompleteness_SharedRelaxesOnlyTheUnusedChecks pins the escape hatch a +// vocabulary model needs. A model that exists to be imported has its enums and +// glossary terms referenced from files it cannot see, so under +// --completeness error it could not pass at all; `shared: true` says the +// references are elsewhere. It says nothing about content, so a gap that being +// imported does not fill — an entity with no invariants, an entity no scenario +// exercises — is still reported. +func TestCompleteness_SharedRelaxesOnlyTheUnusedChecks(t *testing.T) { + t.Parallel() + + // A vocabulary model with nothing missing from it: every entity has an + // invariant and a scenario that exercises it. All that is left is the enum + // and the glossary term it exists to publish, which nothing here uses. + const vocabulary = ` +kind: DomainModel +version: v1 +glossary: + Payer: Whoever hands over the money; named only by the models that import this one. +enums: + PaymentMethod: + values: + - name: card +scenarios: + - name: A charge is settled + actors: [Payment] + steps: + - A card is tapped and the transfer is recorded. + invariants_touched: [payment-amount-positive] +entities: + Payment: + definition: One completed transfer of money. + invariants: + - id: payment-amount-positive + statement: A payment's amount is greater than zero. +` + // A gap a shared model does not get to skip: an entity nothing governs and + // no scenario exercises is missing from the model itself, which being + // imported does not fill. + const contentGap = " Refund:\n definition: Money returned.\n" + + cases := []struct { + name string + src string + // unused is whether the defined-but-locally-unused findings are expected. + unused bool + // blocking is HasBlocking under --completeness error. + blocking bool + gap bool + }{ + {name: "not shared", src: vocabulary, unused: true, blocking: true}, + { + name: "shared", + src: strings.Replace(vocabulary, "version: v1", "version: v1\nshared: true", 1), + }, + { + name: "shared, with a gap that is not about being used", + src: strings.Replace(vocabulary, "version: v1", "version: v1\nshared: true", 1) + contentGap, + blocking: true, + gap: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + res, err := Run(testModelPath, []byte(tc.src), nil) + if err != nil { + t.Fatal(err) + } + for _, msg := range []string{ + `glossary term "Payer" is defined but never referenced`, + `enum "PaymentMethod" is defined but no attribute uses it`, + } { + if got := findingWithMessage(res.Findings, msg); got != tc.unused { + t.Errorf("finding %q present = %v, want %v: %+v", msg, got, tc.unused, res.Findings) + } + } + for _, msg := range []string{ + `entity "Refund" has no invariants`, + `no scenario exercises entity "Refund"`, + } { + if got := findingWithMessage(res.Findings, msg); got != tc.gap { + t.Errorf("finding %q present = %v, want %v: %+v", msg, got, tc.gap, res.Findings) + } + } + // The boundary this exists for: only the shared model with nothing + // else missing passes --completeness error. + if got := res.HasBlocking(true); got != tc.blocking { + t.Errorf("HasBlocking(true) = %v, want %v: %+v", got, tc.blocking, res.Findings) + } + }) + } +} + func TestDerivedRequiresDerivation(t *testing.T) { src := ` kind: DomainModel diff --git a/internal/model/model.go b/internal/model/model.go index cb5f91a..8efc5c1 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -23,6 +23,12 @@ type Model struct { Version string `json:"version"` Title string `json:"title,omitempty"` Description string `json:"description,omitempty"` + // Shared marks a model other models import. The references that justify its + // vocabulary then live in files it cannot see, so the completeness checks + // that flag a definition nothing here uses do not apply to it. It is the + // counterpart of Imports: one model declares what it reaches for, the other + // declares that it is reached for. + Shared bool `json:"shared,omitempty"` // Imports are the other model files this one references, each bound to the // scope written at its reference sites. Resolution does not recurse: an // imported model's own imports are not reachable from here (ADR-0010, diff --git a/internal/schema/v1/modelith.schema.json b/internal/schema/v1/modelith.schema.json index 3d9786f..5da8abf 100644 --- a/internal/schema/v1/modelith.schema.json +++ b/internal/schema/v1/modelith.schema.json @@ -25,6 +25,10 @@ "description": "Optional one-paragraph summary of what this model covers.", "type": "string" }, + "shared": { + "description": "True if other models import this one. Its enums and glossary terms are then expected to be referenced from outside, which no single file can show, so the completeness checks that flag a definition nothing in THIS file uses do not apply to it. Checks about missing content — an entity with no invariants, an entity no scenario exercises — still do.", + "type": "boolean" + }, "imports": { "description": "Other model files whose items this model references. Each entry is a path relative to this file — \"..\" is fine; an absolute path is not portable across checkouts and is rejected — and binds a scope this model writes before an imported item's name (\"payments.PaymentMethod\"). Resolution does not recurse: only items defined directly in a listed file are reachable.", "type": "array", From 44571aab747182ebc19d543aa069e3b4275f757b Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 16:12:01 -0700 Subject: [PATCH 14/22] docs(schema): say a dot in an attribute type is reserved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkQualifiedTypes runs whether or not a model has `imports:`, so any dot in an attribute type is now an error — `decimal(10.2)` and `google.protobuf.Timestamp` included. That is deliberate: an unconditional rule is what makes a typo in a cross-model reference reportable instead of readable as an exotic primitive. It was documented in the schema reference and nowhere in the JSON Schema, which is the contract an editor fetches through the $schema header, and which still described `type` as a primitive or an enum name. Say it in both, and pin the unconditional part with two resolution cases in a model that imports nothing. Signed-off-by: Joe Beda --- docs/06-schema-reference.md | 8 ++++++-- internal/lint/imports_test.go | 21 +++++++++++++++++++++ internal/schema/v1/modelith.schema.json | 2 +- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 3035664..94dd6a9 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -182,7 +182,7 @@ and covered in full in [Reading the Diagrams](./04-reading-the-diagrams.md): | Field | Type | Required | Notes | |---|---|---|---| | `name` | string | yes | Attribute name. | -| `type` | string | yes | A **primitive** (lowercase, e.g. `string`, `integer`, `boolean`, `timestamp`) or the **PascalCase name of a defined [enum](#enum)**. A PascalCase type that names no enum is flagged. | +| `type` | string | yes | A **primitive** (lowercase, e.g. `string`, `integer`, `boolean`, `timestamp`) or the **PascalCase name of a defined [enum](#enum)**. A PascalCase type that names no enum is flagged. A dot is reserved for a [cross-model reference](#imports) and is an error anywhere else, `imports` or no `imports`. | | `description` | string | no | | | `derived` | boolean | no | True if computed from other state rather than stored. Forces `derivation`. | | `derivation` | string | no | How a derived attribute is computed. Required when `derived` is true. | @@ -313,7 +313,11 @@ Six rules are worth knowing before you use this: has to be exactly `scope.Name` — one dot, a slug before it, a PascalCase item name after. `payments.v2.PaymentMethod`, `.PaymentMethod` and `payments.` are errors that say which way they are malformed, not types quietly read as - primitives. + primitives. The dot is reserved in **every** model, including one with no + `imports` at all: that is what makes a typo in a reference reportable, and it + means `decimal(10.2)` or `google.protobuf.Timestamp` is an error, not a type. + Name such a type without the dot (`decimal`, `Timestamp`) — this format + describes concepts, not wire formats. - **The binding is local.** Two models may import the same file under different scopes, and neither one's choice is visible to the other. Two imports binding the *same* scope in one model is an error — give one of them an explicit, diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index 92f9f0c..23d51d8 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -179,6 +179,27 @@ func TestImports_Resolution(t *testing.T) { `references the scope "payments", which no import binds`, }}, }, + { + // The dot is reserved whether or not the model imports anything, + // which is what makes a typo in a reference reportable — and what + // makes a type borrowed from a wire format an error. The schema's + // own `type` description says so. + name: "a dotted type is an error in a model with no imports", + attrType: "decimal(10.2)", + want: []wantFinding{{ + SeverityError, CategorySemantic, typePath, + `attribute type "decimal(10.2)" is a malformed cross-model reference ` + + `(the scope "decimal(10" is not lowercase kebab-case)`, + }}, + }, + { + name: "a wire-format type name is an error in a model with no imports", + attrType: "google.protobuf.Timestamp", + want: []wantFinding{{ + SeverityError, CategorySemantic, typePath, + `attribute type "google.protobuf.Timestamp" is a malformed cross-model reference (more than one dot)`, + }}, + }, { name: "qualified type naming an unimported scope", imports: []string{`"./payments.modelith.yaml"`}, diff --git a/internal/schema/v1/modelith.schema.json b/internal/schema/v1/modelith.schema.json index 5da8abf..73f7576 100644 --- a/internal/schema/v1/modelith.schema.json +++ b/internal/schema/v1/modelith.schema.json @@ -251,7 +251,7 @@ "minLength": 1 }, "type": { - "description": "Conceptual type: a primitive (e.g. \"string\", \"integer\", \"boolean\", \"timestamp\") or the PascalCase name of an enum defined under top-level \"enums\".", + "description": "Conceptual type: a primitive (e.g. \"string\", \"integer\", \"boolean\", \"timestamp\") or the PascalCase name of an enum defined under top-level \"enums\". A dot is RESERVED for cross-model references: a type containing one must be a well-formed \"scope.Name\" naming an enum in an imported model, so \"decimal(10.2)\" and \"google.protobuf.Timestamp\" are rejected. The rule holds in every model, including one with no \"imports\" — that is what makes a typo in a reference reportable instead of readable as an exotic primitive.", "type": "string", "minLength": 1 }, From 1318c6693ba5a4432d6cf3e3672e8d21fd611f72 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 16:14:12 -0700 Subject: [PATCH 15/22] fix(render): stop labelling the imports link with the source path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Imports bullet rendered as [./payments.modelith.yaml](./payments.modelith.md): a link that says .yaml and lands on Markdown. Show the source path as a code span and label the link for what it leads to, so the two files are two things on the page. mdLinkText was the imports label's escaper and had no other caller, so it goes with it; the code span carries the same author text with the fence-widening the rest of the renderer already relies on. The hostile-path test now checks the stronger property directly — strip the code spans from a bullet and the fixed skeleton is all that is left. Signed-off-by: Joe Beda --- docs/05-parking-garage/garage.modelith.md | 2 +- docs/06-schema-reference.md | 9 +-- internal/render/markdown/markdown.go | 29 +++------ internal/render/markdown/markdown_test.go | 78 +++++++++++++++++++---- 4 files changed, 80 insertions(+), 38 deletions(-) diff --git a/docs/05-parking-garage/garage.modelith.md b/docs/05-parking-garage/garage.modelith.md index 20e4c2e..8474e80 100644 --- a/docs/05-parking-garage/garage.modelith.md +++ b/docs/05-parking-garage/garage.modelith.md @@ -8,7 +8,7 @@ Tracks the state of a single parking garage: its spots, the cars that occupy the Items defined in these models are referenced below as `scope.Name`. -- **`payments`** — [./payments.modelith.yaml](./payments.modelith.md) +- **`payments`** — `./payments.modelith.yaml` ([rendered](./payments.modelith.md)) ## Glossary diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 94dd6a9..f6703fd 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -344,10 +344,11 @@ Six rules are worth knowing before you use this: your project extends, so it assumes the least, and even a sibling directory is out of reach. -Rendered Markdown names each import and links it to that model's rendered `.md`, -and a qualified type links straight to the item's heading there. The renderer -never opens an imported file, so a link points at where the Markdown *would* be: -render the imported model too, or the link dangles. +Rendered Markdown names each import, shows the path as written, and links +separately to that model's rendered `.md`; a qualified type links straight to +the item's heading there. The renderer never opens an imported file, so a link +points at where the Markdown *would* be: render the imported model too, or the +link dangles. The linter reports a qualified type that doesn't resolve as an **error**, while an *unqualified* PascalCase type that names no enum is only a **warning**. The diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index 1d97cc0..288386e 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -75,6 +75,11 @@ func renderInvariants(b *strings.Builder, m *model.Model) { // linking each to its rendered Markdown. It never opens them: the imported // definitions are not restated here, only pointed at, so this document stays a // pure function of its own source (ADR-0010). +// +// The source path is shown as text and the link is labelled for what it leads +// to, because they are two different files. Labelling the link with the ".yaml" +// path while pointing it at the ".md" told the reader they were clicking +// through to the model source. func renderImports(b *strings.Builder, m *model.Model) { if len(m.Imports) == 0 { return @@ -83,9 +88,11 @@ func renderImports(b *strings.Builder, m *model.Model) { b.WriteString("Items defined in these models are referenced below as `scope.Name`.\n\n") // A path — and the scope a bare import derives from it — is author-supplied // text that reaches a published page, so both go through the same escaping - // as every other string here, and the link destination through its own. + // as every other string here, and the link destination through its own. The + // link's own label is a fixed word, so it carries nothing to escape. for _, imp := range m.Imports { - fmt.Fprintf(b, "- **%s** — [%s](%s)\n", codeSpan(imp.Scope), mdLinkText(imp.Path), linkTarget(model.RenderedPath(imp.Path))) + fmt.Fprintf(b, "- **%s** — %s ([rendered](%s))\n", + codeSpan(imp.Scope), codeSpan(imp.Path), linkTarget(model.RenderedPath(imp.Path))) } b.WriteString("\n") } @@ -421,24 +428,6 @@ func codeSpan(s string) string { return fence + s + fence } -// mdLinkText escapes a value for use as a Markdown link label. A code span -// would be the natural way to show a path, but a code span cannot escape the -// "]" that ends the label: its inertness would rest on the rule that code spans -// bind more tightly than link brackets, which the CommonMark spec states and -// widely used renderers get wrong. A backslash escape holds everywhere, so the -// label is plain text with every character that carries inline meaning escaped. -// An ordinary relative path holds none of them and passes through unchanged. -func mdLinkText(s string) string { - var b strings.Builder - for _, r := range oneLine(s) { - if strings.ContainsRune("\\`*_[]()<>&|~!#", r) { - b.WriteByte('\\') - } - b.WriteRune(r) - } - return b.String() -} - // linkTarget percent-encodes a path for use as a Markdown link destination. // Escaping a value for a table cell is not escaping it for a URL: a closing // parenthesis ends the destination early, whitespace ends it, and an angle diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index f3390c6..5cc6bbb 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -181,8 +181,11 @@ func TestRenderImports_SectionAndLinkedTypes(t *testing.T) { for _, want := range []string{ "## Imports\n", - "- **`payments`** — [../payments/payments.modelith.yaml](../payments/payments.modelith.md)\n", - "- **`billing`** — [./legacy/pay-v2.modelith.yaml](./legacy/pay-v2.modelith.md)\n", + // The source path is the model file; the link leads to the rendered + // Markdown beside it. Labelling the link with the .yaml path pointed the + // reader at a file they would not land on. + "- **`payments`** — `../payments/payments.modelith.yaml` ([rendered](../payments/payments.modelith.md))\n", + "- **`billing`** — `./legacy/pay-v2.modelith.yaml` ([rendered](./legacy/pay-v2.modelith.md))\n", "| `paidWith` | [payments.PaymentMethod](../payments/payments.modelith.md#paymentmethod) | |\n", "| `plan` | [billing.Plan](./legacy/pay-v2.modelith.md#plan) | |\n", // A scope no import binds is a lint error; the renderer states it rather @@ -231,16 +234,19 @@ func TestRenderImports_HostilePathCannotEscapeItsMarkup(t *testing.T) { got := Render(m) for _, want := range []string{ - // The label is backslash-escaped plain text, so nothing in it opens a - // tag or closes the label early. - `- **` + "`payments`" + `** — [./p.modelith.yaml\) \ \[click me\]\(\#](./p.modelith.yaml%29%20%3Cimg%20src%3Dx%20onerror%3Dalert%281%29%3E%20%5Bclick%20me%5D%28%23.md)` + "\n", + // The path is a code span, so nothing in it opens a tag, and the link + // beside it carries a fixed label with nothing to escape. + "- **`payments`** — `./p.modelith.yaml) [click me](#`" + + ` ([rendered](./p.modelith.yaml%29%20%3Cimg%20src%3Dx%20onerror%3Dalert%281%29%3E%20%5Bclick%20me%5D%28%23.md))` + "\n", // A pipe survives in the destination as %7C rather than being escaped // for a table cell it is not in. - "[./a\\|b.modelith.yaml](./a%7Cb.modelith.md)\n", - "[./a b.modelith.yaml](./a%20b.modelith.md)\n", + "`./a|b.modelith.yaml` ([rendered](./a%7Cb.modelith.md))\n", + "`./a b.modelith.yaml` ([rendered](./a%20b.modelith.md))\n", // Every control character is replaced before the value is written. - "- **`line break`** — [./a \\#\\# Injected b.modelith.yaml](./a%0A%23%23%20Injected%0A%0Ab.modelith.md)\n", - "[./a\\`b\\`\\`c.modelith.yaml](./a%60b%60%60c.modelith.md)\n", + "- **`line break`** — `./a ## Injected b.modelith.yaml` ([rendered](./a%0A%23%23%20Injected%0A%0Ab.modelith.md))\n", + // A path holding backticks widens the span's fence rather than closing + // it early. + "```./a`b``c.modelith.yaml``` ([rendered](./a%60b%60%60c.modelith.md))\n", // The deep link is assembled from escaped parts: escaping the finished // link instead would put a backslash inside the destination. "| `paidWith` | [payments.PaymentMethod](./p.modelith.yaml%29%20%3Cimg%20src%3Dx%20onerror%3Dalert%281%29%3E%20%5Bclick%20me%5D%28%23.md#paymentmethod) | |\n", @@ -250,10 +256,19 @@ func TestRenderImports_HostilePathCannotEscapeItsMarkup(t *testing.T) { } } - // Every occurrence of a breakout token is a backslash-escaped one. - for _, tok := range []string{"= 0 && close+len(fence) < len(rest) && rest[close+len(fence)] == '`' { + // A longer run is not this fence's closer; keep looking. + next := strings.Index(rest[close+1:], fence) + if next < 0 { + close = -1 + break + } + close += 1 + next + } + if close < 0 { + b.WriteString(fence) // unclosed: the fence is literal text + continue + } + i += close + len(fence) + } + return b.String() +} + // TestCodeSpan_FencesAroundBackticks checks the CommonMark rule the imports // section leans on: a span's fence is longer than any backtick run inside it, so // a value holding backticks stays intact instead of closing the span early. From f9cd4f674da50ce4c6e2cf09b094c2480915bcba Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 16:14:57 -0700 Subject: [PATCH 16/22] docs(parking-garage): say what shared: true is for where it comes up The walkthrough tells the reader the imported model "needs no changes at all", which is true of the reference itself and now sits next to a payments model carrying shared: true. Name the one thing an imported model may want to say, and why this one says it. Signed-off-by: Joe Beda --- docs/05-parking-garage/index.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/05-parking-garage/index.md b/docs/05-parking-garage/index.md index 5700744..9fd1122 100644 --- a/docs/05-parking-garage/index.md +++ b/docs/05-parking-garage/index.md @@ -286,5 +286,12 @@ an error, not a shrug — and the [rendered model](./garage.modelith.md) links [payments model](./payments.modelith.md). One definition, in the context that owns it. +The one thing the payments model *may* want to say is that it is on the +receiving end of this. A model whose enums are used only by the models that +import it collects an "enum is defined but no attribute uses it" advisory for +each one, since the uses are in files it cannot see. `shared: true` at the top +level retires that class of advisory — and nothing else. This one declares it, +though its own `Payment` happens to use the enum too. + Full rules, including how to name a scope explicitly when the filename won't do: [Imports](../06-schema-reference.md#imports). From 6fdbd62a874d31cce83e9382d6067db191e8ebe1 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 17:02:05 -0700 Subject: [PATCH 17/22] fix(render): relativize import links against the output directory render -o /x.md and --stdout emitted import links relative to the source directory regardless of where the output landed, so a link built for outdir/main.md pointed at outdir/payments.modelith.md, which modelith never wrote. The link target is now the imported model's default rendered path (RenderedPath resolved against the source directory) expressed relative to the actual output directory; this resolves whenever the imported model was itself rendered to its default location. --stdout has no output file to relativize against, so its links stay source-relative, as they would from a default render. The common case (no -o, both directories equal) is left byte-identical to avoid gratuitous golden churn; only render -o and --stdout change. Narrows defaultOut's comment, which previously claimed the renderer's links and defaultOut "cannot disagree" unconditionally. Round 2 finding R2-1: .scratch/reviews/imports-slice1.md Signed-off-by: Joe Beda --- cmd/modelith/main.go | 23 ++++- cmd/modelith/main_test.go | 70 +++++++++++++++ docs/06-schema-reference.md | 13 ++- docs/07-cli.md | 6 ++ internal/render/markdown/markdown.go | 55 +++++++++--- internal/render/markdown/markdown_test.go | 101 +++++++++++++++++++--- 6 files changed, 238 insertions(+), 30 deletions(-) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index b5bd2d3..5f27576 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "runtime/debug" "strings" @@ -222,9 +223,17 @@ func renderCmd() *cobra.Command { if err != nil { return err } - rendered := markdown.Render(m) + + sourceDir, err := filepath.Abs(filepath.Dir(in)) + if err != nil { + return fmt.Errorf("resolving %s: %w", in, err) + } if stdout { + // There is no output file to relativize import links against, so + // they stay relative to the source — the same links a default, + // beside-the-source render would produce. + rendered := markdown.Render(m, sourceDir, sourceDir) _, err := fmt.Fprint(cmd.OutOrStdout(), rendered) return err } @@ -233,6 +242,11 @@ func renderCmd() *cobra.Command { if target == "" { target = defaultOut(in) } + outDir, err := filepath.Abs(filepath.Dir(target)) + if err != nil { + return fmt.Errorf("resolving %s: %w", target, err) + } + rendered := markdown.Render(m, sourceDir, outDir) if check { existing, err := os.ReadFile(target) @@ -262,9 +276,10 @@ func renderCmd() *cobra.Command { return cmd } -// defaultOut is where render writes when no -o is given. It is the same -// mapping the renderer uses for its links into an imported model's Markdown, so -// the two cannot disagree about where a model's .md lives. +// defaultOut is where render writes when no -o is given: a model's .md beside +// its .yaml source. It is the rendered location an import link assumes for the +// model it names (see importLinkTarget in internal/render/markdown) — a link +// resolves only when the imported model was, in fact, rendered here. func defaultOut(in string) string { return model.RenderedPath(in) } // ---- schema ---- diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index 2fd3328..1510a1c 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -170,6 +170,76 @@ func TestRenderStdoutDoesNotWriteFile(t *testing.T) { } } +// TestRenderOutFlagRelativizesImportLinks pins the R2-1 fix at the CLI level: +// `render -o /x.md` used to emit import links relative to the source +// directory, so a link built for `-o` landed at a path that didn't exist next +// to the file `-o` actually wrote. The output directory here (a fresh temp +// dir) shares no ancestor with the source's temp dir short of the OS temp +// root, so a fix that only handles a common-prefix case would still fail +// this. +func TestRenderOutFlagRelativizesImportLinks(t *testing.T) { + srcDir := t.TempDir() + const payments = `kind: DomainModel +version: v1 +entities: + Payment: + definition: A payment. +enums: + PaymentMethod: + values: + - {name: card, definition: A card payment.} +` + paymentsYAML := writeTemp(t, srcDir, "payments.modelith.yaml", payments) + if _, err := run(t, "render", paymentsYAML); err != nil { + t.Fatalf("rendering the imported model failed: %v", err) + } + + const main = `kind: DomainModel +version: v1 +imports: + - ./payments.modelith.yaml +entities: + Ticket: + definition: A parking ticket. + attributes: + - {name: paidWith, type: payments.PaymentMethod, description: how the fee was paid} +` + mainYAML := writeTemp(t, srcDir, "main.modelith.yaml", main) + + outDir := t.TempDir() // a distinct temp dir: no shared ancestor but the OS temp root. + target := filepath.Join(outDir, "rendered.md") + if _, err := run(t, "render", "-o", target, mainYAML); err != nil { + t.Fatalf("render -o failed: %v", err) + } + rendered, err := os.ReadFile(target) + if err != nil { + t.Fatalf("expected %s to be written: %v", target, err) + } + + link := importLinkFromMarkdown(t, string(rendered)) + resolved := filepath.Join(filepath.Dir(target), link) + if _, err := os.Stat(resolved); err != nil { + t.Fatalf("import link %q resolved to %s, which does not exist: %v\nrendered output:\n%s", link, resolved, err, rendered) + } +} + +// importLinkFromMarkdown extracts the destination of the single "[rendered](...)" +// link an Imports section emits, failing the test if it can't find exactly one. +func importLinkFromMarkdown(t *testing.T, md string) string { + t.Helper() + const marker = "([rendered](" + start := strings.Index(md, marker) + if start < 0 { + t.Fatalf("no import link found in:\n%s", md) + } + start += len(marker) + end := strings.IndexByte(md[start:], ')') + if end < 0 { + t.Fatalf("unterminated import link in:\n%s", md) + } + return md[start : start+end] +} + func TestRenderInvalidFileGivesFriendlyError(t *testing.T) { // Missing the required `definition`, so structural validation fails. const invalid = `kind: DomainModel diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index f6703fd..85ca977 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -348,7 +348,12 @@ Rendered Markdown names each import, shows the path as written, and links separately to that model's rendered `.md`; a qualified type links straight to the item's heading there. The renderer never opens an imported file, so a link points at where the Markdown *would* be: render the imported model too, or the -link dangles. +link dangles. That location is the imported model's **default** rendered path +— beside its own `.yaml`, per [`modelith render`](./07-cli.md) with no `-o` — +expressed relative to wherever this Markdown is written, so `-o` a different +directory than the source keeps the link resolving. `--stdout` has no output +location to relativize against, so its links stay relative to the source, as +they would from a default, beside-the-source render. The linter reports a qualified type that doesn't resolve as an **error**, while an *unqualified* PascalCase type that names no enum is only a **warning**. The @@ -446,8 +451,10 @@ The JSON Schema covers structure. [`modelith lint`](./07-cli.md) adds: - a scenario `invariants_touched` or an action `preserves` that references an invariant id no entity or model-level invariant declares; - an [import](#imports) that is absolute, unreadable, not a domain model, - binds a scope another import already bound, or is a bare path whose - filename yields no valid slug; + binds a scope another import already bound, is a bare path whose + filename yields no valid slug, resolves outside the repository holding + this model, contains a control character, or declares a schema version + this modelith doesn't support; - a qualified attribute `type` whose scope isn't imported, or that names no enum in the model it resolves to. - **Warnings** (likely-but-not-certainly wrong): diff --git a/docs/07-cli.md b/docs/07-cli.md index 9534f78..4c1eb13 100644 --- a/docs/07-cli.md +++ b/docs/07-cli.md @@ -65,6 +65,12 @@ Renders the model to a single Markdown document with an embedded Mermaid | `--stdout` | `false` | Write to stdout instead of a file. | | `--check` | `false` | Verify the committed output is up to date; non-zero exit on drift. | +If the model has [`imports`](./06-schema-reference.md#imports), the rendered +links to them are relative to wherever `-o` writes — `-o` a different +directory than the source and they still resolve, as long as the imported +model is rendered to *its* default location too. `--stdout` has no output file +to relativize against, so its links stay relative to the source. + The committed Markdown is the day-to-day read. `--check` is the CI gate that keeps it honest: diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index 288386e..cb89ce0 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -5,6 +5,7 @@ package markdown import ( "fmt" + "path/filepath" "regexp" "sort" "strings" @@ -27,8 +28,13 @@ var ( backtickRunRE = regexp.MustCompile("`+") ) -// Render produces the full Markdown document for the model. -func Render(m *model.Model) string { +// Render produces the full Markdown document for the model. sourceDir is the +// directory holding the model's own .yaml (imports are written relative to +// it); outDir is the directory this Markdown is being written to (or +// sourceDir itself, for a caller with no output file — e.g. `--stdout`). Both +// must be absolute, or both relative to the same base, so the import links +// built from them are well-defined; see importLinkTarget. +func Render(m *model.Model, sourceDir, outDir string) string { var b strings.Builder b.WriteString(generatedBanner) @@ -45,10 +51,10 @@ func Render(m *model.Model) string { b.WriteString("\n\n") } - renderImports(&b, m) + renderImports(&b, m, sourceDir, outDir) renderGlossary(&b, m) renderEnums(&b, m) - renderEntities(&b, m) + renderEntities(&b, m, sourceDir, outDir) renderDiagram(&b, m) renderInvariants(&b, m) renderScenarios(&b, m) @@ -80,7 +86,7 @@ func renderInvariants(b *strings.Builder, m *model.Model) { // to, because they are two different files. Labelling the link with the ".yaml" // path while pointing it at the ".md" told the reader they were clicking // through to the model source. -func renderImports(b *strings.Builder, m *model.Model) { +func renderImports(b *strings.Builder, m *model.Model, sourceDir, outDir string) { if len(m.Imports) == 0 { return } @@ -92,7 +98,7 @@ func renderImports(b *strings.Builder, m *model.Model) { // link's own label is a fixed word, so it carries nothing to escape. for _, imp := range m.Imports { fmt.Fprintf(b, "- **%s** — %s ([rendered](%s))\n", - codeSpan(imp.Scope), codeSpan(imp.Path), linkTarget(model.RenderedPath(imp.Path))) + codeSpan(imp.Scope), codeSpan(imp.Path), linkTarget(importLinkTarget(imp.Path, sourceDir, outDir))) } b.WriteString("\n") } @@ -100,19 +106,48 @@ func renderImports(b *strings.Builder, m *model.Model) { // importAnchors maps each bound scope to the rendered Markdown of the model it // names, for linking a qualified type. The first binding of a scope wins; a // second one is a lint error, and the renderer stays deterministic either way. -func importAnchors(m *model.Model) map[string]string { +func importAnchors(m *model.Model, sourceDir, outDir string) map[string]string { if len(m.Imports) == 0 { return nil } out := make(map[string]string, len(m.Imports)) for _, imp := range m.Imports { if _, seen := out[imp.Scope]; !seen { - out[imp.Scope] = model.RenderedPath(imp.Path) + out[imp.Scope] = importLinkTarget(imp.Path, sourceDir, outDir) } } return out } +// importLinkTarget is the Markdown link destination for an import: the +// imported model's *default* rendered path — model.RenderedPath(impPath) +// resolved against sourceDir, i.e. where `modelith render` puts that model's +// Markdown when it is rendered in place, the documented convention — expressed +// relative to outDir. The renderer has no way to know if the imported model +// was actually rendered anywhere else, so a link only resolves when it was +// rendered at that default location. +// +// When outDir and sourceDir are the same directory (the common case: no -o, +// or --stdout, both render beside the source), impPath's own rendered form is +// already correctly relative to outDir, so it is returned unchanged rather +// than round-tripped through Join/Rel, which would needlessly normalize away +// a leading "./". +func importLinkTarget(impPath, sourceDir, outDir string) string { + rendered := model.RenderedPath(impPath) + if outDir == sourceDir { + return rendered + } + abs := filepath.Join(sourceDir, filepath.FromSlash(rendered)) + rel, err := filepath.Rel(outDir, abs) + if err != nil { + // sourceDir and outDir aren't comparable (e.g. different Windows + // volumes) — the pre-relativized path is no more wrong than any + // alternative, and it's better than an empty link. + return rendered + } + return filepath.ToSlash(rel) +} + // typeCell renders an attribute type for a table cell, linking a "scope.Name" // reference into the imported model's Markdown. Headings render as "### `Name`", // so the anchor is the item's name lowercased. A scope no import binds — a lint @@ -178,7 +213,7 @@ func renderEnums(b *strings.Builder, m *model.Model) { } } -func renderEntities(b *strings.Builder, m *model.Model) { +func renderEntities(b *strings.Builder, m *model.Model, sourceDir, outDir string) { names := m.EntityNames() if len(names) == 0 { return @@ -191,7 +226,7 @@ func renderEntities(b *strings.Builder, m *model.Model) { subtypes[p] = append(subtypes[p], n) } } - importTargets := importAnchors(m) + importTargets := importAnchors(m, sourceDir, outDir) b.WriteString("## Entities\n\n") for _, name := range names { ent := m.Entities[name] diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index 5cc6bbb..3d0d587 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -9,6 +9,16 @@ import ( "github.com/stacklok/modelith/internal/model" ) +// render calls Render with sourceDir == outDir, the default beside-the-source +// case every test in this file exercises except the ones specifically about +// relativizing an import link against a different output directory (see +// TestRenderImports_LinkRelativeToOutputDir). The two directories are never +// read from disk — Render only uses them to compute relative import links — +// so any distinct absolute-looking path pair does the job. +func render(m *model.Model) string { + return Render(m, "/src", "/src") +} + // firstDiff reports the first line where want and got differ, so a golden // failure points at the change instead of just saying "they differ." func firstDiff(want, got string) string { @@ -45,7 +55,7 @@ func TestRenderInvariantsSection(t *testing.T) { {ID: "cross-entity-rule", Statement: "Spans the `Project` and the `Policy`."}, }, } - got := Render(with) + got := render(with) if !strings.Contains(got, "## Invariants\n") { t.Fatalf("expected a top-level Invariants section, got:\n%s", got) } @@ -58,8 +68,8 @@ func TestRenderInvariantsSection(t *testing.T) { "Project": {Definition: "A container."}, }, } - if strings.Contains(Render(without), "## Invariants") { - t.Fatalf("did not expect an Invariants section when there are none:\n%s", Render(without)) + if strings.Contains(render(without), "## Invariants") { + t.Fatalf("did not expect an Invariants section when there are none:\n%s", render(without)) } } @@ -76,7 +86,7 @@ func TestRenderEntity_DerivedMarker(t *testing.T) { }, }, } - got := Render(withDerivation) + got := render(withDerivation) if !strings.Contains(got, "**Derived:** Computed on demand from `Score` records.\n\n") { t.Fatalf("expected a derivation-specific marker, got:\n%s", got) } @@ -89,7 +99,7 @@ func TestRenderEntity_DerivedMarker(t *testing.T) { }, }, } - got = Render(withoutDerivation) + got = render(withoutDerivation) if !strings.Contains(got, "**Derived.** Computed on demand from other state; never persisted.\n\n") { t.Fatalf("expected a generic derived marker when derivation is omitted, got:\n%s", got) } @@ -99,8 +109,8 @@ func TestRenderEntity_DerivedMarker(t *testing.T) { "Leaderboard": {Definition: "A ranked view over current scores."}, }, } - if strings.Contains(Render(notDerived), "**Derived") { - t.Fatalf("did not expect a derived marker on a non-derived entity:\n%s", Render(notDerived)) + if strings.Contains(render(notDerived), "**Derived") { + t.Fatalf("did not expect a derived marker on a non-derived entity:\n%s", render(notDerived)) } } @@ -115,7 +125,7 @@ func TestRenderEntity_SymmetricRelationship(t *testing.T) { {Entity: "Node", Cardinality: "1:0..1", Role: "`Predecessor`"}, }}, }} - got := Render(m) + got := render(m) if want := "- `Node` — n:n — symmetric — `Peer`\n"; !strings.Contains(got, want) { t.Fatalf("expected a symmetric marker %q, got:\n%s", want, got) } @@ -140,7 +150,7 @@ func TestGoldenExample(t *testing.T) { if err != nil { t.Fatal(err) } - got := Render(m) + got := render(m) want, err := os.ReadFile(golden) if err != nil { @@ -177,7 +187,7 @@ func TestRenderImports_SectionAndLinkedTypes(t *testing.T) { }, }, } - got := Render(m) + got := render(m) for _, want := range []string{ "## Imports\n", @@ -198,11 +208,76 @@ func TestRenderImports_SectionAndLinkedTypes(t *testing.T) { } } - if strings.Contains(Render(&model.Model{Entities: m.Entities}), "## Imports") { + if strings.Contains(render(&model.Model{Entities: m.Entities}), "## Imports") { t.Error("did not expect an Imports section when there are no imports") } } +// TestRenderImports_LinkRelativeToOutputDir pins the R2-1 fix: `render -o +// /x.md` and `--stdout` used to emit import links relative to the +// *source* directory regardless of where the output landed, so a link into +// `outdir/payments.modelith.md` dangled — the file modelith actually wrote was +// `payments.modelith.md` beside the source. The link target must instead be +// the imported model's default rendered path (RenderedPath resolved against +// sourceDir) expressed relative to outDir. The second case has sourceDir and +// outDir share no ancestor but "/", the case an implementation that assumes a +// common prefix gets wrong. +func TestRenderImports_LinkRelativeToOutputDir(t *testing.T) { + t.Parallel() + + m := &model.Model{ + Imports: []model.Import{ + {Scope: "payments", Path: "../payments/payments.modelith.yaml", ScopeFromPath: true}, + {Scope: "billing", Path: "./legacy/pay-v2.modelith.yaml"}, + }, + Entities: map[string]model.Entity{ + "Ticket": { + Definition: "A temporary entry credential.", + Attributes: []model.Attribute{ + {Name: "paidWith", Type: "payments.PaymentMethod"}, + }, + }, + }, + } + + for _, tc := range []struct { + name string + sourceDir, outDir string + payments, billing string + }{ + { + // `-o ../out/x.md`: a sibling directory sharing an ancestor + // ("/repo") with the source. + name: "sibling output directory", + sourceDir: "/repo/models", + outDir: "/repo/out", + payments: "../payments/payments.modelith.md", + billing: "../models/legacy/pay-v2.modelith.md", + }, + { + name: "no common ancestor beyond root", + sourceDir: "/src/models", + outDir: "/var/output", + payments: "../../src/payments/payments.modelith.md", + billing: "../../src/models/legacy/pay-v2.modelith.md", + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := Render(m, tc.sourceDir, tc.outDir) + for _, want := range []string{ + fmt.Sprintf("- **`payments`** — `../payments/payments.modelith.yaml` ([rendered](%s))\n", tc.payments), + fmt.Sprintf("- **`billing`** — `./legacy/pay-v2.modelith.yaml` ([rendered](%s))\n", tc.billing), + fmt.Sprintf("[payments.PaymentMethod](%s#paymentmethod)", tc.payments), + } { + if !strings.Contains(got, want) { + t.Errorf("expected %q in:\n%s", want, got) + } + } + }) + } +} + // TestRenderImports_HostilePathCannotEscapeItsMarkup pins that an import path — // author-supplied text that reaches a published page — is escaped for the exact // position it lands in. A path is not validated by the schema beyond a minimum @@ -231,7 +306,7 @@ func TestRenderImports_HostilePathCannotEscapeItsMarkup(t *testing.T) { }, }, } - got := Render(m) + got := render(m) for _, want := range []string{ // The path is a code span, so nothing in it opens a tag, and the link @@ -348,7 +423,7 @@ func TestRenderEntity_SubtypeHierarchy(t *testing.T) { "Card": {Definition: "A card.", SubtypeOf: "PaymentMethod"}, "BankTransfer": {Definition: "A transfer.", SubtypeOf: "PaymentMethod"}, }} - got := Render(m) + got := render(m) if !strings.Contains(got, "**Subtype of** `PaymentMethod`") { t.Errorf("expected child to name its supertype:\n%s", got) } From 01c3899d3e7a2006c23e0a18f6e64c6b24301be6 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 17:04:52 -0700 Subject: [PATCH 18/22] fix(lint): make entity-position cross-model refs honest with the imports layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related findings, one fix: reportQualifiedEntityRefs is the sole authority on cross-model references in an entity position (relationship.entity, subtypeOf) — unsupported there, but a real reference to the scope all the same. R2-3: runImports didn't know that, so an import referenced only from an entity position got both the "not supported in an entity position" error and a contradictory "never referenced — drop it" completeness warning. reportQualifiedEntityRefs now also returns the scopes it found, and runImports treats them as used, same as an attribute type would. R2-4: runSemantic and runSubtypes independently skip a value matching the qualified-reference pattern, trusting reportQualifiedEntityRefs already reported it — but runStructural only called it after the unsupported-schema-version early return, so on an unsupported version the reference went unreported by anyone: neither the skipped structural finding nor the semantic/subtype checks that deferred to it. It now runs before that return too, so the trust the skip relies on always holds. Round 2 findings R2-3, R2-4: .scratch/reviews/imports-slice1.md Signed-off-by: Joe Beda --- internal/lint/imports.go | 29 ++++++++----- internal/lint/imports_test.go | 77 +++++++++++++++++++++++++++++++++++ internal/lint/lint.go | 33 +++++++++------ 3 files changed, 117 insertions(+), 22 deletions(-) diff --git a/internal/lint/imports.go b/internal/lint/imports.go index 8b374cb..a4f2d5e 100644 --- a/internal/lint/imports.go +++ b/internal/lint/imports.go @@ -63,8 +63,11 @@ type importedModel struct { // type against them, and reports an import nothing references. // // modelPath is the path of the model being linted; imports resolve relative to -// its directory. -func runImports(modelPath string, m *model.Model, files Files, res *Result) { +// its directory. entityScopes are the scopes named by a cross-model reference +// in an entity position (relationship.entity, subtypeOf) — unsupported there, +// but still a real reference: an import bound to one of them is not also +// reported as unreferenced (see reportQualifiedEntityRefs). +func runImports(modelPath string, m *model.Model, files Files, res *Result, entityScopes map[string]bool) { byScope, claimed := loadImports(modelPath, m, files, res) used := checkQualifiedTypes(m, byScope, claimed, res) // An unreferenced import is a completeness finding, alongside the unused @@ -72,7 +75,7 @@ func runImports(modelPath string, m *model.Model, files Files, res *Result) { // nothing uses. Sharing their category means sharing their promotion under // --completeness error. for _, scope := range sortedMapKeys(byScope) { - if used[scope] { + if used[scope] || entityScopes[scope] { continue } imp := byScope[scope] @@ -305,25 +308,31 @@ func malformedRefReason(typ string) string { } // reportQualifiedEntityRefs reports a cross-model reference in an entity -// position — relationship.entity or subtypeOf — and returns the instance paths -// it reported, so the schema's own finding for the same value is suppressed. +// position — relationship.entity or subtypeOf. It returns the instance paths +// it reported, so the schema's own finding for the same value is suppressed, +// and the scopes those references named, so an import that exists to support +// one of them is not also reported as unreferenced (runImports) even though no +// attribute type resolves it. // // Both fields carry pattern ^[A-Z][A-Za-z0-9]+$, so "payments.Card" already // fails validation with a message about a pattern. This says what is actually // wrong, in the spirit of the unsupported-version check. Cross-model entity // references are deferred, not planned against: ADR-0010 records why. -func reportQualifiedEntityRefs(inst any, res *Result) map[string]bool { - reported := map[string]bool{} +func reportQualifiedEntityRefs(inst any, res *Result) (reported map[string]bool, scopes map[string]bool) { + reported = map[string]bool{} + scopes = map[string]bool{} doc, ok := inst.(map[string]any) if !ok { - return reported + return reported, scopes } entities, ok := doc["entities"].(map[string]any) if !ok { - return reported + return reported, scopes } report := func(path, value string) { reported[path] = true + scope, _, _ := strings.Cut(value, ".") + scopes[scope] = true res.Findings = append(res.Findings, Finding{ Severity: SeverityError, Category: CategoryStructural, @@ -356,5 +365,5 @@ func reportQualifiedEntityRefs(inst any, res *Result) map[string]bool { } } } - return reported + return reported, scopes } diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index 23d51d8..4802b4f 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -696,6 +696,83 @@ entities: } } +// TestImports_EntityPositionReferenceCountsAsUsingTheImport pins R2-3: an +// import referenced only from an entity position (subtypeOf or +// relationship.entity) used to get both the "not supported in an entity +// position" error and a "never referenced — drop it" completeness warning for +// the same import — contradictory advice, since dropping the import does not +// fix the unsupported reference. reportQualifiedEntityRefs already knows the +// scope was reached for; runImports must count that as use, the same as an +// attribute type would. +func TestImports_EntityPositionReferenceCountsAsUsingTheImport(t *testing.T) { + t.Parallel() + + files := fakeFiles{"docs/payments.modelith.yaml": paymentsModel} + src := `kind: DomainModel +version: v1 +imports: + - "./payments.modelith.yaml" +entities: + Card: + definition: A store card. + subtypeOf: payments.Invoice +` + res, err := Run(importerPath, []byte(src), files) + if err != nil { + t.Fatal(err) + } + // Only the entity-position error is expected: no "/imports/0 ... never + // referenced" finding alongside it. + assertFindings(t, importFindings(res.Findings), []wantFinding{{ + SeverityError, CategoryStructural, "/entities/Card/subtypeOf", + "is a cross-model reference, which is not supported in an entity position", + }}) +} + +// TestRun_QualifiedEntityReferenceReportedOnUnsupportedVersion pins R2-4: +// reportQualifiedEntityRefs runs inside runStructural, but used to run only +// after the unsupported-version early return, so a cross-model reference in an +// entity position went unreported on a document whose version this build +// doesn't understand — only the version error surfaced. runSemantic and +// runSubtypes independently skip a value matching the same pattern, trusting +// that reportQualifiedEntityRefs already reported it; an early return that +// skips the call makes that trust false. It must run regardless of whether the +// version is supported. +func TestRun_QualifiedEntityReferenceReportedOnUnsupportedVersion(t *testing.T) { + t.Parallel() + + src := `kind: DomainModel +version: v99 +entities: + Card: + definition: A store card. + subtypeOf: payments.Card + relationships: + - entity: payments.Card + cardinality: "1:1" +` + res, err := Run(importerPath, []byte(src), fakeFiles{}) + if err != nil { + t.Fatal(err) + } + want := []wantFinding{ + {SeverityError, CategoryStructural, "/entities/Card/relationships/0/entity", + "is a cross-model reference, which is not supported in an entity position"}, + {SeverityError, CategoryStructural, "/entities/Card/subtypeOf", + "is a cross-model reference, which is not supported in an entity position"}, + } + var got []Finding + for _, f := range res.Findings { + if f.Path == want[0].path || f.Path == want[1].path { + got = append(got, f) + } + } + assertFindings(t, got, want) + if !res.HasBlocking(false) { + t.Error("an unsupported cross-model entity reference must block even on an unsupported version") + } +} + // TestRun_QualifiedEntityReferenceDoesNotGateTheImportsLayer pins that two // unrelated mistakes are reported in one run. The cross-model entity reference // used to count as a structural failure, which skipped the imports layer diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 4a1bcc4..101b30f 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -88,7 +88,7 @@ func Run(path string, src []byte, files Files) (*Result, error) { } // Layer 1: structural validation against the JSON Schema. - structuralOK := runStructural(src, res) + structuralOK, entityScopes := runStructural(src, res) // If it does not even parse into our typed model, stop — semantic and // completeness checks need a model to work with. The structural layer has @@ -117,7 +117,7 @@ func Run(path string, src []byte, files Files) (*Result, error) { // in an entity position is not one of those rejections (see runStructural), // so it does not take the imports layer down with it. if structuralOK { - runImports(path, m, files, res) + runImports(path, m, files, res, entityScopes) } runRelationshipShape(m, res) runSubtypes(m, res) @@ -144,8 +144,11 @@ func Structural(data []byte) []Finding { // runStructural validates against the JSON Schema. Returns true if the schema // accepted the document — which the cross-model entity references reported here // do not affect, since they are a supported-feature limit rather than a shape -// the imports list depends on. -func runStructural(data []byte, res *Result) bool { +// the imports list depends on — plus the scopes those references named, so the +// imports layer can tell an import that exists to support one of them from one +// genuinely unreferenced (runSemantic and runSubtypes rely on every such +// reference having been reported here; see reportQualifiedEntityRefs). +func runStructural(data []byte, res *Result) (ok bool, entityScopes map[string]bool) { jsonBytes, err := yaml.YAMLToJSON(data) if err != nil { res.Findings = append(res.Findings, Finding{ @@ -153,7 +156,7 @@ func runStructural(data []byte, res *Result) bool { Category: CategoryStructural, Message: fmt.Sprintf("not valid YAML: %v", err), }) - return false + return false, nil } inst, err := jsonschema.UnmarshalJSON(bytes.NewReader(jsonBytes)) @@ -163,7 +166,7 @@ func runStructural(data []byte, res *Result) bool { Category: CategoryStructural, Message: fmt.Sprintf("could not decode document: %v", err), }) - return false + return false, nil } // Dispatch on the declared format version. modelith — not the schema — is @@ -184,7 +187,13 @@ func runStructural(data []byte, res *Result) bool { Message: fmt.Sprintf("unsupported schema version %q; this modelith supports: %s "+ "(upgrade modelith, or set a supported version)", v, strings.Join(schema.SupportedVersions(), ", ")), }) - return false + // A cross-model entity reference is reported here regardless of + // whether the version is one this build understands: runSemantic and + // runSubtypes skip it on the assumption it was, and an early return + // before this call would leave it unreported instead of just + // unvalidated. + _, entityScopes := reportQualifiedEntityRefs(inst, res) + return false, entityScopes } } } @@ -196,7 +205,7 @@ func runStructural(data []byte, res *Result) bool { Category: CategoryStructural, Message: fmt.Sprintf("internal: %v", err), }) - return false + return false, nil } // Say what a cross-model entity reference actually is before the schema @@ -205,22 +214,22 @@ func runStructural(data []byte, res *Result) bool { // counted against the document's structural validity: a broken import in the // same file is an unrelated mistake, and holding the imports layer back // until this one is fixed would hide it. - qualified := reportQualifiedEntityRefs(inst, res) + qualified, entityScopes := reportQualifiedEntityRefs(inst, res) before := len(res.Findings) if err := sch.Validate(inst); err != nil { if ve, ok := err.(*jsonschema.ValidationError); ok { collectLeaves(ve, res, qualified) - return len(res.Findings) == before + return len(res.Findings) == before, entityScopes } res.Findings = append(res.Findings, Finding{ Severity: SeverityError, Category: CategoryStructural, Message: err.Error(), }) - return false + return false, entityScopes } - return true + return true, entityScopes } func collectLeaves(e *jsonschema.ValidationError, res *Result, skip map[string]bool) { From 7cff4cf78b3f320ad0bd2f67b05c06c73dc56d7d Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 17:06:49 -0700 Subject: [PATCH 19/22] docs(plugin): bring the skills up to the imports/shared contract domain-model-author still described the pre-imports attribute `type` rule ("a primitive in lowercase, or the PascalCase name of a defined enum") with no mention of the dot reservation, `imports:`, or `shared:`. An agent following it could emit `decimal(10.2)` or `money.Amount` and hit an error the skill gave it no way to anticipate. Verified against a build of this branch: `type: "decimal(10.2)"` lints clean on main and errors here. Adds `shared:` and `imports:` conventions to domain-model-author, narrows the attribute-type bullet to state the dot reservation and the cross-model-reference form, and updates domain-model-lint's structural/ semantic/completeness breakdown to include the entity-position cross-model-reference error, the import-resolution errors, the malformed-type error, and how `shared: true` narrows the completeness checks it relaxes. domain-model-context gets one line: an `imports:` entry means the vocabulary a `scope.Name` type names lives in another file, worth loading too. Round 2 finding R2-2: .scratch/reviews/imports-slice1.md Signed-off-by: Joe Beda --- plugin/skills/domain-model-author/SKILL.md | 30 ++++++++++++++-- plugin/skills/domain-model-context/SKILL.md | 3 ++ plugin/skills/domain-model-lint/SKILL.md | 39 ++++++++++++++------- 3 files changed, 58 insertions(+), 14 deletions(-) diff --git a/plugin/skills/domain-model-author/SKILL.md b/plugin/skills/domain-model-author/SKILL.md index 40bd071..b238d40 100644 --- a/plugin/skills/domain-model-author/SKILL.md +++ b/plugin/skills/domain-model-author/SKILL.md @@ -123,6 +123,25 @@ Follow the format exactly (see the schema reference). Key conventions: sentence: it is the only label drawn on the diagram line, so prose there collides with neighbouring lines and warns. Put the explanation in `note`. `ownership` needs no label — it draws as a solid vs dashed line. +- **`shared: true`** (top level) marks a model other models are expected to + import. It relaxes the "defined but never referenced" completeness checks + for *this* model's own enums and glossary terms — the references that + justify them live in files this model can't see — but not the + entity-invariant or scenario-coverage checks, which are about gaps in this + model itself. Mark a pure-vocabulary model (one that exists to be imported, + not used on its own) `shared: true` so it can pass `--completeness error`. +- **`imports`** (top level) lists other model files this one references, each + a path relative to this file: a bare string (`./payments.modelith.yaml`, + whose filename gives the scope — `payments`) or `{scope, path}` to name the + scope explicitly. Reference an item from an imported model as `scope.Name` + in an attribute `type` — **only there**; `relationship.entity` and + `subtypeOf` cannot be qualified this way (not supported yet, and the linter + says so explicitly if you try). An import must resolve inside the + repository holding this model (the nearest ancestor directory with a `.git` + entry, or this model's own directory if there is none) and name a + readable, supported-version domain model, or it's a lint error. An import + nothing references is a completeness warning: drop it, or reference one of + its enums. - **`glossary`** (top level) defines non-entity vocabulary — roles like `Owner`, states, domain nouns — as `Term: "definition"`. Define any role or actor name here; an undefined role warns, and an unused glossary term warns. @@ -132,8 +151,15 @@ Follow the format exactly (see the schema reference). Key conventions: is not a valid type; use the PascalCase name of an enum you defined under `enums`. - **Attribute `type`** is a primitive in **lowercase** (`string`, `integer`, - `boolean`, `timestamp`) or the **PascalCase name of a defined enum**. A - PascalCase type that names no enum warns. Mark computed values with + `boolean`, `timestamp`), the **PascalCase name of a defined enum**, or a + **cross-model reference** — `scope.Name`, naming an enum in a model bound by + `imports` (see above). A PascalCase type that names no enum warns; a + `scope.Name` type whose scope isn't imported, or that names no enum in the + model it resolves to, is an **error**. **A dot anywhere in `type` is + reserved for a cross-model reference** — `decimal(10.2)`, + `google.protobuf.Timestamp`, and any other dotted string that isn't a + well-formed `scope.Name` is a lint error, not a primitive you can invent; + there is no dotted primitive form in this format. Mark computed values with `derived: true` plus a `derivation:` (required when derived). - **`actions`** items are either a bare string (`create`) or a structured object `{name, actor?, preserves?, description?}`. Use the object form to tie an diff --git a/plugin/skills/domain-model-context/SKILL.md b/plugin/skills/domain-model-context/SKILL.md index 2d97e8a..5ce4169 100644 --- a/plugin/skills/domain-model-context/SKILL.md +++ b/plugin/skills/domain-model-context/SKILL.md @@ -27,6 +27,9 @@ relationships, and don't violate invariants. - the **canonical names** — use `Project`, never "workspace" or "container"; - the **relationships and cardinality** — what owns what; - the **invariants** — rules your code must not break. +4. If it has an `imports:` list, its `scope.Name` references (in attribute + `type` values) name vocabulary that lives in the imported file, not this + one — load that file too before treating a `scope.Name` term as unknown. ## How to apply it while coding diff --git a/plugin/skills/domain-model-lint/SKILL.md b/plugin/skills/domain-model-lint/SKILL.md index 8e9d557..da15644 100644 --- a/plugin/skills/domain-model-lint/SKILL.md +++ b/plugin/skills/domain-model-lint/SKILL.md @@ -38,7 +38,9 @@ the flag. ## Interpret the three layers - **Structural (error).** The file violates the JSON Schema — wrong type, - missing required field, bad `cardinality`. Must be fixed; the model won't + missing required field, bad `cardinality` — or names a cross-model reference + (`scope.Name`) in `relationship.entity` or `subtypeOf`, which is not + supported anywhere but an attribute `type`. Must be fixed; the model won't parse cleanly otherwise. - **Semantic (error or warning).** Errors (always block, flag-independent): a relationship points at an entity that doesn't exist; a scenario's @@ -46,18 +48,31 @@ the flag. or top-level `invariants` entry declares; the same invariant id is declared twice (entity-level and top-level invariants share one id namespace); reciprocal cardinalities that aren't inverses; reciprocal declarations that both claim - `ownership: owned`. Warnings: a backticked term resolves to no entity, role, - or actor; an action `actor` that's neither an entity nor a glossary term; a - PascalCase attribute `type` that names no defined enum — usually a typo or a - concept that was never named; a relationship `role` written as prose, which - belongs in `note`; an ambiguous reciprocal pairing, where one end declares the - same relationship twice and the other declares it back. Decide which it is and - propose the fix. + `ownership: owned`; an `imports` entry that's absolute, unreadable, not a + domain model, binds a scope another entry already bound, resolves outside + the repository holding this model, or is a bare path whose filename yields + no valid scope slug; an attribute `type` containing a dot that isn't a + well-formed `scope.Name` (a dot is always reserved for a cross-model + reference — `decimal(10.2)` and `google.protobuf.Timestamp` are errors, not + primitives); a `scope.Name` type whose scope no import binds, or that names + no enum in the model it resolves to. Warnings: a backticked term resolves to + no entity, role, or actor; an action `actor` that's neither an entity nor a + glossary term; a PascalCase attribute `type` that names no defined enum — + usually a typo or a concept that was never named; a relationship `role` + written as prose, which belongs in `note`; an ambiguous reciprocal pairing, + where one end declares the same relationship twice and the other declares it + back. Decide which it is and propose the fix. - **Completeness (advisory warning).** Gaps, not bugs: an entity with no - invariants, an entity no scenario exercises, a glossary term defined but never - referenced, or an enum no attribute uses. These are what `--completeness error` - promotes to blocking. For each, ask whether it's a real gap (write the missing - invariant or scenario) or genuinely fine. + invariants, an entity no scenario exercises, a glossary term defined but + never referenced, an enum no attribute uses, or an import nothing + references. `shared: true` on the model being linted relaxes the two + vocabulary-only checks (the unused glossary term and unused enum) — it does + not relax the invariant or scenario-coverage checks, and it does nothing for + an unreferenced import (that check is about *this* model's own `imports`, + not about being imported). These are what `--completeness error` promotes to + blocking. For each, ask whether it's a real gap (write the missing invariant + or scenario, or drop the unused import) or genuinely fine — a + `shared: true` model in particular. ## How to report back From 8de4124ee39b64f9ca86d62b3ce7354f46331781 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 17:27:42 -0700 Subject: [PATCH 20/22] docs(plugin): add two import error classes to domain-model-lint's list domain-model-lint/SKILL.md's semantic-error list for imports omitted the control-character and unsupported-schema-version cases that docs/06-schema-reference.md gained in the previous round. Verified both error strings against the built binary rather than paraphrasing from the docs. Round 3 finding: .scratch/reviews/r3-docs.md Signed-off-by: Joe Beda --- plugin/skills/domain-model-lint/SKILL.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/plugin/skills/domain-model-lint/SKILL.md b/plugin/skills/domain-model-lint/SKILL.md index da15644..50f0388 100644 --- a/plugin/skills/domain-model-lint/SKILL.md +++ b/plugin/skills/domain-model-lint/SKILL.md @@ -50,13 +50,14 @@ the flag. that aren't inverses; reciprocal declarations that both claim `ownership: owned`; an `imports` entry that's absolute, unreadable, not a domain model, binds a scope another entry already bound, resolves outside - the repository holding this model, or is a bare path whose filename yields - no valid scope slug; an attribute `type` containing a dot that isn't a - well-formed `scope.Name` (a dot is always reserved for a cross-model - reference — `decimal(10.2)` and `google.protobuf.Timestamp` are errors, not - primitives); a `scope.Name` type whose scope no import binds, or that names - no enum in the model it resolves to. Warnings: a backticked term resolves to - no entity, role, or actor; an action `actor` that's neither an entity nor a + the repository holding this model, is a bare path whose filename yields no + valid scope slug, has a path containing a control character, or declares a + schema version this modelith doesn't support; an attribute `type` containing + a dot that isn't a well-formed `scope.Name` (a dot is always reserved for a + cross-model reference — `decimal(10.2)` and `google.protobuf.Timestamp` are + errors, not primitives); a `scope.Name` type whose scope no import binds, or + that names no enum in the model it resolves to. Warnings: a backticked term + resolves to no entity, role, or actor; an action `actor` that's neither an entity nor a glossary term; a PascalCase attribute `type` that names no defined enum — usually a typo or a concept that was never named; a relationship `role` written as prose, which belongs in `note`; an ambiguous reciprocal pairing, From bf84a8b903bff1afb86203a88ce945e7a33e065c Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 17:31:01 -0700 Subject: [PATCH 21/22] fix(lint): assign the named return instead of shadowing it internal/lint/lint.go:195 declared a new, block-scoped entityScopes with := on the unsupported-version early-return path, shadowing runStructural's named return instead of assigning it. The explicit return false, entityScopes right after still returned the correct (shadowed) value, so this was latent, not live -- but a bare return relying on the named return would have silently returned nil. Added TestRunStructural_UnsupportedVersionReturnsEntityScopes, a white-box test calling runStructural directly (Run() never reaches this path with structuralOK true, so nothing downstream currently observes the value). Confirmed empirically that this one-line hunk alone is not observable by reverting it: the test still passes, matching the round-3 finding's own characterization. It does fail against the documented risk (shadow + relying on the named return), verified separately and not left in the tree. Round 3 finding: .scratch/reviews/r3-lint.md Signed-off-by: Joe Beda --- internal/lint/lint.go | 2 +- internal/lint/lint_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 101b30f..9b72935 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -192,7 +192,7 @@ func runStructural(data []byte, res *Result) (ok bool, entityScopes map[string]b // runSubtypes skip it on the assumption it was, and an early return // before this call would leave it unreported instead of just // unvalidated. - _, entityScopes := reportQualifiedEntityRefs(inst, res) + _, entityScopes = reportQualifiedEntityRefs(inst, res) return false, entityScopes } } diff --git a/internal/lint/lint_test.go b/internal/lint/lint_test.go index 349e9c9..567502e 100644 --- a/internal/lint/lint_test.go +++ b/internal/lint/lint_test.go @@ -133,6 +133,37 @@ entities: } } +// TestRunStructural_UnsupportedVersionReturnsEntityScopes locks down the named +// return runStructural hands back on the unsupported-version early-return +// path (lint.go:195). That line used to declare a new, block-scoped +// entityScopes with `:=`, shadowing the function's named return instead of +// assigning it; the explicit `return false, entityScopes` right after still +// returned the correct (shadowed) value, so the bug was latent, not live — +// but it would have gone silently wrong the moment that return became a bare +// `return`, which is exactly the idiom named returns invite. Calling +// runStructural directly (white-box, same package) is the only way to +// observe this return value: Run() never reaches this path with structuralOK +// true, so nothing downstream currently consumes it. +func TestRunStructural_UnsupportedVersionReturnsEntityScopes(t *testing.T) { + src := ` +kind: DomainModel +version: v99 +entities: + Ticket: + definition: A parking ticket. + subtypeOf: payments.Invoice +` + res := &Result{} + ok, entityScopes := runStructural([]byte(src), res) + if ok { + t.Fatal("expected ok=false for an unsupported version") + } + want := map[string]bool{"payments": true} + if len(entityScopes) != len(want) || !entityScopes["payments"] { + t.Fatalf("expected entityScopes %+v from the entity-position reference, got %+v", want, entityScopes) + } +} + func TestUndefinedRelationshipTargetIsError(t *testing.T) { src := ` kind: DomainModel From e93ab27d969d868508bb36f41f9fe4c94630a5b6 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sun, 26 Jul 2026 17:32:19 -0700 Subject: [PATCH 22/22] fix(render): don't relativize an absolute import path importLinkTarget joined an import path against sourceDir without checking filepath.IsAbs, so an absolute import path (a lint error, but render only runs structural validation, so still reachable) produced two different link shapes for the same model: unchanged under the default output location, silently re-rooted under sourceDir when -o pointed elsewhere. Return the rendered path unchanged when it's absolute, alongside the existing outDir == sourceDir shortcut. Added TestRenderImports_AbsolutePathIsNotRelativized, confirmed to fail against the pre-fix code (reverted the guard, verified the sibling-output-dir case relativizes to a bogus ../models/etc/... link, then restored). Round 3 finding: .scratch/reviews/r3-render.md Signed-off-by: Joe Beda --- internal/render/markdown/markdown.go | 9 ++++- internal/render/markdown/markdown_test.go | 46 +++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/internal/render/markdown/markdown.go b/internal/render/markdown/markdown.go index cb89ce0..1cb787f 100644 --- a/internal/render/markdown/markdown.go +++ b/internal/render/markdown/markdown.go @@ -132,9 +132,16 @@ func importAnchors(m *model.Model, sourceDir, outDir string) map[string]string { // already correctly relative to outDir, so it is returned unchanged rather // than round-tripped through Join/Rel, which would needlessly normalize away // a leading "./". +// +// An absolute impPath is a lint error (imports must be relative), but render +// only runs structural validation, so one can still reach here. Joining it +// against sourceDir would silently re-root it as if it were relative, +// producing a different, misleading link under -o than the one produced with +// no -o. Returning it unchanged keeps both cases consistent with each other +// and matches what the model actually declared. func importLinkTarget(impPath, sourceDir, outDir string) string { rendered := model.RenderedPath(impPath) - if outDir == sourceDir { + if outDir == sourceDir || filepath.IsAbs(rendered) { return rendered } abs := filepath.Join(sourceDir, filepath.FromSlash(rendered)) diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index 3d0d587..f646bc7 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -278,6 +278,52 @@ func TestRenderImports_LinkRelativeToOutputDir(t *testing.T) { } } +// TestRenderImports_AbsolutePathIsNotRelativized pins that an absolute import +// path is rendered unchanged rather than joined against sourceDir. An +// absolute path is a lint error (imports must be relative), but render only +// runs structural validation, so this is reachable. Before the fix, +// importLinkTarget joined it against sourceDir whenever outDir differed from +// sourceDir, silently re-rooting it under sourceDir and producing a +// different, misleading link than the same model rendered with no -o. +func TestRenderImports_AbsolutePathIsNotRelativized(t *testing.T) { + t.Parallel() + + m := &model.Model{ + Imports: []model.Import{ + {Scope: "payments", Path: "/etc/payments.modelith.yaml"}, + }, + Entities: map[string]model.Entity{ + "Ticket": { + Definition: "A parking ticket.", + Attributes: []model.Attribute{ + {Name: "paidWith", Type: "payments.PaymentMethod"}, + }, + }, + }, + } + + for _, tc := range []struct { + name string + sourceDir, outDir string + }{ + {name: "default, no -o", sourceDir: "/repo/models", outDir: "/repo/models"}, + {name: "-o into a sibling directory", sourceDir: "/repo/models", outDir: "/repo/out"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := Render(m, tc.sourceDir, tc.outDir) + for _, want := range []string{ + "- **`payments`** — `/etc/payments.modelith.yaml` ([rendered](/etc/payments.modelith.md))\n", + "[payments.PaymentMethod](/etc/payments.modelith.md#paymentmethod)", + } { + if !strings.Contains(got, want) { + t.Errorf("expected %q in:\n%s", want, got) + } + } + }) + } +} + // TestRenderImports_HostilePathCannotEscapeItsMarkup pins that an import path — // author-supplied text that reaches a published page — is escaped for the exact // position it lands in. A path is not validated by the schema beyond a minimum