From 6fc09bd0392c45ffb3d8a395e32507db9f65e7ff Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 17:24:15 -0700 Subject: [PATCH 1/6] docs(adr): staleness is a content comparison, not a commit comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the two decisions behind slice 3 that clear the bar in .claude/rules/adr.md: deps check compares the origin's bytes against the recorded digest rather than comparing commit SHAs, and # modelith-imported: dates the arrival of a version rather than the last check. Extends ADR-0015 with the freshness half of the vendoring lifecycle. Nothing in ADR-0015 becomes untrue, so this supersedes none of it — but it does narrow one of its expectations: 0015 assumed check would diff the two files, and it does not. Its enforcing tests land with the implementation in the following commits. Signed-off-by: Joe Beda --- .../0016-staleness-is-a-content-comparison.md | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 project-docs/adr/0016-staleness-is-a-content-comparison.md diff --git a/project-docs/adr/0016-staleness-is-a-content-comparison.md b/project-docs/adr/0016-staleness-is-a-content-comparison.md new file mode 100644 index 0000000..6022c24 --- /dev/null +++ b/project-docs/adr/0016-staleness-is-a-content-comparison.md @@ -0,0 +1,76 @@ +# A vendored copy is stale when its content differs from its origin + +`modelith deps check` decides staleness by comparing the origin's bytes against +the digest the header records, not by comparing commit SHAs. `# modelith-imported:` +dates the arrival of a version, not the last check, which is what forecloses a +"last checked" key. Extends ADR-0015 with the freshness half of the vendoring +lifecycle; nothing in ADR-0015 becomes untrue. + +## Context + +ADR-0015 settled how a vendored copy is verified *offline*: `lint` re-hashes the +file and compares it to `# modelith-digest:`, which answers "has my copy been +tampered with?". It left open the question the network can answer, "has the model +changed upstream?" — the job of `deps check` and `deps update`. + +The header carries both a `commit` and a `digest`, so the obvious implementation +compares the recorded commit against the commit that last touches the path now. +`deps import` already fetches that SHA, so the machinery is in hand. + +## Decision + +**Staleness is a content comparison.** `check` fetches the file at the recorded +ref and compares `provenance.Digest` of those bytes against `# modelith-digest:`. + +The comparison works because of a property of how `import` writes the header: +the digest is computed over the *upstream bytes before stamping*, and `Digest` +strips header lines, so what is recorded is the **origin's** content digest +rather than the local file's. Comparing it against the origin now is therefore +independent of whether anyone edited the local copy — which is what keeps +`check` and `lint` answering two genuinely different questions from one anchor. + +Commit comparison is wrong in both directions. A merge commit or a +whitespace-only touch reports stale when nothing a reader would care about +moved, and a SHA that differs cannot say what differs. **The commit SHA stays in +the header and stays in the output as reporting**, resolved only when the +content has actually changed — so a clean check over N copies costs N `gh` +calls, not 2N. + +**`# modelith-imported:` dates the content.** A copy whose origin has not moved +is left byte-for-byte alone by `deps update`, so the date records when *this +version arrived* rather than when the copy was last confirmed. That is the more +useful of the two meanings: `git blame` on the header line and the date agree, +and `modelith deps update *.modelith.yaml` becomes a habit with a clean diff. + +The rejected alternative is a second key, `# modelith-checked:`. It would make +every check a write, which is exactly the property that makes `check` safe to +run in CI and safe to run against a read-only checkout. + +**One write condition** follows from both, and no case needs a branch of its +own. `deps update` writes when `Digest(local) != Digest(upstream)`, or when the +ref changed. A hand-edited copy is therefore not current whatever upstream did, +and updating it restores it. + +## Consequences + +- **A pinned copy reports up to date forever.** A copy at `v2.1.0` is never + stale by this test, even when `v2.3.0` has been out for months. `check` names + the ref in every line of output so the verdict is visibly a statement about + the pin rather than about the world. Chasing newer tags needs an answer to + "which tags count as newer" — semver parsing, prereleases, projects that do + not tag semantically — and modelith has no basis to decide that. Tracked as a + follow-up. +- **Stale exits 1**, matching `render --check` drift. modelith has exactly one + failure code and does not gain a second; a failure to check lands on the same + code. +- **`deps update` overwrites local edits**, consistent with `deps import`, which + already replaces an edited copy. These files are committed, so a discarded + edit is recoverable from `git diff`; refusing would strand the user, whose + only other remedy does the same thing. +- **ADR-0015 anticipated that `check` would diff the two files** so a human + could judge a documentation-only change. It does not. `git diff` answers that + question after an update with the user's own tooling, and reverting an update + is one command. What is given up is seeing what moved *before* deciding to + take it. +- Pinned by `TestADR_0016_StalenessIsContentNotCommit` and + `TestADR_0016_ACurrentCopyIsNotRewritten`. From 63328c7f1889db7f558baa9bf4e240b907bd1431 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 17:33:29 -0700 Subject: [PATCH 2/6] feat(deps): report and refresh a vendored copy against its origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the shared core of deps check and deps update: one survey that reads a copy, fetches what its origin serves now, and reports the three distinct answers that fall out — whether the origin moved, whether the copy on disk drifted, and whether the ref being consulted is the recorded one. Staleness is a content comparison against the recorded digest, not a commit comparison (ADR-0016). Because import writes that digest over the upstream bytes before stamping, it is the origin's digest rather than the local file's, so the verdict holds whether or not anyone edited the copy. The commit SHA stays reporting: it is resolved only when the content actually differs, so a clean check costs one gh call per copy instead of two. There is one write condition — the copy does not hold what the origin serves, or the ref changed — and every case falls out of it, including restoring a hand-edited copy whose origin has not moved. A restore re-stamps the recorded header verbatim, reproducing the bytes the original import wrote. A batch continues past a per-file failure, so one unreachable copy does not hide the state of the files after it. The single exception is gh itself being unusable, which would fail every remaining file identically; ExecRunner marks those with ErrToolUnavailable, matching gh's own text for the auth cases since it reports them on stderr with no typed error to unwrap. A refetch makes the same two refusals a first fetch does — an origin that has itself become a copy, or has stopped being a domain model — because the file at an address is not the file that was there last time. A header's parts are reassembled into a blob URL and handed back to ParseSource rather than pasted into an endpoint here, so a hand-written header gets the host check and the dot-segment rejection that escapePath depends on. The tests build their fixtures with the real Import and were verified by mutation: removing the no-op early return, deciding staleness by commit, fetching the commit eagerly, and re-dating a restore each fail a test. Signed-off-by: Joe Beda --- internal/deps/deps.go | 34 +- internal/deps/refresh.go | 373 +++++++++++++++++++++ internal/deps/refresh_test.go | 593 ++++++++++++++++++++++++++++++++++ 3 files changed, 998 insertions(+), 2 deletions(-) create mode 100644 internal/deps/refresh.go create mode 100644 internal/deps/refresh_test.go diff --git a/internal/deps/deps.go b/internal/deps/deps.go index 439741c..b647010 100644 --- a/internal/deps/deps.go +++ b/internal/deps/deps.go @@ -51,16 +51,46 @@ func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, 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) + return nil, unusable{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) + failed := fmt.Errorf("%s %s: %s", name, strings.Join(args, " "), msg) + if unauthenticated(msg) { + return nil, unusable{failed} + } + return nil, failed } return nil, fmt.Errorf("%s %s: %w", name, strings.Join(args, " "), err) } return out, nil } +// unauthenticated reports whether gh refused for want of credentials rather +// than because of anything about the request. +// +// It matches gh's own text because gh is a separate program: it reports this on +// stderr and exits 1, with no typed error to unwrap and no distinguishing exit +// code. The three needles are the ones gh's binary actually carries — "gh auth +// login" appears in every logged-out message, "GH_TOKEN" in the two automation +// ones, and HTTP 401 is a token that exists and is refused. Reading it wrong +// costs a batch that stops when it could have continued, or one that repeats +// the same paragraph per file. +func unauthenticated(msg string) bool { + for _, needle := range []string{"gh auth login", "GH_TOKEN", "HTTP 401"} { + if strings.Contains(msg, needle) { + return true + } + } + return false +} + +// unusable marks a failure of the tool itself rather than of the request, so +// errors.Is(err, ErrToolUnavailable) reports true without the sentinel's text +// appearing in the message the user reads. +type unusable struct{ error } + +func (unusable) Is(target error) bool { return target == ErrToolUnavailable } + // 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 { diff --git a/internal/deps/refresh.go b/internal/deps/refresh.go new file mode 100644 index 0000000..7e0d686 --- /dev/null +++ b/internal/deps/refresh.go @@ -0,0 +1,373 @@ +package deps + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "strings" + "time" + + "github.com/stacklok/modelith/internal/model" + "github.com/stacklok/modelith/internal/provenance" +) + +// ErrToolUnavailable marks a failure of gh itself rather than of the request: +// it is not installed, or it holds no usable credentials. Every file in a run +// would fail it identically, so a batch stops on it instead of repeating the +// same paragraph once per copy. +var ErrToolUnavailable = errors.New("gh is unavailable") + +// State is one vendored copy measured against its origin. +// +// The questions below are deliberately distinct and no two of them share a +// comparison. Collapsing questions about a vendored file has caused a bug once +// already (ADR-0015's provenance rule). Every comparison here runs through +// provenance.Digest, which strips header lines, so all of them are content +// against content and none is perturbed by the header a refresh rewrites. +type State struct { + // Path is the copy on disk, as it was named on the command line. + Path string + // Header is what that copy records about where it came from. + Header *provenance.Header + // Ref is the ref consulted: the header's, or the override. + Ref string + // Local is the bytes on disk. Upstream is what the origin serves now. + Local, Upstream []byte + // Model is the origin's current content, parsed. + Model *model.Model +} + +// Moved reports whether the origin has changed since this copy was taken, which +// is the question deps check answers. It compares against the recorded digest, +// which import wrote over the upstream bytes before stamping — so this holds +// whether or not the copy on disk was edited (ADR-0016). +func (s *State) Moved() bool { return provenance.Digest(s.Upstream) != s.Header.Digest } + +// Edited reports whether the copy on disk drifted from the version its own +// header claims. lint answers this offline and it is not check's business; it +// is here because it is half of why an update writes. +func (s *State) Edited() bool { return provenance.Digest(s.Local) != s.Header.Digest } + +// Repinned reports whether the ref being consulted is not the recorded one. +func (s *State) Repinned() bool { return s.Ref != s.Header.Ref } + +// Current reports whether the copy on disk already holds what the origin +// serves. With Repinned it is the whole write condition for deps update: a +// hand-edited copy is not current whatever the origin did, so updating it +// restores it, and no case needs a branch of its own (ADR-0016). +func (s *State) Current() bool { return provenance.Digest(s.Local) == provenance.Digest(s.Upstream) } + +// Report is what happened to one file in a check or update run. +type Report struct { + // Path is the file as it was named on the command line. + Path string + // Skipped says the file carries no provenance header, so it is a model this + // repository owns and neither command has anything to say about it. + Skipped bool + // Err is why this file could not be checked. The run continues past it. + Err error + // State is nil when the file was skipped or failed. + State *State + // Written says update rewrote the file. Restored distinguishes the two + // reasons it might have: a new version arrived, or only the copy on disk + // had drifted from the version it claims. + Written, Restored bool + // Commit is the origin's commit for this file. It is resolved only when + // something moved, so a clean check costs one gh call per copy, not two. + Commit string + // NewImports are imports a refresh brought that the copy on disk did not + // have. Vendoring is not transitive, so they are reported, never followed. + NewImports []string +} + +// Stale reports whether this file needs updating, which is the verdict deps +// check prints and the condition its exit code carries. +func (r *Report) Stale() bool { return r.State != nil && r.State.Moved() } + +// CheckOptions are the inputs to Check. +type CheckOptions struct { + // Paths are the files to check. One that carries no provenance header is + // skipped, because the glob a user passes to lint holds their own models too. + Paths []string + // Run is the command seam; nil uses ExecRunner. + Run Runner +} + +// UpdateOptions are the inputs to Update. +type UpdateOptions struct { + // Paths are the files to update. + Paths []string + // Ref re-pins the copy, overriding the ref its header records. It applies to + // exactly one file: one ref across several unrelated origins is meaningless. + Ref string + // Now stamps a refreshed header's imported date, in local time. + Now time.Time + // Run is the command seam; nil uses ExecRunner. + Run Runner +} + +// Check reports, for each path, whether the vendored copy there has fallen +// behind its origin. It writes nothing, which is what makes it safe to run in +// CI and against a read-only checkout. +// +// A per-file failure lands in that file's Report and the run continues. A +// non-nil error means the run stopped early — only gh being unusable does that. +func Check(ctx context.Context, opts CheckOptions) ([]Report, error) { + return survey(ctx, surveyOptions{paths: opts.Paths, run: opts.Run}) +} + +// Update brings each vendored copy forward to what its origin serves now. +// +// It writes when the copy on disk does not already hold the origin's content, +// or when the ref moved. It does not touch the model that imports the copy: an +// item this copy used to define may have been renamed or removed upstream, and +// modelith lint is what reports that, from the importing model's seat. +func Update(ctx context.Context, opts UpdateOptions) ([]Report, error) { + if opts.Ref != "" && len(opts.Paths) != 1 { + return nil, fmt.Errorf( + "--ref re-pins one copy and %d files were named — a single ref across several origins names a different version in each. Run it once per copy", + len(opts.Paths)) + } + return survey(ctx, surveyOptions{paths: opts.Paths, ref: opts.Ref, now: opts.Now, run: opts.Run, write: true}) +} + +type surveyOptions struct { + paths []string + ref string + now time.Time + run Runner + write bool +} + +func survey(ctx context.Context, opts surveyOptions) ([]Report, error) { + runner := opts.run + if runner == nil { + runner = ExecRunner{} + } + reports := make([]Report, 0, len(opts.paths)) + for _, p := range opts.paths { + rep, err := visit(ctx, runner, p, opts) + reports = append(reports, rep) + if err != nil { + // gh itself is unusable, so every file left would fail identically. + // Stop, and hand back what was learned before it. + return reports, err + } + } + return reports, nil +} + +// visit measures one file and, when asked to write, writes it. It returns a +// non-nil error only to abort the whole run; everything else is a Report. +func visit(ctx context.Context, runner Runner, path string, opts surveyOptions) (Report, error) { + rep := Report{Path: path} + + local, err := os.ReadFile(path) + if err != nil { + var pe *fs.PathError + if errors.As(err, &pe) { + // The Report already carries the path and the caller prints it, so + // the PathError's own copy of it would be the second in one line. + err = fmt.Errorf("cannot be read: %w", pe.Err) + } + rep.Err = err + return rep, nil + } + if !provenance.Present(local) { + rep.Skipped = true + return rep, nil + } + + // Classification is generous and exemption is strict (ADR-0015): the file + // above is a copy however broken its header, but a header with any defect in + // it earns no verdict here. Every comparison below rests on the recorded + // digest, and a header whose digest is malformed or whose origin is missing + // would produce a confident answer built on nothing. + h, problems := provenance.Parse(local) + if len(problems) > 0 { + rep.Err = fmt.Errorf( + "its provenance header is not usable (%s) — run `modelith lint %s` for the full report", + problems[0].Message, path) + return rep, nil + } + + ref := h.Ref + if opts.ref != "" { + ref = opts.ref + } + src, err := sourceFromHeader(h, ref) + if err != nil { + rep.Err = err + return rep, nil + } + + upstream, err := fetchContent(ctx, runner, src) + if err != nil { + if errors.Is(err, ErrToolUnavailable) { + return rep, err + } + rep.Err = fmt.Errorf("%w%s", err, movedHint(src, err)) + return rep, nil + } + m, err := checkFetched(upstream, src) + if err != nil { + rep.Err = err + return rep, nil + } + + st := &State{Path: path, Header: h, Ref: ref, Local: local, Upstream: upstream, Model: m} + rep.State = st + + if !opts.write { + // The commit is reporting, not verdict: resolve it only when the origin + // actually moved, so a clean check costs one call per copy (ADR-0016). + if st.Moved() { + commit, err := fetchCommit(ctx, runner, src) + if err != nil { + if errors.Is(err, ErrToolUnavailable) { + return rep, err + } + rep.Err = err + } + rep.Commit = commit + } + return rep, nil + } + + switch { + case st.Current() && !st.Repinned(): + return rep, nil + + case !st.Moved() && !st.Repinned(): + // A restore. The origin still holds the version this header describes + // and only the copy on disk drifted, so re-stamping the recorded header + // reproduces the bytes the original import wrote: no commit is fetched, + // because nothing moved, and imported: does not change, because no new + // version arrived (ADR-0016). + if err := write(path, upstream, h); err != nil { + rep.Err = err + return rep, nil + } + rep.Written, rep.Restored, rep.Commit = true, true, h.Commit + return rep, nil + } + + // A refresh: a new version, a new pin, or both. + commit, err := fetchCommit(ctx, runner, src) + if err != nil { + if errors.Is(err, ErrToolUnavailable) { + return rep, err + } + rep.Err = err + return rep, nil + } + next := *h + next.Ref = ref + next.Commit = commit + next.Imported = opts.now.Format("2006-01-02") + next.Digest = provenance.Digest(upstream) + if err := write(path, upstream, &next); err != nil { + rep.Err = err + return rep, nil + } + rep.Written, rep.Commit = true, commit + rep.NewImports = newImports(local, m) + return rep, nil +} + +func write(path string, content []byte, h *provenance.Header) error { + if err := os.WriteFile(path, provenance.Stamp(content, h), 0o644); err != nil { + return fmt.Errorf("writing %s: %w", path, err) + } + return nil +} + +// sourceFromHeader rebuilds the fetch address from what a header records. +// +// It reassembles the blob URL and hands it back to ParseSource rather than +// picking the origin apart here, so a hand-written header gets the same +// treatment a typed one does: the host check, and the dot-segment rejection +// that escapePath depends on to keep an endpoint inside the repository's +// contents namespace. Passing ref explicitly is what lets a ref containing a +// slash split correctly, which is knowable here and is not from a URL alone. +func sourceFromHeader(h *provenance.Header, ref string) (Source, error) { + if h.Fetch != "git" { + // Unreachable while git is the only method, but a header naming a + // method a future build understands would parse without problems and + // still have no address this function can assemble. + return Source{}, fmt.Errorf( + "modelith cannot refresh a copy fetched with %q — this build knows how to fetch %s", + h.Fetch, strings.Join(provenance.Methods(), ", ")) + } + return ParseSource(fmt.Sprintf("%s/blob/%s/%s", strings.TrimSuffix(h.Origin, "/"), ref, h.Path), ref) +} + +// checkFetched applies to a refetch the same two refusals import applies to a +// first fetch, because the file at an address is not the file that was there +// last time. +func checkFetched(upstream []byte, src Source) (*model.Model, error) { + if provenance.Present(upstream) { + return nil, fmt.Errorf( + "%s now carries a %s line at its origin, so modelith reads it as somebody else's copy rather than a model's home. Vendoring a copy of a copy would record the wrong repository as this model's home — vendor it from the origin its own header names instead", + src.Path, provenance.LinePrefix) + } + m, err := model.Parse(upstream) + if err != nil { + return nil, fmt.Errorf( + "%s no longer parses as a domain model at its origin, so the copy here was left alone: %w", + src.Path, 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 no longer a domain model at its origin — it %s, not \"DomainModel\" — so the copy here was left alone", + src.Path, declares) + } + return m, nil +} + +// movedHint explains the failure a refetch has that a first fetch does not: the +// address came from a header written some time ago, and a model can move or be +// deleted in its own repository without anything here noticing. +// +// It is offered only for a 404. A rejected credential or an unreachable network +// says nothing about where the file lives, and sending the reader after a move +// that did not happen wastes their time. The ref/path split ambiguity splitHint +// exists for cannot arise here: a header records ref and path as separate keys, +// so nothing is being guessed. +func movedHint(src Source, err error) string { + if !isNotFound(err) { + return "" + } + return fmt.Sprintf( + "\n\n%s/%s has no %q at %q. Either the model moved, in which case import it from its new address, or it was deleted, in which case this copy is orphaned and whether to keep it is your call", + src.Owner, src.Repo, src.Path, src.Ref) +} + +// newImports are the import paths the origin declares that the copy on disk did +// not. Vendoring fetches one file and resolution is not transitive (ADR-0015), +// so they are reported and never followed. +// +// Only the new ones, so a copy whose imports were understood at import time does +// not re-warn on every update. A copy that no longer parses locally is treated +// as having had none, which over-reports rather than staying quiet. +func newImports(local []byte, upstream *model.Model) []string { + had := map[string]bool{} + if m, err := model.Parse(local); err == nil { + for _, imp := range m.Imports { + had[imp.Path] = true + } + } + var added []string + for _, imp := range upstream.Imports { + if !had[imp.Path] { + added = append(added, imp.Path) + } + } + return added +} diff --git a/internal/deps/refresh_test.go b/internal/deps/refresh_test.go new file mode 100644 index 0000000..49300a4 --- /dev/null +++ b/internal/deps/refresh_test.go @@ -0,0 +1,593 @@ +package deps + +import ( + "context" + "errors" + "os" + "strings" + "testing" + "time" +) + +// laterSHA is the commit an origin has moved to. It differs from sha in every +// position that matters, so a test cannot pass by comparing a prefix. +const laterSHA = "a91b0c37e5d248fa6c0913be4b7f25d81a3c6e90" + +// updatedAt is the day an update runs, a day after importedAt, so a header's +// imported: line proves which of the two wrote it. +var ( + importedAt = time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local) + updatedAt = time.Date(2026, 7, 28, 9, 0, 0, 0, time.Local) +) + +// vendored writes a copy of content into a fresh temp dir exactly as deps +// import would, and hands back its path with the runner that served it. The +// fixture is built by the real Import so a test reasons about a copy that looks +// like one a user has, rather than one hand-assembled to suit the assertion. +// +// The runner's call log is cleared before returning, so a test counting calls +// counts only its own. +func vendored(t *testing.T, content string) (string, *fakeRunner) { + t.Helper() + r := &fakeRunner{content: content, sha: sha} + res, err := Import(context.Background(), Options{ + URL: blobURL, Dir: t.TempDir(), Now: importedAt, Run: r, + }) + if err != nil { + t.Fatalf("building the fixture: %v", err) + } + r.calls = nil + return res.Path, r +} + +func readFile(t *testing.T, path string) string { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return string(b) +} + +func check(t *testing.T, r Runner, paths ...string) []Report { + t.Helper() + reports, err := Check(context.Background(), CheckOptions{Paths: paths, Run: r}) + if err != nil { + t.Fatalf("check aborted: %v", err) + } + return reports +} + +func update(t *testing.T, r Runner, ref string, paths ...string) []Report { + t.Helper() + reports, err := Update(context.Background(), UpdateOptions{ + Paths: paths, Ref: ref, Now: updatedAt, Run: r, + }) + if err != nil { + t.Fatalf("update aborted: %v", err) + } + return reports +} + +// moved is upstream with a real change to its content: an enum gains a value. +const moved = `# yaml-language-server: $schema=https://modelith.sh/schema/domain-model/v1.json +kind: DomainModel +version: v1 +title: Payments +enums: + PaymentMethod: + values: + - name: card + - name: bank-transfer +` + +func TestCheck_ReportsWhetherTheOriginMoved(t *testing.T) { + t.Parallel() + + t.Run("an unmoved origin is up to date", func(t *testing.T) { + t.Parallel() + path, r := vendored(t, upstream) + rep := check(t, r, path)[0] + if rep.Err != nil { + t.Fatalf("unexpected error: %v", rep.Err) + } + if rep.Stale() { + t.Error("reported stale against an origin serving the same bytes") + } + if rep.State.Ref != "main" { + t.Errorf("checked against ref %q, want the header's %q", rep.State.Ref, "main") + } + }) + + t.Run("a moved origin is stale and names the new commit", func(t *testing.T) { + t.Parallel() + path, r := vendored(t, upstream) + r.content, r.sha = moved, laterSHA + rep := check(t, r, path)[0] + if !rep.Stale() { + t.Fatal("reported up to date against an origin serving different bytes") + } + if rep.Commit != laterSHA { + t.Errorf("Commit = %q, want the origin's %q", rep.Commit, laterSHA) + } + }) + + t.Run("check writes nothing", func(t *testing.T) { + t.Parallel() + path, r := vendored(t, upstream) + before := readFile(t, path) + r.content, r.sha = moved, laterSHA + check(t, r, path) + if after := readFile(t, path); after != before { + t.Error("check rewrote the file it was only asked to report on") + } + }) +} + +// TestADR_0016_StalenessIsContentNotCommit pins the decision that makes check +// worth running: the verdict is what the origin serves, not which commit last +// touched the path. A commit that changes no bytes — a merge, a rename, a +// whitespace touch — must not report as a change, and a change must report even +// if the commit somehow did not move. +// +// Neither half computes a digest. Both read the verdict off content the test +// controls directly, so an implementation that hashed the wrong bytes could not +// satisfy them by hashing them the same wrong way. +func TestADR_0016_StalenessIsContentNotCommit(t *testing.T) { + t.Parallel() + + t.Run("a new commit over identical bytes is not stale", func(t *testing.T) { + t.Parallel() + path, r := vendored(t, upstream) + r.sha = laterSHA // the commit moved; the file did not + rep := check(t, r, path)[0] + if rep.Stale() { + t.Error("a commit that changed no bytes reported as stale") + } + // The commit is reporting, not verdict, so nothing asked for it. + if rep.Commit != "" { + t.Errorf("resolved a commit for an unmoved origin: %q", rep.Commit) + } + }) + + t.Run("changed bytes under the same commit are stale", func(t *testing.T) { + t.Parallel() + path, r := vendored(t, upstream) + r.content = moved // the file changed; the commit did not + rep := check(t, r, path)[0] + if !rep.Stale() { + t.Error("changed bytes under an unchanged commit reported as up to date") + } + }) +} + +// TestCheck_CostsOneCallPerCleanCopy pins the lazy commit fetch. Resolving the +// SHA for every copy would double the API cost of the command people are meant +// to run habitually, and nothing else would fail if it were resolved eagerly. +func TestCheck_CostsOneCallPerCleanCopy(t *testing.T) { + t.Parallel() + + path, r := vendored(t, upstream) + check(t, r, path) + if len(r.calls) != 1 { + t.Errorf("a clean check made %d gh calls, want 1: %+v", len(r.calls), r.calls) + } + + r.calls, r.content, r.sha = nil, moved, laterSHA + check(t, r, path) + if len(r.calls) != 2 { + t.Errorf("a stale check made %d gh calls, want 2: %+v", len(r.calls), r.calls) + } +} + +func TestCheck_SkipsAFileWithNoProvenanceHeader(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + own := dir + "/ours.modelith.yaml" + if err := os.WriteFile(own, []byte(upstream), 0o644); err != nil { + t.Fatal(err) + } + r := &fakeRunner{content: upstream, sha: sha} + rep := check(t, r, own)[0] + if !rep.Skipped { + t.Error("a model this repository owns was not skipped") + } + if len(r.calls) != 0 { + t.Errorf("a skipped file still cost %d gh calls: %+v", len(r.calls), r.calls) + } +} + +// TestSurvey_ContinuesPastAFailure pins that one unreachable copy does not hide +// the state of the files after it. Aborting on the third of eight turns one run +// into five sequential ones. +func TestSurvey_ContinuesPastAFailure(t *testing.T) { + t.Parallel() + + good, r := vendored(t, upstream) + missing := t.TempDir() + "/gone.modelith.yaml" + + reports := check(t, r, missing, good) + if len(reports) != 2 { + t.Fatalf("got %d reports, want 2", len(reports)) + } + if reports[0].Err == nil { + t.Error("an unreadable file did not report an error") + } + if reports[1].Err != nil || reports[1].State == nil { + t.Errorf("the file after the failure was not checked: %+v", reports[1]) + } +} + +// TestSurvey_StopsWhenTheToolIsUnusable pins the single exception to +// continue-on-failure. gh being absent or unauthenticated fails every file +// identically, so repeating the paragraph once per copy is noise. +func TestSurvey_StopsWhenTheToolIsUnusable(t *testing.T) { + t.Parallel() + + a, _ := vendored(t, upstream) + b, _ := vendored(t, upstream) + + r := &unusableRunner{} + reports, err := Check(context.Background(), CheckOptions{Paths: []string{a, b}, Run: r}) + if !errors.Is(err, ErrToolUnavailable) { + t.Fatalf("Check returned %v, want an ErrToolUnavailable", err) + } + if len(reports) != 1 { + t.Errorf("got %d reports, want the run to stop after the first: %+v", len(reports), reports) + } + if r.calls != 1 { + t.Errorf("the run made %d calls after gh proved unusable, want 1", r.calls) + } +} + +// unusableRunner is gh being absent: every call fails the same way. +type unusableRunner struct{ calls int } + +func (u *unusableRunner) Run(context.Context, string, ...string) ([]byte, error) { + u.calls++ + return nil, unusable{errors.New("gh is not installed — modelith delegates fetching to it")} +} + +// TestUnauthenticatedMatchesGhsOwnText pins the needles against the messages gh +// actually emits, quoted from its binary. gh reports this on stderr and exits 1, +// so there is no typed error to unwrap and nothing else to key on. +func TestUnauthenticatedMatchesGhsOwnText(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + msg string + want bool + }{ + {"logged out entirely", "To get started with GitHub CLI, please run: gh auth login", true}, + {"a workflow with no token", "gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable.", true}, + {"automation with no token", "gh: To use GitHub CLI in automation, set the GH_TOKEN environment variable.", true}, + {"a token that is refused", "gh: Bad credentials (HTTP 401)", true}, + {"a file that is not there", "gh: HTTP 404: Not Found (https://api.github.com/repos/a/b/contents/c)", false}, + {"a repository that is private", "gh: HTTP 403: Forbidden", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := unauthenticated(tc.msg); got != tc.want { + t.Errorf("unauthenticated(%q) = %v, want %v", tc.msg, got, tc.want) + } + }) + } +} + +// TestADR_0016_ACurrentCopyIsNotRewritten pins the property that makes update +// safe to run habitually over a glob: it produces a diff only where something +// changed. It also pins what imported: means — a date that moved here would be +// dating the run rather than the version. +// +// The assertion is on the file's bytes, not on an absence of errors: a rewrite +// that happened to produce the same header would still be a rewrite this test +// must not accept, and comparing bytes is the only way to see one. +func TestADR_0016_ACurrentCopyIsNotRewritten(t *testing.T) { + t.Parallel() + + path, r := vendored(t, upstream) + before := readFile(t, path) + + rep := update(t, r, "", path)[0] + if rep.Written { + t.Error("update reported writing a copy whose origin had not moved") + } + if after := readFile(t, path); after != before { + t.Errorf("update rewrote a current copy:\n before %q\n after %q", before, after) + } + if !strings.Contains(before, "# modelith-imported: 2026-07-27") { + t.Errorf("the fixture's imported date is not the import's:\n%s", before) + } +} + +func TestUpdate_RefreshesAMovedOrigin(t *testing.T) { + t.Parallel() + + path, r := vendored(t, upstream) + r.content, r.sha = moved, laterSHA + + rep := update(t, r, "", path)[0] + if !rep.Written || rep.Restored { + t.Fatalf("want a refresh, got Written=%v Restored=%v (%v)", rep.Written, rep.Restored, rep.Err) + } + after := readFile(t, path) + for _, want := range []string{ + "# modelith-commit: " + laterSHA, + "# modelith-imported: 2026-07-28", + "- name: bank-transfer", + } { + if !strings.Contains(after, want) { + t.Errorf("the refreshed copy does not contain %q:\n%s", want, after) + } + } + // The refreshed copy must verify against the digest it now records, or the + // next lint reports the update as tampering. + if reports := check(t, r, path); reports[0].Stale() { + t.Error("the copy is still stale immediately after an update") + } + if reports := check(t, r, path); reports[0].State.Edited() { + t.Error("the refreshed copy does not verify against its own new digest") + } +} + +// TestUpdate_RestoresAnEditedCopy pins the case the write condition folds in +// rather than special-cases: a copy that drifted from the version it claims is +// not holding what its origin serves, so it is not current, so update writes. +// Restoring reproduces the original import's bytes exactly — no commit is +// fetched, because nothing moved, and imported: does not move, because no new +// version arrived. +func TestUpdate_RestoresAnEditedCopy(t *testing.T) { + t.Parallel() + + path, r := vendored(t, upstream) + pristine := readFile(t, path) + + edited := strings.Replace(pristine, "title: Payments", "title: Payments (ours)", 1) + if edited == pristine { + t.Fatal("the test did not edit the copy") + } + if err := os.WriteFile(path, []byte(edited), 0o644); err != nil { + t.Fatal(err) + } + + r.calls = nil + rep := update(t, r, "", path)[0] + if !rep.Written || !rep.Restored { + t.Fatalf("want a restore, got Written=%v Restored=%v (%v)", rep.Written, rep.Restored, rep.Err) + } + if after := readFile(t, path); after != pristine { + t.Errorf("the restore did not reproduce the imported bytes:\n want %q\n got %q", pristine, after) + } + if len(r.calls) != 1 { + t.Errorf("a restore made %d gh calls, want 1 — the origin did not move: %+v", len(r.calls), r.calls) + } +} + +func TestUpdate_Repin(t *testing.T) { + t.Parallel() + + t.Run("more than one file is an error", func(t *testing.T) { + t.Parallel() + a, r := vendored(t, upstream) + b, _ := vendored(t, upstream) + _, err := Update(context.Background(), UpdateOptions{ + Paths: []string{a, b}, Ref: "v2.2.0", Now: updatedAt, Run: r, + }) + if err == nil || !strings.Contains(err.Error(), "--ref re-pins one copy") { + t.Fatalf("want a refusal naming the flag, got %v", err) + } + }) + + // A tag can point at a commit whose file content is byte-identical to the + // previous tag's. The header still has to move, because the ref it records + // is now wrong — and a header that lies about its own pin is worse than a + // stale one, because nothing later can detect it. + t.Run("identical content still rewrites the header", func(t *testing.T) { + t.Parallel() + path, r := vendored(t, upstream) + r.sha = laterSHA + + rep := update(t, r, "v2.2.0", path)[0] + if !rep.Written || rep.Restored { + t.Fatalf("want a refresh, got Written=%v Restored=%v (%v)", rep.Written, rep.Restored, rep.Err) + } + after := readFile(t, path) + for _, want := range []string{ + "# modelith-ref: v2.2.0", + "# modelith-commit: " + laterSHA, + "# modelith-imported: 2026-07-28", + } { + if !strings.Contains(after, want) { + t.Errorf("the re-pinned copy does not contain %q:\n%s", want, after) + } + } + }) + + t.Run("the new ref is what gets fetched", func(t *testing.T) { + t.Parallel() + path, r := vendored(t, upstream) + r.sha = laterSHA + update(t, r, "v2.2.0", path) + if got := strings.Join(r.calls[0], " "); !strings.Contains(got, "?ref=v2.2.0") { + t.Errorf("the content fetch did not use the new ref: %s", got) + } + }) +} + +// TestUpdate_ReportsImportsItDidNotFollow pins that a refresh bringing new +// imports says so. Vendoring is not transitive, so an import that arrives in an +// updated copy resolves to nothing until the user vendors it directly, and the +// update is the only moment anything can tell them. +func TestUpdate_ReportsImportsItDidNotFollow(t *testing.T) { + t.Parallel() + + path, r := vendored(t, upstream+"imports:\n - ./ledger.modelith.yaml\n") + r.content = moved + "imports:\n - ./ledger.modelith.yaml\n - ./tax.modelith.yaml\n" + r.sha = laterSHA + + rep := update(t, r, "", path)[0] + if rep.Err != nil { + t.Fatal(rep.Err) + } + // Only the new one: the copy already declared ledger at import time, and + // re-warning about it on every update trains the reader to skip the note. + if want := []string{"./tax.modelith.yaml"}; strings.Join(rep.NewImports, ",") != strings.Join(want, ",") { + t.Errorf("NewImports = %v, want %v", rep.NewImports, want) + } + if len(r.calls) != 2 { + t.Errorf("an update followed %d calls, want 2 — imports are reported, not fetched: %+v", len(r.calls), r.calls) + } +} + +// TestVisit_RefusesAnUnusableHeader pins ADR-0015's rule applied here: +// classification is generous, exemption is strict. A file with a broken header +// is still a copy, so it is not skipped — but every comparison this package +// makes rests on that header, so it earns no verdict and the user is sent to +// the tool that explains header defects. +func TestVisit_RefusesAnUnusableHeader(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + mangle func(string) string + wantErr string + }{ + { + name: "a malformed digest", + mangle: func(s string) string { return strings.Replace(s, "# modelith-digest: sha256:", "# modelith-digest: ", 1) }, + wantErr: "sha256:<64 hex digits>", + }, + { + name: "an unknown key", + mangle: func(s string) string { return "# modelith-source: elsewhere\n" + s }, + wantErr: "unknown provenance key", + }, + { + name: "a missing origin", + mangle: func(s string) string { return strings.Replace(s, "# modelith-origin: https://github.com/acme/billing\n", "", 1) }, + wantErr: `missing "# modelith-origin"`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + path, r := vendored(t, upstream) + mangled := tc.mangle(readFile(t, path)) + if err := os.WriteFile(path, []byte(mangled), 0o644); err != nil { + t.Fatal(err) + } + rep := check(t, r, path)[0] + if rep.Skipped { + t.Fatal("a copy with a broken header was skipped rather than reported") + } + if rep.Err == nil { + t.Fatal("a copy with a broken header got a verdict") + } + for _, want := range []string{tc.wantErr, "modelith lint"} { + if !strings.Contains(rep.Err.Error(), want) { + t.Errorf("error does not contain %q: %v", want, rep.Err) + } + } + if len(r.calls) != 0 { + t.Errorf("a copy with a broken header was still fetched: %+v", r.calls) + } + }) + } +} + +// TestVisit_RefusesWhatTheOriginBecame pins that the two refusals import makes +// on a first fetch are made again on a refetch. The file at an address is not +// the file that was there last time, and writing either of these over a good +// copy would be worse than leaving it stale. +func TestVisit_RefusesWhatTheOriginBecame(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + upstream string + wantErr string + }{ + { + name: "the origin is now itself a copy", + upstream: "# modelith-vendored: " + "DO NOT EDIT\n" + moved, + wantErr: "somebody else's copy", + }, + { + name: "the origin no longer parses", + upstream: "kind: DomainModel\nentities: [oops\n", + wantErr: "no longer parses as a domain model", + }, + { + name: "the origin is no longer a domain model", + upstream: "kind: SomethingElse\nversion: v1\n", + wantErr: "no longer a domain model", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + path, r := vendored(t, upstream) + before := readFile(t, path) + r.content, r.sha = tc.upstream, laterSHA + + rep := update(t, r, "", path)[0] + if rep.Err == nil || !strings.Contains(rep.Err.Error(), tc.wantErr) { + t.Fatalf("want an error containing %q, got %v", tc.wantErr, rep.Err) + } + if after := readFile(t, path); after != before { + t.Error("the copy was overwritten with content the fetch should have refused") + } + }) + } +} + +// TestMovedHint offers the explanation a refetch needs and a first fetch does +// not: the address came out of a header written some time ago, and a model can +// move or be deleted without anything here noticing. It is owed only for a 404 — +// a rejected credential says nothing about where the file lives. +func TestMovedHint(t *testing.T) { + t.Parallel() + + path, r := vendored(t, upstream) + r.fail = "/contents/" + + rep := check(t, r, path)[0] + if rep.Err == nil { + t.Fatal("a failed fetch reported no error") + } + for _, want := range []string{"the model moved", "it was deleted", "docs/payments.modelith.yaml"} { + if !strings.Contains(rep.Err.Error(), want) { + t.Errorf("the 404 error does not contain %q: %v", want, rep.Err) + } + } +} + +// TestSourceFromHeader pins that a header's parts go back through the one +// parser rather than being pasted into an endpoint here. A hand-written header +// is untrusted input in exactly the way a typed URL is, and the guarantees +// escapePath depends on live in ParseSource. +func TestSourceFromHeader(t *testing.T) { + t.Parallel() + + path, r := vendored(t, upstream) + traversal := strings.Replace(readFile(t, path), + "# modelith-path: docs/payments.modelith.yaml", + "# modelith-path: docs/../../etc/passwd", 1) + if err := os.WriteFile(path, []byte(traversal), 0o644); err != nil { + t.Fatal(err) + } + + rep := check(t, r, path)[0] + if rep.Err == nil { + t.Fatal("a header with a traversal path was fetched") + } + if !strings.Contains(rep.Err.Error(), `".." path segment`) { + t.Errorf("the error does not name the traversal: %v", rep.Err) + } + if len(r.calls) != 0 { + t.Errorf("a traversal path still reached gh: %+v", r.calls) + } +} From 115c29518957c2ecb3a73fe225cfde9947a7d2ae Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 17:33:41 -0700 Subject: [PATCH 3/6] feat(cli): deps check and deps update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two commands rather than one with --dry-run, over the one survey in internal/deps: check never writes, which is what makes it safe in CI and against a read-only checkout, and two independent notions of "is it stale" would be free to disagree. Both take explicit file arguments like lint, shell-globbed. A file with no provenance header is skipped without complaint, because the glob a user already passes to lint holds their own models too — and a closing summary counts both, so a user who globbed wrong is not told "all up to date" about nothing. Verdicts go to stdout, per-file failures to stderr. Every check line names the ref it was checked against. A copy pinned to a tag is up to date for as long as that tag points where it did, and modelith does not look for newer releases; naming the ref is what keeps that verdict from reading as a statement about the world. A stale copy exits 1, matching render --check drift, and so does a copy that could not be reached — not being able to tell is not evidence of currency. update prints the transition rather than a diff: git diff answers "what changed" with the user's own tooling. When it writes anything it repeats ADR-0014's trust warning, since an origin trusted last month says nothing about the commit that landed yesterday, and points at modelith lint, because an item a copy used to define may have been renamed upstream and update cannot see the models that reference it. A no-op run prints neither. --ref re-pins one copy and refuses several: one ref names a different version in every other repository. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 240 ++++++++++++++++++++++++++++++++++++-- cmd/modelith/main_test.go | 169 +++++++++++++++++++++++++++ 2 files changed, 399 insertions(+), 10 deletions(-) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 1fded25..10eb15c 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -104,7 +104,7 @@ 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()) + cmd.AddCommand(depsImportCmd(), depsCheckCmd(), depsUpdateCmd()) return cmd } @@ -186,20 +186,240 @@ func printImportResult(out, errOut io.Writer, res *deps.Result, wd string) { 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, wd)) - 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, ", ")) + printTransitiveNote(out, name, res.TheirImports, "declares") + printTrustWarning(errOut) +} + +// printTransitiveNote says that a model reaches for models of its own and that +// modelith did not follow them. verb differs between a first import, where the +// imports are simply what the model has, and an update, where they are what it +// gained since the copy on disk was taken. +func printTransitiveNote(out io.Writer, name string, imports []string, verb string) { + n := len(imports) + if n == 0 { + return } + declares := fmt.Sprintf("%s %d imports of its own", verb, n) + if n == 1 { + declares = fmt.Sprintf("%s an import of its own", verb) + } + 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(imports, ", ")) +} + +// printTrustWarning is ADR-0014's warning, printed whenever third-party content +// lands in the repository. That an origin was trusted at import time says +// nothing about the commit that landed since, so an update repeats it. +func printTrustWarning(errOut io.Writer) { 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") } +func depsCheckCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "check ...", + Short: "Report which vendored copies have fallen behind their origin", + Long: strings.TrimSpace(` +Report which vendored copies have fallen behind their origin. + +A copy is stale when its origin now serves different content, compared against +the digest the copy's own header records. A file with no provenance header is +skipped, so the glob you already pass to lint works here unchanged; a closing +line says how many were skipped. Exits non-zero when any copy is stale. + +This command answers what the origin has done. Whether a copy here has been +hand-edited is a different question, and modelith lint answers it offline. + +A copy pinned to a tag is reported as up to date for as long as that tag points +where it did; modelith does not look for newer releases. Every line names the +ref it was checked against. + +Fetching is delegated to the gh CLI, which must be installed and authenticated. +Nothing is written.`), + Example: strings.TrimSpace(` + modelith deps check docs/payments.modelith.yaml + modelith deps check docs/*.modelith.yaml`), + Args: cobra.MinimumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + reports, err := deps.Check(cmd.Context(), deps.CheckOptions{Paths: args}) + blocking := printCheckReports(cmd.OutOrStdout(), cmd.ErrOrStderr(), reports) + if err != nil { + return err + } + if blocking { + return errBlocking + } + return nil + }, + } + return cmd +} + +func depsUpdateCmd() *cobra.Command { + var ref string + cmd := &cobra.Command{ + Use: "update ...", + Short: "Bring vendored copies forward to what their origins serve", + Long: strings.TrimSpace(` +Bring vendored copies forward to what their origins serve. + +A copy whose origin has not moved is left byte for byte alone, so running this +over a glob produces a diff only where something actually changed. A copy that +was hand-edited is not holding what its origin serves, so it is rewritten and +the edits are discarded — make the change at the origin instead. + +--ref re-pins one copy to a different tag or branch, which is how a pinned copy +moves; a copy tracking a branch moves on a bare update. It applies to exactly +one file, because one ref names a different version in every other repository. + +This command writes the copies and nothing else. It does not edit any model's +imports list, and it does not lint: an item a copy used to define may have been +renamed or removed upstream, and modelith lint is what reports that from the +importing model's seat. + +Fetching is delegated to the gh CLI, which must be installed and authenticated.`), + Example: strings.TrimSpace(` + modelith deps update docs/payments.modelith.yaml + modelith deps update docs/*.modelith.yaml + modelith deps update --ref v2.2.0 docs/payments.modelith.yaml`), + Args: cobra.MinimumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + reports, err := deps.Update(cmd.Context(), deps.UpdateOptions{ + Paths: args, + Ref: ref, + Now: time.Now(), + }) + blocking := printUpdateReports(cmd.OutOrStdout(), cmd.ErrOrStderr(), reports) + if err != nil { + return err + } + if blocking { + return errBlocking + } + return nil + }, + } + cmd.Flags().StringVar(&ref, "ref", "", "re-pin the copy to this ref (one file only)") + return cmd +} + +// printCheckReports reports a check run and says whether it should exit +// non-zero: a stale copy or a copy that could not be reached both qualify. +func printCheckReports(out, errOut io.Writer, reports []deps.Report) bool { + stale := 0 + for i := range reports { + r := &reports[i] + switch { + case r.Skipped: + case r.Err != nil: + fmt.Fprintf(errOut, "%s: %v\n", r.Path, r.Err) + case r.Stale(): + stale++ + fmt.Fprintf(out, "%s: stale at %s — the origin is now at %s\n", + r.Path, r.State.Ref, shortSHA(r.Commit)) + default: + fmt.Fprintf(out, "%s: up to date at %s\n", r.Path, r.State.Ref) + } + } + reached, failed, skipped := tally(reports) + line := fmt.Sprintf("checked %s, %d stale", plural(reached, "vendored copy", "vendored copies"), stale) + if stale == 0 && reached > 0 { + line = fmt.Sprintf("checked %s, all up to date", plural(reached, "vendored copy", "vendored copies")) + } + printSummary(out, line, failed, skipped) + return stale > 0 || failed > 0 +} + +// printUpdateReports reports an update run. A failure exits non-zero; a copy +// that was merely stale does not, because it is no longer stale. +func printUpdateReports(out, errOut io.Writer, reports []deps.Report) bool { + written := 0 + for i := range reports { + r := &reports[i] + switch { + case r.Skipped: + case r.Err != nil: + fmt.Fprintf(errOut, "%s: %v\n", r.Path, r.Err) + case r.Restored: + written++ + fmt.Fprintf(out, "%s: restored to %s — local edits discarded, make the change at its origin\n", + r.Path, shortSHA(r.Commit)) + case r.Written: + written++ + fmt.Fprintf(out, "%s: %s → %s at %s\n", + r.Path, shortSHA(r.State.Header.Commit), shortSHA(r.Commit), r.State.Ref) + default: + fmt.Fprintf(out, "%s: up to date at %s\n", r.Path, r.State.Ref) + } + } + reached, failed, skipped := tally(reports) + printSummary(out, fmt.Sprintf("updated %d of %s", written, + plural(reached, "vendored copy", "vendored copies")), failed, skipped) + + if written == 0 { + // Nothing arrived, so there is nothing to warn about and nothing whose + // vocabulary can have moved under the models that import it. + return failed > 0 + } + for i := range reports { + if r := &reports[i]; len(r.NewImports) > 0 { + printTransitiveNote(out, filepath.Base(r.Path), r.NewImports, "now declares") + } + } + fmt.Fprint(out, "\nRun `modelith lint` on the models that import these copies. An item a copy\n"+ + "used to define may have been renamed or removed upstream, which breaks a\n"+ + "reference this command cannot see.\n") + printTrustWarning(errOut) + return failed > 0 +} + +// tally counts what a run covered: copies reached, files that failed, and files +// that carried no provenance header. +func tally(reports []deps.Report) (reached, failed, skipped int) { + for i := range reports { + switch r := &reports[i]; { + case r.Skipped: + skipped++ + case r.Err != nil: + failed++ + default: + reached++ + } + } + return reached, failed, skipped +} + +// printSummary closes a run with what it covered, so a user whose glob matched +// the wrong files is not told "all up to date" about nothing. +func printSummary(out io.Writer, line string, failed, skipped int) { + if failed > 0 { + line += fmt.Sprintf(", %s could not be reached", plural(failed, "file", "files")) + } + if skipped > 0 { + line += fmt.Sprintf("; skipped %s with no provenance header", plural(skipped, "file", "files")) + } + fmt.Fprintf(out, "\n%s\n", line) +} + +func plural(n int, one, many string) string { + if n == 1 { + return fmt.Sprintf("%d %s", n, one) + } + return fmt.Sprintf("%d %s", n, many) +} + +// shortSHA abbreviates a commit for a line a human reads. Two 40-character +// SHAs either side of an arrow is a line nobody reads, and the full value is in +// the file's header a moment later. +func shortSHA(s string) string { + if len(s) > 7 { + return s[:7] + } + return s +} + // 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. diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index 14666f2..0aece57 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -554,6 +554,175 @@ func TestDepsImportRejectsBadArguments(t *testing.T) { } } +// report builds a Report for a copy that was reached and found current at the +// given ref. Calling provenance.Digest here is fixture plumbing, not the +// assertion: what the digest covers is pinned in internal/provenance, and what +// it decides is pinned in internal/deps. These tests are about the printing. +func report(path, ref, commit string) deps.Report { + const content = "kind: DomainModel\n" + return deps.Report{ + Path: path, + State: &deps.State{ + Path: path, Ref: ref, + Header: &provenance.Header{Commit: commit, Digest: provenance.Digest([]byte(content))}, + Local: []byte(content), + Upstream: []byte(content), + }, + Commit: commit, + } +} + +func TestDepsCheckOutput(t *testing.T) { + // Stale is an origin serving bytes that no longer hash to what the header + // recorded. + stale := report("docs/ledger.modelith.yaml", "main", "a91b0c37e5d248fa") + stale.State.Upstream = []byte("kind: DomainModel\ntitle: moved on\n") + + t.Run("verdicts name the ref and the summary counts what was covered", func(t *testing.T) { + var out, errOut bytes.Buffer + reports := []deps.Report{ + report("docs/payments.modelith.yaml", "v2.1.0", "4f2c1e9c8b3ad0e5"), + stale, + {Path: "docs/ours.modelith.yaml", Skipped: true}, + {Path: "docs/gone.modelith.yaml", Err: errors.New("cannot be read: no such file or directory")}, + } + blocking := printCheckReports(&out, &errOut, reports) + if !blocking { + t.Error("a stale copy did not make the run exit non-zero") + } + for _, want := range []string{ + // Naming the ref is what keeps a pinned copy's verdict honest: it + // reads as a statement about the pin, not about the world. + "docs/payments.modelith.yaml: up to date at v2.1.0", + "docs/ledger.modelith.yaml: stale at main — the origin is now at a91b0c3", + "checked 2 vendored copies, 1 stale, 1 file could not be reached; skipped 1 file with no provenance header", + } { + if !strings.Contains(out.String(), want) { + t.Errorf("stdout does not contain %q:\n%s", want, out.String()) + } + } + if !strings.Contains(errOut.String(), "docs/gone.modelith.yaml: cannot be read") { + t.Errorf("the failure is not on stderr:\n%s", errOut.String()) + } + }) + + // A user whose glob matched only their own models must not read "all up to + // date" as a statement about their vendored copies. + t.Run("a run that reached nothing says so", func(t *testing.T) { + var out, errOut bytes.Buffer + blocking := printCheckReports(&out, &errOut, []deps.Report{ + {Path: "a.modelith.yaml", Skipped: true}, + {Path: "b.modelith.yaml", Skipped: true}, + }) + if blocking { + t.Error("a run with nothing to check exited non-zero") + } + if want := "checked 0 vendored copies, 0 stale; skipped 2 files with no provenance header"; !strings.Contains(out.String(), want) { + t.Errorf("stdout does not contain %q:\n%s", want, out.String()) + } + }) + + // A file that could not be reached is not evidence that it is current. + t.Run("a failure alone exits non-zero", func(t *testing.T) { + var out, errOut bytes.Buffer + if !printCheckReports(&out, &errOut, []deps.Report{{Path: "x", Err: errors.New("boom")}}) { + t.Error("a run where nothing could be reached exited zero") + } + }) +} + +func TestDepsUpdateOutput(t *testing.T) { + t.Run("a refresh names both commits, and the run closes with what to do next", func(t *testing.T) { + var out, errOut bytes.Buffer + refreshed := report("docs/ledger.modelith.yaml", "main", "a91b0c37e5d248fa") + refreshed.State.Header.Commit = "4f2c1e9c8b3ad0e5" + refreshed.Written = true + refreshed.NewImports = []string{"./tax.modelith.yaml"} + + if printUpdateReports(&out, &errOut, []deps.Report{ + refreshed, + report("docs/payments.modelith.yaml", "v2.1.0", "4f2c1e9c8b3ad0e5"), + }) { + t.Error("a successful update exited non-zero") + } + for _, want := range []string{ + "docs/ledger.modelith.yaml: 4f2c1e9 → a91b0c3 at main", + "docs/payments.modelith.yaml: up to date at v2.1.0", + "updated 1 of 2 vendored copies", + "now declares an import of its own (./tax.modelith.yaml)", + "Run `modelith lint` on the models that import these copies", + } { + if !strings.Contains(out.String(), want) { + t.Errorf("stdout does not contain %q:\n%s", want, out.String()) + } + } + if !strings.Contains(errOut.String(), "Only vendor from sources you trust") { + t.Errorf("an update that wrote did not repeat the trust warning:\n%s", errOut.String()) + } + }) + + // Nothing arrived, so there is nothing to warn about and no vocabulary that + // can have moved under the models importing these copies. Printing either + // on a no-op run is the fastest way to teach people to skip both. + t.Run("a no-op run warns about nothing", func(t *testing.T) { + var out, errOut bytes.Buffer + printUpdateReports(&out, &errOut, []deps.Report{ + report("docs/payments.modelith.yaml", "main", "4f2c1e9c8b3ad0e5"), + }) + if errOut.Len() != 0 { + t.Errorf("a no-op update printed to stderr:\n%s", errOut.String()) + } + if strings.Contains(out.String(), "modelith lint") { + t.Errorf("a no-op update told the user to lint:\n%s", out.String()) + } + }) + + t.Run("a restore says the edits went", func(t *testing.T) { + var out, errOut bytes.Buffer + restored := report("docs/payments.modelith.yaml", "main", "4f2c1e9c8b3ad0e5") + restored.Written, restored.Restored = true, true + printUpdateReports(&out, &errOut, []deps.Report{restored}) + if want := "restored to 4f2c1e9 — local edits discarded, make the change at its origin"; !strings.Contains(out.String(), want) { + t.Errorf("stdout does not contain %q:\n%s", want, out.String()) + } + }) +} + +// TestDepsRejectsBadArguments covers the paths that fail before any fetch, so +// they need no network and no gh. +func TestDepsRejectsBadArguments(t *testing.T) { + dir := t.TempDir() + ours := writeTemp(t, dir, "ours.modelith.yaml", minimalValid) + theirs := writeTemp(t, dir, "theirs.modelith.yaml", minimalValid) + + t.Run("--ref across several copies is refused", func(t *testing.T) { + _, err := run(t, "deps", "update", "--ref", "v2.2.0", ours, theirs) + if err == nil || !strings.Contains(err.Error(), "--ref re-pins one copy") { + t.Fatalf("want a refusal naming the flag, got %v", err) + } + }) + + t.Run("no files at all is a usage error", func(t *testing.T) { + for _, sub := range []string{"check", "update"} { + if _, err := run(t, "deps", sub); err == nil { + t.Errorf("deps %s with no arguments succeeded", sub) + } + } + }) + + // A glob of this repository's own models reaches nothing and needs no gh: + // the header check happens before any fetch. + t.Run("a glob with no vendored copy in it needs no network", func(t *testing.T) { + out, err := run(t, "deps", "check", ours, theirs) + if err != nil { + t.Fatalf("checking two unvendored files failed: %v", err) + } + if want := "skipped 2 files with no provenance header"; !strings.Contains(out, want) { + t.Errorf("output does not contain %q:\n%s", want, out) + } + }) +} + func TestSchemaOutputsValidJSON(t *testing.T) { out, err := run(t, "schema") if err != nil { From ea8e6cd009f338b7a769910ea6ef98e5eae67113 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 17:33:48 -0700 Subject: [PATCH 4/6] fix(lint): point a digest mismatch at deps update, not deps import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remedy for an edited vendored copy was a blob URL the message assembled from the header's parts. deps update takes the file's own path instead, reads the header itself, and — when the origin has not moved — rewrites the copy to exactly the bytes the original import wrote. That is a restore rather than a re-import, which is what the user wants and what the message should say. A model linted without a path names the file generically rather than offering a path that would not work. Signed-off-by: Joe Beda --- internal/lint/lint.go | 2 +- internal/lint/provenance.go | 24 ++++++++++++++---------- internal/lint/provenance_test.go | 6 +++++- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/internal/lint/lint.go b/internal/lint/lint.go index f63e2d8..c317900 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -90,7 +90,7 @@ func Run(path string, src []byte, files Files) (*Result, error) { // 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) + vendored := runProvenance(path, src, res) // Layer 1: structural validation against the JSON Schema. structuralOK, entityScopes := runStructural(src, res) diff --git a/internal/lint/provenance.go b/internal/lint/provenance.go index 12c0593..d87c532 100644 --- a/internal/lint/provenance.go +++ b/internal/lint/provenance.go @@ -16,7 +16,7 @@ import ( // 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 { +func runProvenance(path string, src []byte, res *Result) bool { h, problems := provenance.Parse(src) if h == nil { return false @@ -43,22 +43,26 @@ func runProvenance(src []byte, res *Result) bool { 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)), + "this vendored file no longer matches the digest its provenance header records (recorded %s, computed %s) — it has been edited since it was imported. Restore it with `modelith deps update %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(path)), }) } } 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) +// refreshTarget is what to hand `modelith deps update` to restore this copy: +// its own path. The header already records where the file came from, so the +// remedy needs nothing the user has to assemble by hand — and restoring a copy +// whose origin has not moved rewrites it to exactly the bytes the import wrote. +// +// A model read from stdin has no path to offer, in which case the remedy names +// the file generically rather than a path that would not work. +func refreshTarget(path string) string { + if path == "" { + return "" } - return h.Origin + return path } // dropOwnedDiagnostics removes the findings that are about a model's own diff --git a/internal/lint/provenance_test.go b/internal/lint/provenance_test.go index a9685ad..707dfdc 100644 --- a/internal/lint/provenance_test.go +++ b/internal/lint/provenance_test.go @@ -187,8 +187,12 @@ func TestVendored_DigestMismatch(t *testing.T) { if found.Severity != SeverityError { t.Errorf("digest mismatch is a %s, want an error", found.Severity) } + // The remedy names this file's own path, not a URL the user would have + // to assemble: the header already records where the copy came from, and + // restoring a copy whose origin has not moved rewrites it to exactly + // the bytes the import wrote. for _, want := range []string{ - "modelith deps import https://github.com/stacklok/some-repo/blob/main/docs/payments.modelith.yaml", + "modelith deps update docs/garage.modelith.yaml", "delete the provenance header", } { if !strings.Contains(found.Message, want) { From 0d3535cef37f96613e89a6a0bbefc4c0f2ad2eab Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 17:35:18 -0700 Subject: [PATCH 5/6] docs(vendoring): document deps check and deps update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pairs the new commands with the section they belong beside: keeping a copy honest is about the copy, keeping it current is about the model it came from. Covers the two tracking modes — a branch that a bare update follows, and a tag that only --ref moves — and states the limitation that falls out of a content comparison, that a pinned copy reads as up to date until its tag moves. Says plainly that an upstream documentation-only change counts as a change, because the digest covers the whole file, and that git diff after an update is how you find that out. Also updates the digest-mismatch example to the remedy lint now prints, adds the deps check and deps update entries to the CLI reference, and points at git grep for finding vendored copies, since neither command discovers them. Signed-off-by: Joe Beda --- docs/07-cli.md | 50 +++++++++++++++++- docs/10-vendoring.md | 118 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 155 insertions(+), 13 deletions(-) diff --git a/docs/07-cli.md b/docs/07-cli.md index 282e1ea..0c60b84 100644 --- a/docs/07-cli.md +++ b/docs/07-cli.md @@ -120,6 +120,54 @@ and authenticated. The command writes the file and prints the `imports:` entry 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. +### `modelith deps check ...` + +Reports which vendored copies have fallen behind their origins. Writes nothing, +and exits non-zero when any copy is stale — or when one could not be reached, +since not being able to tell is not evidence that it is current. + +```sh +modelith deps check docs/*.modelith.yaml +``` + +A copy is stale when its origin serves different content, compared against the +digest in the copy's own header. A commit that touched the path without +changing the file is not a change. Whether a copy *here* has been hand-edited +is a different question, and `lint` answers it offline. + +Every line names the ref it checked against, because a copy pinned to a tag is +up to date for as long as that tag points where it did — modelith does not look +for newer releases. + +### `modelith deps update [--ref ] ...` + +Brings vendored copies forward to what their origins serve. + +```sh +modelith deps update docs/*.modelith.yaml +modelith deps update --ref v2.2.0 docs/payments.modelith.yaml +``` + +| Argument / flag | Meaning | +|---|---| +| `...` | Vendored copies to update. Files with no provenance header are skipped. | +| `--ref` | Re-pin the copy to this ref. Applies to **one file**: a single ref names a different version in every other repository. | + +A copy whose origin has not moved is left byte for byte alone, so running this +over a glob produces a diff only where something changed. A copy that was +hand-edited is not holding what its origin serves, so it is rewritten and the +edits go. + +It writes the copies and nothing else — it does not edit any model's `imports:` +and it does not lint. Run `modelith lint` afterwards: an item a copy used to +define may have been renamed or removed upstream, which breaks references +`update` cannot see. + +Both commands take file arguments the way `lint` does and skip files with no +provenance header, so the glob you already lint works unchanged. Find your +copies with `git grep -l '# modelith-vendored'`. + 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. +is linted differently, how the two tracking modes differ, and why vendoring +fetches one file rather than a tree. diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md index 844d60b..a1afbac 100644 --- a/docs/10-vendoring.md +++ b/docs/10-vendoring.md @@ -1,7 +1,7 @@ --- 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. +description: Copy a model from another repository, reference its items, and keep the copy current as its origin moves on. --- # Vendoring a model from another repository @@ -140,25 +140,114 @@ 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. +has been edited since it was imported. Restore it with `modelith deps update +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. +Both remedies are real. `deps update` puts the copy back to what its origin +serves — and if the origin has not moved, that is byte for byte what the import +wrote. 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. +## Keeping the copy current + +The section above is about your copy. This one is about the model it came from, +which moves on without you. + +```sh +modelith deps check docs/*.modelith.yaml +``` + +``` +docs/payments.modelith.yaml: up to date at v2.1.0 +docs/ledger.modelith.yaml: stale at main — the origin is now at a91b0c3 + +checked 2 vendored copies, 1 stale +``` + +`deps check` writes nothing and exits non-zero when any copy is stale, so it +works as a scheduled CI job. `deps update` takes the same arguments and brings +the copies forward: + +```sh +modelith deps update docs/ledger.modelith.yaml +``` + +``` +docs/ledger.modelith.yaml: 4f2c1e9 → a91b0c3 at main + +updated 1 of 1 vendored copy +``` + +Then read `git diff` to see what actually changed, and run `modelith lint`. An +item the copy used to define may have been renamed or removed upstream, which +breaks references in *your* model — `update` cannot see those, because it does +not know which of your models import the copy. + +Both commands take file arguments, the same way `lint` does, and skip any file +with no provenance header. That means the glob you already lint works +unchanged; the closing line tells you how many files were skipped, so a glob +that matched none of your copies does not read as good news. To find them: + +```sh +git grep -l '# modelith-vendored' +``` + +### Two ways to track a model + +Which one you are on is whatever `# modelith-ref:` records. + +- **Tracking a branch.** `deps update` fetches whatever that branch has now. + You get upstream's changes as they land, and `deps check` tells you when + there are some. +- **Pinned to a tag.** `deps update` alone does nothing, because the tag still + points where it did. Moving to a new version is explicit: + + ```sh + modelith deps update --ref v2.2.0 docs/payments.modelith.yaml + ``` + + `--ref` re-pins one copy at a time. A single ref names a different version in + every other repository, so it is refused with several files. + +:::note[A pinned copy is always "up to date"] + +`deps check` compares content against the ref your header records. On a tag, +that never changes, so a copy pinned to `v2.1.0` reports as up to date for as +long as `v2.1.0` exists — even after `v2.3.0` ships. modelith does not look for +newer releases, which is why every line of output names the ref it checked +against. + +::: + +### What "stale" means + +A copy is stale when its origin serves **different content**, compared against +the digest in the copy's own header. It is not a commit comparison: a merge or +a whitespace-only touch upstream moves the commit without changing the model, +and that is not something you need to act on. + +That has one consequence worth knowing: an upstream change to a `description:` +counts, because the digest covers the whole file. You will be told a +documentation-only change is waiting for you, and `git diff` after the update +is how you find out that is all it was. + +`deps update` writes only where something changed, so running it over a glob +produces a diff exactly where one belongs. A copy that was hand-edited is not +holding what its origin serves, so it gets rewritten too, and the edits go — +make the change at the origin instead. + ## 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: +`deps import` tells you they exist, and `deps update` says the same thing again +if a refresh brings imports the copy did not have before: ``` Note: payments.modelith.yaml declares an import of its own (./ledger.modelith.yaml). @@ -198,8 +287,13 @@ already solves. - **`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. +- **No newer-release detection.** `deps check` tells you whether the ref you + pinned still serves what you have. It does not tell you a newer tag exists, + because deciding which tags count as newer means guessing at a versioning + scheme modelith has no way to know. 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) +[ADR-0010](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0010-cross-model-references-by-vendoring.md), +[ADR-0015](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md), and -[ADR-0015](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md). +[ADR-0016](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0016-staleness-is-a-content-comparison.md). From 8652eb08d1a5281c987981055d56a1e961015f81 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Mon, 27 Jul 2026 17:39:15 -0700 Subject: [PATCH 6/6] fix(deps,cli): don't crash on an aborted run, or summarise one that never ran MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found by running the real binary against a real origin, which is where they were only ever going to show up. survey appended a Report for the file it abandoned when gh proved unusable. That Report had neither a verdict nor a per-file failure, so the printer fell through to its default branch and dereferenced a nil State: `deps check` on a machine with no gh panicked instead of saying gh is not installed. The abort error is the whole story for that file, so it gets no Report — and the printers now treat a Report with no verdict as nothing to say, since a command whose job is reporting calmly on other people's files must not be the thing that crashes. The unit test for the abort had asserted the buggy shape, expecting a Report for the abandoned file. It now pins that there is none. Separately, a run refused before it started — `--ref` with several files — still printed "updated 0 of 0 vendored copies" above the reason, which reads as the outcome of a run that happened. Nothing reached means nothing to summarise. Signed-off-by: Joe Beda --- cmd/modelith/main.go | 16 +++++++++++++--- cmd/modelith/main_test.go | 21 ++++++++++++++++++++- internal/deps/refresh.go | 7 +++++-- internal/deps/refresh_test.go | 8 ++++++-- 4 files changed, 44 insertions(+), 8 deletions(-) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 10eb15c..b7b2279 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -244,7 +244,9 @@ Nothing is written.`), Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { reports, err := deps.Check(cmd.Context(), deps.CheckOptions{Paths: args}) - blocking := printCheckReports(cmd.OutOrStdout(), cmd.ErrOrStderr(), reports) + // A run that got nowhere has nothing to summarise, and "checked 0 + // vendored copies" above the reason would read as the outcome. + blocking := len(reports) > 0 && printCheckReports(cmd.OutOrStdout(), cmd.ErrOrStderr(), reports) if err != nil { return err } @@ -291,7 +293,8 @@ Fetching is delegated to the gh CLI, which must be installed and authenticated.` Ref: ref, Now: time.Now(), }) - blocking := printUpdateReports(cmd.OutOrStdout(), cmd.ErrOrStderr(), reports) + // See depsCheckCmd: nothing reached means nothing to summarise. + blocking := len(reports) > 0 && printUpdateReports(cmd.OutOrStdout(), cmd.ErrOrStderr(), reports) if err != nil { return err } @@ -315,6 +318,11 @@ func printCheckReports(out, errOut io.Writer, reports []deps.Report) bool { case r.Skipped: case r.Err != nil: fmt.Fprintf(errOut, "%s: %v\n", r.Path, r.Err) + case r.State == nil: + // A file the run abandoned rather than judged. survey does not hand + // one back, and this keeps a future one that does from crashing a + // command whose whole job is to report calmly on other people's + // files. case r.Stale(): stale++ fmt.Fprintf(out, "%s: stale at %s — the origin is now at %s\n", @@ -346,6 +354,8 @@ func printUpdateReports(out, errOut io.Writer, reports []deps.Report) bool { written++ fmt.Fprintf(out, "%s: restored to %s — local edits discarded, make the change at its origin\n", r.Path, shortSHA(r.Commit)) + case r.State == nil: + // See printCheckReports: a file the run abandoned rather than judged. case r.Written: written++ fmt.Fprintf(out, "%s: %s → %s at %s\n", @@ -384,7 +394,7 @@ func tally(reports []deps.Report) (reached, failed, skipped int) { skipped++ case r.Err != nil: failed++ - default: + case r.State != nil: reached++ } } diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index 0aece57..217d238 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -629,6 +629,19 @@ func TestDepsCheckOutput(t *testing.T) { t.Error("a run where nothing could be reached exited zero") } }) + + // survey does not produce a Report with neither a verdict nor a failure — + // but one reached the printer once and took the whole command down with a + // nil dereference. A command whose job is reporting calmly on other + // people's files must not be the thing that crashes. + t.Run("a report with no verdict is not a crash", func(t *testing.T) { + var out, errOut bytes.Buffer + printCheckReports(&out, &errOut, []deps.Report{{Path: "abandoned.modelith.yaml"}}) + printUpdateReports(&out, &errOut, []deps.Report{{Path: "abandoned.modelith.yaml"}}) + if strings.Contains(out.String(), "1 vendored copy") { + t.Errorf("an unjudged file was counted as reached:\n%s", out.String()) + } + }) } func TestDepsUpdateOutput(t *testing.T) { @@ -696,10 +709,16 @@ func TestDepsRejectsBadArguments(t *testing.T) { theirs := writeTemp(t, dir, "theirs.modelith.yaml", minimalValid) t.Run("--ref across several copies is refused", func(t *testing.T) { - _, err := run(t, "deps", "update", "--ref", "v2.2.0", ours, theirs) + out, err := run(t, "deps", "update", "--ref", "v2.2.0", ours, theirs) if err == nil || !strings.Contains(err.Error(), "--ref re-pins one copy") { t.Fatalf("want a refusal naming the flag, got %v", err) } + // A run refused before it started has nothing to summarise. Printing + // "updated 0 of 0 vendored copies" above the reason reads as the + // outcome of a run that happened. + if strings.Contains(out, "vendored cop") { + t.Errorf("a refused run printed a summary:\n%s", out) + } }) t.Run("no files at all is a usage error", func(t *testing.T) { diff --git a/internal/deps/refresh.go b/internal/deps/refresh.go index 7e0d686..7fc3440 100644 --- a/internal/deps/refresh.go +++ b/internal/deps/refresh.go @@ -149,12 +149,15 @@ func survey(ctx context.Context, opts surveyOptions) ([]Report, error) { reports := make([]Report, 0, len(opts.paths)) for _, p := range opts.paths { rep, err := visit(ctx, runner, p, opts) - reports = append(reports, rep) if err != nil { // gh itself is unusable, so every file left would fail identically. - // Stop, and hand back what was learned before it. + // Stop, and hand back what was learned before this one. The file + // that hit it gets no Report: it was abandoned mid-flight, so it has + // neither a verdict nor a per-file failure to report, and the error + // returned here is the whole story. return reports, err } + reports = append(reports, rep) } return reports, nil } diff --git a/internal/deps/refresh_test.go b/internal/deps/refresh_test.go index 49300a4..27d285e 100644 --- a/internal/deps/refresh_test.go +++ b/internal/deps/refresh_test.go @@ -233,8 +233,12 @@ func TestSurvey_StopsWhenTheToolIsUnusable(t *testing.T) { if !errors.Is(err, ErrToolUnavailable) { t.Fatalf("Check returned %v, want an ErrToolUnavailable", err) } - if len(reports) != 1 { - t.Errorf("got %d reports, want the run to stop after the first: %+v", len(reports), reports) + // The file that hit it gets no Report. It was abandoned mid-flight, so it + // has neither a verdict nor a per-file failure, and a Report carrying + // neither is a hole every consumer has to remember to check — one of them + // did not, and dereferenced its nil State. + if len(reports) != 0 { + t.Errorf("got %d reports, want none — the run never judged a file: %+v", len(reports), reports) } if r.calls != 1 { t.Errorf("the run made %d calls after gh proved unusable, want 1", r.calls)