feat(component): add DeleteOnlyIfOwned to keep objects the owner does not own

[sourcehawk]

Description

Closes #233

A gated-off resource (GatedBy, DeleteWhen, a disabled component feature gate) is deleted by name, so an object that a user created under the same name is deleted too. This PR adds the DeleteOnlyIfOwned() resource option: a deletion first reads the live object and deletes it only when the object carries an owner reference with the owner's UID, with UID and resourceVersion preconditions on the delete. Without the option, the behavior is unchanged.

Changes

• New component.DeleteOnlyIfOwned() resource option. It applies to every path that goes through deleteEntry: Delete(), DeleteWhen(), GatedBy(), a disabled component feature gate, and suspension with DeleteOnSuspend().
• An object without the owner's reference is kept with no write, no ResourceDeleted event and no reconcile error. The skip is logged.
• With DeleteOnSuspend(), the suspension checks the owner reference before Suspend() and the apply, because that apply would set the reference and the delete after it would then find the object owned. An object without the reference is not written and counts as suspended. The suspension apply also carries the observed resourceVersion, through a new internal withResourceVersion apply option. If the object changes between the read and the apply, the apply is rejected, the resource reports Suspending, and the reconcile does not fail.
• A precondition conflict on the delete (the object changed or was replaced after the read) keeps the object, logs the skip and does not fail the reconcile.
• DeleteOnlyIfOwned() with ReadOnly() or Unowned() is a build error, because such a resource never carries the owner reference.
• BlockOnForeignController() keeps its documented meaning. With both options, an object another owner controls is kept, and a conflict does not fail the reconcile.
• The owner reference match that orphanResources used inline is now a shared referencesOwner predicate, used by both paths.
• Docs: docs/component.md (option table and a section for the option), docs/guidelines.md (the GatedBy guidance), and the building-components and structuring-operators skills. Plugin references are synced.

Challenges

The design choices that the issue did not settle:

• What counts as owned. Any owner reference with the owner's UID, not only the controller reference. This matches how OrphanWhen finds the owner's reference, and it means an object co-owned through a non-controller reference is still deleted. To keep an object that another owner controls, add BlockOnForeignController().
• The read. It uses the same reader as BlockOnForeignController(): ReconcileContext.APIReader when set, the cached client otherwise. The preconditions make the delete safe with a stale cache too. The fresh read only makes a wrong keep or a conflict less likely.
• Conflict handling. With DeleteOnlyIfOwned(), a 409 from the precondition is not an error, as the issue asks. With BlockOnForeignController() alone, the conflict is still returned, so that option's behavior does not change.
• Status and metrics. A deleted resource does not contribute to the component condition and records no metric, so a kept object reports nothing misleading. The only "deleted" signal is the ResourceDeleted event, and it is recorded only when the delete happened.
• Scope limit. A cluster-scoped resource of a namespace-scoped owner never gets the owner reference, so the option never deletes it. Build cannot see the scope, so this is documented in the GoDoc and in docs/component.md and not rejected.

Testing

Unit tests in pkg/component/delete_test.go (fake client) cover: delete of an owned object with an event; delete through a non-controller owner reference; the UID and resourceVersion preconditions on the delete call; keep of an object with no owner reference (no Delete call, same resourceVersion, no event); keep of an object that only another owner references; an absent object; a conflict after the read (no error, object kept, no event); a non-conflict delete error still returned; and the combination with BlockOnForeignController(), both for the foreign-controller skip and for a conflict on the delete (no error, unlike BlockOnForeignController() alone). suspend_test.go covers a suspension where the cache shows the object owned and the API reader does not. I ran the new tests against the old deleteEntry first and they failed.

Envtest specs in pkg/component/delete_owned_test.go run each of GatedBy, DeleteWhen(true) and a disabled component feature gate three ways: an owned object is deleted, a user-created object is kept unchanged, and without the option the user-created object is still deleted. Three more specs cover suspension with DeleteOnSuspend(): an owned object is deleted, a user-created object keeps its resourceVersion while the component reports Suspended, the same holds when the desired object still carries the owner reference from an earlier apply, and an object disowned between the read and the suspension apply is not written or deleted. Option resolution tests cover the flag and the two build errors.

make all passes.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Suspension can adopt and then delete an initially unowned object despite the new protection.

1 open finding

• Delete-on-suspend claims and deletes pre-existing ownerless objects · New

What changed in this PR

Adds opt-in ownership checks to component deletion, protecting same-name user-created objects as requested in #233.

Changes:

• Adds DeleteOnlyIfOwned() with validation and guarded deletion.
• Shares owner-reference matching and adds unit and integration coverage.
• Updates documentation and plugin guidance.

File	Description
plugin/​skills/​structuring-operators/​SKILL.md	Adds gated-resource safety guidance.
plugin/​skills/​structuring-operators/​references/​guidelines.md	Syncs operator guidance.
plugin/​skills/​building-components/​SKILL.md	Documents the new option.
plugin/​skills/​building-components/​references/​component.md	Syncs option documentation.
pkg/​component/​suspend.go	Updates deletion commentary.
pkg/​component/​resource_options.go	Defines and validates the option.
pkg/​component/​resource_options_test.go	Tests resolution and invalid combinations.
pkg/​component/​orphan.go	Extracts shared ownership matching.
pkg/​component/​delete.go	Checks ownership and handles delete conflicts.
pkg/​component/​delete_test.go	Tests guarded deletion behavior.
pkg/​component/​delete_owned_test.go	Covers deletion triggers with integration tests.
docs/​guidelines.md	Explains safe gated deletion.
docs/​component.md	Documents behavior and limitations.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[sourcehawk]

Review round 1 (review base 687b35e, review effort: Balanced, from the **Review effort:** line of the review body).

Findings: one posted finding (delete-on-suspend adopts and then deletes an unowned object). It was correct and is fixed in b70ff47. The review had no suppressed comments.

Comment verdicts for this round (evaluator: a fresh agent that did not see the finding):

Declaration	Lines (base, now)	Sentence	Verdict	Source	Caller code	Judge
DeleteOnlyIfOwned (resource_options.go)	23, 26	"During suspension with DeleteOnSuspend, an object that this read finds without such an owner reference is not suspended either and counts as suspended."	KEEP	suspendResource: the read through APIReader or Client, then the referencesOwner check returns SuspensionStatusSuspended before Suspend() and the apply	The controller does not read Suspended as proof that the object was scaled down or deleted	evaluator
DeleteOnlyIfOwned		"Without this option or BlockOnForeignController, a deletion removes the object by name, whoever owns it."	KEEP (corrects a false base sentence that left out BlockOnForeignController)	deleteEntry: the unguarded Delete runs only when neither option is set	Whether to register the option on a name a user may also use	evaluator
DeleteOnlyIfOwned		An earlier draft, "...so the suspension apply never takes it over", was cut: the check read the cache. The code now reads through the API reader, and the sentence was narrowed.	CUT	rec.Client.Get in the first draft		evaluator
DeleteOnlyIfOwned		"The delete carries the observed UID and resourceVersion as preconditions."	Proposal: CUT (describes the body). Kept: the issue asks for the preconditions as a guarantee, and a caller relies on them to know that a replaced object is not deleted.			evaluator, proposal not applied

Comments with no flag: suspend.go (why the ownership read uses the API reader; why the ownership check runs before the apply), suspend_test.go and delete_owned_test.go (why the suspendable mock has no Suspend/Mutate expectations). Each holds a fact the code cannot show, so they stay.

Docs: docs/component.md now says that a suspension with DeleteOnSuspend() skips the apply for an unowned object, and that BlockOnForeignController() keeps an object another owner controls without this option. Plugin references are synced.

[Copilot]

🟡 Changes recommended

The suspension read can preserve stale owner references, allowing an ownerless replacement object to be adopted and deleted.

1 open finding

• Decode suspension ownership into an empty object · New

1 resolved since last review

• Delete-on-suspend claims and deletes pre-existing ownerless objects

🧠 Review effort: Balanced

[sourcehawk]

Review round 2 (review base b70ff47, review effort: Balanced, from the **Review effort:** line of the review body).

Findings: one posted finding (read the suspension ownership check into an empty object). Applied in 79b56dc. I could not reproduce the stale reference with envtest or the fake client, but the change matches observeController and removes the reliance on decoder behavior. The thread reply has the details. The review had no suppressed comments.

Comment verdicts for this round: the gate lists no flag. One ADDED comment in delete_owned_test.go explains why the desired object carries the owner reference while the live object does not. That setup intent is not visible from the code, so the comment stays. No doc comment and no doc page changed.

[Copilot]

🟡 Changes recommended

The suspension apply can still adopt and delete an object replaced after the ownership check.

1 open finding

• Bind suspension apply to ownership snapshot to prevent stale adoption · New

1 resolved since last review

• Decode suspension ownership into an empty object

🧠 Review effort: Balanced

[sourcehawk]

Review round 3 (review base 79b56dc, review effort: Balanced, from the **Review effort:** line of the review body).

Findings: one posted finding (bind the suspension apply to the ownership read). I first declined it. I then fixed it in d170fdf, because the race deletes the object the option exists to keep. The thread reply has the details. The review had no suppressed comments.

Comment verdicts (evaluator: a fresh agent that did not see the finding):

Declaration	Lines (base, now)	Sentence	Verdict	Source	Caller code	Judge
DeleteOnlyIfOwned	26, 29	Draft: "The suspension apply with DeleteOnSuspend is bound to the read the same way: ..."	CUT (names the mechanism, and "the same way" is loose: the apply carries only resourceVersion)			evaluator
DeleteOnlyIfOwned		"When the object changes between the read and the suspension apply with DeleteOnSuspend, the resource reports Suspending and is not written, and the reconcile does not fail."	KEEP	suspendResource conflict branch; withResourceVersion in applyResource; envtest spec	No error-driven requeue: the controller acts on the Suspending condition	evaluator
deleteResources	24, 24	"Returns the errors of all entries joined, or nil when no entry failed; an entry left in place is not an error."	Replaces a sentence that was partly false: skipped entries and conflicts also return nil	deleteEntry skip and conflict branches return false, nil	The caller does not read nil as "every object is gone"	evaluator (final gate against origin/main)

Comments with no flag: create.go applyConfig/applyOption, which follow the deleteConfig/deleteOption convention in delete.go; withResourceVersion, which states the conflict contract; and suspend.go, which says why the apply is bound to the read. They stay.

Proposals from the final gate against origin/main that I did not apply: the other deleteResources base sentences. They predate this PR, and no finding is about them.

[Copilot]

🟢 Approval recommended

Static review found no unresolved blocking issues, with regression coverage for ownership safeguards and conflict handling; tests were not run during this review.

0 open findings

1 resolved since last review

• Bind suspension apply to ownership snapshot to prevent stale adoption

🧠 Review effort: Balanced

[Copilot]

🟢 Approval recommended

Ownership safeguards, conflict handling, regression coverage, and documentation address the requested behavior with no unresolved blocking findings.

0 open findings

🧠 Review effort: Balanced