fix(comp): exclude XRs being deleted from impact analysis

[jcogilvie]

Description of your changes

An XR carrying a metadata.deletionTimestamp is on Crossplane's teardown path: the composite reconciler deletes its composed resources rather than composing them. Such an XR will never adopt the CompositionRevision the diffed composition would produce, and rendering it yields zero composed resources to diff against — so including it in impact analysis is meaningless by construction. At best it shows up as a misleading ✓ unchanged row (plus a wasted function render); at worst, as reported in #452, it fails outright and aborts the whole run with a non-zero exit code.

classifyXR now drops these XRs with a new FilterReason "deleting", following the reason-extensible model established for #388:

• Evaluated before the update-policy rules, and not re-included by --include-manual — deletion supersedes both, the same way a revision-selector mismatch does.
• Counted in a new AffectedResourcesSummary.FilteredByDeletion, kept separate from the other filter counters so the breakdown survives in default-discovery mode where individual impacts aren't surfaced.
• In --resource mode, surfaced as a "filtered" impact entry carrying the deletionTimestamp as filterDetail, so a user who explicitly names a deleting composite is told why it was skipped instead of getting silence. A long-stuck timestamp is also the diagnostic signal for a wedged deletion.

Scoped to comp; the xr mirror case is deferred

Per the repo's triage-across-commands convention I checked xr too. The right behavior there is not an exclusion — xr diffs exactly the resource the user named, so filtering it out would leave them with no output — but a user-visible advisory that the cluster copy is terminating.

crossplane-diff has no advisory channel to carry that. The default logger is logging.NewNopLogger() (main.go:177), so logger.Info is invisible without --verbose — which is why the one existing warning in the codebase, "Resource already belongs to another composite. Applying this diff will assume ownership!" (resource_manager.go:163), is in practice never seen. Structured output has no non-error equivalent either. An ad-hoc Fprintf to stderr would sit outside the dual-emission contract the project documents for errors, so it is deferred to a follow-up that adds a real channel rather than invented here. Tracked in #459.

Accuracy-first deletionTimestamp reader

unstructured.GetDeletionTimestamp silently reads a present-but-wrong-typed value as absent. Since this decides whether a resource is evaluated at all, a wrong answer would quietly change the diff — so DeletionTimestamp treats absent / explicitly-null / empty as "not deleting" and a non-string value as an error. (Explicit null is how round-tripped Kubernetes YAML spells "unset" for timestamp fields, hence the distinction.)

A note on the reported error message

I was not able to reproduce the reporter's exact cannot apply an object with managed fields already set synthetically. That message comes from client-go's dynamic Apply guard, which rejects any object with metadata.managedFields populated, reached via CalculateDiff → DryRunApply for the XR itself. The only path that can feed an unstripped object there is diff_calculator.go's nested-XR branch (desiredXR = renderedXR), since the root branch goes through mergedXR.SetManagedFields(nil); I probed that branch and could not reach it on main either. v0.10.0..HEAD has no changes to the four relevant files, so the reporter is running exactly this logic — the difference is likely their crossplane render version.

This change is justified on its own merits regardless (a deleting XR's impact analysis is meaningless), and it makes that code path unreachable for deleting XRs, which is the reported trigger. If the error resurfaces for a non-deleting XR, that's a separate managed-fields-stripping bug worth its own issue.

Testing

• Unit: TestDeletionTimestamp pins the reader's semantics (absent / null / empty / valid / wrong-type). TestDefaultCompDiffProcessor_partitionXRsByUpdatePolicy gains five cases: dropped-as-deleting, deletion-beats-Manual-even-with---include-manual, deletion-beats-a-matching-selector, null-timestamp-kept, and malformed-errors. TestDefaultDiffProcessor_warnIfDeleting covers the xr warning including the nested-XR and malformed cases. Test_allFilteredMessage covers the now-combinatorial all-filtered summary line across all seven reason combinations.
• Integration: CompositionDiffExcludesDeletingXRs (default discovery, JSON — asserts filteredByDeletion: 1 and that only the live XR is evaluated), ResourceFilterSurfacesDeletingXR (--resource mode — asserts the filtered entry with reason deleting).
• Both integration cases and the three deletion unit cases were mutation-tested: reverting the production branch makes each fail.
• Test-harness addition: deleteAfterSetup (issues a delete after setup; fixtures carry a finalizer so envtest leaves them Terminating, which is how a mid-deletion resource is observed with no controllers running).

Fixes #452

I have:

• Read and followed Crossplane's contribution process.
• Run earthly -P +reviewable to ensure this PR is ready for review.
• Added or updated unit tests.
• Added or updated integration or e2e tests.
• Updated documentation as needed (user-facing behavior in README.md; architecture in design/design-doc-cli-diff.md and its diagrams).

Need help with this checklist? See the cheat sheet.

🤖 Generated with Claude Code

[jcogilvie]

Note

Posted by Claude Code, an AI agent, on behalf of @jcogilvie.

Amended (d507a4b, on top of your rebase onto main): replaced the bespoke DeletionTimestamp reader with the standard unstructured.GetDeletionTimestamp().

The reader was over-defensive and I couldn't justify it on review. Its stated purpose was to hard-error on a present-but-wrong-typed value rather than silently reading it as absent — but that case isn't reachable:

• Both call sites receive only apiserver-sourced objects: classifyXR gets XRs from FindComposites listings, and the xr-side advisory (deferred to Add a warning channel: non-fatal advisories are invisible today #459) gets existingXRFromCluster from FetchCurrentObject.
• metadata.deletionTimestamp is apiserver-owned core metadata — not settable on create, always serialized as RFC3339 or absent. No user-supplied manifest reaches either site.

I'd over-applied the precedent from #388. That one exists for compositionUpdatePolicy and compositionRevisionSelector, and it's right there — those are spec fields, user-authored and only as well-formed as the XRD's schema requires, so a malformed value is genuinely reachable and must not be silently defaulted. deletionTimestamp isn't in that category, and the difference is now recorded in the design doc so the two readers don't look inconsistent.

Verified empirically rather than from memory that the stdlib accessor handles every reachable input correctly — absent, explicit null, and empty string all yield nil; only the unreachable wrong-type case differs. So the net change is -111/+25: the reader, its test, an unreachable table case, and an error return that was threaded through classifyXR → partitionXRsByUpdatePolicy all go away.

One small behavioral improvement fell out: the timestamp in filterDetail is now normalized to UTC/RFC3339. metav1.Time renders machine-local by default, which would have made the surfaced detail differ between runs on differently-configured machines.

The --include-manual interaction and the null-timestamp case are still covered; only the unreachable wrong-type case was dropped.

Not rebasing #458/#460 onto this yet, per your call — they'd need redoing after this merges anyway.