feat(comp): skip impact analysis for unchanged compositions

[jcogilvie]

Description of your changes

Note

Stacked on #457 (issue #452). Base is fix/452-exclude-deleting-xrs, so the diff shown here is #453's change alone. Retarget to main once #457 merges.

Applying a composition identical to its in-cluster version creates no new CompositionRevision, so no XR adopts anything it hasn't already adopted. Any downstream delta computed in that situation is therefore caused by something other than the composition, and the tool cannot tell the causes apart:

1. Convergence lag — an EnvironmentConfig or other required resource changed and the XRs haven't re-reconciled yet.
2. Blocked reconciliation — paused annotation, failing function, RBAC, a wedged XR. Real, but it's a cluster health finding.
3. A modeling artifact of crossplane-diff itself — we render with our pulled function images against our reconstruction of observed state. A mutable function tag or unmodeled cluster behavior produces a delta that doesn't exist in the cluster at all.

Reporting these under === Impact Analysis === attributes cluster state to a change that doesn't exist. It also returned ExitCodeDiffDetected (3) for a composition the user didn't change — CompositionDiff.HasChanges() returns true on any XRStatusChanged impact even when CompositionDiff == nil — which in CI reads as "this composition change has impact" and would gate a pipeline on drift the user didn't cause.

So the per-XR work (one function render per affected XR — the dominant cost of comp) is now skipped for an unchanged composition, evaluated per-composition so a mixed input behaves sensibly. The two impact sections are replaced by an explicit note, and CompositionDiff.ImpactAnalysisSkipped (serialized as impactAnalysisSkipped) lets structured-output consumers distinguish "not evaluated" from "no affected XRs found".

Full reasoning, including the nested-composition case and the arguments considered on the other side, is written up in this comment on #453.

--analyze-unchanged opts back in

One workflow the skip would otherwise remove is legitimate: establishing an "is my cluster converged?" baseline before editing a composition, so any diff afterwards is attributable to the edit. There's no other way to express that today, since xr requires each XR file individually. The flag is cheap — ProcessorConfig already carries IncludeManual, EventualState, and IgnorePaths in exactly this shape.

The gate is deliberately NOT the displayed diff

calculateCompositionDiff now returns a compositionComparison carrying both the diff to show (nil when equal) and whether the composition changed at all. The two come apart only under --ignore-paths, which is applied to the composition comparison too. Gating on the displayed diff would mean that masking a path which is load-bearing for rendering — anything under spec.pipeline[].input, say — silently skips the analysis for a composition that genuinely changes the rendered output. The second comparison is local (no API calls) and only runs when --ignore-paths is in play and the masked diff came back equal.

Testing

• Unit: TestDefaultCompDiffProcessor_calculateCompositionDiff is a new table pinning the display-diff / changed split, including the case where --ignore-paths masks the only difference — that must still count as changed. TestDefaultCompDiffProcessor_DiffComposition gains UnchangedCompositionSkipsImpactAnalysis (skip note present, both section headers absent) and UnchangedCompositionWithAnalyzeUnchanged (sections return).
• Integration: UnchangedCompositionSkipsImpactAnalysis and UnchangedCompositionAnalyzeUnchangedEvaluatesXRs use the same fixtures deliberately — fixtures that DO produce a downstream delta when evaluated. So the pair asserts the contrast directly: skipped ⇒ no report and exit 0; --analyze-unchanged ⇒ the downstream modification and exit 3. That delta (an annotation-key discrepancy in the fixtures) is itself a clean example of a bucket-3 artifact, which is why the opt-in is explicit.
• Both the ignore-paths gate and the integration skip were mutation-tested: reverting the production logic makes each fail.

Two existing tests changed, both deliberately

• CompositionDiffIgnorePaths feeds composition-no-changes.yaml and asserts on the impact sections to validate XR-level ignore paths. It now passes --analyze-unchanged, so it keeps testing what it was written to test rather than being rewritten.
• MultipleCompositionDiffImpact turns out to feed a second composition (xnopresources-v2.diff.example.org) that is byte-identical to its in-cluster version — updated-composition-2.yaml and original-composition-2.yaml are the same file. It has therefore always been a mixed changed/unchanged case rather than the "two changed compositions" its name suggests. Rather than change the fixture, I updated its expectation and reworded its reason to say what it actually covers, so it now serves as the per-composition-skip case. Flagging it explicitly in case you'd rather I make the fixture genuinely differ instead.

TestDefaultCompDiffProcessor_DiffComposition_StderrErrorOutput also needed its input composition to differ from the cluster's (a label), since otherwise the XR failure it tests became unreachable.

Fixes #453

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