feat: vendor a model from another repository (#25, slice 2) - #40
Conversation
…ding it 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 <joe@stacklok.com>
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 <joe@stacklok.com>
Adds internal/provenance, the package that owns the "# modelith-<key>: " 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 <joe@stacklok.com>
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 <joe@stacklok.com>
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 <joe@stacklok.com>
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 <joe@stacklok.com>
Adds `modelith deps import <url> [dir]`, the command that acquires a model and
stamps it as a vendored copy. <url> 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 <joe@stacklok.com>
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 <joe@stacklok.com>
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 <joe@stacklok.com>
--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 <joe@stacklok.com>
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 "./<basename>", ignoring the destination directory, so `deps import <url> docs/vendor/` printed a path that resolves to nothing. Signed-off-by: Joe Beda <joe@stacklok.com>
Review loop — round 1 (real
|
| # | Where | Sev | Fix |
|---|---|---|---|
| R1-1 | deps.ParseSource |
med | --ref mis-split the path when the URL's ref contained a slash. A browse URL cannot express both a pin and a re-split, so there is no silent fix: a failed fetch now reports what modelith read as ref and as path, plus both remedies. |
| R1-2 | deps.Import |
med | Overwrote any file sharing the basename — including a model this repo wrote. New guardTarget refuses a file with no provenance header, and one vendored from a different origin/path. Same-model refresh still reports replaced. |
| R1-3 | deps.Import |
med | err != nil || m.Kind != "DomainModel" discarded the parse error, so a model from a newer modelith reported "check that the URL names a *.modelith.yaml file". Split; the parse error is wrapped with %w. |
| R1-4 | deps.ParseSource |
low | Case-sensitive host check rejected GitHub.com with a self-contradictory message. Now ToLower + www. trim. |
| R1-5 | deps.ParseSource |
low | ./../empty path segments passed through escapePath intact. Rejected now — this is what makes the nolint:gosec justification on ExecRunner.Run actually true, and that comment says so. |
| R1-6 | printImportResult |
low | Printed ./<basename> regardless of [dir], so deps import <url> docs/vendor/ printed an unresolvable path. |
| R1-7 | internal/provenance |
low | Present/Strip never implemented the leading-comment-block rule Parse already applied — ADR-0015's "one rule parses and strips" was half-built. Now shared. |
| R1-8 | renderCmd |
low | --check skipped a vendored copy unconditionally, so a committed vendored .md went stale after a re-import with no signal. |
R1-8, per @jbeda's direction
Check it, but stay quiet about failures that come from outside this repo. So --check now skips a vendored copy in exactly two cases — no committed .md, or a copy this build cannot render at all (newer schema version, unknown shape). modelith lint owns that second error and reports it once; repeating it here would fail a build over something nobody can fix from this side. Anything else, including drift in a committed vendored .md, is a normal --check failure.
Evidence
task check green. Probes against a locally built binary, since deleted:
- R1-4 — a
GitHub.comURL now reaches the fetch instead of being rejected. - R1-5 —
.../blob/main/../../../user/reposrejected by name. - R1-7 — an owned model carrying
# modelith-note:mid-body gets its completeness warnings back (they were being suppressed); the same line added below a real header now trips the digest mismatch. - R1-8 — full cycle: no
.md→skipped, exit 0;renderby name →up to date; edit the copy and restamp its digest →out of date, exit 1. Aversion: v99vendored copy with a committed.md→skipped, exit 0, while the identical file without a header still fails.
Tests added
TestDigest_CoversALineBelowTheHeader; TestImport_RefusesToOverwriteWhatItDidNotWrite (both refusal cases); ParseSource cases for host casing, .., and empty segments; Import_Rejections cases for kind / no-kind / unparseable and the split hint; TestRenderCheckChecksACommittedVendoredMarkdown and TestRenderCheckStaysQuietOnAVendoredCopyItCannotRender; plus Present and Parse_Problems cases.
Docs
docs/07-cli.md (the --ref limit, the destination guard), docs/08-github-action.md and docs/10-vendoring.md (the new --check behaviour, the overwrite refusals).
No blocking findings remain. Still draft pending a decision on a scoped second round over these fixes.
…failing 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 <joe@stacklok.com>
Review loop — round 2 (approximated, two scoped angles)Not Angles were chosen on failure mode, not diff size: seven of the eight round-one fixes changed messages or refusals, where a mistake is visible on first run. Two changed behaviour — the digest definition and a CI gate — where a mistake is silent. Those two got the round. A —
|
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 <joe@stacklok.com>
Review loop — round 3 (real
|
| # | Sev | Finding | Fix |
|---|---|---|---|
| #5 | LOW | The misconfigured--o guard ran 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 settled before anything can exempt a file. |
| #1 | MED | deps import printed the destination verbatim, so an absolute directory — what an agent driving the command usually passes — produced an imports: entry internal/lint/imports.go rejects outright as absolute. |
Relativized against the working directory. |
| #2 | MED | The ref/path split hint was appended to every fetch failure, so "gh is not installed" arrived with a paragraph about slashed refs attached. | Offered only for a not-found. Gated on gh's text, with a comment saying why (gh is a separate program; there is no typed error to unwrap). |
| #4 | LOW/MED | guardTarget compared origins byte-for-byte, refusing a refresh differing only in the casing GitHub itself ignores — and explaining it by claiming two different models shared the filename. Same wrong message when the model had simply moved upstream. |
Casing ignored via EqualFold; a moved path gets its own message naming both remedies. |
| #6 | LOW | The import refusal asserted the file's provenance line "names where it came from", which the code never checked. | Message now gives both readings. |
Verified after fixing
#3 own.modelith.yaml → error: cannot read committed output … exit 1
#5 -o nodir/out.md unrenderable → error: cannot check nodir/out.md: nodir is not a directory
#2 PATH=/nonexistent deps import → "gh is not installed …" and nothing else
regression guard: a clean vendored copy still skips (exit 0), and drift-checks once rendered
Tests added: TestRenderCheckDoesNotExemptAModelThisRepoOwns (covers both the stray-comment model and a displaced header), the unrenderable case folded into TestRenderCheckRejectsAnOutPathUnderNoDirectory, TestSplitHint (6 error shapes), TestImport_RefreshIsNotRefusedOverCasing, TestImport_RefusesAMovedPathWithTheRightRemedy, and an absolute-destination case for the import entry. Docs updated in 10-vendoring.md and 08-github-action.md.
What the review confirmed clean
guardTarget's Parse cannot nil-deref; the dot/empty-segment rejection holds against double-encoding (%252e%252e) and the host normalization has no bypass (www.github.com.evil.com still rejected, Origin rebuilt from a literal); Stamp's hoist and Strip's lines[:0] aliasing are safe; --stdout/--check exclusivity makes the target hoist inert; docs match behaviour.
Worth recording
Round two was my own subagents reviewing my own fixes, and I said at the time that it shared my blind spots. It did — #3 is a hole in exactly the reasoning I had used to justify round two's own fix, and neither my subagent nor I saw it. The independent round found it in one pass. task check green.
1 participant
Slice 2 of #25: vendoring. Slice 1 (local imports) shipped in #34; this adds the half that gets a model from another repository into yours and teaches every other part of the tool that the copy is not your work.
Design was settled in an interview before any code: ADR-0015 records the decisions and supersedes ADR-0010's Digest section. The rest of ADR-0010 stands.
What lands
modelith deps import <url> [dir]— paste the GitHub blob URL from your browser. Fetches viagh(argv array, no shell), stamps a provenance header, writes the file. Destination is a positional directory defaulting to., git-clone style.linthonours it: suppresses the completeness category and the errors the file's ownimports:would raise, and nothing else. Verifies the copy against the digest.render --checkskips it — a vendored model's.mdbelongs to its home repo.Three decisions worth your attention
The surface digest is gone, replaced by SHA-256 over the file with its header lines removed. ADR-0010 specified a digest covering structure and normative content only. That needed a canonicalisation pass and a permanent tax — every future schema field classified surface-or-documentation, forever — and it loses on its main job: the realistic accidental edit is a teammate fixing a typo in somebody else's model, which a surface digest deliberately ignores. A
surface:key can be added later with a real complaint driving it.One rule parses and strips the header. The digest is defined by the strip, so two rules that could disagree would mean the digest covers bytes the parser never saw. A line counts only at column zero for the same reason.
Vendoring fetches one file, deliberately. The closure would need a vendor directory, rewritten paths (breaking the unmodified-copy invariant the digest rests on), diamond dedupe, 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.
deps importnames the fetched model's own imports; lint says at the reference site to import them directly.Acceptance criteria
Agreed before implementation; each verified from a shell against the real binary, including a live fetch from this repo.
deps importfetches, stamps, prints the snippet, the trust warning, and the transitive noteTestADR_0015_ImportFetchesOneFileNotATree,TestDepsImportOutput# modelith-*linesTestADR_0015_DigestCoversTheFileWithoutItsHeaderscope.NameTestVendored_ImportsStillResolveWhenTheyCanTestADR_0015_HeaderSuppressesCompletenessOnly,TestVendored_ImportsRaiseNothingTestVendored_DigestMismatchrender --checkskips it;render <file>still renders itTestRenderCheckSkipsAVendoredCopyTestImports_Resolutionfetch:value is a loud errorTestADR_0015_UnknownFetchMethodIsAnErrorTestADR_0015_*08-github-action.mdupdateddocs/10-vendoring.mdCriterion 4 is stated as a difference, not a hand-written expectation: the same model is linted twice, once owned and once vendored, and every finding that disappears must be a completeness finding. A suppression that was too broad cannot pass it. The digest test builds its expectation with
crypto/sha256directly rather than callingStrip— a test reusing 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.Not in this change
deps update/deps check(slice 3), transitive vendoring, editing yourimports:for you, a non-ghtransport, #38.#39 is a knowingly accepted gap, recorded in ADR-0015's consequences: an unbalanced code fence in a vendored model's prose still corrupts the importing repo's Markdown. Vendoring strengthens the case for that rule — it is exactly where someone else's prose lands in your document — but it shares no code with this change, and it is a visibly broken document rather than a privilege boundary.
No schema change
imports:already exists and a header comment is not schema, soTestSchemaStructSyncand the schema-URL tests are not in play. Meaningfully smaller blast radius than slice 1.Also here
Import.ScopeFromPathis deleted (first commit) — the onlyjson:"-"exception toTestSchemaStructSync, whose zero value meant "already validated" and so failed open. An explicit scope is schema-validated before the imports layer runs, so a scope that fails there can only have been derived from a filename.Review
task checkgreen. Round one of the review loop has not run — no/code-reviewand no approximation of one. Staying draft until it does.