fix(ci): stop double-parsing every file in the API gate's content walk

branch-review on #3184 measured a real regression: switching CRD/
APIService discovery to content-based scanning made every file in
packages/+internal a candidate, and each was split into documents and
fully unmarshaled twice — once per parser — inflating a full snapshot
load from ~0.5s to ~3.4s.

Split each file's documents once and probe each document's kind once
before deciding which shape to try, so the common case (a document
that is neither a CRD nor an APIService — the overwhelming majority
of what the wider walk now sees) pays for at most one parse attempt
instead of two. Add a raw substring fast-reject, at both the
per-document and whole-file level, ahead of any YAML parsing: neither
kind this package cares about can appear without its own name
appearing as a literal substring first.

Also fixes two smaller gaps flagged in the same review: an untrusted
PR head checkout could point discovery at an arbitrarily large file
with no size guard (capped at 8 MiB), and a group's merged APIService
resource only recorded the first contributing file's path, dropping
the others from the report.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>

internal/apigate/parse.go

@@ -17,6 +17,7 @@ limitations under the License.
 package apigate
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
 	"regexp"
@@ -87,55 +88,75 @@ func ParseCRDs(origin string, data []byte) ([]Resource, error) {
 		if len(strings.TrimSpace(string(docBytes))) == 0 {
 			continue
 		}
-		var doc struct {
-			Kind string `json:"kind"`
-			Spec struct {
-				Group string `json:"group"`
-				Names struct {
-					Kind   string `json:"kind"`
-					Plural string `json:"plural"`
-				} `json:"names"`
-				Versions []struct {
-					Name   string `json:"name"`
-					Served bool   `json:"served"`
-					Schema struct {
-						OpenAPIV3Schema Schema `json:"openAPIV3Schema"`
-					} `json:"schema"`
-				} `json:"versions"`
-			} `json:"spec"`
+		res, ok, err := crdFromDoc(origin, i, docBytes)
+		if err != nil {
+			return nil, err
 		}
-		if err := unmarshalYAMLDoc(docBytes, &doc); err != nil {
-			continue
+		if ok {
+			out = append(out, res)
 		}
-		if doc.Kind != "CustomResourceDefinition" {
-			continue
-		}
-		if doc.Spec.Group == "" || doc.Spec.Names.Plural == "" {
-			return nil, fmt.Errorf("%s (document %d): CRD missing spec.group or spec.names.plural", origin, i)
-		}
-		res := Resource{
-			Group:    doc.Spec.Group,
-			Kind:     doc.Spec.Names.Kind,
-			Plural:   doc.Spec.Names.Plural,
-			Source:   SourceCRD,
-			Origin:   origin,
-			Versions: map[string]Schema{},
-		}
-		for _, v := range doc.Spec.Versions {
-			// Only served versions are live API surface. Skipping unserved
-			// versions keeps the classifier from diffing schemas no client can
-			// call, and makes the standard CRD deprecation step (flip
-			// served:true -> false) surface as a removed served version.
-			if v.Name == "" || !v.Served {
-				continue
-			}
-			res.Versions[v.Name] = v.Schema.OpenAPIV3Schema
-		}
-		out = append(out, res)
 	}
 	return out, nil
 }
 
+// crdFromDoc attempts to parse a single YAML document as a
+// CustomResourceDefinition. ok is false whenever the document is not a CRD,
+// including when it fails to parse at all; err is non-nil only when the
+// document unambiguously claims to be a CRD (right kind) but is structurally
+// incomplete. docKind is checked first so the fuller CRD-shaped unmarshal —
+// and its cost — is paid only for documents that are actually CRDs, since
+// LoadSnapshot's content-based walk runs this against every YAML file in the
+// tree, the overwhelming majority of which are not CRDs at all.
+func crdFromDoc(origin string, i int, docBytes []byte) (Resource, bool, error) {
+	if docKind(docBytes) != "CustomResourceDefinition" {
+		return Resource{}, false, nil
+	}
+	var doc struct {
+		Spec struct {
+			Group string `json:"group"`
+			Names struct {
+				Kind   string `json:"kind"`
+				Plural string `json:"plural"`
+			} `json:"names"`
+			Versions []struct {
+				Name   string `json:"name"`
+				Served bool   `json:"served"`
+				Schema struct {
+					OpenAPIV3Schema Schema `json:"openAPIV3Schema"`
+				} `json:"schema"`
+			} `json:"versions"`
+		} `json:"spec"`
+	}
+	if err := unmarshalYAMLDoc(docBytes, &doc); err != nil {
+		// docKind above already parsed this document successfully, so this
+		// would be surprising; tolerate it like any other unparseable
+		// document rather than erroring.
+		return Resource{}, false, nil
+	}
+	if doc.Spec.Group == "" || doc.Spec.Names.Plural == "" {
+		return Resource{}, false, fmt.Errorf("%s (document %d): CRD missing spec.group or spec.names.plural", origin, i)
+	}
+	res := Resource{
+		Group:    doc.Spec.Group,
+		Kind:     doc.Spec.Names.Kind,
+		Plural:   doc.Spec.Names.Plural,
+		Source:   SourceCRD,
+		Origin:   origin,
+		Versions: map[string]Schema{},
+	}
+	for _, v := range doc.Spec.Versions {
+		// Only served versions are live API surface. Skipping unserved
+		// versions keeps the classifier from diffing schemas no client can
+		// call, and makes the standard CRD deprecation step (flip
+		// served:true -> false) surface as a removed served version.
+		if v.Name == "" || !v.Served {
+			continue
+		}
+		res.Versions[v.Name] = v.Schema.OpenAPIV3Schema
+	}
+	return res, true, nil
+}
+
 // storageVarGroups maps the storage-map variable prefixes used in
 // pkg/apiserver/apiserver.go to their API group. The apps map is intentionally
 // omitted: apps.cozystack.io resources are discovered from the cozyrds, which
@@ -221,31 +242,42 @@ func ParseAPIServices(origin string, data []byte) []Resource {
 		if len(strings.TrimSpace(string(docBytes))) == 0 {
 			continue
 		}
-		var doc struct {
-			Kind string `json:"kind"`
-			Spec struct {
-				Group   string `json:"group"`
-				Version string `json:"version"`
-			} `json:"spec"`
+		if res, ok := apiServiceFromDoc(origin, docBytes); ok {
+			out = append(out, res)
 		}
-		if err := unmarshalYAMLDoc(docBytes, &doc); err != nil {
-			continue
-		}
-		if doc.Kind != "APIService" || doc.Spec.Group == "" || doc.Spec.Version == "" {
-			continue
-		}
-		out = append(out, Resource{
-			Group:    doc.Spec.Group,
-			Kind:     "(aggregated apiserver)",
-			Plural:   apiServicePlural,
-			Source:   SourceAPIService,
-			Origin:   origin,
-			Versions: map[string]Schema{doc.Spec.Version: {}},
-		})
 	}
 	return out
 }
 
+// apiServiceFromDoc attempts to parse a single YAML document as an
+// APIService. See crdFromDoc for why docKind is checked before the fuller
+// unmarshal.
+func apiServiceFromDoc(origin string, docBytes []byte) (Resource, bool) {
+	if docKind(docBytes) != "APIService" {
+		return Resource{}, false
+	}
+	var doc struct {
+		Spec struct {
+			Group   string `json:"group"`
+			Version string `json:"version"`
+		} `json:"spec"`
+	}
+	if err := unmarshalYAMLDoc(docBytes, &doc); err != nil {
+		return Resource{}, false
+	}
+	if doc.Spec.Group == "" || doc.Spec.Version == "" {
+		return Resource{}, false
+	}
+	return Resource{
+		Group:    doc.Spec.Group,
+		Kind:     "(aggregated apiserver)",
+		Plural:   apiServicePlural,
+		Source:   SourceAPIService,
+		Origin:   origin,
+		Versions: map[string]Schema{doc.Spec.Version: {}},
+	}, true
+}
+
 // helmDirectiveLine matches a line that is entirely Go-template/Helm markup
 // (`{{- if ... }}`, `{{- end }}`, `{{- /* comment */ -}}`), including its
 // trailing newline.
@@ -269,12 +301,52 @@ func unmarshalYAMLDoc(doc []byte, out any) error {
 	return yaml.Unmarshal(helmDirectiveLine.ReplaceAll(doc, nil), out)
 }
 
+// docKind cheaply extracts a YAML document's top-level kind field, without
+// paying for a full CRD- or APIService-shaped unmarshal on documents that
+// are neither. LoadSnapshot's discovery walk scans every .yaml/.yml file
+// under packages/ and internal/ rather than a curated directory allowlist
+// (see discoverYAMLFiles), so the overwhelming majority of documents it
+// sees are not API manifests at all; probing kind first, once, keeps that
+// walk cheap.
+// mightBeAPIManifest is a cheap fast-reject shared by docKind (per document)
+// and LoadSnapshot (per whole file, before even splitting it into
+// documents). Both kinds this package looks for (crdFromDoc,
+// apiServiceFromDoc) appear as their own kind name verbatim in a valid
+// manifest; content containing neither substring cannot be either kind. This
+// is the overwhelming majority of what a content-based walk across the whole
+// tree sees — Deployments, Services, plain values trees, and everything else
+// that is neither a CRD nor an APIService — so rejecting on a raw substring
+// scan avoids the cost of the `---` split, the YAML parse attempt, and (for
+// a document that isn't valid strict YAML at all — an ordinary Helm
+// template) the further regex-stripped retry in unmarshalYAMLDoc.
+func mightBeAPIManifest(data []byte) bool {
+	return bytes.Contains(data, []byte("CustomResourceDefinition")) || bytes.Contains(data, []byte("APIService"))
+}
+
+func docKind(docBytes []byte) string {
+	if !mightBeAPIManifest(docBytes) {
+		return ""
+	}
+	var probe struct {
+		Kind string `json:"kind"`
+	}
+	if err := unmarshalYAMLDoc(docBytes, &probe); err != nil {
+		return ""
+	}
+	return probe.Kind
+}
+
+// yamlDocSeparator matches a `---` document-separator line. Compiled once at
+// package init rather than per splitYAMLDocs call, since LoadSnapshot's
+// content-based walk now runs this against every YAML file in the tree.
+var yamlDocSeparator = regexp.MustCompile(`(?m)^---\s*$`)
+
 // splitYAMLDocs splits a YAML stream into individual document byte slices on
 // `---` separators. It intentionally mirrors the simple splitting Helm and
 // kubectl use; CRD manifests in this repo do not embed `---` inside block
 // scalars, so line-based splitting is safe.
 func splitYAMLDocs(data []byte) [][]byte {
-	parts := regexp.MustCompile(`(?m)^---\s*$`).Split(string(data), -1)
+	parts := yamlDocSeparator.Split(string(data), -1)
 	out := make([][]byte, 0, len(parts))
 	for _, p := range parts {
 		out = append(out, []byte(p))

internal/apigate/parse_test.go

@@ -402,6 +402,99 @@ spec: {group: metrics.k8s.io, version: v1beta1}
 	}
 }
 
+// TestLoadSnapshotSkipsOversizedFiles guards the size cap added alongside
+// content-based discovery: since every .yaml/.yml file in the tree is now a
+// candidate (no directory allowlist), an untrusted PR head checkout could
+// otherwise force an unbounded read/parse of a pathologically large file. A
+// first-party CRD past maxYAMLFileSize must be excluded from discovery.
+func TestLoadSnapshotSkipsOversizedFiles(t *testing.T) {
+	root := t.TempDir()
+	write := func(rel, content string) {
+		p := filepath.Join(root, filepath.FromSlash(rel))
+		if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
+			t.Fatal(err)
+		}
+	}
+	padding := strings.Repeat("# padding\n", (maxYAMLFileSize/len("# padding\n"))+1)
+	write("packages/system/oversized/crds/widgets.yaml", padding+sampleCRD)
+	// A small, legitimate resource so the empty-snapshot guard doesn't fire
+	// for an unrelated reason.
+	write("packages/system/postgres-rd/cozyrds/postgres.yaml", sampleCozyRD)
+
+	snap, err := LoadSnapshot(root)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, ok := snap[resourceKey{Group: "example.cozystack.io", Plural: "widgets"}]; ok {
+		t.Fatalf("CRD in a file past maxYAMLFileSize must be excluded: %v", snap)
+	}
+}
+
+// TestLoadSnapshotMergesAPIServiceOriginAcrossFiles covers the traceability
+// fix: when a group's versions are registered via several sibling APIService
+// objects in different files (the real-world shape — one object per
+// version), the merged resource's Origin must mention every contributing
+// file, not just the first one encountered.
+func TestLoadSnapshotMergesAPIServiceOriginAcrossFiles(t *testing.T) {
+	root := t.TempDir()
+	write := func(rel, content string) {
+		p := filepath.Join(root, filepath.FromSlash(rel))
+		if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
+			t.Fatal(err)
+		}
+	}
+	write("packages/system/cozyplane/templates/apiserver-v1alpha1.yaml", `apiVersion: apiregistration.k8s.io/v1
+kind: APIService
+metadata: {name: v1alpha1.sdn.cozystack.io}
+spec: {group: sdn.cozystack.io, version: v1alpha1}
+`)
+	write("packages/system/cozyplane/templates/apiserver-v1beta1.yaml", `apiVersion: apiregistration.k8s.io/v1
+kind: APIService
+metadata: {name: v1beta1.sdn.cozystack.io}
+spec: {group: sdn.cozystack.io, version: v1beta1}
+`)
+
+	snap, err := LoadSnapshot(root)
+	if err != nil {
+		t.Fatal(err)
+	}
+	res, ok := snap[resourceKey{Group: "sdn.cozystack.io", Plural: apiServicePlural}]
+	if !ok {
+		t.Fatalf("APIService group was not discovered: %v", snap)
+	}
+	for _, want := range []string{
+		"packages/system/cozyplane/templates/apiserver-v1alpha1.yaml",
+		"packages/system/cozyplane/templates/apiserver-v1beta1.yaml",
+	} {
+		if !strings.Contains(res.Origin, want) {
+			t.Fatalf("expected Origin to mention %q, got %q", want, res.Origin)
+		}
+	}
+}
+
+// BenchmarkLoadSnapshotRealTree tracks LoadSnapshot's wall-clock cost against
+// this repository's own checkout — the actual worst case the CI gate runs
+// against on every PR. Content-based discovery (see discoverYAMLFiles) scans
+// every .yaml/.yml file under packages/ and internal/, not a curated
+// directory allowlist, so this is meant to catch a future change that
+// reintroduces redundant per-file work (e.g. splitting or parsing the same
+// file's documents more than once). Run with:
+//
+//	go test ./internal/apigate/ -run '^$' -bench LoadSnapshotRealTree
+func BenchmarkLoadSnapshotRealTree(b *testing.B) {
+	for b.Loop() {
+		if _, err := LoadSnapshot("../.."); err != nil {
+			b.Fatal(err)
+		}
+	}
+}
+
 // TestLoadSnapshotEmptyErrors covers the silent-bypass guard: an empty result
 // (e.g. wrong directory) must be an error, not a clean pass.
 func TestLoadSnapshotEmptyErrors(t *testing.T) {

internal/apigate/snapshot.go

@@ -87,10 +87,15 @@ func LoadSnapshot(dir string) (Snapshot, error) {
 	}
 
 	// Typed groups (CRD manifests) and APIService-backed groups: both are
-	// discovered by content from the same file walk. APIService resources
-	// are accumulated separately and merged by group below, since a group's
-	// versions are commonly registered as several sibling APIService objects
-	// (one per version) rather than a single document listing all of them.
+	// discovered by content from the same file walk, splitting each file's
+	// documents exactly once and probing each document's kind exactly once
+	// (docKind) before deciding which of the two shapes to try — the walk
+	// now scans every YAML file under packages/ and internal/ rather than a
+	// curated directory allowlist, so most documents are neither and this
+	// keeps the common case cheap. APIService resources are accumulated
+	// separately and merged by group below, since a group's versions are
+	// commonly registered as several sibling APIService objects (one per
+	// version) rather than a single document listing all of them.
 	yamlFiles, err := discoverYAMLFiles(dir)
 	if err != nil {
 		return nil, err
@@ -101,33 +106,48 @@ func LoadSnapshot(dir string) (Snapshot, error) {
 		if err != nil {
 			return nil, err
 		}
-		origin := rel(dir, path)
-
-		resources, err := ParseCRDs(origin, data)
-		if err != nil {
-			// A file that does not parse as YAML at all (not even one
-			// tolerated document) is almost always an upstream/templated
-			// artifact, not a first-party CRD; skip it rather than failing
-			// the whole gate.
+		if !mightBeAPIManifest(data) {
+			// Whole file contains neither marker substring anywhere, so no
+			// document within it can be a CRD or an APIService either;
+			// skip the `---` split (a regexp pass over the full file) too.
 			continue
 		}
-		for _, res := range resources {
-			if isFirstPartyGroup(res.Group) {
-				add(res)
-			}
-		}
+		origin := rel(dir, path)
 
-		for _, res := range ParseAPIServices(origin, data) {
-			if !isFirstPartyGroup(res.Group) {
+		for i, docBytes := range splitYAMLDocs(data) {
+			if len(strings.TrimSpace(string(docBytes))) == 0 {
 				continue
 			}
-			key := resourceKey{Group: res.Group, Plural: res.Plural}
-			existing, ok := apiServices[key]
-			if !ok {
-				apiServices[key] = res
-				continue
+			switch docKind(docBytes) {
+			case "CustomResourceDefinition":
+				res, ok, err := crdFromDoc(origin, i, docBytes)
+				if err != nil {
+					// A document unambiguously claiming to be a first-party
+					// CRD but structurally incomplete; skip just this
+					// document rather than losing any other resource that
+					// shares its file.
+					continue
+				}
+				if ok && isFirstPartyGroup(res.Group) {
+					add(res)
+				}
+			case "APIService":
+				res, ok := apiServiceFromDoc(origin, docBytes)
+				if !ok || !isFirstPartyGroup(res.Group) {
+					continue
+				}
+				key := resourceKey{Group: res.Group, Plural: res.Plural}
+				existing, ok := apiServices[key]
+				if !ok {
+					apiServices[key] = res
+					continue
+				}
+				maps.Copy(existing.Versions, res.Versions)
+				if existing.Origin != res.Origin {
+					existing.Origin += ", " + res.Origin
+				}
+				apiServices[key] = existing
 			}
-			maps.Copy(existing.Versions, res.Versions)
 		}
 	}
 	for key, res := range apiServices {
@@ -165,9 +185,19 @@ func LoadSnapshot(dir string) (Snapshot, error) {
 	return snap, nil
 }
 
+// maxYAMLFileSize bounds how large a single discovered file is read and
+// parsed. Discovery is now content-based across every YAML file in the tree
+// (no directory allowlist — see crdRoots), so this walk runs against
+// untrusted PR head checkouts with no other size guard; real chart
+// CRD/APIService manifests in this repo are well under a megabyte, so this
+// is generous headroom, not a tight fit, and only bounds a pathological or
+// adversarial file.
+const maxYAMLFileSize = 8 << 20 // 8 MiB
+
 // discoverYAMLFiles walks the crdRoots under dir and returns the path of
-// every .yaml/.yml file found. Filtering to the CRD / APIService documents we
-// actually care about happens at parse time in LoadSnapshot.
+// every .yaml/.yml file found, up to maxYAMLFileSize. Filtering to the CRD /
+// APIService documents we actually care about happens at parse time in
+// LoadSnapshot.
 func discoverYAMLFiles(dir string) ([]string, error) {
 	var out []string
 	for _, root := range crdRoots {
@@ -185,6 +215,13 @@ func discoverYAMLFiles(dir string) ([]string, error) {
 			if ext := filepath.Ext(path); ext != ".yaml" && ext != ".yml" {
 				return nil
 			}
+			info, err := d.Info()
+			if err != nil {
+				return err
+			}
+			if info.Size() > maxYAMLFileSize {
+				return nil
+			}
 			out = append(out, path)
 			return nil
 		})