feat(deps): keep a vendored copy current with deps check and deps update - #42
Draft
jbeda wants to merge 6 commits into
Draft
feat(deps): keep a vendored copy current with deps check and deps update#42jbeda wants to merge 6 commits into
jbeda wants to merge 6 commits into
Conversation
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 <joe@stacklok.com>
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 <joe@stacklok.com>
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 <joe@stacklok.com>
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 <joe@stacklok.com>
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 <joe@stacklok.com>
…ever ran 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 <joe@stacklok.com>
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #25, slice 3 — the vendoring lifecycle. Slice 1 (local imports) shipped in #34, slice 2 (vendoring) in #40. This adds finding out that a copy has gone stale, and refreshing it.
Design settled in a grilling interview before any code. The brief and its acceptance criteria are at
.scratch/design/issue-25-slice3-brief.md(local, gitignored); the decisions that clear the ADR bar areproject-docs/adr/0016-staleness-is-a-content-comparison.md.This does not close #25. Qualified entity references in
relationship.entityandsubtypeOfare still deferred with a friendly error; that is slice 4, and #25 stays open as its umbrella.What lands
The decisions worth arguing with
lintanswers "has my copy been tampered with?"deps checkanswers "has the model changed upstream?" They do not overlap.checkdeliberately does not re-verify the local digest.Staleness is a content comparison, not a commit comparison. A property of slice 2's design makes this clean:
importrecords the digest over the upstream bytes before stamping, so# modelith-digest:is the origin's digest rather than the local file's. Comparing it against what the origin serves now is independent of local edits. Commit comparison is wrong in both directions — a merge or whitespace touch reports stale over nothing, and a SHA cannot say what differs. The commit stays as reporting, resolved only when the content actually differs, so a clean check costs oneghcall per copy rather than two.One write condition, and every case falls out of it:
A restore re-stamps the recorded header verbatim, reproducing the original import's bytes exactly, so
# modelith-imported:dates the arrival of a version rather than the last check. That is what forecloses a# modelith-checked:key: recording a check would make every check a write.Explicit file arguments only,
MinimumNArgs(1)likelint. No tree walking, no zero-arg form. A file with no provenance header is skipped, and a closing summary counts both, so a glob that matched none of your copies does not read as good news.A pinned copy reports up to date forever. Every line names the ref it checked against so the verdict reads as a statement about the pin. Newer-release detection needs an answer to "which tags count as newer" that modelith has no basis to give — filed separately.
Corrected during implementation
The interview settled on a warning when the fetched version's
scope:changed. That hazard does not exist. It rested on slice 1's brief, but ADR-0012 superseded that mid-slice-1:scope:left the schema and the importer binds the slug from the import path's basename.updaterewrites the same file at the same path, so the basename cannot move.The real hazard in that shape is upstream renaming or removing an item you reference, which
updatecannot detect — it does not know which models import the copy. So it says to runmodelith lintafter writing, and why.Verification
task checkgreen. The unit tests build their fixtures with the realImportand 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 that should catch them.The adversarial run against a real origin found two bugs the unit tests did not (
8652eb0):deps checkon a machine with noghpanicked with a nil dereference instead of reporting thatghis missing.surveyappended a Report for the file it abandoned, carrying neither a verdict nor a failure, and the printer fell through and dereferenced its nilState. The unit test for the abort had asserted the buggy shape.--refwith several files) printedupdated 0 of 0 vendored copiesabove the reason.End-to-end against
stacklok/modelithitself, all confirmed by hand: import → clean check → no-op update leaves the file byte-identical → hand-edit →lintnamesdeps update <path>→ update restores byte-identically → lint clean; a tag-pinned copy reporting up to date;--ref mainre-pinning to genuinely different content; a stale verdict exiting 1; a glob mixing owned, vendored, missing, and broken-header files.Review
Round one has not run.
depsdoes network I/O and overwrites committed files, so I'd suggest/code-review high. Draft until it clears.