From e7323e5783cbcf7063659e7242564d4420778b2f Mon Sep 17 00:00:00 2001 From: ShocOne <62835948+ShocOne@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:17:32 +0100 Subject: [PATCH] chore: pin the corpus to a revision, and make a pin record its measurement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The github pin's URL named a branch tip, so it re-pinned itself every time the vendor published: the hash stopped matching, every test reading the document failed, and the only remedy was to restate the pin — which is the review the lock exists to force. It now names the commit serving what it pinned, at the same 808 paths and 1220 operations. The tags that repository publishes are not maintained; the newest dates from 2022 and carries 30% less surface than the branch does. A Describer is now an argument rather than a package variable nothing installed. RewriteLock silently wrote "unparsed" and zero counts for every pin it restated, so a re-pin recorded that nothing had measured the document. specmodel.Describe is the implementation to pass. The lock's own test refuses a moving ref and a version no describer produced, so neither can return. Co-Authored-By: Claude Opus 5 (1M context) --- internal/corpus/corpus.go | 35 ++++++++------- internal/corpus/corpus_test.go | 55 +++++++++++++++++++++++ internal/corpus/refresh.go | 10 ++--- internal/corpus/refresh_test.go | 37 ++++++++++----- internal/corpus/testdata/corpus.lock.json | 6 +-- internal/specmodel/describe.go | 18 ++++++++ internal/specmodel/describe_test.go | 34 ++++++++++++++ 7 files changed, 160 insertions(+), 35 deletions(-) create mode 100644 internal/specmodel/describe.go create mode 100644 internal/specmodel/describe_test.go diff --git a/internal/corpus/corpus.go b/internal/corpus/corpus.go index 6c97c94..a1d3828 100644 --- a/internal/corpus/corpus.go +++ b/internal/corpus/corpus.go @@ -331,9 +331,11 @@ func verifyAgainstPin(id string, doc []byte, pin Pin, source string) error { return nil } - // Parsed only to describe the mismatch. What a reviewer needs is what - // changed, not that a hash differed. - version, paths, operations := describe(doc) + // The mismatch is named by digest, which is what proves it. A reader + // reaching this has no parser to hand — Ensure is called from a test + // helper, not from a caller that decoded the document first — so the + // description degrades rather than the failure being withheld. + version, paths, operations := unparsed(doc) return fmt.Errorf(`the pinned %s document is not what %s served. pinned sha256:%s version %s %d path(s) / %d operation(s) @@ -348,22 +350,25 @@ internal/corpus/testdata/corpus.lock.json`, id) } -// Describer parses a document enough to say what it is, for failure messages. +// Describer parses a document enough to say what it is. // -// A hook rather than a direct call on the OpenAPI parsing package, because -// that package's own tests are in-package and are among this package's most -// important consumers: importing it here would make them an import cycle. The -// CLI installs the real parser; anything that has not is describing a failure -// it is already reporting by digest. -var Describer = func([]byte) (version string, paths, operations int) { +// A parameter rather than a direct call on the OpenAPI parsing package, +// because that package's own tests are in-package and are among this +// package's most important consumers: importing it here would make them an +// import cycle. specmodel.Describe is the implementation to pass. +// +// Every caller that writes a measurement down takes one, so a pin cannot +// record what nothing measured. +type Describer func(doc []byte) (version string, paths, operations int) + +// unparsed is the describer for a caller with no parser to hand: it reports +// only what it can, which is nothing. Reserved for a failure message that +// already names the change by digest, and never for a value written to the +// lock. +func unparsed([]byte) (version string, paths, operations int) { return "unparsed", 0, 0 } -// describe reports what a document says about itself, best effort. -func describe(doc []byte) (version string, paths, operations int) { - return Describer(doc) -} - func shortSHA(s string) string { if len(s) <= 12 { return s diff --git a/internal/corpus/corpus_test.go b/internal/corpus/corpus_test.go index ea027f2..0064694 100644 --- a/internal/corpus/corpus_test.go +++ b/internal/corpus/corpus_test.go @@ -101,7 +101,35 @@ func TestUnit_Corpus_LockPinsTheDocumentsTheTestsRead(t *testing.T) { if pin.PinnedAt.IsZero() { t.Errorf("%s: no pinnedAt, so the cache directory name is not stable", id) } + if pin.Version == "" || pin.Version == "unparsed" { + t.Errorf("%s: version %q, so the pin records no measurement of what it pinned", id, pin.Version) + } + if ref := movingRef(pin.UpstreamURL); ref != "" { + t.Errorf("%s: the upstream URL names %q, a ref that moves; pin a commit or a tag instead", id, ref) + } + } +} + +// movingRefs are the branch names a source-hosting URL carries when it names +// the tip of a branch rather than a fixed revision. +var movingRefs = []string{"main", "master", "HEAD", "latest", "trunk", "develop"} + +// movingRef reports the moving ref an upstream URL names, empty when it names +// none. +// +// A pin whose URL follows a branch re-pins itself every time the vendor +// publishes: the hash stops matching, every test reading it fails, and the +// only remedy is to restate the pin, which is the review the lock exists to +// force. Pinning a revision makes the pin mean something. +func movingRef(upstream string) string { + for _, segment := range strings.Split(upstream, "/") { + for _, ref := range movingRefs { + if segment == ref { + return ref + } + } } + return "" } func slicesEqual(a, b []string) bool { @@ -434,3 +462,30 @@ func TestUnit_Corpus_NoUserCacheDirFallsBackToTempNotToRelative(t *testing.T) { t.Errorf("the fallback cache %q is not under tfpfgen/corpus", dir) } } + +// TestUnit_Corpus_AMovingRefIsRejected proves the guard above catches the +// shape it exists for, so it cannot pass by accident on a lock that happens +// to hold no branch URL. +func TestUnit_Corpus_AMovingRefIsRejected(t *testing.T) { + t.Parallel() + + for _, url := range []string{ + "https://raw.githubusercontent.com/o/r/main/spec.json", + "https://raw.githubusercontent.com/o/r/master/spec.json", + "https://example.invalid/latest/openapi.yaml", + } { + if movingRef(url) == "" { + t.Errorf("%s names a moving ref and was accepted", url) + } + } + + for _, url := range []string{ + "https://raw.githubusercontent.com/o/r/67c14c7efb01cdeeac0ecd8cee9fae8d7a80e2aa/spec.json", + "https://raw.githubusercontent.com/o/r/v2.1.0/spec.json", + "https://pubhub.devnetcloud.com/media/000-v7-apis/docs/reference/unified-oas/api.yaml", + } { + if ref := movingRef(url); ref != "" { + t.Errorf("%s names a fixed revision and was rejected as %q", url, ref) + } + } +} diff --git a/internal/corpus/refresh.go b/internal/corpus/refresh.go index 1b2b8df..74e1f3f 100644 --- a/internal/corpus/refresh.go +++ b/internal/corpus/refresh.go @@ -57,18 +57,18 @@ func (u Upstream) Describe() string { // It never writes and never consults the cache: the question is what the // vendor is serving right now, which a cache would answer wrongly by // construction. -func CheckUpstream(id string) (Upstream, error) { +func CheckUpstream(id string, describe Describer) (Upstream, error) { pin, err := PinFor(id) if err != nil { return Upstream{}, err } - return checkPin(pin) + return checkPin(pin, describe) } // checkPin is CheckUpstream with the pin supplied, so RewriteLock can measure // against the on-disk lock rather than the embedded one. -func checkPin(pin Pin) (Upstream, error) { +func checkPin(pin Pin, describe Describer) (Upstream, error) { doc, source, err := fetch(pin) if err != nil { return Upstream{}, err @@ -100,7 +100,7 @@ func checkPin(pin Pin) (Upstream, error) { // Pins are measured against the on-disk lock rather than the embedded copy: // the embedded bytes are whatever was compiled in, and a rewrite must not // silently discard an edit made since. -func RewriteLock(ids []string) error { +func RewriteLock(ids []string, describe Describer) error { path := LockPath() raw, err := os.ReadFile(path) //nolint:gosec // a fixed name under the repository @@ -134,7 +134,7 @@ func RewriteLock(ids []string) error { return fmt.Errorf("%s has no pin for %q", path, id) } - up, err := checkPin(pin) + up, err := checkPin(pin, describe) if err != nil { return fmt.Errorf("%s: %w", id, err) } diff --git a/internal/corpus/refresh_test.go b/internal/corpus/refresh_test.go index ed5e0a9..397b2a8 100644 --- a/internal/corpus/refresh_test.go +++ b/internal/corpus/refresh_test.go @@ -31,7 +31,7 @@ func TestUnit_Refresh_CheckUpstreamMeasuresWithoutJudging(t *testing.T) { url, _ := serve(t, aDocument) - matching, err := checkPin(pinFor(t, url, aDocument)) + matching, err := checkPin(pinFor(t, url, aDocument), unparsed) if err != nil { t.Fatalf("checkPin: %v", err) } @@ -40,7 +40,7 @@ func TestUnit_Refresh_CheckUpstreamMeasuresWithoutJudging(t *testing.T) { } moved := pinFor(t, url, "what the lock used to pin") - differing, err := checkPin(moved) + differing, err := checkPin(moved, unparsed) if err != nil { t.Fatalf("checkPin: %v", err) } @@ -60,12 +60,19 @@ func TestUnit_Refresh_CheckUpstreamMeasuresWithoutJudging(t *testing.T) { func TestUnit_Refresh_CheckUpstreamRefusesAnUnpinnedID(t *testing.T) { t.Parallel() - if _, err := CheckUpstream("no-such-document"); err == nil || + if _, err := CheckUpstream("no-such-document", unparsed); err == nil || !strings.Contains(err.Error(), "pins no document") { t.Fatalf("CheckUpstream of an unpinned id: %v", err) } } +// stubDescriber stands where a real parser goes, answering values no fixture +// could produce by accident so a pin carrying them proves the describer was +// consulted. +func stubDescriber([]byte) (version string, paths, operations int) { + return "9.9.9", 7, 11 +} + // writeLock writes a lock file for RewriteLock tests and points EnvLockPath at // it. func writeLock(t *testing.T, content string) string { @@ -118,7 +125,7 @@ func TestUnit_Refresh_RewriteLockRestatesOnlyThePinsThatMoved(t *testing.T) { } }`) - if err := RewriteLock([]string{"moved", "unmoved"}); err != nil { + if err := RewriteLock([]string{"moved", "unmoved"}, stubDescriber); err != nil { t.Fatalf("RewriteLock: %v", err) } @@ -147,9 +154,15 @@ func TestUnit_Refresh_RewriteLockRestatesOnlyThePinsThatMoved(t *testing.T) { if moved["unknownEntryKey"] != "kept too" { t.Error("rewriting dropped an entry key it does not own") } - // The default Describer cannot parse, and says so rather than guessing. - if moved["version"] != "unparsed" { - t.Errorf("the moved pin's version = %v", moved["version"]) + // The pin records what the describer measured. A rewrite that ignored it + // would write the old version back, or none at all, and either reads as + // a document that did not move. + if moved["version"] != "9.9.9" { + t.Errorf("the moved pin's version = %v, want the describer's measurement", moved["version"]) + } + if moved["pathCount"] != float64(7) || moved["operationCount"] != float64(11) { + t.Errorf("the moved pin's counts = %v/%v, want the describer's measurement", + moved["pathCount"], moved["operationCount"]) } unmoved := entries["unmoved"].(map[string]any) @@ -163,22 +176,22 @@ func TestUnit_Refresh_RewriteLockRestatesOnlyThePinsThatMoved(t *testing.T) { // and an unreachable upstream. None may write. func TestUnit_Refresh_RewriteLockFailsClosed(t *testing.T) { t.Setenv(EnvLockPath, filepath.Join(t.TempDir(), "absent", LockFile)) - if err := RewriteLock(nil); err == nil || !strings.Contains(err.Error(), "reading") { + if err := RewriteLock(nil, unparsed); err == nil || !strings.Contains(err.Error(), "reading") { t.Errorf("a missing lock: %v", err) } writeLock(t, "{not json") - if err := RewriteLock(nil); err == nil || !strings.Contains(err.Error(), "parsing") { + if err := RewriteLock(nil, unparsed); err == nil || !strings.Contains(err.Error(), "parsing") { t.Errorf("an unparsable lock: %v", err) } writeLock(t, `{"formatVersion": "1"}`) - if err := RewriteLock(nil); err == nil || !strings.Contains(err.Error(), "no openapi object") { + if err := RewriteLock(nil, unparsed); err == nil || !strings.Contains(err.Error(), "no openapi object") { t.Errorf("a lock with no openapi object: %v", err) } writeLock(t, `{"openapi": {}}`) - if err := RewriteLock([]string{"ghost"}); err == nil || !strings.Contains(err.Error(), `no pin for "ghost"`) { + if err := RewriteLock([]string{"ghost"}, unparsed); err == nil || !strings.Contains(err.Error(), `no pin for "ghost"`) { t.Errorf("an id the lock does not pin: %v", err) } @@ -194,7 +207,7 @@ func TestUnit_Refresh_RewriteLockFailsClosed(t *testing.T) { if err != nil { t.Fatal(err) } - if err := RewriteLock([]string{"dead"}); err == nil || !strings.Contains(err.Error(), "dead:") { + if err := RewriteLock([]string{"dead"}, unparsed); err == nil || !strings.Contains(err.Error(), "dead:") { t.Errorf("an unreachable upstream: %v", err) } after, err := os.ReadFile(path) //nolint:gosec // a path this test built diff --git a/internal/corpus/testdata/corpus.lock.json b/internal/corpus/testdata/corpus.lock.json index 4b5e2f0..5018aa3 100644 --- a/internal/corpus/testdata/corpus.lock.json +++ b/internal/corpus/testdata/corpus.lock.json @@ -7,9 +7,9 @@ "mirrorUrl": "", "operationCount": 1220, "pathCount": 808, - "pinnedAt": "2026-08-07T14:30:00Z", - "sha256": "80850db290cde4eb487e0efb587cf27f305e77b6bef96933ed8a09b5169d5b1d", - "upstreamUrl": "https://raw.githubusercontent.com/github/rest-api-description/main/descriptions/api.github.com/api.github.com.json", + "pinnedAt": "2026-08-14T16:14:25.482999Z", + "sha256": "81c0ff2bbff9099b569058ef387e0ef83c33ef00ca0346e8a092777a71c192e3", + "upstreamUrl": "https://raw.githubusercontent.com/github/rest-api-description/67c14c7efb01cdeeac0ecd8cee9fae8d7a80e2aa/descriptions/api.github.com/api.github.com.json", "version": "1.1.4" }, "thousandeyes": { diff --git a/internal/specmodel/describe.go b/internal/specmodel/describe.go new file mode 100644 index 0000000..d08db1f --- /dev/null +++ b/internal/specmodel/describe.go @@ -0,0 +1,18 @@ +package specmodel + +// Describe reports what a document says about itself: the version its info +// object declares, and how much surface it carries. +// +// It satisfies corpus.Describer, which is where a pin's recorded counts come +// from. Those counts are what catch a truncated download that happens to +// parse, so a document this cannot read reports zero rather than a guess. +func Describe(doc []byte) (version string, paths, operations int) { + loaded, err := Load(doc) + if err != nil { + return "", 0, 0 + } + for _, path := range loaded.Paths { + operations += len(path.Operations) + } + return loaded.Info.Version, len(loaded.Paths), operations +} diff --git a/internal/specmodel/describe_test.go b/internal/specmodel/describe_test.go new file mode 100644 index 0000000..51aa3b6 --- /dev/null +++ b/internal/specmodel/describe_test.go @@ -0,0 +1,34 @@ +package specmodel + +import "testing" + +// TestUnit_Specmodel_DescribeMeasuresWhatAPinRecords proves Describe reports +// the three values a corpus pin is written from, and reports nothing rather +// than a guess for bytes it cannot read — a truncated download must not pass +// as a document with a plausible shape. +func TestUnit_Specmodel_DescribeMeasuresWhatAPinRecords(t *testing.T) { + const doc = `openapi: 3.0.3 +info: {title: T, version: "4.5.6"} +paths: + /widgets: + get: + responses: + "200": {description: ok} + post: + responses: + "201": {description: made} + /widgets/{id}: + delete: + responses: + "204": {description: gone} +` + version, paths, operations := Describe([]byte(doc)) + if version != "4.5.6" || paths != 2 || operations != 3 { + t.Errorf("Describe = %q, %d paths, %d operations; want 4.5.6, 2, 3", version, paths, operations) + } + + version, paths, operations = Describe([]byte("\t{[ not a document")) + if version != "" || paths != 0 || operations != 0 { + t.Errorf("unreadable bytes described as %q, %d, %d", version, paths, operations) + } +}