From d47f91738a67942fe931abb68d9648163265863a Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 12:21:10 -0700 Subject: [PATCH 01/13] refactor(model): re-derive an import's derived scope instead of recording it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Import carried a ScopeFromPath bool marked json:"-", the only field in the model to opt out of TestSchemaStructSync. It recorded syntax provenance — whether the scope came from the filename or was written out — inside the data model, and its zero value meant "already validated", so anything constructing an Import in Go would have failed open. Nothing needs the flag. The imports layer runs only after structural validation passes, and the schema holds an explicitly written scope to the same pattern the linter checks, so a scope that fails the linter's check can only have been derived from a filename. Testing the scope alone is therefore equivalent, and the advice that follows — name the scope explicitly — still lands on the case it was written for. Adds the complement of the existing unusable-slug case: the same filename rescued by the explicit form, which pins that a written scope is not held to what its filename would have yielded. Signed-off-by: Joe Beda --- internal/lint/imports.go | 8 +++++--- internal/lint/imports_test.go | 10 ++++++++++ internal/model/model.go | 7 +------ internal/model/model_test.go | 2 +- internal/render/markdown/markdown_test.go | 6 +++--- 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/internal/lint/imports.go b/internal/lint/imports.go index a4f2d5e..54c4a49 100644 --- a/internal/lint/imports.go +++ b/internal/lint/imports.go @@ -122,9 +122,11 @@ func loadImports(modelPath string, m *model.Model, files Files, res *Result) (by // 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) + // reference to nothing. A written scope is held to this same pattern by + // the schema, which runs before this layer and gates it, so a scope that + // fails here was derived from the filename — which is what the advice + // below assumes. TestInvariant_ScopeSlugMatchesSchema guards the two. + scopeOK := 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", diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index 4802b4f..6773740 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -142,6 +142,7 @@ func TestImports_Resolution(t *testing.T) { "docs/shipping.modelith.yml": paymentsModel, "payments/payments.modelith.yaml": paymentsModel, "docs/legacy/pay-v2.modelith.yaml": paymentsModel, + "docs/PayMents.modelith.yaml": paymentsModel, "docs/not-a-model.yaml": "kind: SomethingElse\nversion: v1\n", } @@ -233,6 +234,15 @@ func TestImports_Resolution(t *testing.T) { imports: []string{"{scope: billing, path: ./legacy/pay-v2.modelith.yaml}"}, attrType: "billing.PaymentMethod", }, + { + // The complement of "bare import whose filename is not a usable + // slug": the same file, rescued by the explicit form. A written + // scope is validated by the schema, so the slug the filename would + // have yielded is never consulted. + name: "explicit scope rescues a filename that is not a usable slug", + imports: []string{"{scope: payments, path: ./PayMents.modelith.yaml}"}, + attrType: "payments.PaymentMethod", + }, { name: "two imports binding the same scope", imports: []string{`"./payments.modelith.yaml"`, `"../payments/payments.modelith.yaml"`}, diff --git a/internal/model/model.go b/internal/model/model.go index 8efc5c1..62189af 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -57,11 +57,6 @@ const ScopeSlug = `[a-z][a-z0-9-]*` 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", @@ -77,7 +72,7 @@ func (i *Import) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(trimmed, &s); err != nil { return err } - *i = Import{Scope: ScopeFromPath(s), Path: s, ScopeFromPath: true} + *i = Import{Scope: ScopeFromPath(s), Path: s} return nil } type rawImport Import diff --git a/internal/model/model_test.go b/internal/model/model_test.go index 5308147..9e51b12 100644 --- a/internal/model/model_test.go +++ b/internal/model/model_test.go @@ -185,7 +185,7 @@ entities: t.Fatalf("unexpected error: %v", err) } want := []Import{ - {Scope: "payments", Path: "./payments.modelith.yaml", ScopeFromPath: true}, + {Scope: "payments", Path: "./payments.modelith.yaml"}, {Scope: "billing", Path: "./legacy/pay-v2.modelith.yaml"}, } if len(m.Imports) != len(want) { diff --git a/internal/render/markdown/markdown_test.go b/internal/render/markdown/markdown_test.go index 8f4ebb2..a2a2bd0 100644 --- a/internal/render/markdown/markdown_test.go +++ b/internal/render/markdown/markdown_test.go @@ -337,7 +337,7 @@ func TestRenderImports_SectionAndLinkedTypes(t *testing.T) { m := &model.Model{ Imports: []model.Import{ - {Scope: "payments", Path: "../payments/payments.modelith.yaml", ScopeFromPath: true}, + {Scope: "payments", Path: "../payments/payments.modelith.yaml"}, {Scope: "billing", Path: "./legacy/pay-v2.modelith.yaml"}, }, Entities: map[string]model.Entity{ @@ -392,7 +392,7 @@ func TestRenderImports_LinkRelativeToOutputDir(t *testing.T) { m := &model.Model{ Imports: []model.Import{ - {Scope: "payments", Path: "../payments/payments.modelith.yaml", ScopeFromPath: true}, + {Scope: "payments", Path: "../payments/payments.modelith.yaml"}, {Scope: "billing", Path: "./legacy/pay-v2.modelith.yaml"}, }, Entities: map[string]model.Entity{ @@ -507,7 +507,7 @@ func TestRenderImports_HostilePathCannotEscapeItsMarkup(t *testing.T) { {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: "line\nbreak", Path: "./a\n## Injected\n\nb.modelith.yaml"}, {Scope: "ticked", Path: "./a`b``c.modelith.yaml"}, }, Entities: map[string]model.Entity{ From fca8e6efb705d2a6ce54ecce9cfe0aaccb020f0f Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 12:41:15 -0700 Subject: [PATCH 02/13] docs(adr): vendoring is a whole-file copy, verified by a content digest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the decisions taken in the slice-2 design interview, superseding ADR-0010's Digest section and leaving the rest of it standing. The surface digest goes. It required a canonicalisation pass and imposed a permanent tax — every future schema field classified surface-or-documentation, forever, or the digest silently drifts — and it loses on its main job: for verify-on-lint the realistic accidental edit is a teammate fixing a typo in somebody else's model, which a surface digest deliberately ignores. A whole-file digest catches strictly more and costs nothing to maintain. Also records the header's shape and its single parse-and-strip rule, that a header suppresses the completeness category and its own imports' errors and nothing else, that vendoring does not recurse, that gh is the only transport and another host asks for an issue, and that the trust warning prints rather than blocks. Signed-off-by: Joe Beda --- .../0015-vendoring-is-a-whole-file-copy.md | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 project-docs/adr/0015-vendoring-is-a-whole-file-copy.md diff --git a/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md b/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md new file mode 100644 index 0000000..bb0ffe9 --- /dev/null +++ b/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md @@ -0,0 +1,145 @@ +# Vendoring is a whole-file copy, verified by a content digest + +A vendored model is fetched whole, stamped with a provenance header comment, +and verified offline against a SHA-256 of its own bytes. Vendoring does not +recurse, the fetch is `gh`-only, and the trust warning prints rather than +blocks. Supersedes ADR-0010's **Digest** section; the rest of ADR-0010 stands. + +## Context + +ADR-0010 specified vendoring in full before any of it was built. Implementing +it, three of its answers turned out to cost more than the problem they solve, +measured against the threat model ADR-0014 settled: a model author already has +commit access and could edit the generated `.md` directly, so there is no +privilege boundary, and vendoring is expected to happen between projects that +already trust each other. The remaining gaps are correctness bugs, not +vulnerabilities, and the agreed answer to the injection risk is to *say so* at +the point of import rather than to armour the renderer further. + +## Decision + +**A content digest, not a surface digest.** The header carries a SHA-256 over +the file's bytes with every `# modelith-: ` line removed. ADR-0010's +surface digest — structure and normative content, excluding `description`, +`definition`, `note`, `derivation`, scenario steps, `title`, and key order — +required a canonicalisation pass and imposed a permanent tax: every future +schema field would have to be classified surface-or-documentation, forever, or +the digest silently drifts. Checked against its two consumers, that buys very +little. For verify-on-lint, which is the job that must work offline with no +network, a whole-file digest is *strictly better*: the realistic accidental +edit is a teammate fixing a typo in someone else's model, which a surface +digest deliberately ignores. It loses only to `deps check`, where a +documentation-only upstream change will now report as a change — and `check` +has both files in hand and diffs them to report what moved, so a human judges. +A `surface:` key can be added later, with a real complaint driving it. + +The digest is not redundant with `commit`. `commit` says what upstream had; +the digest says whether the bytes here still match what was fetched, which is +a question `lint` has to answer without a network. + +**The header.** One namespaced key per comment line, mirroring the +`# yaml-language-server:` line already in every model: + +```yaml +# modelith-vendored: DO NOT EDIT — this file is a copy. Change it at its origin. +# modelith-fetch: git +# modelith-origin: https://github.com/stacklok/some-repo +# modelith-path: docs/payments.modelith.yaml +# modelith-ref: main +# modelith-commit: 4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21 +# modelith-imported: 2026-07-27 +# modelith-digest: sha256:9a1f... +``` + +Parsing and stripping are the *same* rule — lines matching +`^# modelith-: ` — so the two cannot disagree, which matters because the +digest is defined by the strip. Such a line below the leading comment block is +an error. The `vendored` key carries the DO NOT EDIT banner as its value: +nothing enforces it, but an agent reading the file will pause on it, and +making the banner a key rather than free text keeps the single strip rule. + +`fetch:` declares the method and scopes which other keys apply — `git` uses +`origin`, `path`, `ref`, and `commit`; a future `https` method would carry +`origin` alone and lean on the digest as its only integrity anchor. An unknown +`modelith-*` key or an unknown `fetch:` value is an error rather than being +ignored, which is the right posture pre-release even though it means an older +binary rejects a newer header. + +**What the header suppresses.** A provenance header suppresses the +completeness category and import-resolution errors, and nothing else. +Structural and semantic checks still run: a vendored file that is not valid +modelith breaks *this* repo's build and is this repo's problem. ADR-0010's +list of ownership-scoped diagnostics — no invariants, no scenario exercises an +entity, unused glossary term, unused enum — is exactly `CategoryCompleteness`, +so the rule needs no new classification. Semantic *warnings* about someone +else's authoring stay: they never fail a build, and suppressing them would +require a second, fuzzier judgement about which semantic checks are +ownership-scoped. + +Suppressing an unresolvable import must suppress its downstream consequence +too. A vendored file's own `imports:` point at paths in its home repo; if the +references into those scopes still errored, every vendored file that has +imports would be broken here and the suppression would be worthless. + +A digest mismatch is an error, not a warning: the file lies about its own +provenance, and both remedies are cheap and honest — re-run `deps update`, or +delete the header, which converts it to a canonical local file and is an +accurate description of having forked it. The diagnostic names both. + +**Vendoring does not recurse.** ADR-0010 already made *resolution* +non-transitive: `scope.Name` reaches only items defined directly in the file +bound to that scope. Fetching stays non-transitive to match. Referencing an +item that a vendored model merely uses rather than defines is a lint error +whose remedy is to vendor that model directly and bind it to its own scope, +and `deps import` says at fetch time when the file it fetched declares imports +of its own. + +The closure was considered and rejected on cost. It requires a vendor +directory layout, rewriting import paths on fetch — which breaks the +unmodified-copy invariant that both the digest and a clean `deps update` +overwrite rest on — a dedupe policy for diamond dependencies, and an answer to +a conflict ADR-0012 makes unavoidable: the *importer* binds the scope, so a +transitively vendored file can arrive bound to one slug by its parent and +another by this repo. Resolving that needs a lock file and an alias layer, +both of which ADR-0010 rejected. It is also consistent with the reason +vendoring is whole-file: referencing something new from an already-imported +model is a local edit, and referencing something from a *different* model is +another explicit import. + +**`gh` is the only transport.** `gh` already solves authentication for the +private and internal repositories the motivating models require, and it +resolves content and commit in two API calls. A `git` clone path would work +with any remote but is a second code path with sparse-checkout handling for a +case with no live instance, which is the bar ADR-0007 sets. A non-GitHub +origin is an error that asks the user to file an issue, so the first real user +becomes the signal to build it. `fetch: git` names what the origin *is* — a +file at a path in a git repo at a ref — not which binary fetched it, so a +`git` fallback or an `https` method slots in without a header migration. + +**The warning prints; it does not block.** ADR-0014 requires `deps import` and +`deps update` to warn that a vendored model is untrusted content that will be +rendered into published Markdown. An interactive confirmation is the wrong +shape: an agent usually drives these commands, so a prompt is either +auto-answered theatre or a wedged non-interactive CI run. Warn-and-proceed +means the fetch has happened by the time the warning is read, which is +acceptable because the file is inert until it is added to `imports:` — and +`deps import` deliberately does not edit `imports:`, so that second, manual +step is the real gate. + +## Consequences + +- `deps import` writes a file and prints the `imports:` snippet to paste. + Rewriting a user's YAML in place would cost comment and formatting + fidelity and would force the command to answer which model it is importing + *into* when a repo holds several. +- No schema change. `imports:` already exists and a header comment is not + schema, so `TestSchemaStructSync` and the schema-URL consistency tests are + not in play. +- Pinned by `TestADR_0015_*`: the digest covers file bytes minus the header + lines, a header suppresses completeness and nothing else, an unknown + `fetch:` value is an error, and `deps import` fetches no transitive imports. +- An unbalanced code fence in a vendored model's prose still corrupts the + importing repo's rendered Markdown (issue #39). Vendoring strengthens the + case for that rule, since it is where someone else's prose lands in your + published document, but it shares no code with this change and is a visibly + broken document rather than a privilege boundary. From 9f3941a91d6d2e63d063d67360efdf00ed2127f7 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 12:25:20 -0700 Subject: [PATCH 03/13] feat(provenance): read and write the header that marks a vendored copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds internal/provenance, the package that owns the "# modelith-: " comment block a vendored model carries: parse, format, stamp, strip, and the digest. Parsing and stripping are one rule, deliberately. The digest is defined as SHA-256 over the file with its provenance lines removed, so if the two rules could disagree about which lines are header, the digest would cover bytes the parser never saw. A line counts only at column zero for the same reason: an indented "# modelith-" comment is an ordinary comment to both. Presence, not a clean parse, is what makes a file vendored. A header with a typo in it still means the file is somebody else's copy, and treating it as this repo's own work would bury the typo under a flood of completeness gaps about a document nobody here controls. fetch: names the method and scopes the keys it requires, so a later method declares its own set rather than inheriting git's. An unknown method points at the issue tracker: the first real user is the signal to implement it. The digest test builds its expectation by hand — the header lines written out and removed by the test, then hashed with crypto/sha256 directly — rather than by calling Strip. A test that reuses the implementation's own helper agrees with it however wrong it is, which is how two HIGH bugs got past CI in the renderer work. internal/provenance joins the ADR-0011 offline set: lint imports it, so it can never reach for the network. Signed-off-by: Joe Beda --- internal/model/network_test.go | 1 + internal/provenance/provenance.go | 277 +++++++++++++++++++++ internal/provenance/provenance_test.go | 319 +++++++++++++++++++++++++ 3 files changed, 597 insertions(+) create mode 100644 internal/provenance/provenance.go create mode 100644 internal/provenance/provenance_test.go diff --git a/internal/model/network_test.go b/internal/model/network_test.go index 104a111..4645a5d 100644 --- a/internal/model/network_test.go +++ b/internal/model/network_test.go @@ -28,6 +28,7 @@ func TestADR_0011_OfflinePackages(t *testing.T) { "github.com/stacklok/modelith/internal/render/mermaid", "github.com/stacklok/modelith/internal/schema", "github.com/stacklok/modelith/internal/model", + "github.com/stacklok/modelith/internal/provenance", } // net/url and net/netip are parsers that perform no I/O; the JSON Schema diff --git a/internal/provenance/provenance.go b/internal/provenance/provenance.go new file mode 100644 index 0000000..082f4eb --- /dev/null +++ b/internal/provenance/provenance.go @@ -0,0 +1,277 @@ +// Package provenance reads and writes the comment header that marks a model +// file as a vendored copy of a model whose home is another repository. +package provenance + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "regexp" + "slices" + "strings" +) + +// LinePrefix begins every provenance line. +// +// A line counts only when it starts at column zero. An indented "# modelith-" +// comment is an ordinary comment: it is neither parsed nor stripped, so the +// bytes Parse reads and the bytes Digest covers can never disagree about which +// lines are header. That single rule is the whole contract — the digest is +// defined by the strip (ADR-0015). +const LinePrefix = "# modelith-" + +// lineRE matches one provenance line and splits it into key and value. The +// value is everything after the colon, trimmed; a free-text value such as the +// vendored banner is deliberately unconstrained. +var lineRE = regexp.MustCompile(`^# modelith-([a-z][a-z0-9-]*):(.*)$`) + +// digestRE is the recorded digest's shape. +var digestRE = regexp.MustCompile(`^sha256:[0-9a-f]{64}$`) + +// Banner is the value deps import writes for the vendored key. Nothing enforces +// it — a digest computed over the file cannot cover the header that records it — +// but an agent editing the file reads it and stops, which is the point. +const Banner = "DO NOT EDIT — this file is a copy. Change it at its origin." + +// Header is the provenance block of a vendored model file. +type Header struct { + Vendored string + Fetch string + Origin string + Path string + Ref string + Commit string + Imported string + Digest string +} + +// keyOrder is the order Format writes the keys in, and the set of keys that +// exist at all: a line naming anything else is a Problem. +var keyOrder = []string{"vendored", "fetch", "origin", "path", "ref", "commit", "imported", "digest"} + +// commonKeys are required whatever the fetch method is. methodKeys are the ones +// each method requires on top, so adding a method means declaring what it +// carries rather than inheriting git's key set by default. +var ( + commonKeys = []string{"vendored", "fetch", "imported", "digest"} + methodKeys = map[string][]string{ + "git": {"origin", "path", "ref", "commit"}, + } +) + +// Methods returns the fetch methods this build understands. +func Methods() []string { + // One entry today; sorted so a diagnostic listing them is deterministic + // when there are more. + out := make([]string, 0, len(methodKeys)) + for m := range methodKeys { + out = append(out, m) + } + slices.Sort(out) + return out +} + +func (h *Header) field(key string) *string { + switch key { + case "vendored": + return &h.Vendored + case "fetch": + return &h.Fetch + case "origin": + return &h.Origin + case "path": + return &h.Path + case "ref": + return &h.Ref + case "commit": + return &h.Commit + case "imported": + return &h.Imported + case "digest": + return &h.Digest + } + return nil +} + +// Problem is a defect in the header itself — an unknown key, a missing required +// one, a line outside the leading comment block. It carries the 1-based line so +// a diagnostic can point at it; Line is zero for a problem about the header as a +// whole rather than about one line. +type Problem struct { + Line int + Message string +} + +// Present reports whether src carries a provenance line at all. +// +// This, not a successful Parse, is what makes a file vendored. A header with a +// typo in it still means the file is somebody else's copy, and treating it as +// this repo's own work would bury the typo under a flood of completeness gaps +// about a document nobody here controls. +func Present(src []byte) bool { + for _, line := range strings.Split(string(src), "\n") { + if lineRE.MatchString(line) { + return true + } + } + return false +} + +// Parse reads the provenance header from src. It returns a nil Header when src +// carries no provenance line at all, which is the ordinary case for a model +// this repo owns. Otherwise it returns what it could read plus every defect it +// found; callers report the defects and may still use the header. +func Parse(src []byte) (*Header, []Problem) { + if !Present(src) { + return nil, nil + } + var ( + h Header + problems []Problem + seen = map[string]int{} + ) + lines := strings.Split(string(src), "\n") + lead := leadingCommentBlock(lines) + for i, line := range lines { + m := lineRE.FindStringSubmatch(line) + if m == nil { + continue + } + num, key, value := i+1, m[1], strings.TrimSpace(m[2]) + if i >= lead { + problems = append(problems, Problem{num, fmt.Sprintf( + "provenance line %q is below the leading comment block — the whole header belongs at the top of the file, before any model content", + LinePrefix+key)}) + continue + } + field := h.field(key) + if field == nil { + problems = append(problems, Problem{num, fmt.Sprintf( + "unknown provenance key %q — this modelith knows %s", + LinePrefix+key, quotedList(keyOrder))}) + continue + } + if prev, dup := seen[key]; dup { + problems = append(problems, Problem{num, fmt.Sprintf( + "provenance key %q appears twice, on lines %d and %d", LinePrefix+key, prev, num)}) + continue + } + seen[key] = num + *field = value + } + + problems = append(problems, h.validate(seen)...) + return &h, problems +} + +// validate checks the key set against the fetch method and the shape of the +// values that have one. seen maps a key to the line it was read from, so a +// problem about a value points at it. +func (h *Header) validate(seen map[string]int) []Problem { + var problems []Problem + + required := append([]string{}, commonKeys...) + method, known := methodKeys[h.Fetch] + switch { + case h.Fetch == "": + // The missing-key report below names it; naming a method that is not + // there as unknown too would report one mistake twice. + case !known: + problems = append(problems, Problem{seen["fetch"], fmt.Sprintf( + "unknown fetch method %q — this modelith supports %s. If you need another, please open an issue at https://github.com/stacklok/modelith/issues", + h.Fetch, quotedList(Methods()))}) + default: + required = append(required, method...) + } + + for _, key := range required { + if *h.field(key) == "" { + problems = append(problems, Problem{0, fmt.Sprintf( + "provenance header is missing %q", LinePrefix+key)}) + } + } + if h.Digest != "" && !digestRE.MatchString(h.Digest) { + problems = append(problems, Problem{seen["digest"], fmt.Sprintf( + "provenance digest %q is not in the form sha256:<64 hex digits>", h.Digest)}) + } + return problems +} + +// Verify reports whether src still hashes to the digest its own header records, +// and returns the digest src actually has. +func (h *Header) Verify(src []byte) (ok bool, got string) { + got = Digest(src) + return got == h.Digest, got +} + +// Strip returns src with every provenance line removed, which is the content +// the digest covers. +func Strip(src []byte) []byte { + lines := strings.Split(string(src), "\n") + kept := lines[:0] + for _, line := range lines { + if lineRE.MatchString(line) { + continue + } + kept = append(kept, line) + } + return []byte(strings.Join(kept, "\n")) +} + +// Digest returns the digest of src as a header records it: SHA-256 over src +// with its provenance lines removed, so stamping a header does not change the +// digest of the file it is stamped into. +func Digest(src []byte) string { + sum := sha256.Sum256(Strip(src)) + return "sha256:" + hex.EncodeToString(sum[:]) +} + +// Format renders h as the header block, every line terminated. +func (h *Header) Format() string { + var b strings.Builder + for _, key := range keyOrder { + if v := *h.field(key); v != "" { + fmt.Fprintf(&b, "%s%s: %s\n", LinePrefix, key, v) + } + } + return b.String() +} + +// Stamp returns src with h's header inserted: after a leading +// "# yaml-language-server:" line when src opens with one, so the editor +// directive stays first, and at the top otherwise. +func Stamp(src []byte, h *Header) []byte { + lines := strings.Split(string(src), "\n") + at := 0 + for i := 0; i < leadingCommentBlock(lines); i++ { + if strings.HasPrefix(lines[i], "# yaml-language-server:") { + at = i + 1 + } + } + head := strings.Join(lines[:at], "\n") + if at > 0 { + head += "\n" + } + return []byte(head + h.Format() + strings.Join(lines[at:], "\n")) +} + +// leadingCommentBlock returns the number of lines in the run at the top of the +// file that are blank or comments — where the header belongs. +func leadingCommentBlock(lines []string) int { + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + return i + } + return len(lines) +} + +func quotedList(items []string) string { + quoted := make([]string, len(items)) + for i, s := range items { + quoted[i] = fmt.Sprintf("%q", s) + } + return strings.Join(quoted, ", ") +} diff --git a/internal/provenance/provenance_test.go b/internal/provenance/provenance_test.go new file mode 100644 index 0000000..6ffb2cc --- /dev/null +++ b/internal/provenance/provenance_test.go @@ -0,0 +1,319 @@ +package provenance + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + "testing" +) + +// vendored is a stamped file: the editor directive, then the header, then a +// model. Tests that need the unstamped content use plain. +const vendored = `# yaml-language-server: $schema=https://modelith.sh/schema/domain-model/v1.json +# modelith-vendored: DO NOT EDIT — this file is a copy. Change it at its origin. +# modelith-fetch: git +# modelith-origin: https://github.com/stacklok/some-repo +# modelith-path: docs/payments.modelith.yaml +# modelith-ref: main +# modelith-commit: 4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21 +# modelith-imported: 2026-07-27 +# modelith-digest: sha256:0000000000000000000000000000000000000000000000000000000000000000 +kind: DomainModel +version: v1 +title: Payments +` + +const plain = `# yaml-language-server: $schema=https://modelith.sh/schema/domain-model/v1.json +kind: DomainModel +version: v1 +title: Payments +` + +func TestPresent(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + src string + want bool + }{ + {"a stamped file", vendored, true}, + {"a model this repo owns", plain, false}, + {"an ordinary comment that only looks like one", "#modelith-fetch: git\nkind: DomainModel\n", false}, + {"an indented provenance line is an ordinary comment", " # modelith-fetch: git\nkind: DomainModel\n", false}, + {"a broken header is still a header", "# modelith-nonsense: x\nkind: DomainModel\n", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := Present([]byte(tc.src)); got != tc.want { + t.Errorf("Present() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestParse_Valid(t *testing.T) { + t.Parallel() + + h, problems := Parse([]byte(vendored)) + if len(problems) != 0 { + t.Fatalf("unexpected problems: %+v", problems) + } + want := Header{ + Vendored: Banner, + Fetch: "git", + Origin: "https://github.com/stacklok/some-repo", + Path: "docs/payments.modelith.yaml", + Ref: "main", + Commit: "4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21", + Imported: "2026-07-27", + Digest: "sha256:" + strings.Repeat("0", 64), + } + if *h != want { + t.Errorf("Parse() = %+v, want %+v", *h, want) + } +} + +func TestParse_NoHeader(t *testing.T) { + t.Parallel() + + h, problems := Parse([]byte(plain)) + if h != nil || problems != nil { + t.Errorf("Parse() = %+v, %+v; want nil, nil", h, problems) + } +} + +func TestParse_Problems(t *testing.T) { + t.Parallel() + + // header builds a stamped file with the named lines replaced or dropped, so + // each case differs from a valid header in exactly one way. + header := func(edits map[string]string) string { + var b strings.Builder + b.WriteString("# yaml-language-server: $schema=x\n") + for _, key := range keyOrder { + value := map[string]string{ + "vendored": Banner, + "fetch": "git", + "origin": "https://github.com/stacklok/some-repo", + "path": "docs/payments.modelith.yaml", + "ref": "main", + "commit": "4f2c1e9", + "imported": "2026-07-27", + "digest": "sha256:" + strings.Repeat("0", 64), + }[key] + if edit, ok := edits[key]; ok { + value = edit + } + if value == "" { + continue + } + b.WriteString(LinePrefix + key + ": " + value + "\n") + } + b.WriteString("kind: DomainModel\n") + return b.String() + } + + cases := []struct { + name string + src string + wantLine int + contains string + }{ + { + name: "an unknown key names the ones that exist", + src: "# modelith-surface: sha256:x\n" + plain, + wantLine: 1, + contains: `unknown provenance key "# modelith-surface"`, + }, + { + name: "a duplicate key names both lines", + src: "# modelith-ref: main\n# modelith-ref: other\n" + plain, + wantLine: 2, + contains: `appears twice, on lines 1 and 2`, + }, + { + name: "a missing required key is reported without a line", + src: header(map[string]string{"commit": ""}), + wantLine: 0, + contains: `missing "# modelith-commit"`, + }, + { + name: "an unknown fetch method points at the issue tracker", + src: header(map[string]string{"fetch": "carrier-pigeon"}), + wantLine: 3, + contains: `unknown fetch method "carrier-pigeon"`, + }, + { + name: "a malformed digest names the shape", + src: header(map[string]string{"digest": "sha256:nope"}), + wantLine: 9, + contains: "sha256:<64 hex digits>", + }, + { + name: "a provenance line below the model content is misplaced", + src: plain + "# modelith-fetch: git\n", + wantLine: 5, + contains: "below the leading comment block", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, problems := Parse([]byte(tc.src)) + var found *Problem + for i := range problems { + if strings.Contains(problems[i].Message, tc.contains) { + found = &problems[i] + break + } + } + if found == nil { + t.Fatalf("no problem containing %q; got %+v", tc.contains, problems) + } + if found.Line != tc.wantLine { + t.Errorf("problem on line %d, want %d: %s", found.Line, tc.wantLine, found.Message) + } + }) + } +} + +// TestDigest_CoversTheFileWithoutItsHeader pins ADR-0015's digest definition. +// +// The expectation is built by hand — the header lines written out and removed +// by the test itself, then hashed with crypto/sha256 directly — rather than by +// calling Strip. A test that reuses the implementation's own helper would agree +// with it however wrong the helper was, which is exactly how two HIGH bugs got +// past CI in the renderer work. +func TestDigest_CoversTheFileWithoutItsHeader(t *testing.T) { + t.Parallel() + + sum := sha256.Sum256([]byte(plain)) + want := "sha256:" + hex.EncodeToString(sum[:]) + + if got := Digest([]byte(plain)); got != want { + t.Errorf("Digest(plain) = %s, want %s", got, want) + } + if got := Digest([]byte(vendored)); got != want { + t.Errorf("Digest(vendored) = %s, want %s — the header must not change the digest of the file it is stamped into", got, want) + } +} + +func TestDigest_ChangesWithTheContent(t *testing.T) { + t.Parallel() + + edited := strings.Replace(vendored, "title: Payments", "title: Payment", 1) + if Digest([]byte(edited)) == Digest([]byte(vendored)) { + t.Error("editing the model left the digest unchanged") + } +} + +// TestDigest_IgnoresEveryHeaderChange pins that the digest is blind to the +// header and to nothing else: rewriting each key in turn must not move it, +// because the header records a value derived from the rest of the file. +func TestDigest_IgnoresEveryHeaderChange(t *testing.T) { + t.Parallel() + + base := Digest([]byte(vendored)) + for _, key := range keyOrder { + src := strings.Replace(vendored, LinePrefix+key+": ", LinePrefix+key+": changed-", 1) + if src == vendored { + t.Fatalf("test did not rewrite %q", key) + } + if got := Digest([]byte(src)); got != base { + t.Errorf("rewriting %q moved the digest to %s", key, got) + } + } +} + +func TestVerify(t *testing.T) { + t.Parallel() + + h := &Header{Digest: Digest([]byte(plain))} + if ok, _ := h.Verify([]byte(vendored)); !ok { + t.Error("a stamped copy of the same content did not verify") + } + edited := strings.Replace(vendored, "title: Payments", "title: Payment", 1) + ok, got := h.Verify([]byte(edited)) + if ok { + t.Error("an edited copy verified") + } + if got == h.Digest { + t.Error("Verify reported the recorded digest as the one it computed") + } +} + +func TestStamp(t *testing.T) { + t.Parallel() + + h := &Header{ + Vendored: Banner, + Fetch: "git", + Origin: "https://github.com/stacklok/some-repo", + Path: "docs/payments.modelith.yaml", + Ref: "main", + Commit: "4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21", + Imported: "2026-07-27", + Digest: Digest([]byte(plain)), + } + + t.Run("below a yaml-language-server line", func(t *testing.T) { + t.Parallel() + got := string(Stamp([]byte(plain), h)) + lines := strings.Split(got, "\n") + if !strings.HasPrefix(lines[0], "# yaml-language-server:") { + t.Errorf("first line is %q, want the editor directive", lines[0]) + } + if !strings.HasPrefix(lines[1], LinePrefix+"vendored:") { + t.Errorf("second line is %q, want the vendored banner", lines[1]) + } + if !strings.Contains(got, "kind: DomainModel") { + t.Error("the model content did not survive stamping") + } + }) + + t.Run("at the top when there is no directive", func(t *testing.T) { + t.Parallel() + src := "kind: DomainModel\nversion: v1\n" + got := string(Stamp([]byte(src), h)) + if !strings.HasPrefix(got, LinePrefix+"vendored:") { + t.Errorf("stamped file starts %q", got[:40]) + } + if !strings.HasSuffix(got, src) { + t.Error("the model content did not survive stamping unchanged") + } + }) + + t.Run("a stamped file parses back to the header", func(t *testing.T) { + t.Parallel() + back, problems := Parse(Stamp([]byte(plain), h)) + if len(problems) != 0 { + t.Fatalf("stamping produced a header with problems: %+v", problems) + } + if *back != *h { + t.Errorf("round trip gave %+v, want %+v", *back, *h) + } + }) + + t.Run("the stamped file verifies against its own digest", func(t *testing.T) { + t.Parallel() + stamped := Stamp([]byte(plain), h) + parsed, _ := Parse(stamped) + if ok, got := parsed.Verify(stamped); !ok { + t.Errorf("a freshly stamped file does not verify: recorded %s, computed %s", parsed.Digest, got) + } + }) +} + +func TestStrip(t *testing.T) { + t.Parallel() + + if got := string(Strip([]byte(vendored))); got != plain { + t.Errorf("Strip() = %q, want %q", got, plain) + } + if got := string(Strip([]byte(plain))); got != plain { + t.Errorf("Strip() changed a file with no header: %q", got) + } +} From b94183efc331eb7040053563b0e8c2ee640c670d Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 12:28:56 -0700 Subject: [PATCH 04/13] feat(lint): honour a provenance header, and suppress only what it owns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A file carrying a provenance header is a copy of a model whose home is another repository. Two classes of diagnostic stop applying to it, and no others. The completeness category goes: entities with no invariants, entities no scenario exercises, unused glossary terms, unused enums, and an import nothing references are all findings about a model its own authors control. ADR-0010's list of ownership-scoped diagnostics turns out to be exactly that category, so the rule needs no new classification. Without it the Action this repo ships — which lints every globbed file with --completeness — turns a user's CI red for gaps in somebody else's document. The errors its own imports list would raise go too. A vendored model's imports name paths in the repository it came from, which do not exist here. Resolution is still attempted, so two vendored peers that import each other still resolve; only the reporting stops. Every entry still claims its scope, which is what keeps a reference through an import that did not load from reading as a reference to nothing — without that second half, every vendored model with imports of its own is broken here and the suppression is worthless. Structural and semantic checks still run. A vendored file that is not a valid domain model breaks this repository's build and is this repository's problem. Presence of a header, not a clean parse of one, is what makes a file vendored: a typo in the header would otherwise be buried under gaps in a document nobody here owns. The header's own defects are reported, and the copy is verified against the digest the header records. A mismatch is an error naming both remedies — refresh it, or delete the header and own the fork — and it reconstructs the exact deps import command from the header's parts. TestVendored_SuppressesCompletenessAndNothingElse states the rule as a difference rather than a hand-written expectation: the same model is linted twice, and every finding that disappears must be a completeness finding. A suppression that was too broad cannot pass it. Signed-off-by: Joe Beda --- internal/lint/imports.go | 20 ++- internal/lint/lint.go | 13 +- internal/lint/provenance.go | 81 ++++++++++ internal/lint/provenance_test.go | 245 ++++++++++++++++++++++++++++++ internal/provenance/provenance.go | 5 + 5 files changed, 360 insertions(+), 4 deletions(-) create mode 100644 internal/lint/provenance.go create mode 100644 internal/lint/provenance_test.go diff --git a/internal/lint/imports.go b/internal/lint/imports.go index 54c4a49..5fec625 100644 --- a/internal/lint/imports.go +++ b/internal/lint/imports.go @@ -67,8 +67,11 @@ type importedModel struct { // 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) +// +// vendored says the model is a copy whose home is another repository, which +// silences the errors its imports list would raise here (see loadImports). +func runImports(modelPath string, m *model.Model, files Files, res *Result, entityScopes map[string]bool, vendored bool) { + byScope, claimed := loadImports(modelPath, m, files, res, vendored) 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 @@ -96,7 +99,15 @@ func runImports(modelPath string, m *model.Model, files Files, res *Result, enti // 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, files Files, res *Result) (byScope map[string]importedModel, claimed map[string]string) { +// +// A vendored model is the exception, and it reports nothing here at all. Its +// imports name paths in the repository it came from, which do not exist in this +// one, so every entry would fail for a reason its authors cannot see and this +// repository is not expected to fix. Resolution is still attempted, because two +// vendored peers that import each other do resolve here; and every entry still +// claims its scope, which is what keeps a reference into an import that did not +// load from reading as a reference to nothing (see checkQualifiedTypes). +func loadImports(modelPath string, m *model.Model, files Files, res *Result, vendored bool) (byScope map[string]importedModel, claimed map[string]string) { byScope = map[string]importedModel{} claimed = map[string]string{} if len(m.Imports) == 0 { @@ -106,6 +117,9 @@ func loadImports(modelPath string, m *model.Model, files Files, res *Result) (by root, inRepo := files.ResolutionRoot(modelPath) for i, imp := range m.Imports { reject := func(format string, args ...any) { + if vendored { + return + } res.Findings = append(res.Findings, Finding{ Severity: SeverityError, Category: CategorySemantic, diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 9b72935..f63e2d8 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -87,6 +87,11 @@ func Run(path string, src []byte, files Files) (*Result, error) { files = OSFiles{} } + // Whether this file is somebody else's copy is settled first: it decides + // which of the layers below are findings about a model this repository + // controls, and it holds even when the file does not parse. + vendored := runProvenance(src, res) + // Layer 1: structural validation against the JSON Schema. structuralOK, entityScopes := runStructural(src, res) @@ -105,6 +110,9 @@ func Run(path string, src []byte, files Files) (*Result, error) { Message: err.Error(), }) } + if vendored { + dropOwnedDiagnostics(res) + } sortFindings(res) return res, nil } @@ -117,7 +125,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, entityScopes) + runImports(path, m, files, res, entityScopes, vendored) } runRelationshipShape(m, res) runSubtypes(m, res) @@ -125,6 +133,9 @@ func Run(path string, src []byte, files Files) (*Result, error) { runPairing(m, res) runCompleteness(m, res) + if vendored { + dropOwnedDiagnostics(res) + } sortFindings(res) return res, nil } diff --git a/internal/lint/provenance.go b/internal/lint/provenance.go new file mode 100644 index 0000000..12c0593 --- /dev/null +++ b/internal/lint/provenance.go @@ -0,0 +1,81 @@ +package lint + +import ( + "fmt" + + "github.com/stacklok/modelith/internal/provenance" +) + +// runProvenance reports defects in the file's provenance header and verifies +// the copy against the digest that header records. It returns whether the file +// is vendored — a copy of a model whose home is another repository. +// +// A vendored file is not this repo's work, so the diagnostics that are findings +// about a model its authors control do not apply to it. What that suppresses is +// exactly two things, and no more (ADR-0015): the completeness category, in +// dropOwnedDiagnostics, and the errors its own imports list would raise, in +// loadImports. Structural and semantic checks still run: a vendored file that is +// not a valid domain model breaks this repo's build and is this repo's problem. +func runProvenance(src []byte, res *Result) bool { + h, problems := provenance.Parse(src) + if h == nil { + return false + } + for _, p := range problems { + msg := p.Message + if p.Line > 0 { + msg = fmt.Sprintf("line %d: %s", p.Line, msg) + } + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategorySemantic, + Path: "", + Message: "provenance header: " + msg, + }) + } + // A digest that is not in the recorded shape is already reported above. + // Comparing against it would report the same mistake a second time, as a + // mismatch it cannot help but produce. + if provenance.ValidDigest(h.Digest) { + if ok, got := h.Verify(src); !ok { + res.Findings = append(res.Findings, Finding{ + Severity: SeverityError, + Category: CategorySemantic, + Path: "", + Message: fmt.Sprintf( + "this vendored file no longer matches the digest its provenance header records (recorded %s, computed %s) — it has been edited since it was imported. Refresh it with `modelith deps import %s`, or delete the provenance header if the change is a deliberate fork, which makes this repository the file's home", + h.Digest, got, refreshTarget(h)), + }) + } + } + return true +} + +// refreshTarget is what to hand `modelith deps import` to replace this copy. For +// a git origin that is the blob URL the header's parts describe; for anything +// else the origin alone, which is all such a method records. +func refreshTarget(h *provenance.Header) string { + if h.Fetch == "git" && h.Origin != "" && h.Ref != "" && h.Path != "" { + return fmt.Sprintf("%s/blob/%s/%s", h.Origin, h.Ref, h.Path) + } + return h.Origin +} + +// dropOwnedDiagnostics removes the findings that are about a model's own +// authoring rather than about this repository's use of it. They are exactly the +// completeness category: entities with no invariants, entities no scenario +// exercises, unused glossary terms, unused enums, and an import nothing +// references. +// +// Without this the Action this repo ships — which lints every globbed file with +// --completeness — turns a user's CI red for gaps in somebody else's document. +func dropOwnedDiagnostics(res *Result) { + kept := res.Findings[:0] + for _, f := range res.Findings { + if f.Category == CategoryCompleteness { + continue + } + kept = append(kept, f) + } + res.Findings = kept +} diff --git a/internal/lint/provenance_test.go b/internal/lint/provenance_test.go new file mode 100644 index 0000000..f9c1a32 --- /dev/null +++ b/internal/lint/provenance_test.go @@ -0,0 +1,245 @@ +package lint + +import ( + "strings" + "testing" + + "github.com/stacklok/modelith/internal/provenance" +) + +// stamp returns src as a vendored copy: a well-formed provenance header whose +// digest matches the content it is stamped into. +func stamp(t *testing.T, src string) string { + t.Helper() + h := &provenance.Header{ + Vendored: provenance.Banner, + Fetch: "git", + Origin: "https://github.com/stacklok/some-repo", + Path: "docs/payments.modelith.yaml", + Ref: "main", + Commit: "4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21", + Imported: "2026-07-27", + Digest: provenance.Digest([]byte(src)), + } + return string(provenance.Stamp([]byte(src), h)) +} + +// gappy is a model with one finding in each layer that a vendored file's status +// could plausibly touch: a semantic error (a relationship to an entity that is +// not defined), a semantic warning (a PascalCase type naming no enum), and +// completeness gaps (no invariants, and no scenario exercising the entity). +const gappy = `kind: DomainModel +version: v1 +entities: + Visit: + definition: One car's stay in the garage. + attributes: + - name: settledWith + type: PaymentMethod + relationships: + - entity: Nonexistent + cardinality: "n:1" + ownership: referenced +` + +func lintSource(t *testing.T, src string, files fakeFiles) *Result { + t.Helper() + res, err := Run(importerPath, []byte(src), files) + if err != nil { + t.Fatal(err) + } + return res +} + +// TestVendored_SuppressesCompletenessAndNothingElse pins ADR-0015's rule as a +// difference rather than as a hand-written expectation: the same model is linted +// twice, once as this repository's own work and once as a vendored copy, and +// every finding that disappears must be a completeness finding. +// +// Written this way the test cannot be satisfied by a suppression that is too +// broad. A rule that also dropped semantic warnings would pass a test that +// merely listed what a vendored file should report. +func TestVendored_SuppressesCompletenessAndNothingElse(t *testing.T) { + t.Parallel() + + files := fakeFiles{".git": ""} + own := lintSource(t, gappy, files) + vendored := lintSource(t, stamp(t, gappy), files) + + byKey := func(f Finding) string { return string(f.Category) + "|" + f.Path + "|" + f.Message } + + kept := map[string]bool{} + for _, f := range vendored.Findings { + kept[byKey(f)] = true + } + + var droppedCompleteness int + for _, f := range own.Findings { + if kept[byKey(f)] { + continue + } + if f.Category != CategoryCompleteness { + t.Errorf("a %s finding was suppressed on a vendored file: %s: %s", f.Category, f.Path, f.Message) + continue + } + droppedCompleteness++ + } + if droppedCompleteness == 0 { + t.Fatal("the fixture produced no completeness findings, so this proves nothing") + } + + // And nothing new appeared: a well-formed header is not itself a finding. + ownKeys := map[string]bool{} + for _, f := range own.Findings { + ownKeys[byKey(f)] = true + } + for _, f := range vendored.Findings { + if !ownKeys[byKey(f)] { + t.Errorf("vendoring added a finding: %s: %s", f.Path, f.Message) + } + } + + // The semantic error and warning both survive, which is the half of the + // rule that a broad suppression would break. + var errs, warns int + for _, f := range vendored.Findings { + switch f.Severity { + case SeverityError: + errs++ + case SeverityWarning: + warns++ + } + } + if errs == 0 || warns == 0 { + t.Errorf("vendored file reported %d error(s) and %d warning(s); both layers should still run", errs, warns) + } +} + +// TestVendored_ImportsRaiseNothing covers the second suppression and the +// downstream consequence that makes it worth anything: an unresolvable import +// is silent, and so is the reference that resolves through it. Without the +// second half, every vendored model that has imports of its own is broken here. +func TestVendored_ImportsRaiseNothing(t *testing.T) { + t.Parallel() + + files := fakeFiles{".git": ""} + src := importer([]string{`"./nowhere.modelith.yaml"`}, "nowhere.PaymentMethod") + + own := lintSource(t, src, files) + if len(importFindings(own.Findings)) == 0 { + t.Fatal("the unstamped fixture reported nothing, so this proves nothing") + } + + vendored := lintSource(t, stamp(t, src), files) + if got := importFindings(vendored.Findings); len(got) != 0 { + t.Errorf("a vendored file reported on its own imports: %+v", got) + } +} + +// TestVendored_ImportsStillResolveWhenTheyCan pins that suppressing the errors +// did not switch resolution off: two vendored peers that import each other are +// both present here, and a reference between them still has to resolve to a real +// enum — or be reported when it does not. +func TestVendored_ImportsStillResolveWhenTheyCan(t *testing.T) { + t.Parallel() + + files := fakeFiles{".git": "", "docs/payments.modelith.yaml": paymentsModel} + src := importer([]string{`"./payments.modelith.yaml"`}, "payments.Nonexistent") + + got := importFindings(lintSource(t, stamp(t, src), files).Findings) + if len(got) != 1 || !strings.Contains(got[0].Message, `names no enum "Nonexistent"`) { + t.Errorf("want the unresolved item reported, got %+v", got) + } +} + +func TestVendored_DigestMismatch(t *testing.T) { + t.Parallel() + + files := fakeFiles{".git": ""} + stamped := stamp(t, gappy) + + t.Run("an untouched copy verifies", func(t *testing.T) { + t.Parallel() + for _, f := range lintSource(t, stamped, files).Findings { + if strings.Contains(f.Message, "digest") { + t.Errorf("unexpected digest finding: %s", f.Message) + } + } + }) + + t.Run("an edited copy is an error naming both remedies", func(t *testing.T) { + t.Parallel() + edited := strings.Replace(stamped, "One car's stay in the garage.", "One car's stay.", 1) + if edited == stamped { + t.Fatal("the test did not edit the model") + } + res := lintSource(t, edited, files) + var found *Finding + for i := range res.Findings { + if strings.Contains(res.Findings[i].Message, "no longer matches the digest") { + found = &res.Findings[i] + break + } + } + if found == nil { + t.Fatal("no digest finding") + } + if found.Severity != SeverityError { + t.Errorf("digest mismatch is a %s, want an error", found.Severity) + } + for _, want := range []string{ + "modelith deps import https://github.com/stacklok/some-repo/blob/main/docs/payments.modelith.yaml", + "delete the provenance header", + } { + if !strings.Contains(found.Message, want) { + t.Errorf("digest message does not offer %q: %s", want, found.Message) + } + } + }) +} + +func TestVendored_HeaderProblemsAreReported(t *testing.T) { + t.Parallel() + + files := fakeFiles{".git": ""} + cases := []struct { + name string + header string + contains string + }{ + {"unknown key", "# modelith-surface: sha256:x\n", "unknown provenance key"}, + {"unknown fetch method", "# modelith-fetch: carrier-pigeon\n", `unknown fetch method "carrier-pigeon"`}, + {"missing required key", "# modelith-fetch: git\n", `missing "# modelith-commit"`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var found bool + for _, f := range lintSource(t, tc.header+gappy, files).Findings { + if strings.Contains(f.Message, tc.contains) { + if f.Severity != SeverityError { + t.Errorf("header problem is a %s, want an error", f.Severity) + } + found = true + } + } + if !found { + t.Errorf("no finding containing %q", tc.contains) + } + }) + } +} + +// TestVendored_MalformedHeaderStillSuppressesCompleteness pins that presence, +// not a clean parse, is what makes a file vendored. A typo in the header would +// otherwise be buried under gaps in a document this repository does not own. +func TestVendored_MalformedHeaderStillSuppressesCompleteness(t *testing.T) { + t.Parallel() + + files := fakeFiles{".git": ""} + for _, f := range lintSource(t, "# modelith-surface: sha256:x\n"+gappy, files).Findings { + if f.Category == CategoryCompleteness { + t.Errorf("completeness finding survived a malformed header: %s: %s", f.Path, f.Message) + } + } +} diff --git a/internal/provenance/provenance.go b/internal/provenance/provenance.go index 082f4eb..ba8697a 100644 --- a/internal/provenance/provenance.go +++ b/internal/provenance/provenance.go @@ -197,6 +197,11 @@ func (h *Header) validate(seen map[string]int) []Problem { return problems } +// ValidDigest reports whether s is a digest in the form a header records. A +// caller that has already reported a malformed one uses this to skip the +// comparison, which such a digest cannot help but fail. +func ValidDigest(s string) bool { return digestRE.MatchString(s) } + // Verify reports whether src still hashes to the digest its own header records, // and returns the digest src actually has. func (h *Header) Verify(src []byte) (ok bool, got string) { From 0ff8b4a7c8c0be3d6f4bf671e2ac5d8605c8c11a Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 12:29:51 -0700 Subject: [PATCH 05/13] feat(render): skip a vendored copy in --check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A vendored model's rendered form belongs to its home repository. It arrives with no committed .md and no obligation to carry one, and --check runs over globs, so without this a vendored file fails a build over a document this repository does not own — and "fix it" would mean regenerating somebody else's Markdown every time their model moved. Naming the file still renders it. That is how a deep link into a vendored model's .md gets something to point at. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 12 ++++++++++++ cmd/modelith/main_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 5f27576..0cd6856 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -15,6 +15,7 @@ import ( "github.com/stacklok/modelith/internal/lint" "github.com/stacklok/modelith/internal/model" + "github.com/stacklok/modelith/internal/provenance" "github.com/stacklok/modelith/internal/render/markdown" "github.com/stacklok/modelith/internal/schema" ) @@ -205,6 +206,17 @@ func renderCmd() *cobra.Command { if err != nil { return fmt.Errorf("%s: %w", in, err) } + // A vendored model's rendered form belongs to its home repository, + // so it arrives with no committed .md and no obligation to carry + // one. --check runs over globs, and demanding one here would make + // this repository regenerate somebody else's document every time + // their model moved (ADR-0015). Naming the file to render it still + // renders it; that is how a deep link into a vendored model's .md + // gets something to point at. + if check && provenance.Present(data) { + fmt.Fprintf(cmd.OutOrStdout(), "%s is a vendored copy — skipped\n", in) + return nil + } // Validate against the schema first so a malformed file fails with a // friendly, located error rather than the raw strict-YAML parse error. if findings := lint.Structural(data); len(findings) > 0 { diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index 1510a1c..9b0b965 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -8,6 +8,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/stacklok/modelith/internal/provenance" ) // run executes the CLI with args, capturing stdout+stderr, and returns the @@ -270,6 +272,43 @@ func TestRenderCheckDetectsDrift(t *testing.T) { } } +// TestRenderCheckSkipsAVendoredCopy covers the half of ADR-0015 that lives in +// the CLI: a vendored model has no committed .md here, and --check runs over +// globs, so demanding one would fail a build over somebody else's document. +// Rendering it by name still works — that is what a deep link points at. +func TestRenderCheckSkipsAVendoredCopy(t *testing.T) { + dir := t.TempDir() + header := "# modelith-vendored: " + provenance.Banner + "\n" + + "# modelith-fetch: git\n" + + "# modelith-origin: https://github.com/stacklok/some-repo\n" + + "# modelith-path: docs/m.modelith.yaml\n" + + "# modelith-ref: main\n" + + "# modelith-commit: 4f2c1e9\n" + + "# modelith-imported: 2026-07-27\n" + + "# modelith-digest: " + provenance.Digest([]byte(minimalValid)) + "\n" + yamlPath := writeTemp(t, dir, "m.modelith.yaml", header+minimalValid) + + // No .md exists at all, which is the state a fresh `deps import` leaves. + out, err := run(t, "render", "--check", yamlPath) + if err != nil { + t.Fatalf("--check failed on a vendored copy: %v (%s)", err, out) + } + if !strings.Contains(out, "vendored copy") { + t.Errorf("output does not say why it was skipped: %q", out) + } + + if _, err := os.Stat(filepath.Join(dir, "m.modelith.md")); !os.IsNotExist(err) { + t.Error("--check wrote a file") + } + + if _, err := run(t, "render", yamlPath); err != nil { + t.Fatalf("rendering a vendored model by name failed: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, "m.modelith.md")); err != nil { + t.Errorf("rendering by name wrote nothing: %v", err) + } +} + func TestSchemaOutputsValidJSON(t *testing.T) { out, err := run(t, "schema") if err != nil { From f7e3b78e4534e2a9cdcd2a1486b3cebbaa55981f Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 12:31:03 -0700 Subject: [PATCH 06/13] fix(lint): say what to do when an item is one import further away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A qualified type whose scope resolves but whose item is not there had one message for two different mistakes. When the imported model is a leaf, the mistake is a typo. When it has imports of its own, the item may genuinely be defined one hop away — and nothing at the reference site says why that does not resolve. Vendoring fetches one file and resolution reaches one hop, so the answer in the second case is always to import that model here too and give it its own scope. The diagnostic now says so, and says it only when the imported model actually has imports; on a leaf it points at the name instead. Signed-off-by: Joe Beda --- internal/lint/imports.go | 27 ++++++++++++++++++++++++++- internal/lint/imports_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/internal/lint/imports.go b/internal/lint/imports.go index 5fec625..247aac0 100644 --- a/internal/lint/imports.go +++ b/internal/lint/imports.go @@ -272,13 +272,38 @@ func checkQualifiedTypes(m *model.Model, byScope map[string]importedModel, claim 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) + broken("%s", unresolvedItemMessage(attr.Type, item, imp)) } } reportUnboundScopes(unbound, res) return used } +// unresolvedItemMessage explains a qualified type whose scope resolved but +// whose item is not there. +// +// The interesting case is an imported model that has imports of its own: the +// item may well be defined in one of them, and nothing about the reference site +// says why that does not resolve. Vendoring fetches one file and resolution +// reaches one hop (ADR-0010, ADR-0015), so the answer is always to import that +// model here as well and give it its own scope — never to expect this one to +// follow the chain. Saying so only when the imported model actually has imports +// keeps the advice off the plain case, where the mistake is a typo. +func unresolvedItemMessage(typ, item string, imp importedModel) string { + msg := fmt.Sprintf("attribute type %q names no enum %q in %q", typ, item, imp.path) + n := len(imp.model.Imports) + if n == 0 { + return msg + " — check the name, or whether you meant to import a different model" + } + theirs := fmt.Sprintf("%d models of its own", n) + if n == 1 { + theirs = "a model of its own" + } + return msg + fmt.Sprintf( + " — that model imports %s, and resolution is not transitive: if %q is defined in one of them, add that model to this model's `imports:` too and reference it with its own scope", + theirs, item) +} + // 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 { diff --git a/internal/lint/imports_test.go b/internal/lint/imports_test.go index 6773740..8bc913e 100644 --- a/internal/lint/imports_test.go +++ b/internal/lint/imports_test.go @@ -67,6 +67,12 @@ entities: definition: A request for payment. ` +// chainedModel is an imported model that imports something itself — the case +// where an unresolved item might really be one hop further away. +const chainedModel = paymentsModel + `imports: + - ./payments.modelith.yaml +` + // importer builds a model that lists the given imports and types one attribute // 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. @@ -143,6 +149,7 @@ func TestImports_Resolution(t *testing.T) { "payments/payments.modelith.yaml": paymentsModel, "docs/legacy/pay-v2.modelith.yaml": paymentsModel, "docs/PayMents.modelith.yaml": paymentsModel, + "docs/chained.modelith.yaml": chainedModel, "docs/not-a-model.yaml": "kind: SomethingElse\nversion: v1\n", } @@ -220,6 +227,27 @@ func TestImports_Resolution(t *testing.T) { `names no enum "Nonexistent"`, }}, }, + { + // The plain case advises a typo; the chained case below advises + // the import. Getting the wrong one is the whole failure mode. + name: "an unresolved item in a leaf model points at the name", + imports: []string{`"./payments.modelith.yaml"`}, + attrType: "payments.Nonexistent", + want: []wantFinding{{ + SeverityError, CategorySemantic, typePath, + "check the name, or whether you meant to import a different model", + }}, + }, + { + name: "an unresolved item in a model that imports others explains non-transitivity", + imports: []string{"{scope: payments, path: ./chained.modelith.yaml}"}, + attrType: "payments.Nonexistent", + want: []wantFinding{{ + SeverityError, CategorySemantic, typePath, + `that model imports a model of its own, and resolution is not transitive: ` + + `if "Nonexistent" is defined in one of them, add that model to this model's ` + "`imports:`" + ` too`, + }}, + }, { name: "qualified type resolving to an entity, not an enum", imports: []string{`"./payments.modelith.yaml"`}, From 1920568477b6be334e4856d5b05ae1f7e1752681 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 12:35:20 -0700 Subject: [PATCH 07/13] feat(deps): vendor a model from another repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `modelith deps import [dir]`, the command that acquires a model and stamps it as a vendored copy. is the address of the file as it appears in a browser on github.com — the artifact a user actually has in hand — and it carries the repo, the ref and the path in one paste. The header stores those parts separately, so the blob-URL grammar stays in the CLI's argument handling and never reaches the format. The destination is a positional directory defaulting to ".", the way git clone takes one. It is a directory, not a path: the filename always comes from the origin, because a bare import binds its scope from the basename, and the explicit {scope, path} form is already the escape hatch for a collision. Transport is gh, executed as an argv array and never through a shell. gh already solves authentication for the private and internal repositories the motivating models require. A non-GitHub origin is an error that asks the user to open an issue, so the first real user is the signal to build a second transport rather than building one against a case nobody has. The commit recorded is the one that last touched the file at that ref, not the head of the ref: it identifies the version of this file and does not move when unrelated commits land on the branch. The command writes the file and nothing else. Rewriting a user's imports list would cost comment and formatting fidelity and would force the command to guess which of a repository's models meant to import this one. That second, manual step is also the gate on the injection risk vendoring carries (ADR-0014) — the file is inert until it is named in an imports list — which is why the trust warning prints and proceeds instead of prompting. An agent usually drives this command, and a prompt is either auto-answered theatre or a wedged CI run. Fetching a file that already carries a provenance header is refused, naming the origin it records: this repository should track the model's home rather than somebody else's copy of it. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 105 ++++++++++- cmd/modelith/main_test.go | 97 ++++++++++ internal/deps/deps.go | 242 +++++++++++++++++++++++++ internal/deps/deps_test.go | 356 +++++++++++++++++++++++++++++++++++++ 4 files changed, 799 insertions(+), 1 deletion(-) create mode 100644 internal/deps/deps.go create mode 100644 internal/deps/deps_test.go diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 0cd6856..00eab8e 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -6,13 +6,16 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "path/filepath" "runtime/debug" "strings" + "time" "github.com/spf13/cobra" + "github.com/stacklok/modelith/internal/deps" "github.com/stacklok/modelith/internal/lint" "github.com/stacklok/modelith/internal/model" "github.com/stacklok/modelith/internal/provenance" @@ -83,10 +86,110 @@ func rootCmd() *cobra.Command { SilenceErrors: true, Version: buildVersion(), } - root.AddCommand(lintCmd(), renderCmd(), schemaCmd()) + root.AddCommand(lintCmd(), renderCmd(), schemaCmd(), depsCmd()) return root } +// ---- deps ---- + +func depsCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "deps", + Short: "Manage models vendored from other repositories", + Long: strings.TrimSpace(` +Manage models vendored from other repositories. + +A vendored model is a copy of a model whose home is elsewhere, committed here +and marked with a provenance header. Commands in this group are the only ones +that use the network; lint and render never do.`), + } + cmd.AddCommand(depsImportCmd()) + return cmd +} + +func depsImportCmd() *cobra.Command { + var ref string + cmd := &cobra.Command{ + Use: "import [dir]", + Short: "Vendor a model from another repository", + Long: strings.TrimSpace(` +Vendor a model from another repository into this one. + + is the address of the file as it appears in a browser on github.com. The +copy is written into [dir] (the working directory by default) with a provenance +header recording where it came from, and is verified against that header by +every later lint. + +Fetching is delegated to the gh CLI, which must be installed and authenticated. +The imports list of the model that will reference this copy is yours to edit; +this command prints the entry to add.`), + Example: strings.TrimSpace(` + modelith deps import https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml + modelith deps import https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml docs/ + modelith deps import --ref v2.1.0 https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml`), + Args: cobra.RangeArgs(1, 2), + RunE: func(cmd *cobra.Command, args []string) error { + dir := "." + if len(args) == 2 { + dir = args[1] + } + info, err := os.Stat(dir) + if err != nil { + return fmt.Errorf("%s: %w", dir, err) + } + if !info.IsDir() { + return fmt.Errorf("%s is not a directory — the destination is a directory, and the filename comes from the origin", dir) + } + + res, err := deps.Import(cmd.Context(), deps.Options{ + URL: args[0], + Dir: dir, + Ref: ref, + Now: time.Now(), + }) + if err != nil { + return err + } + + printImportResult(cmd.OutOrStdout(), cmd.ErrOrStderr(), res) + return nil + }, + } + cmd.Flags().StringVar(&ref, "ref", "", "ref to fetch, overriding the one in the URL (a tag pins the copy)") + return cmd +} + +// printImportResult reports a completed import: what was written, the entry to +// paste, whether the fetched model reaches for models of its own, and the trust +// warning ADR-0014 requires. +// +// The warning prints and does not prompt. An agent usually drives this command, +// so a prompt is either auto-answered theatre or a wedged non-interactive run. +// The fetched file is inert until it is named in an imports list, and this +// command deliberately does not do that — the manual step is the real gate. +func printImportResult(out, errOut io.Writer, res *deps.Result) { + verb := "wrote" + if res.Replaced { + verb = "replaced" + } + name := filepath.Base(res.Path) + fmt.Fprintf(out, "%s %s at %s\n", verb, res.Path, res.Header.Commit) + fmt.Fprintf(out, "\nAdd it to the model that references it, as a path relative to that model:\n\n"+ + " imports:\n - ./%s\n", name) + if n := len(res.TheirImports); n > 0 { + declares := fmt.Sprintf("declares %d imports of its own", n) + if n == 1 { + declares = "declares an import of its own" + } + fmt.Fprintf(out, "\nNote: %s %s (%s).\n"+ + "modelith vendors one file, not a dependency tree, and resolution is not\n"+ + "transitive — if you need items from those models, import them directly.\n", + name, declares, strings.Join(res.TheirImports, ", ")) + } + fmt.Fprint(errOut, "\nWarning: a vendored model is untrusted content that will be rendered\n"+ + "into your published Markdown. Only vendor from sources you trust.\n") +} + // ---- lint ---- func lintCmd() *cobra.Command { diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index 9b0b965..dc89706 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "github.com/stacklok/modelith/internal/deps" "github.com/stacklok/modelith/internal/provenance" ) @@ -309,6 +310,102 @@ func TestRenderCheckSkipsAVendoredCopy(t *testing.T) { } } +// TestDepsImportOutput covers what a completed import tells the user. All three +// parts are load-bearing: the snippet is the second, manual step this command +// deliberately leaves to the user, the note is the whole answer to +// non-transitive vendoring, and the warning is what ADR-0014 requires in place +// of hardening the renderer further. +func TestDepsImportOutput(t *testing.T) { + res := &deps.Result{ + Path: filepath.Join("docs", "payments.modelith.yaml"), + Header: &provenance.Header{Commit: "4f2c1e9"}, + } + + t.Run("a leaf model", func(t *testing.T) { + var out, errOut bytes.Buffer + printImportResult(&out, &errOut, res) + for _, want := range []string{"wrote docs/payments.modelith.yaml at 4f2c1e9", "imports:", "- ./payments.modelith.yaml"} { + if !strings.Contains(out.String(), want) { + t.Errorf("stdout does not contain %q:\n%s", want, out.String()) + } + } + if strings.Contains(out.String(), "dependency tree") { + t.Error("a leaf model got the transitive-imports note") + } + if !strings.Contains(errOut.String(), "Only vendor from sources you trust") { + t.Errorf("the trust warning is missing from stderr:\n%s", errOut.String()) + } + }) + + t.Run("a model with imports of its own", func(t *testing.T) { + var out, errOut bytes.Buffer + chained := *res + chained.TheirImports = []string{"./ledger.modelith.yaml"} + printImportResult(&out, &errOut, &chained) + for _, want := range []string{ + "declares an import of its own (./ledger.modelith.yaml)", + "resolution is not", + "import them directly", + } { + if !strings.Contains(out.String(), want) { + t.Errorf("stdout does not contain %q:\n%s", want, out.String()) + } + } + }) + + t.Run("a replaced copy says so", func(t *testing.T) { + var out, errOut bytes.Buffer + replaced := *res + replaced.Replaced = true + printImportResult(&out, &errOut, &replaced) + if !strings.HasPrefix(out.String(), "replaced ") { + t.Errorf("stdout does not report the replacement:\n%s", out.String()) + } + }) +} + +// TestDepsImportRejectsBadArguments covers the paths that fail before any fetch +// is attempted, so they need no network and no gh. +func TestDepsImportRejectsBadArguments(t *testing.T) { + dir := t.TempDir() + file := writeTemp(t, dir, "not-a-dir", "x") + + cases := []struct { + name string + args []string + want string + }{ + { + name: "a host modelith cannot fetch from", + args: []string{"deps", "import", "https://gitlab.com/acme/billing/-/blob/main/m.modelith.yaml", dir}, + want: "github.com/stacklok/modelith/issues", + }, + { + name: "a URL that names no file", + args: []string{"deps", "import", "https://github.com/acme/billing", dir}, + want: "not a GitHub file URL", + }, + { + name: "a destination that is a file", + args: []string{"deps", "import", "https://github.com/acme/billing/blob/main/m.modelith.yaml", file}, + want: "is not a directory", + }, + { + name: "a destination that does not exist", + args: []string{"deps", "import", "https://github.com/acme/billing/blob/main/m.modelith.yaml", filepath.Join(dir, "nope")}, + want: "no such file or directory", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := run(t, tc.args...) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("want an error containing %q, got %v", tc.want, err) + } + }) + } +} + func TestSchemaOutputsValidJSON(t *testing.T) { out, err := run(t, "schema") if err != nil { diff --git a/internal/deps/deps.go b/internal/deps/deps.go new file mode 100644 index 0000000..f459335 --- /dev/null +++ b/internal/deps/deps.go @@ -0,0 +1,242 @@ +// Package deps acquires a model from another repository and stamps it as a +// vendored copy. +// +// It implements no network transport of its own. Every fetch is delegated to +// the gh CLI, executed as an argv array and never through a shell, so this +// binary holds no TLS configuration and no credentials (ADR-0011). +package deps + +import ( + "context" + "errors" + "fmt" + "net/url" + "os" + "os/exec" + "path" + "path/filepath" + "strings" + "time" + + "github.com/stacklok/modelith/internal/model" + "github.com/stacklok/modelith/internal/provenance" +) + +// Runner runs an external command and returns its standard output. It is the +// seam the gh calls go through, so Import is testable without a network. +type Runner interface { + Run(ctx context.Context, name string, args ...string) ([]byte, error) +} + +// ExecRunner runs commands with os/exec. +type ExecRunner struct{} + +// Run executes name with args and returns its standard output. Standard error +// is folded into the returned error, because gh reports why it refused there. +func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + cmd := exec.CommandContext(ctx, name, args...) + var stderr strings.Builder + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + if errors.Is(err, exec.ErrNotFound) { + return nil, fmt.Errorf("%s is not installed — modelith delegates fetching to it; install it from https://cli.github.com and run `%s auth login`", name, name) + } + if msg := strings.TrimSpace(stderr.String()); msg != "" { + return nil, fmt.Errorf("%s %s: %s", name, strings.Join(args, " "), msg) + } + return nil, fmt.Errorf("%s %s: %w", name, strings.Join(args, " "), err) + } + return out, nil +} + +// Source is a model file in another repository, as an origin URL parsed into +// the parts a fetch and a later refresh both need. +type Source struct { + Origin string // https://github.com/owner/repo + Owner string + Repo string + Ref string + Path string // path within the repository +} + +// ParseSource reads a GitHub blob URL — the address of the file as it appears +// in a browser — into its parts. A non-empty ref overrides the one in the URL, +// which is also how a branch whose name contains a slash is disambiguated from +// the path that follows it. +func ParseSource(raw, ref string) (Source, error) { + u, err := url.Parse(strings.TrimSpace(raw)) + if err != nil { + return Source{}, fmt.Errorf("%q is not a URL: %w", raw, err) + } + if u.Host != "github.com" { + return Source{}, fmt.Errorf( + "modelith can currently fetch only from github.com, and %q is on %q. Support for other hosts is not written yet because nobody has needed it — if you do, please open an issue at %s saying where your models live", + raw, u.Host, issuesURL) + } + // A URL copied from the browser often carries ?plain=1 and a #L12 anchor. + // Neither is part of the file's address. + parts := strings.Split(strings.Trim(u.Path, "/"), "/") + if len(parts) < 5 || parts[2] != "blob" { + return Source{}, fmt.Errorf( + "%q is not a GitHub file URL — it should look like https://github.com///blob//, which is the address you get by opening the file on github.com", + raw) + } + src := Source{ + Owner: parts[0], + Repo: parts[1], + Origin: "https://github.com/" + parts[0] + "/" + parts[1], + } + rest := strings.Join(parts[3:], "/") + // A ref may contain slashes ("release/v2"), and the URL gives no way to tell + // where it ends. Splitting at the first segment is right for the common + // case; an explicit ref settles the rest. + switch { + case ref != "" && strings.HasPrefix(rest, ref+"/"): + src.Ref, src.Path = ref, strings.TrimPrefix(rest, ref+"/") + default: + src.Ref, src.Path, _ = strings.Cut(rest, "/") + if ref != "" { + src.Ref = ref + } + } + if src.Path == "" { + return Source{}, fmt.Errorf("%q names no file inside the repository", raw) + } + return src, nil +} + +const issuesURL = "https://github.com/stacklok/modelith/issues" + +// Options are the inputs to Import. +type Options struct { + // URL is the GitHub blob URL of the model to vendor. + URL string + // Dir is the directory to write into. Empty means the working directory. + Dir string + // Ref overrides the ref in URL. + Ref string + // Now stamps the header's imported date, in local time. + Now time.Time + // Run is the command seam; nil uses ExecRunner. + Run Runner +} + +// Result reports what Import did, so the caller can print it. +type Result struct { + // Path is where the vendored copy was written. + Path string + // Header is what was stamped into it. + Header *provenance.Header + // Replaced says an earlier copy was overwritten. + Replaced bool + // TheirImports are the fetched model's own imports. Vendoring fetches one + // file, so these were not followed; the caller says so. + TheirImports []string +} + +// Import fetches the model at opts.URL and writes it into opts.Dir as a +// vendored copy carrying a provenance header. +// +// It writes the file and nothing else. The importing model's `imports:` list is +// the caller's to edit: rewriting a user's YAML costs comment and formatting +// fidelity, and a repository with several models gives no way to guess which +// one meant to import this. That second, manual step is also the gate on the +// injection risk vendoring carries (ADR-0014) — the file is inert until it is +// named in an imports list. +func Import(ctx context.Context, opts Options) (*Result, error) { + src, err := ParseSource(opts.URL, opts.Ref) + if err != nil { + return nil, err + } + runner := opts.Run + if runner == nil { + runner = ExecRunner{} + } + + content, err := fetchContent(ctx, runner, src) + if err != nil { + return nil, err + } + if provenance.Present(content) { + return nil, fmt.Errorf( + "%s is itself a vendored copy — it carries a provenance header naming where it came from. Vendor it from that origin instead, so this repository tracks the model's home rather than somebody else's copy of it", + opts.URL) + } + m, err := model.Parse(content) + if err != nil || m.Kind != "DomainModel" { + return nil, fmt.Errorf("%s is not a domain model — check that the URL names a *.modelith.yaml file", opts.URL) + } + + commit, err := fetchCommit(ctx, runner, src) + if err != nil { + return nil, err + } + + h := &provenance.Header{ + Vendored: provenance.Banner, + Fetch: "git", + Origin: src.Origin, + Path: src.Path, + Ref: src.Ref, + Commit: commit, + Imported: opts.Now.Format("2006-01-02"), + Digest: provenance.Digest(content), + } + + target := filepath.Join(opts.Dir, path.Base(src.Path)) + _, statErr := os.Stat(target) + replaced := statErr == nil + if err := os.WriteFile(target, provenance.Stamp(content, h), 0o644); err != nil { + return nil, fmt.Errorf("writing %s: %w", target, err) + } + + res := &Result{Path: target, Header: h, Replaced: replaced} + for _, imp := range m.Imports { + res.TheirImports = append(res.TheirImports, imp.Path) + } + return res, nil +} + +// fetchContent returns the file's bytes. The raw media type asks the API for +// the content itself rather than a JSON envelope carrying it base64-encoded, so +// nothing here has to decode. +func fetchContent(ctx context.Context, runner Runner, src Source) ([]byte, error) { + endpoint := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", + src.Owner, src.Repo, escapePath(src.Path), url.QueryEscape(src.Ref)) + out, err := runner.Run(ctx, "gh", "api", "-H", "Accept: application/vnd.github.raw", endpoint) + if err != nil { + return nil, err + } + return out, nil +} + +// fetchCommit returns the commit that last touched the file at this ref. +// +// That, rather than the head of the ref, is what identifies the version of this +// file: it does not move when unrelated commits land on the branch, so a later +// freshness check reports the model changing rather than the repository being +// busy. +func fetchCommit(ctx context.Context, runner Runner, src Source) (string, error) { + endpoint := fmt.Sprintf("repos/%s/%s/commits?path=%s&sha=%s&per_page=1", + src.Owner, src.Repo, url.QueryEscape(src.Path), url.QueryEscape(src.Ref)) + out, err := runner.Run(ctx, "gh", "api", endpoint, "--jq", ".[0].sha") + if err != nil { + return "", err + } + sha := strings.TrimSpace(string(out)) + if sha == "" || sha == "null" { + return "", fmt.Errorf("%s/%s has no commit touching %q at %q", src.Owner, src.Repo, src.Path, src.Ref) + } + return sha, nil +} + +// escapePath escapes each segment of a repository path, leaving the separators +// alone so the API still sees a path. +func escapePath(p string) string { + segments := strings.Split(p, "/") + for i, s := range segments { + segments[i] = url.PathEscape(s) + } + return strings.Join(segments, "/") +} diff --git a/internal/deps/deps_test.go b/internal/deps/deps_test.go new file mode 100644 index 0000000..4c49309 --- /dev/null +++ b/internal/deps/deps_test.go @@ -0,0 +1,356 @@ +package deps + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stacklok/modelith/internal/provenance" +) + +// fakeRunner answers the gh calls Import makes from a map keyed by the API +// endpoint. A hand-written fake rather than a mock: it behaves like gh, +// answering the two endpoints it knows and failing the way gh fails on +// anything else, so a test cannot accidentally assert a response the real +// command could never produce. +type fakeRunner struct { + content string + sha string + // calls records every argv, so a test can assert what was executed. + calls [][]string + // fail, when set, is returned for any call whose endpoint contains it. + fail string +} + +func (f *fakeRunner) Run(_ context.Context, name string, args ...string) ([]byte, error) { + f.calls = append(f.calls, append([]string{name}, args...)) + endpoint := args[len(args)-1] + for _, a := range args { + if strings.HasPrefix(a, "repos/") { + endpoint = a + } + } + if f.fail != "" && strings.Contains(endpoint, f.fail) { + return nil, fmt.Errorf("gh: HTTP 404: Not Found (%s)", endpoint) + } + switch { + case strings.Contains(endpoint, "/contents/"): + return []byte(f.content), nil + case strings.Contains(endpoint, "/commits"): + return []byte(f.sha + "\n"), nil + } + return nil, fmt.Errorf("gh: unexpected endpoint %q", endpoint) +} + +const upstream = `# yaml-language-server: $schema=https://modelith.sh/schema/domain-model/v1.json +kind: DomainModel +version: v1 +title: Payments +enums: + PaymentMethod: + values: + - name: card +` + +const blobURL = "https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml" + +const sha = "4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21" + +func TestParseSource(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + raw string + ref string + want Source + wantErr string + }{ + { + name: "a browser blob URL", + raw: blobURL, + want: Source{ + Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", + Ref: "main", Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "a query and anchor are not part of the address", + raw: blobURL + "?plain=1#L12", + want: Source{ + Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", + Ref: "main", Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "an explicit ref overrides the one in the URL", + raw: blobURL, + ref: "v2.1.0", + want: Source{ + Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", + Ref: "v2.1.0", Path: "docs/payments.modelith.yaml", + }, + }, + { + // The URL alone cannot say where a slashed ref ends and the path + // begins; naming the ref settles it. + name: "an explicit slashed ref splits the rest correctly", + raw: "https://github.com/acme/billing/blob/release/v2/docs/payments.modelith.yaml", + ref: "release/v2", + want: Source{ + Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", + Ref: "release/v2", Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "another host asks for an issue rather than guessing", + raw: "https://gitlab.com/acme/billing/-/blob/main/payments.modelith.yaml", + wantErr: "github.com/stacklok/modelith/issues", + }, + { + name: "a repository URL names no file", + raw: "https://github.com/acme/billing", + wantErr: "not a GitHub file URL", + }, + { + name: "a tree URL is not a file URL", + raw: "https://github.com/acme/billing/tree/main/docs", + wantErr: "not a GitHub file URL", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := ParseSource(tc.raw, tc.ref) + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("want an error containing %q, got %v", tc.wantErr, err) + } + return + } + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("ParseSource() = %+v, want %+v", got, tc.want) + } + }) + } +} + +func importInto(t *testing.T, dir string, r *fakeRunner, url string) (*Result, error) { + t.Helper() + return Import(context.Background(), Options{ + URL: url, + Dir: dir, + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: r, + }) +} + +func TestImport_StampsAVerifiableCopy(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + r := &fakeRunner{content: upstream, sha: sha} + res, err := importInto(t, dir, r, blobURL) + if err != nil { + t.Fatal(err) + } + + written, err := os.ReadFile(res.Path) + if err != nil { + t.Fatal(err) + } + if got, want := res.Path, filepath.Join(dir, "payments.modelith.yaml"); got != want { + t.Errorf("wrote %s, want %s", got, want) + } + if res.Replaced { + t.Error("reported replacing a file that did not exist") + } + + h, problems := provenance.Parse(written) + if len(problems) != 0 { + t.Fatalf("the stamped copy has header problems: %+v", problems) + } + want := provenance.Header{ + Vendored: provenance.Banner, + Fetch: "git", + Origin: "https://github.com/acme/billing", + Path: "docs/payments.modelith.yaml", + Ref: "main", + Commit: sha, + Imported: "2026-07-27", + Digest: provenance.Digest([]byte(upstream)), + } + if *h != want { + t.Errorf("stamped header = %+v, want %+v", *h, want) + } + if ok, got := h.Verify(written); !ok { + t.Errorf("the freshly written copy does not verify: computed %s", got) + } + + // The editor directive stays first; the model survives byte for byte. + if !strings.HasPrefix(string(written), "# yaml-language-server:") { + t.Error("the editor directive is no longer the first line") + } + if !strings.Contains(string(written), " PaymentMethod:\n") { + t.Error("the model content did not survive the stamp") + } +} + +// TestImport_CallsGhWithTheExpectedEndpoints asserts the whole argv of both +// calls, not that they merely contain something. The endpoints are the contract +// with gh — a ref silently dropped from the content fetch would return whatever +// the default branch holds, and the copy would be stamped with a ref it is not +// actually at. +// +// Nothing here goes through a shell; that is a property of ExecRunner passing +// an argv array to os/exec, which no assertion about argument text could show. +func TestImport_CallsGhWithTheExpectedEndpoints(t *testing.T) { + t.Parallel() + + r := &fakeRunner{content: upstream, sha: sha} + if _, err := importInto(t, t.TempDir(), r, blobURL); err != nil { + t.Fatal(err) + } + want := [][]string{ + {"gh", "api", "-H", "Accept: application/vnd.github.raw", + "repos/acme/billing/contents/docs/payments.modelith.yaml?ref=main"}, + {"gh", "api", + "repos/acme/billing/commits?path=docs%2Fpayments.modelith.yaml&sha=main&per_page=1", + "--jq", ".[0].sha"}, + } + if len(r.calls) != len(want) { + t.Fatalf("want %d gh calls, got %d: %+v", len(want), len(r.calls), r.calls) + } + for i, call := range r.calls { + if strings.Join(call, "\x00") != strings.Join(want[i], "\x00") { + t.Errorf("call %d:\n got %q\nwant %q", i, call, want[i]) + } + } +} + +// TestImport_EscapesAPathThatNeedsIt pins that a path segment reaches the API +// escaped rather than as a second query parameter or a truncated path. +func TestImport_EscapesAPathThatNeedsIt(t *testing.T) { + t.Parallel() + + r := &fakeRunner{content: upstream, sha: sha} + url := "https://github.com/acme/billing/blob/main/docs/a b&c/payments.modelith.yaml" + if _, err := importInto(t, t.TempDir(), r, url); err != nil { + t.Fatal(err) + } + const wantContents = "repos/acme/billing/contents/docs/a%20b&c/payments.modelith.yaml?ref=main" + if got := r.calls[0][len(r.calls[0])-1]; got != wantContents { + t.Errorf("content endpoint = %q, want %q", got, wantContents) + } +} + +func TestImport_ReportsTheirImportsWithoutFetchingThem(t *testing.T) { + t.Parallel() + + chained := upstream + "imports:\n - ./ledger.modelith.yaml\n - ./tax.modelith.yaml\n" + r := &fakeRunner{content: chained, sha: sha} + res, err := importInto(t, t.TempDir(), r, blobURL) + if err != nil { + t.Fatal(err) + } + want := []string{"./ledger.modelith.yaml", "./tax.modelith.yaml"} + if strings.Join(res.TheirImports, ",") != strings.Join(want, ",") { + t.Errorf("TheirImports = %v, want %v", res.TheirImports, want) + } + // Two calls and no more: the imports were reported, not followed. + if len(r.calls) != 2 { + t.Errorf("want two gh calls, got %d: %+v", len(r.calls), r.calls) + } +} + +func TestImport_Rejections(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + runner *fakeRunner + url string + wantErr string + }{ + { + name: "a file that is already a vendored copy", + runner: &fakeRunner{content: "# modelith-origin: https://github.com/other/repo\n" + upstream, sha: sha}, + url: blobURL, + wantErr: "itself a vendored copy", + }, + { + name: "a file that is not a domain model", + runner: &fakeRunner{content: "kind: SomethingElse\nversion: v1\n", sha: sha}, + url: blobURL, + wantErr: "not a domain model", + }, + { + name: "a path with no commits at that ref", + runner: &fakeRunner{content: upstream, sha: "null"}, + url: blobURL, + wantErr: "no commit touching", + }, + { + name: "gh refusing the fetch", + runner: &fakeRunner{content: upstream, sha: sha, fail: "/contents/"}, + url: blobURL, + wantErr: "HTTP 404", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + _, err := importInto(t, dir, tc.runner, tc.url) + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("want an error containing %q, got %v", tc.wantErr, err) + } + entries, readErr := os.ReadDir(dir) + if readErr != nil { + t.Fatal(readErr) + } + if len(entries) != 0 { + t.Errorf("a rejected import left %d file(s) behind", len(entries)) + } + }) + } +} + +func TestImport_ReplacesAnEarlierCopy(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + r := &fakeRunner{content: upstream, sha: sha} + if _, err := importInto(t, dir, r, blobURL); err != nil { + t.Fatal(err) + } + moved := &fakeRunner{content: upstream + " # a later revision\n", sha: "9" + sha[1:]} + res, err := importInto(t, dir, moved, blobURL) + if err != nil { + t.Fatal(err) + } + if !res.Replaced { + t.Error("did not report replacing the earlier copy") + } + written, err := os.ReadFile(res.Path) + if err != nil { + t.Fatal(err) + } + h, _ := provenance.Parse(written) + if h.Commit != moved.sha { + t.Errorf("commit is %s, want the newer %s", h.Commit, moved.sha) + } + if ok, _ := h.Verify(written); !ok { + t.Error("the replaced copy does not verify against its own digest") + } + if strings.Count(string(written), provenance.LinePrefix+"origin:") != 1 { + t.Error("the replaced copy carries more than one header") + } +} From 179d69b276af0e9b6d11355f7f9d96289812e314 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 12:40:23 -0700 Subject: [PATCH 08/13] docs(vendoring): document deps import and how a vendored file is treated Adds docs/10-vendoring.md: what deps import writes, what each header key records, the two-step add-it-to-your-imports flow, the trust warning, the digest check and both of its remedies, and why vendoring fetches one file rather than a dependency tree. The page is numbered 10 rather than inserted next to the schema reference, which would have renumbered three pages and changed their published URLs for an ordering nicety. Cross-references it from the CLI reference (a new `modelith deps` section), the Action page (a vendored file in the glob reports no completeness gaps and is skipped by check-rendered, which is user-visible there), the schema reference's "nothing is fetched" rule, and the landing page's table. Renames the four tests that pin ADR-0015 to the TestADR_0015_ prefix, per testing.md, and adds the one the rule was missing: an unknown fetch method is an error naming the methods that work and asking for an issue. The gosec suppression on the exec seam is documented rather than blanket: the command is the literal "gh", the arguments are literals plus an endpoint built from an already-validated URL, and exec passes an argv array, so there is no shell for a metacharacter to be syntax in. Signed-off-by: Joe Beda --- docs/06-schema-reference.md | 2 + docs/07-cli.md | 29 +++++ docs/08-github-action.md | 15 +++ docs/10-vendoring.md | 168 +++++++++++++++++++++++++ docs/index.md | 1 + internal/deps/deps.go | 7 ++ internal/deps/deps_test.go | 6 +- internal/lint/provenance_test.go | 4 +- internal/provenance/provenance_test.go | 35 +++++- 9 files changed, 262 insertions(+), 5 deletions(-) create mode 100644 docs/10-vendoring.md diff --git a/docs/06-schema-reference.md b/docs/06-schema-reference.md index 85ca977..b6853d2 100644 --- a/docs/06-schema-reference.md +++ b/docs/06-schema-reference.md @@ -335,6 +335,8 @@ Six 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)). + Getting a model from *another* repository into yours is a separate, explicit + step — see [Vendoring](./10-vendoring.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 diff --git a/docs/07-cli.md b/docs/07-cli.md index 4c1eb13..f5917a4 100644 --- a/docs/07-cli.md +++ b/docs/07-cli.md @@ -87,3 +87,32 @@ into another validator. ```sh modelith schema > modelith.schema.json ``` + +## `modelith deps` + +Manages models **vendored** from other repositories — copies committed here and +marked with a provenance header. This is the only command group that uses the +network; `lint` and `render` never do. + +### `modelith deps import [dir]` + +Fetches a model and writes it into `dir` (the working directory by default) as +a vendored copy. + +```sh +modelith deps import https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml docs/ +``` + +| Argument / flag | Meaning | +|---|---| +| `` | The address of the file as it appears in a browser on github.com. | +| `[dir]` | Destination **directory**, defaulting to `.`. The filename always comes from the origin. | +| `--ref` | Ref to fetch, overriding the one in the URL. A tag pins the copy; also how you disambiguate a branch name containing a slash. | + +Fetching is delegated to [`gh`](https://cli.github.com), which must be installed +and authenticated. The command writes the file and prints the `imports:` entry +to add — it does not edit your model. + +See [Vendoring a model from another +repository](./10-vendoring.md) for what the header records, how a vendored file +is linted differently, and why vendoring fetches one file rather than a tree. diff --git a/docs/08-github-action.md b/docs/08-github-action.md index 3a67014..e8eae87 100644 --- a/docs/08-github-action.md +++ b/docs/08-github-action.md @@ -52,6 +52,21 @@ and verifies it against the release's published checksums before running it. The pinning your `uses:` reference to a commit SHA, this keeps CI runs reproducible: a given action commit always installs the same `modelith` version. +## Vendored models in the glob + +If your glob matches a model [vendored from another +repository](./10-vendoring.md), the action treats it as somebody else's +document: + +- **Completeness gaps in it are not reported**, so `completeness: error` cannot + fail your build over a model you did not write. +- **`check-rendered` skips it**, so you are not asked to commit a `.md` whose + home is another repository. + +Everything else still applies. A vendored file that is not a valid domain +model, or that has been edited since it was imported, fails the build — both +are about *your* repository's copy, and both are yours to fix. + ## Regenerating the Markdown The action **gates**; it does not commit. When `check-rendered` fails, run diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md new file mode 100644 index 0000000..f5a93bd --- /dev/null +++ b/docs/10-vendoring.md @@ -0,0 +1,168 @@ +--- +sidebar_position: 10 +title: Vendoring a model from another repository +description: Copy a model from another repository, keep track of where it came from, and reference its items. +--- + +# Vendoring a model from another repository + +[Imports](./06-schema-reference.md#imports) let one model reference an item +another one defines — but only across files that are already in your +repository. **Vendoring** is how a model from *somewhere else* gets there: you +fetch a copy, commit it, and modelith records where it came from. + +```sh +modelith deps import https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml docs/ +``` + +That URL is the address of the file as it appears in your browser on +github.com — open the model on GitHub and copy the address bar. + +## What you get + +A copy of the file, byte for byte, with a **provenance header** added at the +top: + +```yaml +# yaml-language-server: $schema=https://modelith.sh/schema/domain-model/v1.json +# modelith-vendored: DO NOT EDIT — this file is a copy. Change it at its origin. +# modelith-fetch: git +# modelith-origin: https://github.com/acme/billing +# modelith-path: docs/payments.modelith.yaml +# modelith-ref: main +# modelith-commit: 4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21 +# modelith-imported: 2026-07-27 +# modelith-digest: sha256:9a1f… +``` + +The header is a comment, not part of the schema — a model you write yourself +never has one, and never meets any of this. + +| Key | What it records | +|---|---| +| `vendored` | That this file is a copy. Nothing enforces it; it is there so a person or an agent about to edit the file stops. | +| `fetch` | How to get it again. `git` today. | +| `origin`, `path`, `ref` | Where it came from and what to track. A tag in `ref` pins the copy; a branch follows it. | +| `commit` | The commit that last touched *this file* at that ref — so it does not move when unrelated commits land. | +| `imported` | When you fetched it. | +| `digest` | SHA-256 of the file with the header lines removed, so stamping the header does not change it. | + +## Then add it to your model + +`deps import` writes the file and stops. It does **not** edit your model's +`imports:` — it prints the line to add: + +```yaml +imports: + - ./payments.modelith.yaml +``` + +The path is relative to the model doing the importing, exactly like any other +import. Until you add that line, the copy is an inert file that nothing reads. + +That second step is deliberate. A vendored model is content someone else wrote +that will be rendered into *your* published Markdown, so `deps import` warns you +and leaves the decision — and the diff — visible. + +:::warning[Only vendor from sources you trust] + +A vendored model's prose ends up in your rendered `.md`. modelith escapes HTML +in prose fields, but the file is still somebody else's text landing in your +docs. Vendoring is designed for projects that already trust each other. + +::: + +## How a vendored file is treated differently + +Once a file carries a provenance header, modelith knows it is not your work. +Two things change, and nothing else: + +- **Completeness findings are suppressed.** Missing invariants, entities no + scenario exercises, unused enums and glossary terms are gaps in a document + its own authors control. Without this, the [GitHub + Action](./08-github-action.md) — which lints every matched file — would fail + your build over someone else's model. +- **Its own `imports:` raise nothing.** A vendored model's imports name paths + in *its* repository, which do not exist in yours. Those are skipped, along + with the references that resolve through them. + +**Structural and semantic checks still run.** A vendored file that is not a +valid domain model breaks your build, and that is your problem to solve — by +fetching a different ref, or by talking to whoever owns it. + +`modelith render --check` skips vendored files too: their rendered Markdown +belongs to their home repository, so you are not asked to commit one. +Rendering a vendored model by naming it still works, which is how a deep link +into it gets something to point at. + +## Keeping the copy honest + +Every `modelith lint` re-checks a vendored file against the digest in its own +header. If someone edits the copy, lint says so: + +``` +error [semantic] (root): this vendored file no longer matches the digest its +provenance header records (recorded sha256:9a1f…, computed sha256:2c7b…) — it +has been edited since it was imported. Refresh it with `modelith deps import +https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml`, or +delete the provenance header if the change is a deliberate fork, which makes +this repository the file's home. +``` + +Both remedies are real. Re-running `deps import` replaces the copy with what +upstream has now. Deleting the header makes the file an ordinary model of +yours — an honest description of having forked it, and it re-enables the +completeness checks, because now it *is* your document. + +This is drift detection, not a security boundary: anyone editing the file can +recompute the header. It catches the well-meaning typo fix, which is the thing +that actually happens. + +## Vendoring is one file, not a dependency tree + +If the model you fetch imports models of its own, **those are not fetched**. +`deps import` tells you they exist: + +``` +Note: payments.modelith.yaml declares an import of its own (./ledger.modelith.yaml). +modelith vendors one file, not a dependency tree, and resolution is not +transitive — if you need items from those models, import them directly. +``` + +This matches how [resolution already +works](./06-schema-reference.md#imports): `payments.Thing` reaches only items +defined *directly* in the file bound to `payments`. If the item you want lives +one hop further away, vendor that model too and give it its own scope. The +linter says so at the reference site when it can tell: + +``` +attribute type "payments.Carrier" names no enum "Carrier" in +"./payments.modelith.yaml" — that model imports a model of its own, and +resolution is not transitive: if "Carrier" is defined in one of them, add that +model to this model's `imports:` too and reference it with its own scope. +``` + +Fetching a tree would mean a directory layout, rewritten paths, and an answer +for what happens when two models want different versions of the same third +model — a package manager, for a problem an explicit second `deps import` +already solves. + +## Requirements and limits + +- **`gh` must be installed and authenticated.** modelith implements no network + transport of its own; it delegates to the [GitHub + CLI](https://cli.github.com), which already solves authentication for private + and internal repositories. +- **GitHub only, for now.** A URL on another host is an error that asks you to + [open an issue](https://github.com/stacklok/modelith/issues). That is not a + brush-off: the header records *how* it was fetched, so adding another + transport is straightforward — what is missing is a real user to build it + for, and an issue is how you become one. +- **`lint` and `render` never touch the network**, whatever you pass them + ([ADR-0011](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0011-network-boundary.md)). + Everything under `modelith deps` is opt-in, and nothing else fetches. + +The design and its trade-offs are +[ADR-0010](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0010-cross-model-references-by-vendoring.md) +and +[ADR-0015](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md). diff --git a/docs/index.md b/docs/index.md index b946d4b..911b13d 100644 --- a/docs/index.md +++ b/docs/index.md @@ -55,3 +55,4 @@ fails on drift (`modelith render --check`) — like a generated-code check. | [Schema](./06-schema-reference.md) | The JSON Schema that defines a valid model | | [`modelith` CLI](./07-cli.md) | The `lint` / `render` engine the agent and CI run | | [GitHub Action](./08-github-action.md) | The same checks in CI | +| [Vendoring](./10-vendoring.md) | Referencing a model that lives in another repository | diff --git a/internal/deps/deps.go b/internal/deps/deps.go index f459335..319ad9c 100644 --- a/internal/deps/deps.go +++ b/internal/deps/deps.go @@ -34,6 +34,13 @@ type ExecRunner struct{} // Run executes name with args and returns its standard output. Standard error // is folded into the returned error, because gh reports why it refused there. func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + // nolint:gosec // G204 flags a variable command and arguments, which is + // what a transport seam is. The command is the literal "gh" at both call + // sites; the arguments are literals plus an endpoint assembled from a URL + // that ParseSource has already validated, with each path segment and query + // value escaped. Nothing here comes from a model file, and there is no + // shell: exec passes an argv array, so a metacharacter is a byte in an + // argument rather than syntax (ADR-0010). cmd := exec.CommandContext(ctx, name, args...) var stderr strings.Builder cmd.Stderr = &stderr diff --git a/internal/deps/deps_test.go b/internal/deps/deps_test.go index 4c49309..c827181 100644 --- a/internal/deps/deps_test.go +++ b/internal/deps/deps_test.go @@ -251,7 +251,11 @@ func TestImport_EscapesAPathThatNeedsIt(t *testing.T) { } } -func TestImport_ReportsTheirImportsWithoutFetchingThem(t *testing.T) { +// TestADR_0015_ImportFetchesOneFileNotATree pins that vendoring does not +// recurse. The fetched model's own imports are reported to the user and +// nothing more: no extra call goes out, so no file arrives that the user did +// not ask for, at a scope this repository never bound. +func TestADR_0015_ImportFetchesOneFileNotATree(t *testing.T) { t.Parallel() chained := upstream + "imports:\n - ./ledger.modelith.yaml\n - ./tax.modelith.yaml\n" diff --git a/internal/lint/provenance_test.go b/internal/lint/provenance_test.go index f9c1a32..a9685ad 100644 --- a/internal/lint/provenance_test.go +++ b/internal/lint/provenance_test.go @@ -51,7 +51,7 @@ func lintSource(t *testing.T, src string, files fakeFiles) *Result { return res } -// TestVendored_SuppressesCompletenessAndNothingElse pins ADR-0015's rule as a +// TestADR_0015_HeaderSuppressesCompletenessOnly pins ADR-0015's rule as a // difference rather than as a hand-written expectation: the same model is linted // twice, once as this repository's own work and once as a vendored copy, and // every finding that disappears must be a completeness finding. @@ -59,7 +59,7 @@ func lintSource(t *testing.T, src string, files fakeFiles) *Result { // Written this way the test cannot be satisfied by a suppression that is too // broad. A rule that also dropped semantic warnings would pass a test that // merely listed what a vendored file should report. -func TestVendored_SuppressesCompletenessAndNothingElse(t *testing.T) { +func TestADR_0015_HeaderSuppressesCompletenessOnly(t *testing.T) { t.Parallel() files := fakeFiles{".git": ""} diff --git a/internal/provenance/provenance_test.go b/internal/provenance/provenance_test.go index 6ffb2cc..16e7fad 100644 --- a/internal/provenance/provenance_test.go +++ b/internal/provenance/provenance_test.go @@ -180,14 +180,14 @@ func TestParse_Problems(t *testing.T) { } } -// TestDigest_CoversTheFileWithoutItsHeader pins ADR-0015's digest definition. +// TestADR_0015_DigestCoversTheFileWithoutItsHeader pins ADR-0015's digest definition. // // The expectation is built by hand — the header lines written out and removed // by the test itself, then hashed with crypto/sha256 directly — rather than by // calling Strip. A test that reuses the implementation's own helper would agree // with it however wrong the helper was, which is exactly how two HIGH bugs got // past CI in the renderer work. -func TestDigest_CoversTheFileWithoutItsHeader(t *testing.T) { +func TestADR_0015_DigestCoversTheFileWithoutItsHeader(t *testing.T) { t.Parallel() sum := sha256.Sum256([]byte(plain)) @@ -201,6 +201,37 @@ func TestDigest_CoversTheFileWithoutItsHeader(t *testing.T) { } } +// TestADR_0015_UnknownFetchMethodIsAnError pins that the fetch method is a +// closed set. A header naming a method this build does not implement is +// reported rather than being read as if it were git — which would verify a +// digest against keys that mean something else — and the diagnostic asks for an +// issue, because a real user wanting another transport is what justifies +// writing one. +func TestADR_0015_UnknownFetchMethodIsAnError(t *testing.T) { + t.Parallel() + + src := strings.Replace(vendored, LinePrefix+"fetch: git", LinePrefix+"fetch: carrier-pigeon", 1) + _, problems := Parse([]byte(src)) + + var found bool + for _, p := range problems { + if strings.Contains(p.Message, `unknown fetch method "carrier-pigeon"`) { + found = true + if !strings.Contains(p.Message, issuesHint) { + t.Errorf("the diagnostic does not ask for an issue: %s", p.Message) + } + if !strings.Contains(p.Message, `"git"`) { + t.Errorf("the diagnostic does not name the methods that work: %s", p.Message) + } + } + } + if !found { + t.Fatalf("no unknown-method problem; got %+v", problems) + } +} + +const issuesHint = "https://github.com/stacklok/modelith/issues" + func TestDigest_ChangesWithTheContent(t *testing.T) { t.Parallel() From 818babe3be91ce371b21c1bb0c069161ff8218ed Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 15:01:23 -0700 Subject: [PATCH 09/13] fix(provenance): treat only the leading comment block as header ADR-0015 settled that one rule parses and strips the provenance header, so the bytes Parse reads and the bytes Digest covers cannot disagree. Parse applied the leading-comment-block half of that rule; Present and Strip matched a "# modelith-" line anywhere in the file. Two things fell through the gap. An edit that added or removed such a line below the header left the digest byte-identical, so drift detection could not see it. And a model this repository owns that happened to carry one was reclassified as vendored, which silently suppressed its completeness findings. Both now read the same leadingCommentBlock the parser does. A "# modelith-" line below the header is content: it moves the digest, and Parse still reports it as misplaced rather than reading it. Signed-off-by: Joe Beda --- internal/provenance/provenance.go | 38 +++++++++++++++++--------- internal/provenance/provenance_test.go | 23 ++++++++++++++-- 2 files changed, 46 insertions(+), 15 deletions(-) diff --git a/internal/provenance/provenance.go b/internal/provenance/provenance.go index ba8697a..8eee783 100644 --- a/internal/provenance/provenance.go +++ b/internal/provenance/provenance.go @@ -13,11 +13,12 @@ import ( // LinePrefix begins every provenance line. // -// A line counts only when it starts at column zero. An indented "# modelith-" -// comment is an ordinary comment: it is neither parsed nor stripped, so the -// bytes Parse reads and the bytes Digest covers can never disagree about which -// lines are header. That single rule is the whole contract — the digest is -// defined by the strip (ADR-0015). +// A line counts only when it starts at column zero and sits in the leading +// comment block. An indented "# modelith-" comment, or one below the first line +// of model content, is an ordinary comment: it is neither parsed nor stripped, +// so the bytes Parse reads and the bytes Digest covers can never disagree about +// which lines are header. That single rule is the whole contract — the digest +// is defined by the strip (ADR-0015). const LinePrefix = "# modelith-" // lineRE matches one provenance line and splits it into key and value. The @@ -109,12 +110,20 @@ type Problem struct { // this repo's own work would bury the typo under a flood of completeness gaps // about a document nobody here controls. func Present(src []byte) bool { - for _, line := range strings.Split(string(src), "\n") { + return headerLines(strings.Split(string(src), "\n")) > 0 +} + +// headerLines counts the provenance lines in the leading comment block, which +// are the only lines that are header at all — the rule Parse, Strip, and +// Present all read from, so none of them can disagree with the others. +func headerLines(lines []string) int { + n := 0 + for _, line := range lines[:leadingCommentBlock(lines)] { if lineRE.MatchString(line) { - return true + n++ } } - return false + return n } // Parse reads the provenance header from src. It returns a nil Header when src @@ -140,7 +149,7 @@ func Parse(src []byte) (*Header, []Problem) { num, key, value := i+1, m[1], strings.TrimSpace(m[2]) if i >= lead { problems = append(problems, Problem{num, fmt.Sprintf( - "provenance line %q is below the leading comment block — the whole header belongs at the top of the file, before any model content", + "provenance line %q is below the leading comment block, so it was read as an ordinary comment rather than as header — the whole header belongs at the top of the file, before any model content", LinePrefix+key)}) continue } @@ -209,13 +218,16 @@ func (h *Header) Verify(src []byte) (ok bool, got string) { return got == h.Digest, got } -// Strip returns src with every provenance line removed, which is the content -// the digest covers. +// Strip returns src with its provenance lines removed, which is the content the +// digest covers. Only the leading comment block is stripped: a "# modelith-" +// line below it is model content's neighbour, not header, and Parse reports it +// as misplaced rather than reading it (see LinePrefix). func Strip(src []byte) []byte { lines := strings.Split(string(src), "\n") + lead := leadingCommentBlock(lines) kept := lines[:0] - for _, line := range lines { - if lineRE.MatchString(line) { + for i, line := range lines { + if i < lead && lineRE.MatchString(line) { continue } kept = append(kept, line) diff --git a/internal/provenance/provenance_test.go b/internal/provenance/provenance_test.go index 16e7fad..be8ca81 100644 --- a/internal/provenance/provenance_test.go +++ b/internal/provenance/provenance_test.go @@ -42,6 +42,7 @@ func TestPresent(t *testing.T) { {"an ordinary comment that only looks like one", "#modelith-fetch: git\nkind: DomainModel\n", false}, {"an indented provenance line is an ordinary comment", " # modelith-fetch: git\nkind: DomainModel\n", false}, {"a broken header is still a header", "# modelith-nonsense: x\nkind: DomainModel\n", true}, + {"a provenance line below the model content is an ordinary comment", plain + "# modelith-fetch: git\n", false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -153,8 +154,8 @@ func TestParse_Problems(t *testing.T) { }, { name: "a provenance line below the model content is misplaced", - src: plain + "# modelith-fetch: git\n", - wantLine: 5, + src: "# modelith-fetch: git\n" + plain + "# modelith-origin: https://github.com/stacklok/some-repo\n", + wantLine: 6, contains: "below the leading comment block", }, } @@ -259,6 +260,24 @@ func TestDigest_IgnoresEveryHeaderChange(t *testing.T) { } } +// TestDigest_CoversALineBelowTheHeader pins the other half of the strip rule: +// only the leading comment block is header, so a "# modelith-" line anywhere +// below it is content the digest covers. Stripping it there would make an edit +// that adds or removes one invisible to drift detection — and would make a model +// this repository owns, which happened to carry such a comment, read as +// somebody else's copy. +func TestDigest_CoversALineBelowTheHeader(t *testing.T) { + t.Parallel() + + added := vendored + "# modelith-fetch: git\n" + if Digest([]byte(added)) == Digest([]byte(vendored)) { + t.Error("a provenance-looking line added below the model left the digest unchanged") + } + if !strings.Contains(string(Strip([]byte(added))), "# modelith-fetch: git") { + t.Error("Strip removed a line below the leading comment block") + } +} + func TestVerify(t *testing.T) { t.Parallel() From e220214b68176f5a91c6b2d9708dc6ff321b101b Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 15:02:27 -0700 Subject: [PATCH 10/13] fix(render): check a vendored copy's committed Markdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --check skipped a vendored model unconditionally. That is right while no .md exists for it — its rendered form belongs to its home repository, and --check runs over globs. It is wrong once this repository commits one, which is the documented way a deep link into a vendored model's Markdown gets a target: refreshing the copy then left that .md describing the version before it, with nothing to say so. The skip is now conditional on there being no committed output. What stays skipped is what the origin owns rather than this repository: a copy this build cannot render at all — a newer schema version, a shape this binary does not know — is reported by `modelith lint`, once, and repeating it here would fail a build over a defect nobody can fix from this side. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 37 ++++++++++++++---- cmd/modelith/main_test.go | 82 ++++++++++++++++++++++++++++++++++----- docs/08-github-action.md | 10 ++++- docs/10-vendoring.md | 14 +++++-- 4 files changed, 121 insertions(+), 22 deletions(-) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 00eab8e..7206e86 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "io/fs" "os" "path/filepath" "runtime/debug" @@ -309,20 +310,43 @@ func renderCmd() *cobra.Command { if err != nil { return fmt.Errorf("%s: %w", in, err) } + target := out + if target == "" { + target = defaultOut(in) + } + // A vendored model's rendered form belongs to its home repository, // so it arrives with no committed .md and no obligation to carry // one. --check runs over globs, and demanding one here would make // this repository regenerate somebody else's document every time // their model moved (ADR-0015). Naming the file to render it still // renders it; that is how a deep link into a vendored model's .md - // gets something to point at. - if check && provenance.Present(data) { - fmt.Fprintf(cmd.OutOrStdout(), "%s is a vendored copy — skipped\n", in) + // gets something to point at — and once such an .md is committed, it + // goes stale like any other, so from then on --check does check it. + // + // What stays skipped is everything the origin owns rather than this + // repository: a copy this build cannot render at all — a newer schema + // version, a shape this binary does not know — is not a --check + // failure, because there is nothing here to fix. `modelith lint` + // reports it, loudly and once. + vendored := provenance.Present(data) + skipVendored := func(tail string) error { + fmt.Fprintf(cmd.OutOrStdout(), "%s is a vendored copy %s\n", in, tail) return nil } + unrenderable := fmt.Sprintf("this modelith cannot render — skipped (run `modelith lint %s` for details)", in) + if check && vendored { + if _, err := os.Stat(target); errors.Is(err, fs.ErrNotExist) { + return skipVendored("with no committed " + filepath.Base(target) + " — skipped") + } + } + // Validate against the schema first so a malformed file fails with a // friendly, located error rather than the raw strict-YAML parse error. if findings := lint.Structural(data); len(findings) > 0 { + if check && vendored { + return skipVendored(unrenderable) + } var b strings.Builder fmt.Fprintf(&b, "%s is not a valid domain model — run `modelith lint %s` for details:", in, in) for _, f := range findings { @@ -336,6 +360,9 @@ func renderCmd() *cobra.Command { } m, err := model.Parse(data) if err != nil { + if check && vendored { + return skipVendored(unrenderable) + } return err } @@ -353,10 +380,6 @@ func renderCmd() *cobra.Command { return err } - target := out - if target == "" { - target = defaultOut(in) - } outDir, err := filepath.Abs(filepath.Dir(target)) if err != nil { return fmt.Errorf("resolving %s: %w", target, err) diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index dc89706..4221575 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -273,21 +273,26 @@ func TestRenderCheckDetectsDrift(t *testing.T) { } } -// TestRenderCheckSkipsAVendoredCopy covers the half of ADR-0015 that lives in -// the CLI: a vendored model has no committed .md here, and --check runs over -// globs, so demanding one would fail a build over somebody else's document. -// Rendering it by name still works — that is what a deep link points at. -func TestRenderCheckSkipsAVendoredCopy(t *testing.T) { - dir := t.TempDir() - header := "# modelith-vendored: " + provenance.Banner + "\n" + +// vendorHeader stamps content the way `deps import` does, so a test fixture is +// a vendored copy that verifies against its own digest. +func vendorHeader(content string) string { + return "# modelith-vendored: " + provenance.Banner + "\n" + "# modelith-fetch: git\n" + "# modelith-origin: https://github.com/stacklok/some-repo\n" + "# modelith-path: docs/m.modelith.yaml\n" + "# modelith-ref: main\n" + "# modelith-commit: 4f2c1e9\n" + "# modelith-imported: 2026-07-27\n" + - "# modelith-digest: " + provenance.Digest([]byte(minimalValid)) + "\n" - yamlPath := writeTemp(t, dir, "m.modelith.yaml", header+minimalValid) + "# modelith-digest: " + provenance.Digest([]byte(content)) + "\n" +} + +// TestRenderCheckSkipsAVendoredCopy covers the half of ADR-0015 that lives in +// the CLI: a vendored model has no committed .md here, and --check runs over +// globs, so demanding one would fail a build over somebody else's document. +// Rendering it by name still works — that is what a deep link points at. +func TestRenderCheckSkipsAVendoredCopy(t *testing.T) { + dir := t.TempDir() + yamlPath := writeTemp(t, dir, "m.modelith.yaml", vendorHeader(minimalValid)+minimalValid) // No .md exists at all, which is the state a fresh `deps import` leaves. out, err := run(t, "render", "--check", yamlPath) @@ -310,6 +315,65 @@ func TestRenderCheckSkipsAVendoredCopy(t *testing.T) { } } +// TestRenderCheckChecksACommittedVendoredMarkdown pins the other side of the +// skip: once this repository has committed a vendored model's .md — the way a +// deep link into it gets a target — that file goes stale like any other, and +// nothing else would say so. Skipping it unconditionally left a refreshed copy +// with a rendered document describing the version before it. +func TestRenderCheckChecksACommittedVendoredMarkdown(t *testing.T) { + dir := t.TempDir() + yamlPath := writeTemp(t, dir, "m.modelith.yaml", vendorHeader(minimalValid)+minimalValid) + + if _, err := run(t, "render", yamlPath); err != nil { + t.Fatalf("rendering a vendored model by name failed: %v", err) + } + out, err := run(t, "render", "--check", yamlPath) + if err != nil { + t.Fatalf("--check failed on a freshly rendered copy: %v (%s)", err, out) + } + if !strings.Contains(out, "up to date") { + t.Errorf("output does not report the check: %q", out) + } + + writeTemp(t, dir, "m.modelith.md", "stale content\n") + if _, err := run(t, "render", "--check", yamlPath); err == nil { + t.Fatal("a stale committed .md for a vendored copy passed --check") + } else if !strings.Contains(err.Error(), "out of date") { + t.Fatalf("expected an out-of-date error, got: %v", err) + } +} + +// TestRenderCheckStaysQuietOnAVendoredCopyItCannotRender pins that --check does +// not turn this repository's build red over a defect in a document it does not +// own. A copy fetched from a repository on a newer modelith can be unreadable +// here; `modelith lint` reports that, once, and --check has nothing to add +// because there is no .md this repository could regenerate. +func TestRenderCheckStaysQuietOnAVendoredCopyItCannotRender(t *testing.T) { + dir := t.TempDir() + future := strings.Replace(minimalValid, "version: v1", "version: v99", 1) + if future == minimalValid { + t.Fatal("fixture did not declare a version to replace") + } + yamlPath := writeTemp(t, dir, "m.modelith.yaml", vendorHeader(future)+future) + writeTemp(t, dir, "m.modelith.md", "a rendered form from a modelith that understood it\n") + + out, err := run(t, "render", "--check", yamlPath) + if err != nil { + t.Fatalf("--check failed on an unrenderable vendored copy: %v (%s)", err, out) + } + if !strings.Contains(out, "cannot render") { + t.Errorf("output does not say why it was skipped: %q", out) + } + + // The same file, owned rather than vendored, is still this repository's + // problem and still fails. + ownPath := writeTemp(t, dir, "own.modelith.yaml", future) + writeTemp(t, dir, "own.modelith.md", "whatever\n") + if _, err := run(t, "render", "--check", ownPath); err == nil { + t.Fatal("an unrenderable model this repository owns passed --check") + } +} + // TestDepsImportOutput covers what a completed import tells the user. All three // parts are load-bearing: the snippet is the second, manual step this command // deliberately leaves to the user, the note is the whole answer to diff --git a/docs/08-github-action.md b/docs/08-github-action.md index e8eae87..7ab8bcc 100644 --- a/docs/08-github-action.md +++ b/docs/08-github-action.md @@ -60,8 +60,14 @@ document: - **Completeness gaps in it are not reported**, so `completeness: error` cannot fail your build over a model you did not write. -- **`check-rendered` skips it**, so you are not asked to commit a `.md` whose - home is another repository. +- **`check-rendered` skips it when you have not committed a `.md` for it**, so + you are not asked to commit one whose home is another repository. If you *do* + commit one — the way a deep link into a vendored model's Markdown gets a + target — it is checked for staleness from then on. +- **`check-rendered` also skips a copy this modelith cannot render**, such as + one written against a newer schema version. `lint` reports that, and repeating + it here would fail your build twice over one problem you cannot fix in your + own repository. Everything else still applies. A vendored file that is not a valid domain model, or that has been edited since it was imported, fails the build — both diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md index f5a93bd..8393e40 100644 --- a/docs/10-vendoring.md +++ b/docs/10-vendoring.md @@ -90,10 +90,16 @@ Two things change, and nothing else: valid domain model breaks your build, and that is your problem to solve — by fetching a different ref, or by talking to whoever owns it. -`modelith render --check` skips vendored files too: their rendered Markdown -belongs to their home repository, so you are not asked to commit one. -Rendering a vendored model by naming it still works, which is how a deep link -into it gets something to point at. +`modelith render --check` skips a vendored file that has no committed `.md`: +its rendered Markdown belongs to its home repository, so you are not asked to +commit one. Rendering a vendored model by naming it still works, which is how a +deep link into it gets something to point at — and once you commit that `.md`, +`--check` treats it like any other and tells you when refreshing the copy has +left it stale. + +The one thing `--check` will not do is fail over a vendored model this modelith +cannot render at all — one written against a newer schema version, say. It says +it skipped it and moves on; `modelith lint` is where that is reported, once. ## Keeping the copy honest From 67015c401cbfe2f65e633a2a0b6cdb26ea84bd90 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 15:02:45 -0700 Subject: [PATCH 11/13] fix(deps): guard the destination, and stop hiding why an import failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five defects in `deps import`, all found by a review round on this branch. Import inspected the destination only with os.Stat, to decide whether to report "wrote" or "replaced". A copy therefore overwrote whatever shared its basename — including a model this repository wrote, which no re-fetch can bring back. guardTarget now refuses a file with no provenance header, and one vendored from a different origin or path; refreshing an earlier copy of the same model still goes through. A failed model.Parse was collapsed into "is not a domain model — check that the URL names a *.modelith.yaml file", so a model authored with a newer modelith sent the user to check the URL, which was fine. The parse error is now wrapped, and a wrong `kind` says which one it found. --ref cannot both pin a different ref and re-split a URL whose own ref contains a slash; nothing can, from a browse URL alone. The failed fetch now says what modelith read as the ref and as the path, and names both remedies. The host check was case-sensitive, so a "GitHub.com" URL was rejected with "only from github.com, and ... is on \"GitHub.com\"". And ParseSource accepted "." and ".." path segments, which escapePath cannot neutralise — escaping a segment's characters leaves a segment that *is* a traversal exactly as it was, so the endpoint could leave the repository's contents namespace. The nolint justification on ExecRunner.Run leaned on that escaping; rejecting dot segments makes the claim it states true. Finally, the printed `imports:` entry was always "./", ignoring the destination directory, so `deps import docs/vendor/` printed a path that resolves to nothing. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 20 ++++++- cmd/modelith/main_test.go | 15 +++++- docs/07-cli.md | 11 +++- docs/10-vendoring.md | 23 ++++++-- internal/deps/deps.go | 98 ++++++++++++++++++++++++++++++---- internal/deps/deps_test.go | 104 ++++++++++++++++++++++++++++++++++++- 6 files changed, 253 insertions(+), 18 deletions(-) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 7206e86..6a462d3 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -175,8 +175,13 @@ func printImportResult(out, errOut io.Writer, res *deps.Result) { } name := filepath.Base(res.Path) fmt.Fprintf(out, "%s %s at %s\n", verb, res.Path, res.Header.Commit) - fmt.Fprintf(out, "\nAdd it to the model that references it, as a path relative to that model:\n\n"+ - " imports:\n - ./%s\n", name) + // The entry is printed relative to the working directory, because that is + // the only thing this command knows: an import path is relative to the model + // that declares it, and which model that will be is the user's to decide — + // the same reason the imports list is not edited here. + fmt.Fprintf(out, "\nAdd it to the model that references it, as a path relative to that model "+ + "(this one is relative to the current directory):\n\n"+ + " imports:\n - %s\n", importEntry(res.Path)) if n := len(res.TheirImports); n > 0 { declares := fmt.Sprintf("declares %d imports of its own", n) if n == 1 { @@ -191,6 +196,17 @@ func printImportResult(out, errOut io.Writer, res *deps.Result) { "into your published Markdown. Only vendor from sources you trust.\n") } +// importEntry writes a path the way an `imports:` entry is written: slash +// separated, and explicitly relative so it reads as a path rather than as a +// scope. +func importEntry(p string) string { + p = filepath.ToSlash(p) + if strings.HasPrefix(p, "/") || strings.HasPrefix(p, "./") || strings.HasPrefix(p, "../") { + return p + } + return "./" + p +} + // ---- lint ---- func lintCmd() *cobra.Command { diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index 4221575..433349c 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -388,7 +388,10 @@ func TestDepsImportOutput(t *testing.T) { t.Run("a leaf model", func(t *testing.T) { var out, errOut bytes.Buffer printImportResult(&out, &errOut, res) - for _, want := range []string{"wrote docs/payments.modelith.yaml at 4f2c1e9", "imports:", "- ./payments.modelith.yaml"} { + // The entry names where the copy actually landed. Printing the bare + // basename told a user who passed a directory to paste a path that + // resolves to nothing. + for _, want := range []string{"wrote docs/payments.modelith.yaml at 4f2c1e9", "imports:", "- ./docs/payments.modelith.yaml"} { if !strings.Contains(out.String(), want) { t.Errorf("stdout does not contain %q:\n%s", want, out.String()) } @@ -417,6 +420,16 @@ func TestDepsImportOutput(t *testing.T) { } }) + t.Run("a copy in the working directory", func(t *testing.T) { + var out, errOut bytes.Buffer + here := *res + here.Path = "payments.modelith.yaml" + printImportResult(&out, &errOut, &here) + if !strings.Contains(out.String(), "- ./payments.modelith.yaml") { + t.Errorf("stdout does not contain the entry:\n%s", out.String()) + } + }) + t.Run("a replaced copy says so", func(t *testing.T) { var out, errOut bytes.Buffer replaced := *res diff --git a/docs/07-cli.md b/docs/07-cli.md index f5917a4..282e1ea 100644 --- a/docs/07-cli.md +++ b/docs/07-cli.md @@ -107,11 +107,18 @@ modelith deps import https://github.com/acme/billing/blob/main/docs/payments.mod |---|---| | `` | The address of the file as it appears in a browser on github.com. | | `[dir]` | Destination **directory**, defaulting to `.`. The filename always comes from the origin. | -| `--ref` | Ref to fetch, overriding the one in the URL. A tag pins the copy; also how you disambiguate a branch name containing a slash. | +| `--ref` | Ref to fetch, overriding the one in the URL. A tag pins the copy; naming a branch whose name contains a slash is also how you tell modelith where the ref ends and the path begins. | + +A browse URL gives no way to tell a slashed ref from the path after it, so +modelith splits at the first segment. `--ref` fixes that split only when it +names the ref that is *in* the URL — it cannot both pin a different ref and +re-split the path. To pin, open the file on the ref you want and import that +URL. When a fetch fails, the error says how the URL was split. Fetching is delegated to [`gh`](https://cli.github.com), which must be installed and authenticated. The command writes the file and prints the `imports:` entry -to add — it does not edit your model. +to add — it does not edit your model. It refuses to overwrite a file at the +destination that is not an earlier copy of the same model. See [Vendoring a model from another repository](./10-vendoring.md) for what the header records, how a vendored file diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md index 8393e40..ea10aeb 100644 --- a/docs/10-vendoring.md +++ b/docs/10-vendoring.md @@ -54,11 +54,14 @@ never has one, and never meets any of this. ```yaml imports: - - ./payments.modelith.yaml + - ./docs/payments.modelith.yaml ``` -The path is relative to the model doing the importing, exactly like any other -import. Until you add that line, the copy is an inert file that nothing reads. +The printed path is relative to the directory you ran the command in, because +that is the only thing `deps import` knows. An import path is relative to the +model that *declares* it, so if that model does not sit beside your working +directory, adjust it — exactly like any other import. Until you add that line, +the copy is an inert file that nothing reads. That second step is deliberate. A vendored model is content someone else wrote that will be rendered into *your* published Markdown, so `deps import` warns you @@ -101,6 +104,20 @@ The one thing `--check` will not do is fail over a vendored model this modelith cannot render at all — one written against a newer schema version, say. It says it skipped it and moves on; `modelith lint` is where that is reported, once. +## What it will not overwrite + +The filename comes from the origin, so a copy can land on a file you already +have. `deps import` refuses two cases rather than clobbering them: + +- **A model you wrote.** No provenance header means the file is yours, and no + re-fetch could bring it back. Import into a different directory, or move the + file aside first. +- **A copy of a *different* model with the same basename.** Two `payments.modelith.yaml` + files from two repositories cannot share a directory; give them separate ones. + +Re-importing over an existing copy of the *same* model is the ordinary refresh, +and that goes through — it reports `replaced` rather than `wrote`. + ## Keeping the copy honest Every `modelith lint` re-checks a vendored file against the digest in its own diff --git a/internal/deps/deps.go b/internal/deps/deps.go index 319ad9c..05cf2a8 100644 --- a/internal/deps/deps.go +++ b/internal/deps/deps.go @@ -10,6 +10,7 @@ import ( "context" "errors" "fmt" + "io/fs" "net/url" "os" "os/exec" @@ -38,7 +39,10 @@ func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, // what a transport seam is. The command is the literal "gh" at both call // sites; the arguments are literals plus an endpoint assembled from a URL // that ParseSource has already validated, with each path segment and query - // value escaped. Nothing here comes from a model file, and there is no + // value escaped and traversal segments rejected outright (escaping a + // segment cannot neutralise a segment that *is* a traversal, so ParseSource + // refuses those rather than passing them on). Nothing here comes from a + // model file, and there is no // shell: exec passes an argv array, so a metacharacter is a byte in an // argument rather than syntax (ADR-0010). cmd := exec.CommandContext(ctx, name, args...) @@ -69,14 +73,18 @@ type Source struct { // ParseSource reads a GitHub blob URL — the address of the file as it appears // in a browser — into its parts. A non-empty ref overrides the one in the URL, -// which is also how a branch whose name contains a slash is disambiguated from -// the path that follows it. +// and when it is the ref the URL names, it is also what disambiguates a branch +// whose name contains a slash from the path that follows it. It cannot do both +// at once: overriding with a *different* ref leaves the split to be taken at the +// first segment, which splitHint explains when the fetch that follows fails. func ParseSource(raw, ref string) (Source, error) { u, err := url.Parse(strings.TrimSpace(raw)) if err != nil { return Source{}, fmt.Errorf("%q is not a URL: %w", raw, err) } - if u.Host != "github.com" { + // A host is case-insensitive, and a browser hands back the "www." form as + // readily as the bare one; neither is a different site. + if host := strings.TrimPrefix(strings.ToLower(u.Host), "www."); host != "github.com" { return Source{}, fmt.Errorf( "modelith can currently fetch only from github.com, and %q is on %q. Support for other hosts is not written yet because nobody has needed it — if you do, please open an issue at %s saying where your models live", raw, u.Host, issuesURL) @@ -89,6 +97,19 @@ func ParseSource(raw, ref string) (Source, error) { "%q is not a GitHub file URL — it should look like https://github.com///blob//, which is the address you get by opening the file on github.com", raw) } + // A dot segment or an empty one is not part of any address github.com + // serves, and url.Parse does not remove one. It has to be rejected here + // rather than escaped later: escapePath escapes the characters within a + // segment, which leaves a segment that *is* a traversal exactly as it was, + // and the endpoint fetchContent builds would then leave the repository's + // contents namespace. + for _, p := range parts { + if p == "" || p == "." || p == ".." { + return Source{}, fmt.Errorf( + "%q has a %q path segment, which is not part of a file's address on github.com — copy the address bar from the file's page rather than assembling the URL by hand", + raw, p) + } + } src := Source{ Owner: parts[0], Repo: parts[1], @@ -163,7 +184,7 @@ func Import(ctx context.Context, opts Options) (*Result, error) { content, err := fetchContent(ctx, runner, src) if err != nil { - return nil, err + return nil, fmt.Errorf("%w%s", err, splitHint(src)) } if provenance.Present(content) { return nil, fmt.Errorf( @@ -171,8 +192,17 @@ func Import(ctx context.Context, opts Options) (*Result, error) { opts.URL) } m, err := model.Parse(content) - if err != nil || m.Kind != "DomainModel" { - return nil, fmt.Errorf("%s is not a domain model — check that the URL names a *.modelith.yaml file", opts.URL) + if err != nil { + return nil, fmt.Errorf( + "%s did not parse as a domain model — check that the URL names a *.modelith.yaml file, and that this modelith is new enough to read it: %w", + opts.URL, err) + } + if m.Kind != "DomainModel" { + declares := fmt.Sprintf("declares kind %q", m.Kind) + if m.Kind == "" { + declares = "declares no kind" + } + return nil, fmt.Errorf("%s is not a domain model — it %s, not \"DomainModel\"", opts.URL, declares) } commit, err := fetchCommit(ctx, runner, src) @@ -192,8 +222,10 @@ func Import(ctx context.Context, opts Options) (*Result, error) { } target := filepath.Join(opts.Dir, path.Base(src.Path)) - _, statErr := os.Stat(target) - replaced := statErr == nil + replaced, err := guardTarget(target, src) + if err != nil { + return nil, err + } if err := os.WriteFile(target, provenance.Stamp(content, h), 0o644); err != nil { return nil, fmt.Errorf("writing %s: %w", target, err) } @@ -205,6 +237,54 @@ func Import(ctx context.Context, opts Options) (*Result, error) { return res, nil } +// guardTarget decides whether writing to target is safe, and reports whether an +// existing copy is being replaced. +// +// The destination filename comes from the origin, so it can collide with a file +// this repository already has: a model of its own, or a copy of a different +// model that happens to share a basename. Import refuses to fetch a file that is +// already somebody else's copy; the same care is owed to what it writes over, +// because a model this repository wrote is not recoverable by fetching again. +func guardTarget(target string, src Source) (replaced bool, err error) { + existing, readErr := os.ReadFile(target) + if errors.Is(readErr, fs.ErrNotExist) { + return false, nil + } + if readErr != nil { + return false, fmt.Errorf("reading %s: %w", target, readErr) + } + if !provenance.Present(existing) { + return false, fmt.Errorf( + "%s already exists and carries no provenance header, so it is a model this repository owns rather than a copy of one — importing would overwrite it. Import into a different directory, or move that file aside first", + target) + } + // A header too malformed to name where it came from is still a vendored + // copy, and replacing it is how it gets repaired; only a header that names + // a *different* model blocks the write. + h, _ := provenance.Parse(existing) + if h.Origin != "" && h.Path != "" && (h.Origin != src.Origin || h.Path != src.Path) { + return false, fmt.Errorf( + "%s is a vendored copy of %s/%s, not of %s/%s — two different models share that filename. Import into a different directory so both can live here", + target, h.Origin, h.Path, src.Origin, src.Path) + } + return true, nil +} + +// splitHint explains the one way ParseSource can be wrong about a URL it +// accepted. A browse URL gives no way to tell where a ref containing a slash +// ends and the path begins, so the split is taken at the first segment; a failed +// fetch is where that guess surfaces, as a 404 that looks like the file is +// simply not there. A single-segment path had nothing to lose to the ref, so it +// gets no hint. +func splitHint(src Source) string { + if !strings.Contains(src.Path, "/") { + return "" + } + return fmt.Sprintf( + "\n\nmodelith read %q as the ref and %q as the path. If the ref has a slash in it that split is wrong: pass the whole ref to --ref, or — if --ref is already pinning a different ref, which cannot re-split the path — open the file on the ref you want and import that URL instead", + src.Ref, src.Path) +} + // fetchContent returns the file's bytes. The raw media type asks the API for // the content itself rather than a JSON envelope carrying it base64-encoded, so // nothing here has to decode. diff --git a/internal/deps/deps_test.go b/internal/deps/deps_test.go index c827181..ca3355a 100644 --- a/internal/deps/deps_test.go +++ b/internal/deps/deps_test.go @@ -106,6 +106,29 @@ func TestParseSource(t *testing.T) { Ref: "release/v2", Path: "docs/payments.modelith.yaml", }, }, + { + // A host is case-insensitive. Rejecting this one produced the + // self-contradictory "only from github.com, and … is on GitHub.com". + name: "the host is matched without regard to case or a www prefix", + raw: "https://WWW.GitHub.com/acme/billing/blob/main/docs/payments.modelith.yaml", + want: Source{ + Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", + Ref: "main", Path: "docs/payments.modelith.yaml", + }, + }, + { + // url.Parse does not remove dot segments, and escapePath cannot + // neutralise a segment that *is* a traversal — it would reach the + // API endpoint intact and leave the contents namespace. + name: "a traversal segment is not an address github.com serves", + raw: "https://github.com/acme/billing/blob/main/../../../user/repos", + wantErr: `has a ".." path segment`, + }, + { + name: "an empty segment is rejected too", + raw: "https://github.com/acme/billing/blob/main//payments.modelith.yaml", + wantErr: `has a "" path segment`, + }, { name: "another host asks for an issue rather than guessing", raw: "https://gitlab.com/acme/billing/-/blob/main/payments.modelith.yaml", @@ -293,7 +316,22 @@ func TestImport_Rejections(t *testing.T) { name: "a file that is not a domain model", runner: &fakeRunner{content: "kind: SomethingElse\nversion: v1\n", sha: sha}, url: blobURL, - wantErr: "not a domain model", + wantErr: `it declares kind "SomethingElse"`, + }, + { + name: "a file with no kind at all", + runner: &fakeRunner{content: "title: Payments\n", sha: sha}, + url: blobURL, + wantErr: "it declares no kind", + }, + { + // A model authored by a newer modelith parses strictly and fails. + // Collapsing that into "is not a domain model" sent the user to + // check the URL, which is not what went wrong. + name: "a model this build cannot read says what the parser said", + runner: &fakeRunner{content: upstream + "futureField: yes\n", sha: sha}, + url: blobURL, + wantErr: "futureField", }, { name: "a path with no commits at that ref", @@ -307,6 +345,15 @@ func TestImport_Rejections(t *testing.T) { url: blobURL, wantErr: "HTTP 404", }, + { + // The likeliest reason a fetch 404s on a URL that parsed is that + // the ref has a slash in it and the path was split at the first + // segment. Saying so is the difference between a dead end and a fix. + name: "a refused fetch says how the URL was split", + runner: &fakeRunner{content: upstream, sha: sha, fail: "/contents/"}, + url: blobURL, + wantErr: `read "main" as the ref and "docs/payments.modelith.yaml" as the path`, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -327,6 +374,61 @@ func TestImport_Rejections(t *testing.T) { } } +// TestImport_RefusesToOverwriteWhatItDidNotWrite pins the guard on the +// destination. The filename comes from the origin, so it can land on a file this +// repository already has — and overwriting a model this repository wrote loses +// work no re-fetch can recover, which the earlier os.Stat-only check did +// silently, reporting it as an ordinary "replaced". +func TestImport_RefusesToOverwriteWhatItDidNotWrite(t *testing.T) { + t.Parallel() + + const own = "kind: DomainModel\nversion: v1\ntitle: Ours\n" + + cases := []struct { + name string + existing string + wantErr string + }{ + { + name: "a model this repository owns", + existing: own, + wantErr: "carries no provenance header", + }, + { + name: "a copy of a different model with the same filename", + existing: "# modelith-vendored: " + provenance.Banner + "\n" + + "# modelith-fetch: git\n" + + "# modelith-origin: https://github.com/other/repo\n" + + "# modelith-path: docs/payments.modelith.yaml\n" + + "# modelith-ref: main\n# modelith-commit: abc\n" + + "# modelith-imported: 2026-07-27\n" + + "# modelith-digest: " + provenance.Digest([]byte(own)) + "\n" + own, + wantErr: "is a vendored copy of https://github.com/other/repo", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + target := filepath.Join(dir, "payments.modelith.yaml") + if err := os.WriteFile(target, []byte(tc.existing), 0o644); err != nil { + t.Fatal(err) + } + _, err := importInto(t, dir, &fakeRunner{content: upstream, sha: sha}, blobURL) + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("want an error containing %q, got %v", tc.wantErr, err) + } + got, readErr := os.ReadFile(target) + if readErr != nil { + t.Fatal(readErr) + } + if string(got) != tc.existing { + t.Errorf("the refused import wrote over the file anyway:\n%s", got) + } + }) + } +} + func TestImport_ReplacesAnEarlierCopy(t *testing.T) { t.Parallel() From 789dd530194749f1e890dd1e79918aa9fe609692 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 15:16:28 -0700 Subject: [PATCH 12/13] fix(provenance,render): keep a broken header loud, and a bad -o path failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round two of the review loop, over round one's own fixes. Making Present ask the same narrow question as Parse and Strip went too far. Prepending one non-comment line to a vendored file — a `---` document marker is enough — puts its whole header below the leading comment block, and the narrow answer then called the file this repository's own work. Every consequence of that was silent: the digest stopped being checked, completeness gaps about somebody else's document appeared, and `deps import` refused to repair the file on the grounds that it carried no header, which was not true. Twelve errors became zero. Present goes back to asking whether a provenance line appears anywhere, which is a classification, not a claim about which lines are header. Parse and Strip keep the narrow rule, so the digest still covers a line snuck in below the header — the defect round one set out to fix. The cost is that a model this repository owns which carries a column-zero "# modelith-" comment still reads as vendored; that failure is loud, and a loud wrong answer beats a silent one. Separately, the vendored skip in `render --check` treated a target under a directory that does not exist as "nothing committed yet", because os.Stat cannot tell that from an uncommitted file. A mistyped -o therefore passed the gate on a vendored model while failing on a model this repository owns. The skip now requires the directory to exist. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 16 ++++++- cmd/modelith/main_test.go | 23 ++++++++++ internal/provenance/provenance.go | 58 +++++++++++++------------- internal/provenance/provenance_test.go | 41 +++++++++++++++++- 4 files changed, 108 insertions(+), 30 deletions(-) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 6a462d3..9b8a109 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -351,8 +351,14 @@ func renderCmd() *cobra.Command { return nil } unrenderable := fmt.Sprintf("this modelith cannot render — skipped (run `modelith lint %s` for details)", in) + // A missing target is the ordinary state for a vendored copy — but + // only when the directory that would hold it exists. A target under + // a directory that does not is a misconfigured -o, and reading that + // as "nothing committed yet" would let a typo pass this gate + // silently, while the same typo on a model this repository owns + // fails. if check && vendored { - if _, err := os.Stat(target); errors.Is(err, fs.ErrNotExist) { + if _, err := os.Stat(target); errors.Is(err, fs.ErrNotExist) && isDir(filepath.Dir(target)) { return skipVendored("with no committed " + filepath.Base(target) + " — skipped") } } @@ -430,6 +436,14 @@ func renderCmd() *cobra.Command { return cmd } +// isDir reports whether p exists and is a directory. Anything else — missing, +// a plain file, unreadable — is false, because every one of those means a +// target under p is not merely uncommitted. +func isDir(p string) bool { + info, err := os.Stat(p) + return err == nil && info.IsDir() +} + // 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 diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index 433349c..9956fcd 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -343,6 +343,29 @@ func TestRenderCheckChecksACommittedVendoredMarkdown(t *testing.T) { } } +// TestRenderCheckRejectsAnOutPathUnderNoDirectory pins that the vendored skip +// does not swallow a misconfigured -o. A target under a directory that does not +// exist stats as ErrNotExist exactly like an uncommitted one, so reading it as +// "nothing committed yet" let a typo pass this gate on a vendored model while +// the same typo on a model this repository owns failed. +func TestRenderCheckRejectsAnOutPathUnderNoDirectory(t *testing.T) { + dir := t.TempDir() + bad := filepath.Join(dir, "nosuchdir", "out.md") + + for _, tc := range []struct{ name, src string }{ + {"a vendored copy", vendorHeader(minimalValid) + minimalValid}, + {"a model this repository owns", minimalValid}, + } { + t.Run(tc.name, func(t *testing.T) { + yamlPath := writeTemp(t, dir, "m.modelith.yaml", tc.src) + out, err := run(t, "render", "--check", "-o", bad, yamlPath) + if err == nil { + t.Fatalf("a target under a missing directory passed --check: %s", out) + } + }) + } +} + // TestRenderCheckStaysQuietOnAVendoredCopyItCannotRender pins that --check does // not turn this repository's build red over a defect in a document it does not // own. A copy fetched from a repository on a newer modelith can be unreadable diff --git a/internal/provenance/provenance.go b/internal/provenance/provenance.go index 8eee783..d707444 100644 --- a/internal/provenance/provenance.go +++ b/internal/provenance/provenance.go @@ -13,12 +13,14 @@ import ( // LinePrefix begins every provenance line. // -// A line counts only when it starts at column zero and sits in the leading -// comment block. An indented "# modelith-" comment, or one below the first line -// of model content, is an ordinary comment: it is neither parsed nor stripped, -// so the bytes Parse reads and the bytes Digest covers can never disagree about -// which lines are header. That single rule is the whole contract — the digest -// is defined by the strip (ADR-0015). +// A line is *header* only when it starts at column zero and sits in the leading +// comment block. Parse reads exactly those lines and Strip removes exactly +// those lines, so the bytes Parse reads and the bytes Digest covers can never +// disagree about which lines are header — the digest is defined by the strip +// (ADR-0015). +// +// Present asks a different question and deliberately answers it more broadly; +// see its own comment. const LinePrefix = "# modelith-" // lineRE matches one provenance line and splits it into key and value. The @@ -103,27 +105,26 @@ type Problem struct { Message string } -// Present reports whether src carries a provenance line at all. +// Present reports whether src carries a provenance line anywhere, which is what +// makes a file vendored. // -// This, not a successful Parse, is what makes a file vendored. A header with a -// typo in it still means the file is somebody else's copy, and treating it as -// this repo's own work would bury the typo under a flood of completeness gaps -// about a document nobody here controls. +// It is deliberately broader than the header rule Parse and Strip share. A +// header with a typo in it — or one displaced below the leading comment block +// by an edit that prepended a line — still means the file is somebody else's +// copy. Asking the narrow question here would answer "not vendored" for a file +// that plainly is one, and every downstream consequence of that answer is +// silent: the digest stops being checked, completeness gaps about a document +// nobody here controls appear, and `deps import` refuses to repair the file on +// the grounds that it carries no header. Answering broadly instead makes the +// same input loud — Parse reports each displaced line, and the missing-key +// checks fire — which is the failure mode to prefer when the two must differ. func Present(src []byte) bool { - return headerLines(strings.Split(string(src), "\n")) > 0 -} - -// headerLines counts the provenance lines in the leading comment block, which -// are the only lines that are header at all — the rule Parse, Strip, and -// Present all read from, so none of them can disagree with the others. -func headerLines(lines []string) int { - n := 0 - for _, line := range lines[:leadingCommentBlock(lines)] { + for _, line := range strings.Split(string(src), "\n") { if lineRE.MatchString(line) { - n++ + return true } } - return n + return false } // Parse reads the provenance header from src. It returns a nil Header when src @@ -218,10 +219,11 @@ func (h *Header) Verify(src []byte) (ok bool, got string) { return got == h.Digest, got } -// Strip returns src with its provenance lines removed, which is the content the -// digest covers. Only the leading comment block is stripped: a "# modelith-" -// line below it is model content's neighbour, not header, and Parse reports it -// as misplaced rather than reading it (see LinePrefix). +// Strip returns src with its header removed, which is the content the digest +// covers. Only the leading comment block is stripped: a "# modelith-" line +// below it is not header, and Parse reports it as misplaced rather than reading +// it (see LinePrefix). Removing it here would let an edit that added or removed +// one leave the digest untouched, which is the drift the digest exists to see. func Strip(src []byte) []byte { lines := strings.Split(string(src), "\n") lead := leadingCommentBlock(lines) @@ -259,8 +261,8 @@ func (h *Header) Format() string { // directive stays first, and at the top otherwise. func Stamp(src []byte, h *Header) []byte { lines := strings.Split(string(src), "\n") - at := 0 - for i := 0; i < leadingCommentBlock(lines); i++ { + at, lead := 0, leadingCommentBlock(lines) + for i := 0; i < lead; i++ { if strings.HasPrefix(lines[i], "# yaml-language-server:") { at = i + 1 } diff --git a/internal/provenance/provenance_test.go b/internal/provenance/provenance_test.go index be8ca81..794ba6e 100644 --- a/internal/provenance/provenance_test.go +++ b/internal/provenance/provenance_test.go @@ -42,7 +42,7 @@ func TestPresent(t *testing.T) { {"an ordinary comment that only looks like one", "#modelith-fetch: git\nkind: DomainModel\n", false}, {"an indented provenance line is an ordinary comment", " # modelith-fetch: git\nkind: DomainModel\n", false}, {"a broken header is still a header", "# modelith-nonsense: x\nkind: DomainModel\n", true}, - {"a provenance line below the model content is an ordinary comment", plain + "# modelith-fetch: git\n", false}, + {"a header displaced below the model content is still a header", plain + "# modelith-fetch: git\n", true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -278,6 +278,45 @@ func TestDigest_CoversALineBelowTheHeader(t *testing.T) { } } +// TestPresent_ADisplacedHeaderStaysLoud pins the reason Present asks a broader +// question than Parse and Strip do. Prepending one non-comment line to a +// vendored file puts its whole header below the leading comment block. Answering +// the narrow question there would call the file this repository's own work, and +// every consequence of that answer is silent: no digest check, completeness +// gaps about somebody else's document, and a `deps import` that refuses to +// repair the file because it "carries no provenance header". +func TestPresent_ADisplacedHeaderStaysLoud(t *testing.T) { + t.Parallel() + + displaced := "---\n" + vendored + if !Present([]byte(displaced)) { + t.Fatal("a vendored file with its header displaced read as this repository's own work") + } + h, problems := Parse([]byte(displaced)) + if h == nil { + t.Fatal("Parse returned no header for a displaced one") + } + var misplaced, missing int + for _, p := range problems { + switch { + case strings.Contains(p.Message, "below the leading comment block"): + misplaced++ + case strings.Contains(p.Message, "is missing"): + missing++ + } + } + if misplaced != len(keyOrder) { + t.Errorf("reported %d misplaced lines, want one per key (%d)", misplaced, len(keyOrder)) + } + if missing == 0 { + t.Error("no key was reported missing, so the header read as complete") + } + // Nothing was stripped, so the digest cannot match what the header records. + if string(Strip([]byte(displaced))) != displaced { + t.Error("Strip removed a line from below the leading comment block") + } +} + func TestVerify(t *testing.T) { t.Parallel() From 7a9dc6557af070f400469509025eccc4d36e3ffc Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 15:47:18 -0700 Subject: [PATCH 13/13] fix(render,deps): claim the --check exemption with a clean header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round three of the review loop — a real /code-review over rounds one and two. Six findings. The `render --check` skip was gated on provenance.Present, which round two deliberately broadened so a *broken* header still marks a file as somebody else's copy. That is right for lint, which reports the defects and suppresses the completeness noise. It is wrong for a gate: a model this repository owns that merely carried a "# modelith-" comment was exempted from --check and exited 0, so a CI job running only `render --check` lost its gate silently. The argument for broadening Present was that the same input stays loud; on this path it did the opposite. The skip is now claimed by a header that parses cleanly, so an exemption needs a valid claim. A misconfigured -o was checked before the vendored skip but after the unrenderable one, so a bad path on a copy this build cannot render still passed at exit 0 — the hole the guard was added to close, one branch over. The target's directory is now settled before anything can exempt a file. `deps import` printed the destination verbatim, so an absolute directory — what an agent driving the command usually passes — produced an `imports:` entry the linter rejects outright. It is relativized against the working directory now. The ref/path split hint was appended to every fetch failure, so "gh is not installed" arrived with a paragraph about slashed refs attached. It is offered only for a not-found now. guardTarget compared origins byte for byte, refusing a refresh that differed only in the casing GitHub itself ignores, and explaining it by claiming two different models shared the filename. The same message was offered when the model had simply moved upstream, where the remedy it named was wrong. Casing is ignored; a moved path gets its own message naming both remedies. Finally, the refusal to import a file carrying a provenance line asserted the line named an origin, which is not something the code checked. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 57 ++++++++++++++++++++------- cmd/modelith/main_test.go | 72 ++++++++++++++++++++++++++++------ docs/08-github-action.md | 4 ++ docs/10-vendoring.md | 18 ++++++++- internal/deps/deps.go | 45 +++++++++++++++++---- internal/deps/deps_test.go | 80 +++++++++++++++++++++++++++++++++++++- 6 files changed, 239 insertions(+), 37 deletions(-) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 9b8a109..1fded25 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -152,7 +152,11 @@ this command prints the entry to add.`), return err } - printImportResult(cmd.OutOrStdout(), cmd.ErrOrStderr(), res) + // A failure to read the working directory only costs the entry + // its relative form, so it is not worth failing a completed + // import over. + wd, _ := os.Getwd() + printImportResult(cmd.OutOrStdout(), cmd.ErrOrStderr(), res, wd) return nil }, } @@ -168,7 +172,7 @@ this command prints the entry to add.`), // so a prompt is either auto-answered theatre or a wedged non-interactive run. // The fetched file is inert until it is named in an imports list, and this // command deliberately does not do that — the manual step is the real gate. -func printImportResult(out, errOut io.Writer, res *deps.Result) { +func printImportResult(out, errOut io.Writer, res *deps.Result, wd string) { verb := "wrote" if res.Replaced { verb = "replaced" @@ -181,7 +185,7 @@ func printImportResult(out, errOut io.Writer, res *deps.Result) { // the same reason the imports list is not edited here. fmt.Fprintf(out, "\nAdd it to the model that references it, as a path relative to that model "+ "(this one is relative to the current directory):\n\n"+ - " imports:\n - %s\n", importEntry(res.Path)) + " imports:\n - %s\n", importEntry(res.Path, wd)) if n := len(res.TheirImports); n > 0 { declares := fmt.Sprintf("declares %d imports of its own", n) if n == 1 { @@ -196,10 +200,21 @@ func printImportResult(out, errOut io.Writer, res *deps.Result) { "into your published Markdown. Only vendor from sources you trust.\n") } -// importEntry writes a path the way an `imports:` entry is written: slash -// separated, and explicitly relative so it reads as a path rather than as a +// importEntry writes a path the way an `imports:` entry is written: relative, +// slash separated, and explicitly so, so it reads as a path rather than as a // scope. -func importEntry(p string) string { +// +// An absolute destination is relativized against wd, because an absolute import +// is a lint error — imports resolve relative to the model that declares them so +// they work in any checkout. An agent driving this command usually passes an +// absolute directory, so printing one back would hand it a line the linter +// rejects. A "../" result is fine; only an absolute one is not. +func importEntry(p, wd string) string { + if filepath.IsAbs(p) && wd != "" { + if rel, err := filepath.Rel(wd, p); err == nil { + p = rel + } + } p = filepath.ToSlash(p) if strings.HasPrefix(p, "/") || strings.HasPrefix(p, "./") || strings.HasPrefix(p, "../") { return p @@ -331,6 +346,14 @@ func renderCmd() *cobra.Command { target = defaultOut(in) } + // A target under a directory that does not exist is a misconfigured + // -o, not a document waiting to be written. It is settled before + // anything below can exempt a file from the check, so no exemption + // can turn a typo into a pass. + if check && !isDir(filepath.Dir(target)) { + return fmt.Errorf("cannot check %s: %s is not a directory", target, filepath.Dir(target)) + } + // A vendored model's rendered form belongs to its home repository, // so it arrives with no committed .md and no obligation to carry // one. --check runs over globs, and demanding one here would make @@ -345,20 +368,26 @@ func renderCmd() *cobra.Command { // version, a shape this binary does not know — is not a --check // failure, because there is nothing here to fix. `modelith lint` // reports it, loudly and once. - vendored := provenance.Present(data) + // + // The exemption is claimed by a *clean* header, not by + // provenance.Present. Present answers a deliberately broad question + // so that a broken header still classifies a file as somebody else's + // copy, which is right for lint: the header defects are reported and + // the completeness noise is suppressed. Skipping on that same answer + // would be silent, and would hand the exemption to a model this + // repository owns that merely carries a "# modelith-" comment. An + // exemption needs a valid claim, so a file whose header does not + // parse cleanly is checked like any other — its defects are already + // failing `modelith lint`. + header, headerProblems := provenance.Parse(data) + vendored := header != nil && len(headerProblems) == 0 skipVendored := func(tail string) error { fmt.Fprintf(cmd.OutOrStdout(), "%s is a vendored copy %s\n", in, tail) return nil } unrenderable := fmt.Sprintf("this modelith cannot render — skipped (run `modelith lint %s` for details)", in) - // A missing target is the ordinary state for a vendored copy — but - // only when the directory that would hold it exists. A target under - // a directory that does not is a misconfigured -o, and reading that - // as "nothing committed yet" would let a typo pass this gate - // silently, while the same typo on a model this repository owns - // fails. if check && vendored { - if _, err := os.Stat(target); errors.Is(err, fs.ErrNotExist) && isDir(filepath.Dir(target)) { + if _, err := os.Stat(target); errors.Is(err, fs.ErrNotExist) { return skipVendored("with no committed " + filepath.Base(target) + " — skipped") } } diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index 9956fcd..14666f2 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -343,22 +343,24 @@ func TestRenderCheckChecksACommittedVendoredMarkdown(t *testing.T) { } } -// TestRenderCheckRejectsAnOutPathUnderNoDirectory pins that the vendored skip -// does not swallow a misconfigured -o. A target under a directory that does not -// exist stats as ErrNotExist exactly like an uncommitted one, so reading it as -// "nothing committed yet" let a typo pass this gate on a vendored model while -// the same typo on a model this repository owns failed. +// TestRenderCheckRejectsAnOutPathUnderNoDirectory pins that no exemption +// swallows a misconfigured -o. A target under a directory that does not exist +// stats as ErrNotExist exactly like an uncommitted one, so reading it as +// "nothing committed yet" let a typo pass this gate. The unrenderable case is +// the one that survived the first attempt at this: its skip returned after the +// target check rather than before it. func TestRenderCheckRejectsAnOutPathUnderNoDirectory(t *testing.T) { - dir := t.TempDir() - bad := filepath.Join(dir, "nosuchdir", "out.md") + unrenderable := strings.Replace(minimalValid, "version: v1", "version: v99", 1) for _, tc := range []struct{ name, src string }{ {"a vendored copy", vendorHeader(minimalValid) + minimalValid}, {"a model this repository owns", minimalValid}, + {"a vendored copy this build cannot render", vendorHeader(unrenderable) + unrenderable}, } { t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() yamlPath := writeTemp(t, dir, "m.modelith.yaml", tc.src) - out, err := run(t, "render", "--check", "-o", bad, yamlPath) + out, err := run(t, "render", "--check", "-o", filepath.Join(dir, "nosuchdir", "out.md"), yamlPath) if err == nil { t.Fatalf("a target under a missing directory passed --check: %s", out) } @@ -366,6 +368,36 @@ func TestRenderCheckRejectsAnOutPathUnderNoDirectory(t *testing.T) { } } +// TestRenderCheckDoesNotExemptAModelThisRepoOwns pins that the --check skip is +// claimed by a clean provenance header, not by provenance.Present. +// +// Present answers a deliberately broad question so a broken header still marks a +// file as somebody else's copy, which is what lint needs. Skipping on that same +// answer handed the exemption to a model this repository owns that merely +// carried a "# modelith-" comment: lint shouted five errors, but a CI job +// running only `render --check` silently lost its gate and exited 0. +func TestRenderCheckDoesNotExemptAModelThisRepoOwns(t *testing.T) { + dir := t.TempDir() + yamlPath := writeTemp(t, dir, "own.modelith.yaml", + minimalValid+"\n# modelith-note: this file is generated\n") + + out, err := run(t, "render", "--check", yamlPath) + if err == nil { + t.Fatalf("a model this repository owns was exempted from --check: %s", out) + } + if strings.Contains(out, "vendored copy") { + t.Errorf("it was reported as a vendored copy:\n%s", out) + } + + // A header with a real defect in it earns no exemption either: the file is + // checked like any other, and lint reports the defect. + displaced := writeTemp(t, dir, "displaced.modelith.yaml", + "---\n"+vendorHeader(minimalValid)+minimalValid) + if _, err := run(t, "render", "--check", displaced); err == nil { + t.Error("a vendored copy with a displaced header was exempted from --check") + } +} + // TestRenderCheckStaysQuietOnAVendoredCopyItCannotRender pins that --check does // not turn this repository's build red over a defect in a document it does not // own. A copy fetched from a repository on a newer modelith can be unreadable @@ -410,7 +442,7 @@ func TestDepsImportOutput(t *testing.T) { t.Run("a leaf model", func(t *testing.T) { var out, errOut bytes.Buffer - printImportResult(&out, &errOut, res) + printImportResult(&out, &errOut, res, "") // The entry names where the copy actually landed. Printing the bare // basename told a user who passed a directory to paste a path that // resolves to nothing. @@ -431,7 +463,7 @@ func TestDepsImportOutput(t *testing.T) { var out, errOut bytes.Buffer chained := *res chained.TheirImports = []string{"./ledger.modelith.yaml"} - printImportResult(&out, &errOut, &chained) + printImportResult(&out, &errOut, &chained, "") for _, want := range []string{ "declares an import of its own (./ledger.modelith.yaml)", "resolution is not", @@ -447,17 +479,33 @@ func TestDepsImportOutput(t *testing.T) { var out, errOut bytes.Buffer here := *res here.Path = "payments.modelith.yaml" - printImportResult(&out, &errOut, &here) + printImportResult(&out, &errOut, &here, "") if !strings.Contains(out.String(), "- ./payments.modelith.yaml") { t.Errorf("stdout does not contain the entry:\n%s", out.String()) } }) + // An agent driving this command usually passes an absolute destination. + // Printing one straight back produced an entry the linter rejects outright: + // imports are relative so they resolve in any checkout. + t.Run("an absolute destination is relativized", func(t *testing.T) { + var out, errOut bytes.Buffer + abs := *res + abs.Path = filepath.Join(string(filepath.Separator), "home", "me", "repo", "docs", "payments.modelith.yaml") + printImportResult(&out, &errOut, &abs, filepath.Join(string(filepath.Separator), "home", "me", "repo")) + if !strings.Contains(out.String(), "- ./docs/payments.modelith.yaml") { + t.Errorf("stdout does not contain a relative entry:\n%s", out.String()) + } + if strings.Contains(out.String(), "- /home/me") { + t.Errorf("stdout still contains an absolute import entry:\n%s", out.String()) + } + }) + t.Run("a replaced copy says so", func(t *testing.T) { var out, errOut bytes.Buffer replaced := *res replaced.Replaced = true - printImportResult(&out, &errOut, &replaced) + printImportResult(&out, &errOut, &replaced, "") if !strings.HasPrefix(out.String(), "replaced ") { t.Errorf("stdout does not report the replacement:\n%s", out.String()) } diff --git a/docs/08-github-action.md b/docs/08-github-action.md index 7ab8bcc..38913dd 100644 --- a/docs/08-github-action.md +++ b/docs/08-github-action.md @@ -69,6 +69,10 @@ document: it here would fail your build twice over one problem you cannot fix in your own repository. +Both skips take a clean provenance header to claim. A file whose header has a +defect in it is checked like any model you wrote, so a stray `# modelith-` +comment cannot quietly switch the gate off. + Everything else still applies. A vendored file that is not a valid domain model, or that has been edited since it was imported, fails the build — both are about *your* repository's copy, and both are yours to fix. diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md index ea10aeb..844d60b 100644 --- a/docs/10-vendoring.md +++ b/docs/10-vendoring.md @@ -104,6 +104,12 @@ The one thing `--check` will not do is fail over a vendored model this modelith cannot render at all — one written against a newer schema version, say. It says it skipped it and moves on; `modelith lint` is where that is reported, once. +Being skipped is an exemption, and it takes a **clean** provenance header to +claim one. A file whose header has a defect in it — a misplaced line, a missing +key — is checked like any other model. That way a mistyped comment can never +quietly switch a gate off: the header defect fails `lint`, and the rendered +output is still checked. + ## What it will not overwrite The filename comes from the origin, so a copy can land on a file you already @@ -115,8 +121,16 @@ have. `deps import` refuses two cases rather than clobbering them: - **A copy of a *different* model with the same basename.** Two `payments.modelith.yaml` files from two repositories cannot share a directory; give them separate ones. -Re-importing over an existing copy of the *same* model is the ordinary refresh, -and that goes through — it reports `replaced` rather than `wrote`. +A copy from the *same* repository at a different path is refused too, because +modelith cannot tell a model that moved upstream from a second model whose file +happens to share a name. The message offers both remedies: delete the copy and +import again if it moved, or import into a different directory if they are two +different models. + +Re-importing over an existing copy of the same model at the same path is the +ordinary refresh, and that goes through — it reports `replaced` rather than +`wrote`. Only the origin's *casing* is ignored in that comparison, because +GitHub treats an owner and repository name case-insensitively. ## Keeping the copy honest diff --git a/internal/deps/deps.go b/internal/deps/deps.go index 05cf2a8..439741c 100644 --- a/internal/deps/deps.go +++ b/internal/deps/deps.go @@ -184,12 +184,12 @@ func Import(ctx context.Context, opts Options) (*Result, error) { content, err := fetchContent(ctx, runner, src) if err != nil { - return nil, fmt.Errorf("%w%s", err, splitHint(src)) + return nil, fmt.Errorf("%w%s", err, splitHint(src, err)) } if provenance.Present(content) { return nil, fmt.Errorf( - "%s is itself a vendored copy — it carries a provenance header naming where it came from. Vendor it from that origin instead, so this repository tracks the model's home rather than somebody else's copy of it", - opts.URL) + "%s carries a %s line, so modelith reads it as somebody else's copy rather than a model's home. If it is a copy, vendor it from the origin its header names instead, so this repository tracks the model's home. If it is not — the line is an ordinary comment that happens to use modelith's reserved prefix at column zero — it has to be indented or removed at the origin before this file can be vendored", + opts.URL, provenance.LinePrefix) } m, err := model.Parse(content) if err != nil { @@ -262,10 +262,24 @@ func guardTarget(target string, src Source) (replaced bool, err error) { // copy, and replacing it is how it gets repaired; only a header that names // a *different* model blocks the write. h, _ := provenance.Parse(existing) - if h.Origin != "" && h.Path != "" && (h.Origin != src.Origin || h.Path != src.Path) { + if h.Origin == "" || h.Path == "" { + return true, nil + } + switch { + // GitHub treats an owner and a repository name case-insensitively, and + // Origin keeps whatever casing the URL was typed with, so comparing these + // byte-for-byte would refuse a refresh over nothing but capitalisation. + case !strings.EqualFold(h.Origin, src.Origin): return false, fmt.Errorf( "%s is a vendored copy of %s/%s, not of %s/%s — two different models share that filename. Import into a different directory so both can live here", target, h.Origin, h.Path, src.Origin, src.Path) + case h.Path != src.Path: + // Same repository, different path: either the model moved upstream or + // this repository has two models from it sharing a basename. Only the + // user knows which, and the two remedies are different. + return false, fmt.Errorf( + "%s is a vendored copy of %s in %s, and you are importing %s from the same repository. If the model moved, delete %s and import again; if these are two different models, import into a different directory so both can live here", + target, h.Path, h.Origin, src.Path, target) } return true, nil } @@ -274,10 +288,15 @@ func guardTarget(target string, src Source) (replaced bool, err error) { // accepted. A browse URL gives no way to tell where a ref containing a slash // ends and the path begins, so the split is taken at the first segment; a failed // fetch is where that guess surfaces, as a 404 that looks like the file is -// simply not there. A single-segment path had nothing to lose to the ref, so it -// gets no hint. -func splitHint(src Source) string { - if !strings.Contains(src.Path, "/") { +// simply not there. +// +// It is offered only for that failure. A missing gh, a rejected credential, an +// unreachable network — none of those say anything about the URL, and adding a +// paragraph about ref splitting to them would send the reader after the wrong +// problem. A single-segment path had nothing to lose to the ref, so it gets no +// hint either. +func splitHint(src Source, err error) string { + if !strings.Contains(src.Path, "/") || !isNotFound(err) { return "" } return fmt.Sprintf( @@ -285,6 +304,16 @@ func splitHint(src Source) string { src.Ref, src.Path) } +// isNotFound reports whether err is gh saying the endpoint does not exist. +// +// It matches gh's text because gh is a separate program: it reports the status +// on stderr and exits 1, so there is no typed error to unwrap. Reading it wrong +// costs a hint that should not have printed, or one that should have. +func isNotFound(err error) bool { + msg := err.Error() + return strings.Contains(msg, "404") || strings.Contains(msg, "Not Found") +} + // fetchContent returns the file's bytes. The raw media type asks the API for // the content itself rather than a JSON envelope carrying it base64-encoded, so // nothing here has to decode. diff --git a/internal/deps/deps_test.go b/internal/deps/deps_test.go index ca3355a..ce3a43d 100644 --- a/internal/deps/deps_test.go +++ b/internal/deps/deps_test.go @@ -310,7 +310,7 @@ func TestImport_Rejections(t *testing.T) { name: "a file that is already a vendored copy", runner: &fakeRunner{content: "# modelith-origin: https://github.com/other/repo\n" + upstream, sha: sha}, url: blobURL, - wantErr: "itself a vendored copy", + wantErr: "reads it as somebody else's copy", }, { name: "a file that is not a domain model", @@ -429,6 +429,84 @@ func TestImport_RefusesToOverwriteWhatItDidNotWrite(t *testing.T) { } } +// TestSplitHint pins that the ref/path hint is offered for the failure it +// explains and no other. It was appended to every fetch error, so "gh is not +// installed" arrived with a paragraph about ref splitting attached — advice +// about a URL that was never in question. +func TestSplitHint(t *testing.T) { + t.Parallel() + + src := Source{Ref: "main", Path: "docs/payments.modelith.yaml"} + cases := []struct { + name string + src Source + err error + want bool + }{ + {"a not-found", src, fmt.Errorf("gh: HTTP 404: Not Found"), true}, + {"gh missing", src, fmt.Errorf("gh is not installed — modelith delegates fetching to it"), false}, + {"a rejected credential", src, fmt.Errorf("gh: HTTP 401: Bad credentials"), false}, + {"a forbidden repository", src, fmt.Errorf("gh: HTTP 403: Forbidden"), false}, + {"an unreachable network", src, fmt.Errorf("dial tcp: lookup api.github.com: no such host"), false}, + {"a single-segment path has nothing to lose to the ref", + Source{Ref: "main", Path: "payments.modelith.yaml"}, fmt.Errorf("gh: HTTP 404: Not Found"), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := splitHint(tc.src, tc.err) != ""; got != tc.want { + t.Errorf("splitHint offered = %v, want %v", got, tc.want) + } + }) + } +} + +// TestImport_RefreshIsNotRefusedOverCasing pins that guardTarget compares an +// origin the way GitHub does. Owner and repository names are case-insensitive +// there, and Source.Origin keeps whatever casing the URL was typed with, so a +// byte-for-byte comparison refused a legitimate refresh — and explained it by +// claiming two different models shared the filename. +func TestImport_RefreshIsNotRefusedOverCasing(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if _, err := importInto(t, dir, &fakeRunner{content: upstream, sha: sha}, + "https://github.com/Acme/Billing/blob/main/docs/payments.modelith.yaml"); err != nil { + t.Fatal(err) + } + res, err := importInto(t, dir, &fakeRunner{content: upstream + " # newer\n", sha: "9" + sha[1:]}, blobURL) + if err != nil { + t.Fatalf("a refresh differing only in the origin's casing was refused: %v", err) + } + if !res.Replaced { + t.Error("did not report replacing the earlier copy") + } +} + +// TestImport_RefusesAMovedPathWithTheRightRemedy pins the other half: the same +// repository at a different path is ambiguous — the model moved, or two models +// there share a basename — and the two remedies differ. Offering only "import +// into a different directory" left the user of a moved model with no way +// forward the message named. +func TestImport_RefusesAMovedPathWithTheRightRemedy(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if _, err := importInto(t, dir, &fakeRunner{content: upstream, sha: sha}, blobURL); err != nil { + t.Fatal(err) + } + _, err := importInto(t, dir, &fakeRunner{content: upstream, sha: sha}, + "https://github.com/acme/billing/blob/main/models/payments.modelith.yaml") + if err == nil { + t.Fatal("a second model from the same repository silently replaced the first") + } + for _, want := range []string{"If the model moved, delete", "import into a different directory"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the error does not offer %q: %v", want, err) + } + } +} + func TestImport_ReplacesAnEarlierCopy(t *testing.T) { t.Parallel()