[sourcehawk]

Description

Fixes #248

A component recorded an Updated<Kind> event when another controller wrote to the object between the read and the apply of the component. This PR compares only the fields that the field manager of the component owns, so a finalizer, label or annotation from another controller is no longer an update. If the read carries no managedFields, for example with a cache that strips them, the whole object is compared as before, and this race still gives a false Updated. The race also still gives a false Updated when the other controller sets a field inside an empty object that the component applies, such as emptyDir: {}.

Changes

• A finalizer, label or annotation that another controller adds between the read and the apply no longer gives an Updated<Kind> event, an updated apply count, or ConvergingOperationUpdated to the status handler.
• A change to an owned field, the removal of one, or a new order of owned list items such as containers, still gives exactly one Updated<Kind> event.
• When the component's managedFields entries before and after the apply have different API versions, as after a move to a new version of a custom resource, the whole object is compared.
• The ConvergingOperation GoDoc, docs/component.md and docs/custom-resource.md now state the owned-field rule, the cases that fall back to the whole object, and the empty-object limit.

Testing

make all passes. The new unit tests use the fake client with WithReturnManagedFields and an interceptor that writes as another field manager right after the read. They cover a finalizer, a label and an annotation from another manager (no event), an owned change during such a write (one event), and an owned removal (one event). The existing classification tests now run twice: with managedFields and without them.

A new envtest spec does the same race against a real API server with a Deployment. It adds a finalizer as another manager, then changes the image and the container port. This exercises the keyed list paths in managedFields. A second spec swaps two containers and expects one Updated. Against the old whole-object comparison, the race spec and the race unit tests fail. A comparison that uses only the owned fields after the apply makes the removal test fail. Unit tests also cover the order check (a swap of owned items counts, an item of another manager does not) and the fallback for entries at different API versions.

No E2E run: no E2E test asserts on these events, and the envtest spec covers the real API server.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Managed-field comparison misses version-converted removals and associative-list reordering while misclassifying foreign additions to empty granular maps, leading to incorrect change detection and update events/counts.

3 open issues

• Associative-list reordering is missed in update detection · New
• Empty-map ownership includes concurrently added foreign fields · New
• Version mismatch can suppress converted-field removal updates · New

What changed in this PR

This PR addresses #248 by classifying component updates using fields owned by the component’s field manager, rather than counting unrelated writes from other controllers.

Changes:

• Compares owned fields before and after apply, retaining whole-object comparison when ownership information is unavailable.
• Adds unit and integration coverage for concurrent writes, owned-field changes, and removals.
• Updates public documentation and generated plugin references to explain the classification behavior.

File	Description
plugin/​skills/​custom-resource-wrappers/​references/​custom-resource.md	Syncs wrapper guidance on apply classification.
plugin/​skills/​building-components/​references/​component.md	Syncs owned-field and fallback documentation.
pkg/​component/​create.go	Passes the field manager into change detection.
pkg/​component/​create_test.go	Runs classification cases with and without managed fields.
pkg/​component/​concepts/​converging.go	Documents owned-field operation semantics.
pkg/​component/​applied_change.go	Implements ownership-based comparison and fallback.
pkg/​component/​applied_change_test.go	Tests concurrent writes, removals, and field-path lookup.
pkg/​component/​applied_change_integration_test.go	Tests Deployment classification against a real API server.
go.mod	Promotes structured-merge-diff to a direct dependency.
docs/​custom-resource.md	Explains operation classification for resource wrappers.
docs/​component.md	Documents owned-field comparison and missing-ownership fallback.

_{Review effort: Balanced}

[sourcehawk]

Review round 1

Review base: 0ee5985d52277715ab78dcaf79e77d030c757170. Fixes: 5755f4c.

The Copilot review had 3 inline findings and no suppressed comments. Two are fixed: the reorder of owned list items, and the fallback when the entry versions differ. One is declined, with the reason in its thread: an empty object owned as one leaf. The docs now state that limit.

Doc comments that grew

The comment gate flagged 2 doc comments, and ConvergingOperationUpdated changed. An evaluator that did not see the findings judged each sentence that this round added. The table shows the final verdicts.

Declaration	Lines (base → now)	Sentence	Verdict	Source	Caller code
appliedObjectChanged (internal)	14 → 16	"A write that sets a field inside an empty object that manager owns, such as emptyDir: {}, does count."	KEEP	applied_change.go, the leaf comparison; envtest measurement, 2026-11-12	A caller does not read true as proof that its own apply changed the object
appliedObjectChanged		"That is the case when before carries no managedFields, as when the cache strips them, when the response holds no apply entry for manager, and when the two entries were recorded at different API versions."	KEEP	applied_change.go, the fallback branches; controller-runtime v0.25.2 pkg/cache/cache.go:411	The read of before keeps managedFields
appliedObjectChanged		"A changed owned field, an owned field the apply removed, and owned list items in a new order all count."	CUT		Described the body. The reorder fact stays in the inline comment at the order check
ConvergingOperation	20 → 23	"When the read returns none, as with a cache that strips them, when the apply response holds no apply entry for the component's field manager, or when the component's entries before and after the apply have different API versions, the framework compares the whole object apart from the status subresource, managedFields, resourceVersion and generation."	KEEP	applied_change.go, the fallback branches; pkg/cache/cache.go:411	Whether to set cache.TransformStripManagedFields() as DefaultTransform
ConvergingOperation		"A write by another field manager in that window then does count as a change."	KEEP	comparableContent, wholeObjectChanged	Code that reacts to Updated expects writes by other controllers in these cases
ConvergingOperation		"It also counts when it sets a field inside an empty object that the component applies, such as emptyDir: {}."	KEEP (by instruction)	applied_change.go, the leaf comparison; envtest measurement, 2026-11-12
ConvergingOperationUpdated	4 → 2	"ConvergingOperationUpdated indicates that the resource existed and the comparison described on ConvergingOperation found a change."	KEEP	create.go, the classification	A status handler treats Updated as a change found by that comparison, not always as a change of its own desired state

The sentence marked "by instruction" documents the limit of the declined finding, as the PR author asked.

Proposals that were not applied, because they are about text from before this review and no finding asks for them:

• ConvergingOperation: cut "The framework derives it by comparing the object observed before the Server-Side Apply ... on the fields the component's field manager owns before or after the apply." The evaluator reads it as a description of the body, and a second copy of the comment in create.go.
• ConvergingOperation: cut "The owned fields come from the object's managedFields." It describes the body.

Removed blocks: the doc of findListItem, because the function is gone. The comment "No entry for manager before the apply means it owned nothing yet" moved with its code.

[Copilot]

🟢 Approval recommended

No active review issues block the ownership-aware update classification change.

No open issues 🎉

3 resolved since last review

• Version mismatch can suppress converted-field removal updates
• Empty-map ownership includes concurrently added foreign fields
• Associative-list reordering is missed in update detection

_{Review effort: Balanced}