Fix missing event filter functions in APIRecorder.WithAnnotations

[frigaut-orange]

Description of your changes

Fixes #

I have:

• Read and followed Crossplane's contribution process.
• Run ./nix.sh flake check to ensure this PR is ready for review.
• Added or updated unit tests.
• Linked a PR or a docs tracking issue to document this change.
• Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

[bobh66]

@frigaut-orange can you please check the lint error and complete the checklist? Also a description of the change and what it's fixing would be helpful. Thanks

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds public Crossplane resource, package, version, cache, and signature APIs. It also updates reconciliation behavior, event recording, Nix tooling, and GitHub workflows with comprehensive tests.

Changes

Event recorder behavior

Layer / File(s)	Summary
Preserve event filters pkg/event/event.go, pkg/event/event_test.go	WithAnnotations preserves configured filters. Tests cover filtering, namespaces, empty events, and annotations.

Reconciliation and cache behavior

Layer / File(s)	Summary
CRD cache transformation pkg/reconciler/customresourcesgate/cache.go, pkg/reconciler/customresourcesgate/cache_test.go	CRD schemas, managed fields, and last-applied annotations are removed before caching.
Managed reconciliation controls pkg/reconciler/managed/reconciler.go, pkg/reconciler/managed/reconciler_modern_test.go	Reconcile-request tokens are tracked and poll intervals support resource annotations with minimum clamping.
ProviderConfig namespace scoping pkg/reconciler/providerconfig/reconciler.go, pkg/reconciler/providerconfig/reconciler_test.go	Usage listing is namespace-scoped for namespaced ProviderConfigs.

Public resource and package APIs

Layer / File(s)	Summary
Version and CRD APIs pkg/version/*, pkg/xcrd/*	Adds semantic-version operations, composite resource annotations, schema builders, CRD generation, and establishment checks.
Package parsing and storage pkg/xpkg/name.go, pkg/xpkg/scheme.go, pkg/xpkg/reader.go, pkg/xpkg/parser/*, pkg/xpkg/cache.go, pkg/xpkg/fetch.go, pkg/xpkg/config.go	Adds package metadata utilities, scheme conversion, reader adapters, YAML parsing, compressed caching, registry fetching, and ImageConfig matching.
Package client and image builder pkg/xpkg/client.go, pkg/xpkg/build.go, pkg/xpkg/layers.go, pkg/xpkg/find.go	Adds package retrieval, caching, verification integration, version listing, OCI extraction, package image construction, layer annotation, and package discovery.
Package validation and signatures pkg/xpkg/lint.go, pkg/xpkg/validate.go, pkg/xpkg/signature/*	Adds package validators, Kubernetes object linters, Cosign verification, and attestation conversion.

Build and workflow infrastructure

Layer / File(s)	Summary
Nix build configuration flake.nix, .github/renovate-entrypoint.sh	Nix uses unstable Go 1.25, serialized builds, updated gomod2nix, and post-build cleanup.
GitHub workflows .github/workflows/*	Workflows use Ubuntu 24.04 and updated pinned actions. Renovate JSON5 validation was added, CodeQL and Codecov were upgraded, and the filesystem Trivy scan was removed.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

• crossplane/crossplane-runtime#947: Contains the same public version, xcrd, and xpkg package additions.
• crossplane/crossplane-runtime#982: Contains the same managed reconciler poll interval and reconcile-request changes.
• crossplane/crossplane-runtime#1058: Contains the same CRD cache transformation and tests.

Suggested reviewers: bobh66

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description contains only the default template and checklist, so it does not explain the implementation or testing details.	Add a concise change summary, explain the related issue and test coverage, and complete or strike through every checklist item.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is 65 characters, stays under 72 characters, and clearly describes the APIRecorder.WithAnnotations event-filter change.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Breaking Changes	✅ Passed	No public Go APIs (exported functions, types, methods, or fields) were removed, renamed, or had signature changes. The change fixes a bug by preserving event filters in APIRecorder.WithAnnotations,...

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/event/event_test.go`:
- Around line 156-162: Refactor the test case struct in the cases map to follow
the args/want table pattern. Create a nested args field containing
filterFunctions, inputResource, inputEvent, and annotations, while keeping want
for the expected Event and reason for test description. This consolidates the
input parameters into a single args struct field as required by the table-driven
test structure.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d8c0c1eb-15a6-4673-84e1-765a85433da7

📥 Commits

Reviewing files that changed from the base of the PR and between a572204 and 07609da.

📒 Files selected for processing (2)

• pkg/event/event.go
• pkg/event/event_test.go

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use the required args and want table shape.

Thanks for adding the coverage. Group filterFunctions, inputResource, inputEvent, and annotations in an args field. Keep the expected event in want.

Proposed table shape

 cases := map[string]struct {
     reason          string
-    filterFunctions []FilterFn
-    inputResource   runtime.Object
-    inputEvent      Event
+    args struct {
+        filterFunctions []FilterFn
+        inputResource   runtime.Object
+        inputEvent      Event
+        annotations     []string
+    }
     want            Event
-    annotations     []string
 }{

As per path instructions, “Enforce table-driven test structure: ... args/want pattern”.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/event/event_test.go` around lines 156 - 162, Refactor the test case
struct in the cases map to follow the args/want table pattern. Create a nested
args field containing filterFunctions, inputResource, inputEvent, and
annotations, while keeping want for the expected Event and reason for test
description. This consolidates the input parameters into a single args struct
field as required by the table-driven test structure.

Source: Path instructions

[coderabbitai]

Actionable comments posted: 8

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (21)

pkg/reconciler/providerconfig/reconciler_test.go-337-419 (1)

337-419: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the Get request key in both new cases.

Both MockGet callbacks discard client.ObjectKey. The tests pass want.request to Reconcile, but they do not verify it. A regression that calls Get with an empty or incorrect key would still populate pc and pass these tests.

Capture the key in each callback and compare it with the case NamespacedName.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/reconciler/providerconfig/reconciler_test.go` around lines 337 - 419, The
MockGet callbacks in ListUsagesScopedToNamespaceWhenNamespaced and
ListUsagesNotScopedToNamespaceWhenClusterScoped must validate the requested key.
Capture the client.ObjectKey argument and compare its NamespacedName with the
corresponding tc.want.request before populating the ProviderConfig, preserving
the existing list-option assertions.

pkg/version/version.go-51-65 (1)

51-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add context to parser errors and update the test assertions.

GetSemVer returns the raw semantic version parse error, and InConstraints either returns that same raw error or the raw constraint parse error. Wrap these with crossplane-runtime/pkg/errors so the error states whether the configured version or supplied constraint is invalid and suggests correcting the value format. Update the test error expectations in pkg/version/version_test.go to match the new wrapped error contract, including tests for GetSemVer if they exist.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/version/version.go` around lines 51 - 65, Update Versioner.GetSemVer and
InConstraints to wrap semantic-version and constraint parser errors with
crossplane-runtime/pkg/errors, identifying whether the configured version or
supplied constraint is invalid and advising correction of its format. Preserve
successful parsing and boolean behavior, and update existing version tests,
including GetSemVer error assertions where present, to expect the wrapped
messages.

Source: Path instructions

pkg/version/version_test.go-120-124 (1)

120-124: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use the required error comparison option.

Update these assertions to compare errors with cmpopts.EquateErrors(), and remove EquateErrors() from the boolean comparison. Add the import github.com/google/go-cmp/cmp/cmpopts so tests pass when the error expectations match.

Thanks for catching this table-driven test.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/version/version_test.go` around lines 120 - 124, Update the error
assertion in the table-driven test around InRange to use cmpopts.EquateErrors(),
importing github.com/google/go-cmp/cmp/cmpopts. Remove the EquateErrors option
from the boolean is comparison, leaving that assertion as a plain boolean
comparison.

Source: Path instructions

pkg/xcrd/crd.go-174-181 (1)

174-181: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Avoid double-wrapping schema parsing failures.

parseSchema wraps validation JSON errors with cannot parse validation schema, then genCrdVersion wraps the same message again. This produces repeated user-facing text like “cannot parse validation schema: cannot parse validation schema: ...” and also uses Wrapf without format arguments. Drop the outer wrap, or add version-specific context such as cannot use the schema of version %q.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xcrd/crd.go` around lines 174 - 181, Update the parseSchema error
handling in genCrdVersion to avoid repeating the “cannot parse validation
schema” context and eliminate the format-less Wrapf usage; return the existing
error directly or replace it with meaningful version-specific context using the
version symbol available in the surrounding function.

Source: Path instructions

pkg/xcrd/crd.go-44-51 (1)

44-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Make two error messages point the user at the field to fix.

Thank you for centralizing the error strings. Two of them do not tell a user what to change:

• errMissingClaimNames surfaces as "invalid resource claim names: missing names". The user cannot tell which field is missing.
• errCustomResourceValidationNil surfaces as "custom resource validation cannot be nil". "nil" is developer wording, and the message omits the version that lacks a schema.

Both errors reach XRD status conditions, so the text is the only guidance the user gets. Could you name the missing field in each message?

As per path instructions: "CRITICAL: Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible."

✏️ Proposed wording

 const (
 	errFmtGenCrd                   = "cannot generate CRD for %q %q"
 	errParseValidation             = "cannot parse validation schema"
 	errInvalidClaimNames           = "invalid resource claim names"
-	errMissingClaimNames           = "missing names"
+	errMissingClaimNames           = "spec.claimNames must be set to offer a claim for this composite resource"
 	errFmtConflictingClaimName     = "%q conflicts with composite resource name"
-	errCustomResourceValidationNil = "custom resource validation cannot be nil"
+	errCustomResourceValidationNil = "spec.versions[*].schema must be set for every version of this composite resource"
 )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xcrd/crd.go` around lines 44 - 51, Update the error message constants
errMissingClaimNames and errCustomResourceValidationNil to identify the specific
claim-name or version schema field requiring correction, using user-facing
wording instead of “nil”; preserve their existing usage and ensure each message
gives enough context to locate the missing configuration.

Source: Path instructions

pkg/xpkg/signature/validate.go-203-206 (1)

203-206: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

%w can render as %!w(<nil>) on this path.

The condition on Line 204 succeeds in two ways. If err != nil, the message is fine. If err == nil and publicKey == nil, %w receives a nil error and the user sees secret "cosign.pub" contains an invalid public key: %!w(<nil>). Splitting the two cases also lets you name the secret in both messages, which helps the user find the resource to fix.

🐛 Suggested change

 		publicKey, err := cryptoutils.UnmarshalPEMToPublicKey(v)
-		if err != nil || publicKey == nil {
-			return nil, errors.Errorf("secret %q contains an invalid public key: %w", kr.SecretRef.Key, err)
+		if err != nil {
+			return nil, errors.Wrapf(err, "key %q in secret %q does not contain a valid PEM encoded public key", kr.SecretRef.Key, kr.SecretRef.Name)
+		}
+		if publicKey == nil {
+			return nil, errors.Errorf("key %q in secret %q does not contain a public key", kr.SecretRef.Key, kr.SecretRef.Name)
 		}

As per path instructions "Ensure all error messages are meaningful to end users".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/signature/validate.go` around lines 203 - 206, Update the public-key
validation in the surrounding validation function to handle unmarshal errors and
nil publicKey results separately. For a non-nil err, preserve the wrapped
underlying error and include kr.SecretRef.Key; for a nil publicKey, return a
distinct meaningful invalid-key error that also names the secret, avoiding %w
with a nil error.

Source: Path instructions

pkg/xpkg/signature/attestation.go-106-113 (1)

106-113: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Consider a comma-ok assertion instead of a forced one.

Line 107 asserts val.(string) without the comma-ok form. If a registry serves an envelope where payload is a number, object, or null, this panics instead of returning an error. The TODO(negz) question is a good one, and the answer is that the DSSE envelope shape is not schema-checked before this point, so the type is not guaranteed by anything in this function. A panic inside the package reconciler is harder to diagnose than a wrapped error.

🛡️ Suggested guard

-	var decodedPayload []byte
-	if val, ok := payloadData["payload"]; ok {
-		decodedPayload, err = base64.StdEncoding.DecodeString(val.(string)) //nolint:forcetypeassert // TODO(negz): Will this always be a string?
-		if err != nil {
-			return nil, "", fmt.Errorf("decoding payload: %w", err)
-		}
-	} else {
-		return nil, "", fmt.Errorf("could not find payload in payload data")
-	}
+	val, ok := payloadData["payload"]
+	if !ok {
+		return nil, "", errors.New("attestation envelope does not contain a payload field")
+	}
+
+	encoded, ok := val.(string)
+	if !ok {
+		return nil, "", errors.Errorf("attestation envelope payload is %T, expected a base64 encoded string", val)
+	}
+
+	decodedPayload, err := base64.StdEncoding.DecodeString(encoded)
+	if err != nil {
+		return nil, "", errors.Wrap(err, "cannot base64 decode attestation envelope payload")
+	}

Note that cosign.VerifyImageAttestations runs before this call, so an anonymous attacker cannot reach the assertion; this is a robustness fix rather than an exploitable path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/signature/attestation.go` around lines 106 - 113, Replace the forced
type assertion val.(string) with a comma-ok assertion to safely check if the
payload field contains a string value. If val is not a string (it could be a
number, object, or null), return an error with appropriate context instead of
panicking. Preserve the existing base64 decoding path and error handling for the
case where the field is present but not a string type, similar to the existing
"could not find payload in payload data" error path.

Source: Path instructions

pkg/xpkg/lint_test.go-583-586 (1)

583-586: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

These case names say v1, but the fixtures are v1alpha1.

v1alpha1Op, v1alpha1CronOp, and v1alpha1WatchOp all come from ops.crossplane.io/v1alpha1. The sibling tests such as TestIsMRD use "v1alpha1" for the same kind of case. Renaming these three keys keeps the subtest output accurate.

💚 Suggested rename

-		"v1": {
+		"v1alpha1": {
 			reason: "Should not return error if object is an operation.",
 			obj:    v1alpha1Op,

Apply the same rename in TestIsCronOperation and TestIsWatchOperation.

As per path instructions: "Check for proper test case naming and reason fields."

Also applies to: 610-613, 637-640

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/lint_test.go` around lines 583 - 586, Rename the operation test-case
keys from “v1” to “v1alpha1” in TestIsOperation, TestIsCronOperation, and
TestIsWatchOperation, matching the versions of v1alpha1Op, v1alpha1CronOp, and
v1alpha1WatchOp. Keep the existing reason fields unchanged.

Source: Path instructions

pkg/xpkg/parser/yaml/parser.go-20-42 (1)

20-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Error returns in these two files drop the cause and the user-facing context. Both sites return errors that reach a package author through Crossplane logs, yet neither uses crossplane-runtime/pkg/errors to wrap. The shared fix is to wrap each returned error with a message that names the operation the user attempted.

• pkg/xpkg/parser/yaml/parser.go#L20-L42: import github.com/crossplane/crossplane-runtime/v2/pkg/errors in place of the standard library errors, then replace both errors.New(errBuildMetaScheme) and errors.New(errBuildObjectScheme) with errors.Wrap(err, ...) so the scheme-registration cause survives.
• pkg/xpkg/name.go#L116-L128: wrap the afero.ReadFile error and the parseNameFromPackage error with errors.Wrapf(err, ..., path) so the user learns which package metadata file failed.

Thank you for porting these helpers over; the wrapping is the last piece that makes the failures diagnosable.

As per path instructions: "Use crossplane-runtime/pkg/errors for wrapping" and "Ensure all error messages are meaningful to end users, not just developers".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/parser/yaml/parser.go` around lines 20 - 42, Wrap both
scheme-building failures in New using crossplane-runtime/pkg/errors instead of
the standard library errors, preserving the original err and providing
meaningful operation-specific messages for BuildMetaScheme and
BuildObjectScheme. Also update pkg/xpkg/name.go lines 116-128: wrap both
afero.ReadFile and parseNameFromPackage errors with errors.Wrapf using the
package metadata path, so each failure identifies the affected file.

Source: Path instructions

pkg/xpkg/scheme_test.go-83-93 (1)

83-93: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The reason for SuccessfulConversion states the opposite of the expectation.

The case expects ok: true, but the reason says "should not return true". A reader who hits this failure gets a contradictory message.

💚 Suggested fix

 		"SuccessfulConversion": {
-			reason: "We should not return true if one of the supplied candidates converted successfully.",
+			reason: "We should return true if one of the supplied candidates converted successfully.",

As per path instructions: "Check for proper test case naming and reason fields."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/scheme_test.go` around lines 83 - 93, Update the reason field in the
SuccessfulConversion test case to describe that a supplied candidate converted
successfully and should produce ok: true, keeping the test setup and expected
values unchanged.

Source: Path instructions

pkg/xpkg/reader.go-33-43 (1)

33-43: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Ownership of rc is asymmetric on the error path.

On success, the returned gzipReadCloser owns rc and closes it in Close. If gzip.NewReader fails, this function returns an error and never closes rc, so the caller must know to close it. That asymmetry leaks the underlying reader whenever a package layer is not valid gzip. Which ownership contract do you prefer?

♻️ Option: close `rc` on the error path

 func GzipReadCloser(rc io.ReadCloser) (io.ReadCloser, error) {
 	r, err := gzip.NewReader(rc)
 	if err != nil {
-		return nil, err
+		_ = rc.Close()
+		return nil, errors.Wrap(err, "cannot read gzip compressed package content")
 	}

If you prefer the current contract, a doc comment that states the caller keeps ownership of rc on error would make it safe to use. Either way, wrapping the raw gzip error with crossplane-runtime/pkg/errors gives the user context about what failed.

As per path instructions: "Use crossplane-runtime/pkg/errors for wrapping ... Ensure all error messages are meaningful to end users".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/reader.go` around lines 33 - 43, Update GzipReadCloser so a failed
gzip.NewReader call closes the supplied rc before returning, preserving the
returned gzipReadCloser ownership on success. Wrap the gzip initialization error
with crossplane-runtime/pkg/errors and a meaningful context message before
returning it.

Source: Path instructions

pkg/xpkg/build_test.go-160-236 (1)

160-236: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add reason values to the TestBuildExamples cases.

The struct declares reason, but no case sets it. Every failure message then prints an empty first line. Could you add a short reason for each of SuccessNoExamples, SuccessExamplesAtRoot, and SuccessExamplesAtCustomDir? It matches the style used in TestBuild.

As per path instructions: "Check for proper test case naming and reason fields."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/build_test.go` around lines 160 - 236, Add concise reason strings to
the reason field of each TestBuildExamples case: SuccessNoExamples,
SuccessExamplesAtRoot, and SuccessExamplesAtCustomDir. Keep the existing test
inputs and expectations unchanged, matching the reason wording style already
used by TestBuild.

Source: Path instructions

pkg/xpkg/build.go-198-201 (1)

198-201: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Error wraps in the build path name the wrong operation. Thank you for wrapping every error with crossplane-runtime/pkg/errors. The shared root cause here is constant reuse: errConfigFile and errBuildObjectScheme describe operations that did not fail, and one path drops the original cause. A user reading the output cannot tell which step failed.

• pkg/xpkg/build.go#L198-L201: wrap the encode failure with a new encode-specific constant instead of errConfigFile.
• pkg/xpkg/build.go#L245-L262: wrap the BuildObjectScheme error with errors.Wrap so the cause survives, and use encode-specific constants for the two do.Encode failures.

As per path instructions: "Use crossplane-runtime/pkg/errors for wrapping" and "CRITICAL: Ensure all error messages are meaningful to end users, not just developers".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/build.go` around lines 198 - 201, The encode failure at
pkg/xpkg/build.go lines 198-201 should use a new encode-specific error constant
instead of errConfigFile. In the BuildObjectScheme and do.Encode paths at
pkg/xpkg/build.go lines 245-262, wrap the BuildObjectScheme error with
crossplane-runtime/pkg/errors so its cause is preserved, and use distinct
encode-specific constants for both do.Encode failures; ensure each message
identifies the failed operation clearly.

Source: Path instructions

pkg/reconciler/managed/reconciler_modern_test.go-2857-2863 (1)

2857-2863: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use the required args and want case structure.

Thank you for adding coverage. Each table stores invocation inputs directly on the test case. Group inputs in args and retain expected results in want. This keeps setup separate from expected behavior.

• pkg/reconciler/managed/reconciler_modern_test.go#L2857-L2863: Move pollInterval, minPollInterval, and annotation into args.
• pkg/reconciler/managed/reconciler_modern_test.go#L2923-L2930: Add args for inputs and want for the expected duration and tolerance.
• pkg/reconciler/managed/reconciler_modern_test.go#L3022-L3027: Move annotation and handled into args.

As per path instructions, enforce table-driven test structure with an args/want pattern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/reconciler/managed/reconciler_modern_test.go` around lines 2857 - 2863,
Restructure the table-driven cases in
pkg/reconciler/managed/reconciler_modern_test.go at lines 2857-2863, 2923-2930,
and 3022-3027 to use the required args/want pattern: move pollInterval,
minPollInterval, and annotation into args at the first site; add args for
invocation inputs and want for expected duration and tolerance at the second;
and move annotation and handled into args at the third. Update the test
invocation and assertions to read from these nested fields while preserving
behavior.

Source: Path instructions

pkg/xpkg/config.go-1-1 (1)

1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Two new files in this package have no Apache license header. Every other file in pkg/xpkg opens with the Apache 2.0 header, so these two look like an oversight rather than a decision.

• pkg/xpkg/config.go#L1-L1: add the Apache 2.0 header above package xpkg.
• pkg/xpkg/config_test.go#L1-L1: add the same header above package xpkg.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/config.go` at line 1, Add the repository’s standard Apache 2.0
license header above the package declaration in pkg/xpkg/config.go at lines 1-1
and pkg/xpkg/config_test.go at lines 1-1, matching the headers used by other
files in the package.

pkg/xpkg/config.go-94-97 (1)

94-97: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

These two error messages do not tell the user which resource to fix.

"cosign verification config is missing" and "rewrite prefix is missing" reach package status conditions, where the reader is a platform operator, not the developer of this file. Neither message names the ImageConfig that is at fault, the image that selected it, or the field to set. Including that context makes the condition actionable.

💡 Proposed messages with context

 	if config.Spec.Verification.Cosign == nil {
 		// Only cosign verification is supported for now.
-		return config.Name, nil, errors.New("cosign verification config is missing")
+		return config.Name, nil, errors.Errorf("ImageConfig %q selected for image %q enables verification but does not set spec.verification.cosign; only cosign verification is supported", config.Name, image)
 	}

 	rewritePrefix := config.Spec.RewriteImage.Prefix
 	if rewritePrefix == "" {
-		return config.Name, "", errors.New("rewrite prefix is missing")
+		return config.Name, "", errors.Errorf("ImageConfig %q selected for image %q must set spec.rewriteImage.prefix", config.Name, image)
 	}

As per path instructions: "CRITICAL: Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible."

Also applies to: 118-121

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/config.go` around lines 94 - 97, Update the validation errors in the
config-loading flow around config.Spec.Verification.Cosign and the
rewrite-prefix check to identify the affected ImageConfig and selected image,
name the missing field, and provide an actionable next step. Preserve the
existing validation behavior and return values while making both messages
suitable for platform operators.

Source: Path instructions

pkg/xpkg/client.go-277-300 (1)

277-300: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

A cache hit never reports the verification ImageConfig.

applied gains the ImageConfigReasonVerify entry at line 331, which only runs on the cache-miss path. Package.AppliedImageConfigs therefore lists the verification config after a fresh pull and omits it after a cache hit for the same package. SupportedImageConfigs says callers clear all reasons and then set the returned ones, so a reconciler would add and remove that entry as the cache warms or is evicted. The doc comment at lines 306-315 explains why verification itself is skipped on a hit, which makes sense. Should the reported ImageConfig still reflect the matched config, so the resulting status stays stable? The same applies to the rewrite and pull-secret entries, which are recorded before the cache lookup and stay consistent today.

Also applies to: 316-332

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/client.go` around lines 277 - 300, Ensure the cache-hit return path
in the package-loading method preserves the complete AppliedImageConfigs set,
including the ImageConfigReasonVerify entry added on the pull path. Populate or
derive the verification config before returning the cached Package, while
retaining the documented behavior of skipping verification itself and keeping
rewrite and pull-secret entries consistent.

pkg/xpkg/client.go-344-362 (1)

344-362: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Avoid closing the cache pipe before the package stream finishes

Thanks for adding the goroutine copy. If c.cache.Store(...) returns early, this closes pipeR, which makes future writes from teeRC return io.ErrClosedPipe and can turn a valid registry pull into “cannot parse package ...”. Consider storing the copy in the goroutine and only closing the pipe writer after the package parser has fully consumed the stream or successfully cached.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/client.go` around lines 344 - 362, The cache goroutine must not
close pipeR while c.parser.Parse is still consuming teeRC, because an early
c.cache.Store return causes subsequent writes to fail. Update the flow around
TeeReadCloser, c.cache.Store, and c.parser.Parse so the cache reader remains
open until parsing has fully consumed the stream, and close the pipe writer only
after parsing completes or the cache operation succeeds, preserving the existing
parse error handling.

pkg/xpkg/cache.go-60-67 (1)

60-67: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Has does not take the read lock.

Get, Store, and Delete all guard access with c.mu, but Has calls Stat without holding c.mu.RLock(). A concurrent Store can be mid-write, so Has can report a partially written entry as present. Was the omission deliberate to keep Has cheap? If not, adding the read lock keeps the "thread-safe" contract in the type comment consistent.

🔒 Proposed fix to guard `Has` with the read lock

 // Has indicates whether an item with the given id is in the cache.
 func (c *FsPackageCache) Has(id string) bool {
+	c.mu.RLock()
+	defer c.mu.RUnlock()
+
 	if fi, err := c.fs.Stat(BuildPath(c.dir, id, cacheContentExt)); err == nil && !fi.IsDir() {
 		return true
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/cache.go` around lines 60 - 67, Update FsPackageCache.Has to acquire
c.mu.RLock before calling Stat and release it afterward, matching the locking
used by Get, Store, and Delete while preserving the existing presence check
behavior.

pkg/xpkg/cache_test.go-184-230 (1)

184-230: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

TestDelete never deletes an existing file, and the failure message names the wrong function.

Line 186 creates /cache/exists.xpkg, but FsPackageCache.Delete resolves BuildPath(c.dir, id, cacheContentExt), which is /cache/exists.gz. The "Success" case therefore takes the same not-exist path as "SuccessNotExist", so the delete-an-existing-entry behavior stays untested. Line 226 also prints Store(...) inside TestDelete.

💚 Proposed fix for the fixture and the message

 func TestDelete(t *testing.T) {
 	fs := afero.NewMemMapFs()
-	_, _ = fs.Create("/cache/exists.xpkg")
+	_, _ = fs.Create("/cache/exists.gz")

 			err := tc.args.cache.Delete(tc.args.id)
 			if diff := cmp.Diff(tc.want, err, test.EquateErrors()); diff != "" {
-				t.Errorf("\n%s\nStore(...): -want err, +got err:\n%s", tc.reason, diff)
+				t.Errorf("\n%s\nDelete(...): -want err, +got err:\n%s", tc.reason, diff)
 			}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/cache_test.go` around lines 184 - 230, Update TestDelete’s
existing-file fixture to use the cache extension resolved by
FsPackageCache.Delete via BuildPath (the .gz path), so the “Success” case
actually removes an existing entry while “SuccessNotExist” remains distinct.
Also correct the comparison error message in TestDelete to name Delete instead
of Store.

pkg/xpkg/config_test.go-31-34 (1)

31-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Test cases have no reason field.

The case struct holds only args and want. Sibling tests in this package (cache_test.go, client_test.go, find_test.go) carry a reason and print it in the failure message, which makes a red build readable without opening the table. Could you add the field here for consistency?

💚 Proposed structure change

 	cases := map[string]struct {
+		reason string
 		args args
 		want want
 	}{
 		"ErrListConfig": {
+			reason: "We should return an error if we cannot list ImageConfigs.",
 			args: args{

 			got, err := s.bestMatch(context.Background(), tc.args.image, tc.args.isValid)
 			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
-				t.Errorf("bestMatch() error -want +got: %s", diff)
+				t.Errorf("\n%s\nbestMatch(...): -want error, +got error:\n%s", tc.reason, diff)
 			}

As per path instructions: "Check for proper test case naming and reason fields."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/config_test.go` around lines 31 - 34, Add a reason string field to
the table-driven case struct in the relevant test, populate each case with a
concise explanation, and include that reason in the failure output so failures
are self-describing, matching the pattern used by sibling tests such as
cache_test.go, client_test.go, and find_test.go.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/reconciler/managed/reconciler.go`:
- Around line 981-1006: Update the reconcile-request handling around
reconcileRequestToken, updateStatus, and reasonReconcileRequestHandled to
persist status.lastHandledReconcileAt successfully before emitting the handled
event. Route every live-resource early return after token detection, including
the external-create grace-period path, through this persistence, and avoid
emitting the event when the status update conflicts or otherwise fails. Add
coverage for the grace-period and status-conflict paths, ensuring events occur
only after the token is actually persisted and include the token/change details.

In `@pkg/xpkg/build.go`:
- Around line 171-184: Update the linter-selection switch in the package build
flow to handle unsupported or empty GVK kinds before calling linter.Lint. Return
a descriptive build error that includes the observed kind, lists the supported
Configuration, Function, and Provider kinds, and guides the user to correct
crossplane.yaml; preserve the existing linter behavior for supported kinds.
- Around line 146-156: Update the example-source initialization flow around
b.exampleSource.Init(ctx) so exReader.Close is deferred only when initialization
succeeds; keep the existing non-not-exist error return, and set examplesExist to
false in the not-exist branch while treating successful initialization as the
existing examples path.

In `@pkg/xpkg/client.go`:
- Around line 264-273: Guard the descriptor returned by c.fetcher.Head in the
digest-resolution branch before dereferencing desc.Digest. If desc is nil,
return a descriptive error indicating that parsedResolvedRef could not be
resolved to a descriptor; preserve the existing wrapped Head error handling and
digest-reference path.

In `@pkg/xpkg/layers.go`:
- Around line 125-152: Update the layer-processing function around the addendums
loop to track whether at least one annotation was applied, rather than checking
len(addendums). Return the original image when no matching labels were found,
while preserving addendum construction for annotated layers and unannotated
layers when rebuilding is necessary.
- Around line 72-78: Update the Layer function to safely handle caller-provided
configs with nil Labels and nil cfg before assigning the annotation label.
Initialize cfg.Labels when needed, and add an appropriate nil-config guard or
return behavior while preserving the existing non-empty annotation flow.

In `@pkg/xpkg/lint.go`:
- Around line 37-53: Update the error-message constants in the lint validation
block, including errNotCRD, errNotMRD, errNotXRD, the webhook and operation type
errors, errNotMeta and its metadata-specific variants, errBadConstraints, and
errFmtCrossplaneIncompatible, so each explains the package requirement,
identifies the expected object or metadata type, and suggests the corrective
action. State that exactly one matching metadata object is required where
applicable, and that version constraints must use valid semantic-version
constraint syntax, while preserving any formatting placeholders used by callers.

In `@pkg/xpkg/signature/validate.go`:
- Around line 177-217: Update buildCosignCheckOpts so it rejects configurations
where Keyless.Identities is empty or omitted and Key is nil, returning an
explicit error that requires at least one keyless identity or a key-backed
verifier configuration. Preserve the existing identity and key setup when either
valid authority is configured, and ensure the validation occurs before returning
unconstrained opts to cosign.

---

Minor comments:
In `@pkg/reconciler/managed/reconciler_modern_test.go`:
- Around line 2857-2863: Restructure the table-driven cases in
pkg/reconciler/managed/reconciler_modern_test.go at lines 2857-2863, 2923-2930,
and 3022-3027 to use the required args/want pattern: move pollInterval,
minPollInterval, and annotation into args at the first site; add args for
invocation inputs and want for expected duration and tolerance at the second;
and move annotation and handled into args at the third. Update the test
invocation and assertions to read from these nested fields while preserving
behavior.

In `@pkg/reconciler/providerconfig/reconciler_test.go`:
- Around line 337-419: The MockGet callbacks in
ListUsagesScopedToNamespaceWhenNamespaced and
ListUsagesNotScopedToNamespaceWhenClusterScoped must validate the requested key.
Capture the client.ObjectKey argument and compare its NamespacedName with the
corresponding tc.want.request before populating the ProviderConfig, preserving
the existing list-option assertions.

In `@pkg/version/version_test.go`:
- Around line 120-124: Update the error assertion in the table-driven test
around InRange to use cmpopts.EquateErrors(), importing
github.com/google/go-cmp/cmp/cmpopts. Remove the EquateErrors option from the
boolean is comparison, leaving that assertion as a plain boolean comparison.

In `@pkg/version/version.go`:
- Around line 51-65: Update Versioner.GetSemVer and InConstraints to wrap
semantic-version and constraint parser errors with
crossplane-runtime/pkg/errors, identifying whether the configured version or
supplied constraint is invalid and advising correction of its format. Preserve
successful parsing and boolean behavior, and update existing version tests,
including GetSemVer error assertions where present, to expect the wrapped
messages.

In `@pkg/xcrd/crd.go`:
- Around line 174-181: Update the parseSchema error handling in genCrdVersion to
avoid repeating the “cannot parse validation schema” context and eliminate the
format-less Wrapf usage; return the existing error directly or replace it with
meaningful version-specific context using the version symbol available in the
surrounding function.
- Around line 44-51: Update the error message constants errMissingClaimNames and
errCustomResourceValidationNil to identify the specific claim-name or version
schema field requiring correction, using user-facing wording instead of “nil”;
preserve their existing usage and ensure each message gives enough context to
locate the missing configuration.

In `@pkg/xpkg/build_test.go`:
- Around line 160-236: Add concise reason strings to the reason field of each
TestBuildExamples case: SuccessNoExamples, SuccessExamplesAtRoot, and
SuccessExamplesAtCustomDir. Keep the existing test inputs and expectations
unchanged, matching the reason wording style already used by TestBuild.

In `@pkg/xpkg/build.go`:
- Around line 198-201: The encode failure at pkg/xpkg/build.go lines 198-201
should use a new encode-specific error constant instead of errConfigFile. In the
BuildObjectScheme and do.Encode paths at pkg/xpkg/build.go lines 245-262, wrap
the BuildObjectScheme error with crossplane-runtime/pkg/errors so its cause is
preserved, and use distinct encode-specific constants for both do.Encode
failures; ensure each message identifies the failed operation clearly.

In `@pkg/xpkg/cache_test.go`:
- Around line 184-230: Update TestDelete’s existing-file fixture to use the
cache extension resolved by FsPackageCache.Delete via BuildPath (the .gz path),
so the “Success” case actually removes an existing entry while “SuccessNotExist”
remains distinct. Also correct the comparison error message in TestDelete to
name Delete instead of Store.

In `@pkg/xpkg/cache.go`:
- Around line 60-67: Update FsPackageCache.Has to acquire c.mu.RLock before
calling Stat and release it afterward, matching the locking used by Get, Store,
and Delete while preserving the existing presence check behavior.

In `@pkg/xpkg/client.go`:
- Around line 277-300: Ensure the cache-hit return path in the package-loading
method preserves the complete AppliedImageConfigs set, including the
ImageConfigReasonVerify entry added on the pull path. Populate or derive the
verification config before returning the cached Package, while retaining the
documented behavior of skipping verification itself and keeping rewrite and
pull-secret entries consistent.
- Around line 344-362: The cache goroutine must not close pipeR while
c.parser.Parse is still consuming teeRC, because an early c.cache.Store return
causes subsequent writes to fail. Update the flow around TeeReadCloser,
c.cache.Store, and c.parser.Parse so the cache reader remains open until parsing
has fully consumed the stream, and close the pipe writer only after parsing
completes or the cache operation succeeds, preserving the existing parse error
handling.

In `@pkg/xpkg/config_test.go`:
- Around line 31-34: Add a reason string field to the table-driven case struct
in the relevant test, populate each case with a concise explanation, and include
that reason in the failure output so failures are self-describing, matching the
pattern used by sibling tests such as cache_test.go, client_test.go, and
find_test.go.

In `@pkg/xpkg/config.go`:
- Line 1: Add the repository’s standard Apache 2.0 license header above the
package declaration in pkg/xpkg/config.go at lines 1-1 and
pkg/xpkg/config_test.go at lines 1-1, matching the headers used by other files
in the package.
- Around line 94-97: Update the validation errors in the config-loading flow
around config.Spec.Verification.Cosign and the rewrite-prefix check to identify
the affected ImageConfig and selected image, name the missing field, and provide
an actionable next step. Preserve the existing validation behavior and return
values while making both messages suitable for platform operators.

In `@pkg/xpkg/lint_test.go`:
- Around line 583-586: Rename the operation test-case keys from “v1” to
“v1alpha1” in TestIsOperation, TestIsCronOperation, and TestIsWatchOperation,
matching the versions of v1alpha1Op, v1alpha1CronOp, and v1alpha1WatchOp. Keep
the existing reason fields unchanged.

In `@pkg/xpkg/parser/yaml/parser.go`:
- Around line 20-42: Wrap both scheme-building failures in New using
crossplane-runtime/pkg/errors instead of the standard library errors, preserving
the original err and providing meaningful operation-specific messages for
BuildMetaScheme and BuildObjectScheme. Also update pkg/xpkg/name.go lines
116-128: wrap both afero.ReadFile and parseNameFromPackage errors with
errors.Wrapf using the package metadata path, so each failure identifies the
affected file.

In `@pkg/xpkg/reader.go`:
- Around line 33-43: Update GzipReadCloser so a failed gzip.NewReader call
closes the supplied rc before returning, preserving the returned gzipReadCloser
ownership on success. Wrap the gzip initialization error with
crossplane-runtime/pkg/errors and a meaningful context message before returning
it.

In `@pkg/xpkg/scheme_test.go`:
- Around line 83-93: Update the reason field in the SuccessfulConversion test
case to describe that a supplied candidate converted successfully and should
produce ok: true, keeping the test setup and expected values unchanged.

In `@pkg/xpkg/signature/attestation.go`:
- Around line 106-113: Replace the forced type assertion val.(string) with a
comma-ok assertion to safely check if the payload field contains a string value.
If val is not a string (it could be a number, object, or null), return an error
with appropriate context instead of panicking. Preserve the existing base64
decoding path and error handling for the case where the field is present but not
a string type, similar to the existing "could not find payload in payload data"
error path.

In `@pkg/xpkg/signature/validate.go`:
- Around line 203-206: Update the public-key validation in the surrounding
validation function to handle unmarshal errors and nil publicKey results
separately. For a non-nil err, preserve the wrapped underlying error and include
kr.SecretRef.Key; for a nil publicKey, return a distinct meaningful invalid-key
error that also names the secret, avoiding %w with a nil error.

---

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 125-129: The codecov/codecov-action step is using the deprecated
`file` input parameter. Replace `file` with `files` in the
codecov/codecov-action step configuration to use the supported parameter name,
keeping the value result/coverage.txt unchanged.

In `@pkg/xcrd/crd.go`:
- Around line 265-287: Change setCrdMetadata to return no value, since it
mutates the provided crd in place and both callers discard its result. Update
both call sites to invoke it without expecting or returning a value, preserving
the existing metadata behavior.
- Around line 147-150: Clarify the intentional compatibility decision around
CompositeResourceStatusProps in the claim CRD schema and track the known
claimConditionTypes defect outside the TODO comment. Preserve the existing
status schema, including claimConditionTypes and connectionDetails, unless an
established issue or requirement directs a breaking schema change.
- Around line 17-23: Update the package documentation comment for xcrd to
briefly describe its current purpose: generating CRDs from
CompositeResourceDefinition objects. Remove the inaccurate CRDSpecTemplate
explanation and outdated controller-tools issue reference.

In `@pkg/xcrd/fuzz_test.go`:
- Around line 26-38: Optionally rename FuzzForCompositeResourceXcrd to match the
corresponding FuzzForCompositeResourceClaim target without repeating the package
name, ensuring no same-package fuzz-target collision. Also optionally add an
f.Add seed before f.Fuzz with representative data that helps exercise version
and schema branches.

In `@pkg/xcrd/schemas.go`:
- Around line 131-169: Refactor the `resourceRefs` schema construction in the
schema-building function so the namespaced variant derives from the base item
schema instead of duplicating the entire literal. Preserve all existing fields
and settings while removing only the `namespace` property for
`CompositeResourceScopeNamespaced`, ensuring future base-schema fields remain
synchronized.
- Around line 347-389: Update CompositeResourcePrinterColumns to compute the
spec path prefix from the CompositeResourceScope once, then construct the
COMPOSITION and COMPOSITIONREVISION JSONPath values from that prefix. Remove the
legacy loop that identifies columns by display name, while preserving the
existing paths for both legacy cluster and other scopes.

In `@pkg/xpkg/build_test.go`:
- Around line 357-369: Update yamlParser so its error behavior matches its
signature: return errors from BuildMetaScheme and BuildObjectScheme instead of
panicking, preserving successful parser construction with a nil error;
alternatively, remove the error result and adjust its call site to stop handling
a nonexistent error.
- Line 137: Check and handle the error returned by yamlParser immediately at the
call site before using pkgp, and likewise handle the error returned by readImg
immediately after its call before sorting or comparing contents. Update the
affected test paths around yamlParser and readImg while preserving their
existing assertions and failure behavior.
- Around line 52-59: Update the fixture-loading logic in init to stop ignoring
errors from each afero.ReadFile call. Fail immediately when a testdata file
cannot be read, including the specific file path in the panic or error message,
while preserving the existing assignments to testCRD, testMeta, and testEx1
through testEx4.
- Around line 97-133: The Build error-case table should also cover unsupported
meta kinds and missing example directories. Add one case using a meta object
with an unsupported kind that exercises the nil-linter path, and another whose
exampleSource Init returns a not-exist error with a nil reader; assert each
Build result returns the expected error without panicking, alongside the
existing ErrInitBackend and ErrParse cases.

In `@pkg/xpkg/cache_test.go`:
- Around line 35-87: Rename the cache tests to include the covered type,
updating TestHas to TestFsPackageCacheHas and applying the same FsPackageCache
prefix to the corresponding TestGet, TestStore, and TestDelete functions. Keep
their test behavior unchanged.

In `@pkg/xpkg/cache.go`:
- Around line 83-109: Update FsPackageCache.Store to remove the created cache
file on any write-finalization failure, including errors from io.Copy or
w.Close, before returning the error. Preserve the existing successful close
behavior and ensure cleanup targets the path built from c.dir, id, and
cacheContentExt.

In `@pkg/xpkg/client_test.go`:
- Around line 157-170: Update CreateTarWithPackageYAML to accept the testing
handle and check the errors from WriteHeader, Write, and Close, calling t.Fatal
with the relevant error when any operation fails. Update its callers to pass the
test handle so fixture-generation failures are reported at the source.
- Around line 698-729: Add a test case alongside ErrorHeadFails in the
CachedClient.Get table covering MockFetcher.MockHead returning a nil descriptor
and nil error, with the existing cache/config setup and an expected error. This
should pin the guard in CachedClient.Get for a nil Head result and ensure it
returns an error instead of panicking.

In `@pkg/xpkg/client.go`:
- Around line 467-489: Update FilterAndSortVersions to use a stable sorting
approach for semantically equal versions, preserving their original input order
(for example, by using stable sorting or semver.Collection with sort.Sort). Keep
filtering invalid tags and ascending semantic-version ordering unchanged, while
ensuring equivalent forms such as 1.0.0 and v1.0.0 produce deterministic
results.

In `@pkg/xpkg/config_test.go`:
- Line 19: Extend TestImageConfigStoreBestMatch coverage to call the exported
methods PullSecretFor, ImageVerificationConfigFor, RewritePath, and
RuntimeConfigFor rather than only the unexported bestMatch. Add table cases for
nil-config early returns, missing cosign and rewrite-prefix errors, and
RewritePath’s longest-prefix trimming so the final registry path is verified.
- Line 436: Remove cmp.AllowUnexported(v1beta1.ImageConfig{}) from the cmp.Diff
call in the config comparison test, leaving the comparison otherwise unchanged.

In `@pkg/xpkg/config.go`:
- Around line 173-184: The image-config selection loop should resolve
equal-length matching prefixes deterministically instead of relying on list
order. Update the comparison in the config-selection logic to retain the longer
prefix, and for equal-length prefixes select the lexically smallest config.Name;
preserve the existing valid-config filtering and longest-prefix behavior.

In `@pkg/xpkg/fetch.go`:
- Around line 131-201: Extract the repeated k8schain.New setup and shared remote
options from K8sFetcher.Fetch, K8sFetcher.Head, and K8sFetcher.Tags into a small
K8sFetcher helper. Have each method reuse that helper while preserving its
existing operation-specific call and error behavior, so credentials, transport,
context, and user-agent configuration remain centralized.
- Around line 37-44: Remove the package-level log suppression from init and
scope discarded log output to the AWS credential-helper invocation around
k8schain.New, restoring the previous logrus output afterward. Keep the change
limited to the fetch initialization flow and avoid affecting applications merely
importing the xpkg package.

In `@pkg/xpkg/find.go`:
- Around line 48-60: Update the matching loop in the function containing
XpkgMatchPattern to remove the redundant match condition from the multi-match
check, using only path != "" after non-matches are continued. Also skip
directory entries with file.IsDir() before accepting a matching file, preserving
the existing multi-match error behavior.

In `@pkg/xpkg/fuzz_test.go`:
- Around line 36-58: Remove the unused createdFiles slice and its deferred
cleanup from the fuzz test body around FuzzFindXpkgInDir, since no filenames are
recorded and the in-memory filesystem is iteration-scoped. Leave the
file-generation and parsing behavior unchanged; do not rename the fuzz function
as part of this cleanup.

In `@pkg/xpkg/name_test.go`:
- Line 26: Add table-driven tests for the exported ParseNameFromMeta function in
pkg/xpkg/name_test.go, using an in-memory afero filesystem and covering
missing-file and invalid-YAML errors plus successful parsing of a valid meta
file. Follow the existing test conventions and assert both returned values and
errors.
- Around line 87-95: In the three table-driven test loops around FriendlyID,
ToDNSLabel, and ParsePackageSourceFromReference, rename the computed
actual-value variable from want to got and rename the range key from name to
tcName. Update each cmp.Diff and subtest reference accordingly, preserving the
existing assertions and error messages.

In `@pkg/xpkg/parser/examples/parser_test.go`:
- Around line 62-67: The parser test cases should use the repository’s standard
args/want table structure and a single cmp.Diff assertion with
cmpopts.EquateErrors() for expected errors; remove expectAnyError and avoid
branching between error assertions, retaining only want.err for exact errors and
one errContains field for annotation checks. Update the Parse call to use
t.Context() so subtests cancel their parsing context.

In `@pkg/xpkg/scheme.go`:
- Around line 88-115: Clarify the documentation for TryConvert to state that
conversion mutates the supplied candidate Hubs, returns the first successful
Hub, and callers should provide fresh candidates for each call. Update
TryConvertToPkg’s documentation to explicitly indicate that it follows the same
candidate-lifecycle contract.

In `@pkg/xpkg/signature/validate.go`:
- Around line 146-163: Optionally extract the nested attestation-checking logic
from Validate into a small helper that returns whether any matching attestation
succeeded along with accumulated errors. Preserve the existing
attestationToPayloadJSON conversion, empty-payload handling, error messages, and
early-success behavior while reducing nesting in Validate.
- Around line 52-70: Use crossplane-runtime/pkg/errors consistently in
pkg/xpkg/signature/validate.go lines 52-70: replace the fmt.Errorf wrappers with
errors.Wrap and remove the unused fmt import. In
pkg/xpkg/signature/attestation.go lines 95-113, replace each fmt.Errorf wrapper
with errors.Wrap, preserving the underlying unmarshal error on the attestation
payload failure so its cause remains visible; update imports accordingly.
- Around line 1-2: Add the standard Apache license header used by the
neighboring attestation.go and doc.go files before the package clause in
validate.go, preserving the existing package declaration and implementation
unchanged.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ea065107-ac35-4c65-8890-dcc457270669

📥 Commits

Reviewing files that changed from the base of the PR and between 07609da and 9da82a1.

⛔ Files ignored due to path filters (16)

• .github/renovate-base.json5 is excluded by none and included by none
• .github/renovate-nix.json5 is excluded by none and included by none
• flake.lock is excluded by !**/*.lock and included by none
• go.mod is excluded by none and included by none
• go.sum is excluded by !**/*.sum and included by none
• gomod2nix.toml is excluded by none and included by none
• pkg/resource/fake/mocks.go is excluded by !**/fake/** and included by **/*.go
• pkg/version/fake/mocks.go is excluded by !**/fake/** and included by **/*.go
• pkg/xpkg/fake/config.go is excluded by !**/fake/** and included by **/*.go
• pkg/xpkg/fake/mocks.go is excluded by !**/fake/** and included by **/*.go
• pkg/xpkg/testdata/examples/ec2/instance.yaml is excluded by !**/testdata/** and included by **/*.yaml
• pkg/xpkg/testdata/examples/ec2/internetgateway.yaml is excluded by !**/testdata/** and included by **/*.yaml
• pkg/xpkg/testdata/examples/ecr/repository.yaml is excluded by !**/testdata/** and included by **/*.yaml
• pkg/xpkg/testdata/examples/provider.yaml is excluded by !**/testdata/** and included by **/*.yaml
• pkg/xpkg/testdata/provider_meta.yaml is excluded by !**/testdata/** and included by **/*.yaml
• pkg/xpkg/testdata/providerconfigs.helm.crossplane.io.yaml is excluded by !**/testdata/** and included by **/*.yaml

📒 Files selected for processing (55)

• .github/renovate-entrypoint.sh
• .github/workflows/backport.yml
• .github/workflows/ci.yml
• .github/workflows/commands.yml
• .github/workflows/renovate.yml
• .github/workflows/stale.yml
• .github/workflows/tag.yml
• flake.nix
• pkg/reconciler/customresourcesgate/cache.go
• pkg/reconciler/customresourcesgate/cache_test.go
• pkg/reconciler/managed/reconciler.go
• pkg/reconciler/managed/reconciler_modern_test.go
• pkg/reconciler/providerconfig/reconciler.go
• pkg/reconciler/providerconfig/reconciler_test.go
• pkg/version/version.go
• pkg/version/version_test.go
• pkg/xcrd/composite.go
• pkg/xcrd/crd.go
• pkg/xcrd/crd_test.go
• pkg/xcrd/fuzz_test.go
• pkg/xcrd/schemas.go
• pkg/xpkg/build.go
• pkg/xpkg/build_test.go
• pkg/xpkg/cache.go
• pkg/xpkg/cache_test.go
• pkg/xpkg/client.go
• pkg/xpkg/client_test.go
• pkg/xpkg/config.go
• pkg/xpkg/config_test.go
• pkg/xpkg/doc.go
• pkg/xpkg/fetch.go
• pkg/xpkg/find.go
• pkg/xpkg/find_test.go
• pkg/xpkg/fuzz_test.go
• pkg/xpkg/layers.go
• pkg/xpkg/lint.go
• pkg/xpkg/lint_test.go
• pkg/xpkg/name.go
• pkg/xpkg/name_test.go
• pkg/xpkg/parser/examples/parser.go
• pkg/xpkg/parser/examples/parser_test.go
• pkg/xpkg/parser/fsreader.go
• pkg/xpkg/parser/fuzz_test.go
• pkg/xpkg/parser/linter.go
• pkg/xpkg/parser/linter_test.go
• pkg/xpkg/parser/parser.go
• pkg/xpkg/parser/parser_test.go
• pkg/xpkg/parser/yaml/parser.go
• pkg/xpkg/reader.go
• pkg/xpkg/scheme.go
• pkg/xpkg/scheme_test.go
• pkg/xpkg/signature/attestation.go
• pkg/xpkg/signature/doc.go
• pkg/xpkg/signature/validate.go
• pkg/xpkg/validate.go

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Persist the request token before emitting the handled event.

Thank you for centralizing status updates. Line 990 emits reasonReconcileRequestHandled before updateStatus persists status.lastHandledReconcileAt. If reconciliation takes the external-create grace-period return at Lines 1212-1216, no status update occurs. The next requeue reads the same unrecorded token and emits the event again. A status-update conflict has the same ordering failure.

Persist the token before emitting the event. Route each live-resource early return after token detection through that persistence. Add coverage for the grace-period and status-conflict paths.

As per path instructions, events must occur only when something actually happens and must include specific details about what changed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/reconciler/managed/reconciler.go` around lines 981 - 1006, Update the
reconcile-request handling around reconcileRequestToken, updateStatus, and
reasonReconcileRequestHandled to persist status.lastHandledReconcileAt
successfully before emitting the handled event. Route every live-resource early
return after token detection, including the external-create grace-period path,
through this persistence, and avoid emitting the event when the status update
conflicts or otherwise fails. Add coverage for the grace-period and
status-conflict paths, ensuring events occur only after the token is actually
persisted and include the token/change details.

Source: Path instructions

[coderabbitai]

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Guard the deferred exReader.Close() against a nil reader.

Thank you for handling the missing examples/ directory gracefully. One nil-safety gap remains here. If b.exampleSource.Init(ctx) fails with a not-exist error, the code continues, but exReader is typically nil in that case. The unconditional defer on Line 152 then calls Close() on a nil io.ReadCloser, which panics when Build returns. The non-not-exist error path is safe, because Line 149 returns before the defer is registered.

Could you register the defer only when Init succeeds? That also lets you derive examplesExist from the same branch.

🐛 Proposed fix

-	// Get examples YAML stream.
-	exReader, err := b.exampleSource.Init(ctx)
-	if err != nil && !os.IsNotExist(err) {
-		return nil, nil, errors.Wrap(err, errInitBackend)
-	}
-
-	defer func() { _ = exReader.Close() }()
-	// examples/ doesn't exist
-	if os.IsNotExist(err) {
-		examplesExist = false
-	}
+	// Get examples YAML stream. A missing examples/ directory is not an error.
+	exReader, err := b.exampleSource.Init(ctx)
+
+	switch {
+	case os.IsNotExist(err):
+		examplesExist = false
+	case err != nil:
+		return nil, nil, errors.Wrap(err, errInitBackend)
+	default:
+		defer func() { _ = exReader.Close() }()
+	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Get examples YAML stream.
	exReader, err := b.exampleSource.Init(ctx)
	if err != nil && !os.IsNotExist(err) {
	return nil, nil, errors.Wrap(err, errInitBackend)
	}
	defer func() { _ = exReader.Close() }()
	// examples/ doesn't exist
	if os.IsNotExist(err) {
	examplesExist = false
	}
	// Get examples YAML stream. A missing examples/ directory is not an error.
	exReader, err := b.exampleSource.Init(ctx)
	switch {
	case os.IsNotExist(err):
	examplesExist = false
	case err != nil:
	return nil, nil, errors.Wrap(err, errInitBackend)
	default:
	defer func() { _ = exReader.Close() }()
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/build.go` around lines 146 - 156, Update the example-source
initialization flow around b.exampleSource.Init(ctx) so exReader.Close is
deferred only when initialization succeeds; keep the existing non-not-exist
error return, and set examplesExist to false in the not-exist branch while
treating successful initialization as the existing examples path.

[coderabbitai]

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Add a default branch so an unknown package kind does not panic.

Thanks for keeping linter selection explicit. If the meta object kind is not Configuration, Function, or Provider, linter stays nil, and Line 182 calls Lint on a nil interface. That panics instead of reporting a build failure. This is reachable whenever a user points the builder at a package whose meta kind is unexpected or whose GVK is empty.

Could you return a descriptive error for that case? A message that names the observed kind and the supported kinds helps the user fix their crossplane.yaml.

🐛 Proposed fix

 	var linter parser.Linter
 
-	switch meta.GetObjectKind().GroupVersionKind().Kind {
+	kind := meta.GetObjectKind().GroupVersionKind().Kind
+
+	switch kind {
 	case pkgmetav1.ConfigurationKind:
 		linter = NewConfigurationLinter()
 	case v1beta1.FunctionKind:
 		linter = NewFunctionLinter()
 	case pkgmetav1.ProviderKind:
 		linter = NewProviderLinter()
+	default:
+		return nil, nil, errors.Errorf("package metadata has unsupported kind %q; the metadata file must declare a Configuration, Function, or Provider", kind)
 	}

As per path instructions: "CRITICAL: Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	var linter parser.Linter
	switch meta.GetObjectKind().GroupVersionKind().Kind {
	case pkgmetav1.ConfigurationKind:
	linter = NewConfigurationLinter()
	case v1beta1.FunctionKind:
	linter = NewFunctionLinter()
	case pkgmetav1.ProviderKind:
	linter = NewProviderLinter()
	}
	if err := linter.Lint(pkg); err != nil {
	return nil, nil, errors.Wrap(err, errLintPackage)
	}
	var linter parser.Linter
	kind := meta.GetObjectKind().GroupVersionKind().Kind
	switch kind {
	case pkgmetav1.ConfigurationKind:
	linter = NewConfigurationLinter()
	case v1beta1.FunctionKind:
	linter = NewFunctionLinter()
	case pkgmetav1.ProviderKind:
	linter = NewProviderLinter()
	default:
	return nil, nil, errors.Errorf("package metadata has unsupported kind %q; the metadata file must declare a Configuration, Function, or Provider", kind)
	}
	if err := linter.Lint(pkg); err != nil {
	return nil, nil, errors.Wrap(err, errLintPackage)
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/build.go` around lines 171 - 184, Update the linter-selection switch
in the package build flow to handle unsupported or empty GVK kinds before
calling linter.Lint. Return a descriptive build error that includes the observed
kind, lists the supported Configuration, Function, and Provider kinds, and
guides the user to correct crossplane.yaml; preserve the existing linter
behavior for supported kinds.

Source: Path instructions

[coderabbitai]

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

desc can be nil here, and line 272 dereferences it.

The Fetcher contract lets Head return a nil descriptor with a nil error, and NopFetcher.Head in pkg/xpkg/fetch.go line 217 does exactly that. K8sFetcher.Head also has a d == nil branch, which confirms nil is a real result. Any CachedClient built with NewNopFetcher() therefore panics on a tag reference before it can report a useful error. A nil check turns the panic into a message the operator can act on.

🛡️ Proposed guard

 		desc, err := c.fetcher.Head(ctx, parsedResolvedRef, secrets...)
 		if err != nil {
 			return nil, errors.Wrapf(err, "cannot resolve %s to digest", parsedResolvedRef.String())
 		}
+		if desc == nil {
+			return nil, errors.Errorf("registry did not return a descriptor for package %s, so its digest cannot be resolved", parsedResolvedRef.String())
+		}
 		digest = desc.Digest.String()

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	var digest string
	if d, ok := parsedResolvedRef.(ociname.Digest); ok {
	digest = d.Identifier()
	} else {
	desc, err := c.fetcher.Head(ctx, parsedResolvedRef, secrets...)
	if err != nil {
	return nil, errors.Wrapf(err, "cannot resolve %s to digest", parsedResolvedRef.String())
	}
	digest = desc.Digest.String()
	}
	var digest string
	if d, ok := parsedResolvedRef.(ociname.Digest); ok {
	digest = d.Identifier()
	} else {
	desc, err := c.fetcher.Head(ctx, parsedResolvedRef, secrets...)
	if err != nil {
	return nil, errors.Wrapf(err, "cannot resolve %s to digest", parsedResolvedRef.String())
	}
	if desc == nil {
	return nil, errors.Errorf("registry did not return a descriptor for package %s, so its digest cannot be resolved", parsedResolvedRef.String())
	}
	digest = desc.Digest.String()
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/client.go` around lines 264 - 273, Guard the descriptor returned by
c.fetcher.Head in the digest-resolution branch before dereferencing desc.Digest.
If desc is nil, return a descriptive error indicating that parsedResolvedRef
could not be resolved to a descriptor; preserve the existing wrapped Head error
handling and digest-reference path.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Layer panics if cfg.Labels is nil.

Layer is exported and takes cfg *v1.Config from the caller. Line 77 assigns into cfg.Labels, and an assignment into a nil map panics at runtime. A caller that passes a freshly declared v1.Config{}, or the config file of an image built without labels, hits this on the first annotated layer. A nil cfg panics as well. Was a non-nil Labels map part of the intended contract? Either a guard or a documented precondition would make this safe for callers outside this repository.

🛡️ Proposed guard

 	if annotation != "" {
+		if cfg == nil {
+			return nil, errors.New("cannot add layer annotation: no image config supplied")
+		}
+
+		if cfg.Labels == nil {
+			cfg.Labels = map[string]string{}
+		}
+
 		cfg.Labels[Label(d.String())] = annotation
 	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Add annotation label to config if a non-empty label is specified. This is
	// an intermediary step. AnnotateLayers must be called on an image for it to
	// have valid layer annotations. It propagates these labels to annotations
	// on the layers.
	if annotation != "" {
	cfg.Labels[Label(d.String())] = annotation
	}
	// Add annotation label to config if a non-empty label is specified. This is
	// an intermediary step. AnnotateLayers must be called on an image for it to
	// have valid layer annotations. It propagates these labels to annotations
	// on the layers.
	if annotation != "" {
	if cfg == nil {
	return nil, errors.New("cannot add layer annotation: no image config supplied")
	}
	if cfg.Labels == nil {
	cfg.Labels = map[string]string{}
	}
	cfg.Labels[Label(d.String())] = annotation
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/layers.go` around lines 72 - 78, Update the Layer function to safely
handle caller-provided configs with nil Labels and nil cfg before assigning the
annotation label. Initialize cfg.Labels when needed, and add an appropriate
nil-config guard or return behavior while preserving the existing non-empty
annotation flow.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

The len(addendums) == 0 short-circuit does not match its comment.

The loop appends one mutate.Addendum for every layer, with or without a matching label. So addendums is empty only when the image has no layers, and the comment "we didn't find any annotations, return original image" never describes the real condition. An image with layers but no matching labels still gets rebuilt from empty.Image, which discards the original layer history and produces a new digest for no gain. Tracking whether any annotation was applied would restore the intended fast path.

🐛 Proposed fix

 	addendums := make([]mutate.Addendum, 0)
+	annotated := false
 
 	for _, l := range layers {
 		d, err := l.Digest()
 		if err != nil {
 			return nil, errors.Wrap(err, errDigest)
 		}
 
 		if annotation, ok := cfgFile.Config.Labels[Label(d.String())]; ok {
+			annotated = true
+
 			addendums = append(addendums, mutate.Addendum{
 				Layer: l,
 				Annotations: map[string]string{
 					AnnotationKey: annotation,
 				},
 			})
 
 			continue
 		}
 
 		addendums = append(addendums, mutate.Addendum{
 			Layer: l,
 		})
 	}
 
-	// we didn't find any annotations, return original image
-	if len(addendums) == 0 {
+	// We didn't find any annotations, so return the original image.
+	if !annotated {
 		return i, nil
 	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	addendums := make([]mutate.Addendum, 0)
	for _, l := range layers {
	d, err := l.Digest()
	if err != nil {
	return nil, errors.Wrap(err, errDigest)
	}
	if annotation, ok := cfgFile.Config.Labels[Label(d.String())]; ok {
	addendums = append(addendums, mutate.Addendum{
	Layer: l,
	Annotations: map[string]string{
	AnnotationKey: annotation,
	},
	})
	continue
	}
	addendums = append(addendums, mutate.Addendum{
	Layer: l,
	})
	}
	// we didn't find any annotations, return original image
	if len(addendums) == 0 {
	return i, nil
	}
	addendums := make([]mutate.Addendum, 0)
	annotated := false
	for _, l := range layers {
	d, err := l.Digest()
	if err != nil {
	return nil, errors.Wrap(err, errDigest)
	}
	if annotation, ok := cfgFile.Config.Labels[Label(d.String())]; ok {
	annotated = true
	addendums = append(addendums, mutate.Addendum{
	Layer: l,
	Annotations: map[string]string{
	AnnotationKey: annotation,
	},
	})
	continue
	}
	addendums = append(addendums, mutate.Addendum{
	Layer: l,
	})
	}
	// We didn't find any annotations, so return the original image.
	if !annotated {
	return i, nil
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/layers.go` around lines 125 - 152, Update the layer-processing
function around the addendums loop to track whether at least one annotation was
applied, rather than checking len(addendums). Return the original image when no
matching labels were found, while preserving addendum construction for annotated
layers and unannotated layers when rebuilding is necessary.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Make package validation errors actionable for package authors.

These errors are returned from the package build path after linting. Messages such as "object is not a CRD" and "package version constraints are poorly formatted" do not identify the failed package requirement or the required correction.

Include the package context, the supported object types or metadata type, and a next step. For example, state that the package must contain exactly one matching metadata object, or that the Crossplane version constraint must use valid semantic-version constraint syntax.

As per path instructions, “all error messages must be meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/lint.go` around lines 37 - 53, Update the error-message constants in
the lint validation block, including errNotCRD, errNotMRD, errNotXRD, the
webhook and operation type errors, errNotMeta and its metadata-specific
variants, errBadConstraints, and errFmtCrossplaneIncompatible, so each explains
the package requirement, identifies the expected object or metadata type, and
suggests the corrective action. State that exactly one matching metadata object
is required where applicable, and that version constraints must use valid
semantic-version constraint syntax, while preserving any formatting placeholders
used by callers.

Source: Path instructions

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect cosign's handling of an empty CheckOpts.Identities and the CRD validation on CosignAuthority.
set -uo pipefail

# 1. Locate the cosign module in the module cache / vendor and inspect identity policy checks.
fd -t d -g 'cosign*' "$(go env GOMODCACHE 2>/dev/null || echo /root/go/pkg/mod)/github.com/sigstore" 2>/dev/null | head

# 2. Search for identity validation logic.
rg -nP -C6 '\bIdentities\b' \
  "$(go env GOMODCACHE 2>/dev/null || echo /root/go/pkg/mod)/github.com/sigstore" \
  --glob '*cosign*/pkg/cosign/verify.go' 2>/dev/null | head -80

# 3. Check whether the crossplane API type requires identities / key (oneOf, +kubebuilder markers).
fd -t f -g '*.go' | xargs rg -nP -C10 'type CosignAuthority struct' 2>/dev/null | head -60

Repository: crossplane/crossplane-runtime

Length of output: 171

---

🌐 Web query:

cosign CheckOpts Identities empty accepts any certificate identity verification

💡 Result:

In the Cosign Go package, when CheckOpts.Identities is empty, Cosign does not perform identity verification, but it also does not automatically accept any certificate identity. Instead, the behavior depends on the context of the verification [1][2]. If you are using Cosign's keyless verification mode (e.g., verifying against a Rekor transparency log), the Cosign CLI explicitly requires identity constraints to be provided via flags like --certificate-identity or --certificate-identity-regexp [3][4][5]. If these are not provided, the CLI will return an error stating that identity verification is required [3][4][5]. Within the cosign library's verify.go, if Identities is provided as an empty list or slice, the logic iterates through the provided identities [1][2]. If the list is empty, the check "none of the expected identities matched" is typically reached or bypassed depending on how the CheckOpts are initialized and passed to the internal verify functions [1][2]. However, because the CLI enforces identity flags in keyless mode, you cannot reach the internal logic with empty Identities unless you are invoking the Go library directly in a custom implementation that intentionally bypasses these checks [6][3]. In summary, while the Identities slice being empty does not inherently "accept" any identity in the logic—it simply means no identity constraints are applied—the Cosign CLI enforces that identities must be specified for keyless verification to prevent insecure practices [3][4][5]. You should always define explicit identity policies to ensure that only intended signers are trusted [7].

Citations:

• 1: https://github.com/sigstore/cosign/blob/main/pkg/cosign/verify.go
• 2: https://github.com/sigstore/cosign/blob/29361993239764ae63c3017198cc80ff5816c08f/pkg/cosign/verify.go
• 3: "cosign verify-blob" or "cosign verify" with local certificate and chain always asks for oidc provider  sigstore/cosign#3572
• 4: Cosign verify does not work with only certificate and certificate chain sigstore/cosign#3720
• 5: Verify using transparent log sigstore/cosign#4590
• 6: https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/verify/verify.go
• 7: CLI: cosign verify --cert-email should be aliased as --cert-identity (or similar) sigstore/cosign#1964

---

🏁 Script executed:

#!/bin/bash
set -uo pipefail

echo "== files =="
git ls-files | sed -n '/pkg\/xpkg\/signature\/validate.go\|go.mod\|api\|config/c/p' | rg 'validate.go|go.mod|go.sum|api.*cosign|config.*cosign|cosign.*api' | head -200

echo
echo "== validate.go outline =="
ast-grep outline pkg/xpkg/signature/validate.go 2>/dev/null || true
echo
wc -l pkg/xpkg/signature/validate.go go.mod 2>/dev/null || true

echo
echo "== validate.go relevant sections =="
sed -n '1,260p' pkg/xpkg/signature/validate.go 2>/dev/null | cat -n

echo
echo "== module versions =="
rg -n 'sigstore/cosign|github.com/sigstore/cosign' go.mod go.sum 2>/dev/null | head -80

echo
echo "== CosignAuthority definitions/usages =="
rg -n -C8 'type CosignAuthority struct|CosignAuthority|Keyless|Identities|keyless|InsecureIgnoreSCT' pkg go.mod api config 2>/dev/null | head -220

Repository: crossplane/crossplane-runtime

Length of output: 12410

---

🏁 Script executed:

#!/bin/bash
set -uo pipefail

# Inspect the vendored/module cache copy if already present in the sandbox.
root="$(go env GOMODCACHE 2>/dev/null || true)"
echo "GOMODCACHE=$root"

if [ -n "$root" ] && ls -1 "$root/github.com/sigstore/cosign"* 2>/dev/null | head -20; then
  for dir in "$root"/github.com/sigstore/cosign*/pkg/cosign/verify.go; do
    [ -f "$dir" ] || continue
    echo "== $dir =="
    rg -n -C8 '\bIdentities\b|func checkOpts|VerifyImageSignatures|VerifyLocalImageSignatures' "$dir" | head -180
    break
  done
else
  echo "cosign module cache not present; using module proxy references for source behavior"
  curl -sSfL "https://raw.githubusercontent.com/sigstore/cosign/v2.3.0/pkg/cosign/verify.go" 2>/dev/null | \
    rg -n -C8 '\bIdentities\b|func checkOpts|VerifyImageSignatures' | head -220 || true
fi

Repository: crossplane/crossplane-runtime

Length of output: 3081

---

🏁 Script executed:

#!/bin/bash
set -uo pipefail

curl -sSfL "https://raw.githubusercontent.com/sigstore/cosign/v3.0.6/pkg/cosign/verify.go" | sed -n '300,420p' | cat -n
printf '\n--- VerifyImageSignatures ---\n'
curl -sSfL "https://raw.githubusercontent.com/sigstore/cosign/v3.0.6/pkg/cosign/verify.go" | sed -n '485,630p' | cat -n
printf '\n--- VerifyImageAttestations relevant ---\n'
curl -sSfL "https://raw.githubusercontent.com/sigstore/cosign/v3.0.6/pkg/cosign/verify.go" | sed -n '640,850p' | cat -n

printf '\n--- v1beta1 source candidates ---\n'
git ls-files | rg 'apis/.*/v1beta1|config/.*/v1beta1|api/.*/v1beta1' | head -50
rg -n -C8 'type CosignAuthority struct|type ImageVerification struct|Keyless|Identities|+kubebuilder' . --glob '*.go' | head -260

printf '\n--- deterministic call-site invariant ---\n'
python3 - <<'PY'
from pathlib import Path
p = Path('pkg/xpkg/signature/validate.go')
s = p.read_text()
build = s[s.index('func (c *CosignValidator) buildCosignCheckOpts'):s.index('\nfunc hashAlgorithm')]
checks = {'keyless_block': 'if kl := a.Keyless; kl != nil' in build,
          'key_block': 'if kr := a.Key; kr != nil' in build,
          'return_nil_if_both_unset': 'return &opts, nil' in build,
          'identity_loop_appends': 'opts.Identities = append(opts.Identities, cosign.Identity{' in build,
          'sig_verifier_load': 'opts.SigVerifier, err = signature.LoadVerifier(publicKey, ha)' in build}
print(checks)
PY

Repository: crossplane/crossplane-runtime

Length of output: 19675

---

🏁 Script executed:

#!/bin/bash
set -uo pipefail

curl -sSfL "https://raw.githubusercontent.com/sigstore/cosign/v3.0.6/pkg/cosign/verify.go" | sed -n '430,540p' | cat -n
curl -sSfL "https://raw.githubusercontent.com/sigstore/cosign/v3.0.6/pkg/cosign/verify.go" | sed -n '340,365p' | cat -n

printf '\n--- v1beta1 source candidates ---\n'
git ls-files | rg '(apis|api|config)/.*/v1beta1' | head -100
rg -n -C8 'type CosignAuthority struct|type ImageVerification struct|Keyless|Identities|oneOf|kubebuilder' --glob '*.go' . | head -300

printf '\n--- validate.go deterministic invariant ---\n'
python3 - <<'PY'
from pathlib import Path
s = Path('pkg/xpkg/signature/validate.go').read_text()
start = s.index('func (c *CosignValidator) buildCosignCheckOpts')
end = s.index('\nfunc hashAlgorithm')
build = s[start:end]
print('both conditional_blocks:', 'if kl := a.Keyless; kl != nil' in build and 'if kr := a.Key; kr != nil' in build)
print('returns_opts_before_both_checks:', build.index('if kr := a.Key; kr != nil') > build.index('if kl := a.Keyless; kl != nil'))
print('identity_appended_only_inside_keyless:', build[build.index('if kl := a.Keyless; kl != nil'):build.index('if kr := a.Key; kr != nil', build.index('if kl := a.Keyless; kl != nil') + 1)].count('opts.Identities = append(opts.Identities, cosign.Identity') == 1)
print('sig_verifier_loaded_only_inside_key:')
kv = build[build.index('if kr := a.Key; kr != nil'):]
print(kv.count('opts.SigVerifier, err = signature.LoadVerifier(publicKey, ha)') == 1)
PY

Repository: crossplane/crossplane-runtime

Length of output: 12511

---

🌐 Web query:

cosign v3.0.6 VerifyImageSealattestations CheckCertificatePolicy Identities empty

💡 Result:

In Cosign, the function CheckCertificatePolicy is responsible for validating that a certificate's subject and issuer match the expected criteria defined in CheckOpts [1][2]. When the Identities slice in CheckOpts is empty (len(co.Identities) == 0), the logic that iterates over and matches these identities is skipped [1][2]. Because the code only iterates over the identities if len(co.Identities) > 0, an empty list of identities does not trigger a verification failure for that specific check [1][2]. Consequently, if no other constraints (such as CertEmail or CertOidcIssuer) are provided in the CheckOpts that would otherwise fail, the function will return nil, indicating success for the certificate policy check [1][2]. When performing operations like verifying attestations (e.g., via cosign verify-attestation), identities are typically populated from CLI flags like --certificate-identity or --certificate-identity-regexp [3][4]. If these are not provided for keyless flows, the verification will fail because the required identity constraints are missing [3]. If you are seeing unexpected behavior where an empty identity set appears to "pass," it is likely because no explicit identity constraints were enforced for that specific verification path [1][2].

Citations:

• 1: https://github.com/sigstore/cosign/blob/main/pkg/cosign/verify.go
• 2: https://github.com/sigstore/cosign/blob/29361993239764ae63c3017198cc80ff5816c08f/pkg/cosign/verify.go
• 3: https://github.com/sigstore/cosign/blob/main/doc/cosign_verify-attestation.md
• 4: https://pkg.go.dev/github.com/sigstore/cosign/v2/pkg/cosign/attestation

---

Broken Authentication (CWE-1390)

Reachability: External

Reachability path

● Entry
  pkg/reconciler/managed/reconciler_modern_test.go:3015
  TestReconcileRequestAnnotation
│
▼
● Sink
  pkg/xpkg/signature/validate.go

Reject empty Cosign authorities before calling cosign

If Keyless.Identities is omitted/empty and there is no Key, buildCosignCheckOpts returns opts with no identity constraints or signature verifier. Cosign’s keyless certificate policy then accepts any Fulcio-issued certificate, so verification can succeed without constraining the trusted signer. Return an explicit error requiring either at least one keyless identity or a key-backed verifier/configuration that enforces signer constraints.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/xpkg/signature/validate.go` around lines 177 - 217, Update
buildCosignCheckOpts so it rejects configurations where Keyless.Identities is
empty or omitted and Key is nil, returning an explicit error that requires at
least one keyless identity or a key-backed verifier configuration. Preserve the
existing identity and key setup when either valid authority is configured, and
ensure the validation occurs before returning unconstrained opts to cosign.

[frigaut-orange]

Closing this one in favor of #1106 as rebase went wrong