🐛 Stage AgentRuns inherit the parent AgentWorkflowRun's labels

[ibolton336]

Stage AgentRuns were created with a fixed three-key label map, so caller labels on the parent AgentWorkflowRun — e.g. konveyor.io/application per ADR 0006 — never reached the runs that actually execute, and label-selector queries silently missed every workflow stage run.

Per the issue's Option 1: the stage run now starts from a copy of the parent's labels, with the three controller-owned keys (app.kubernetes.io/managed-by, konveyor.io/agentworkflowrun, konveyor.io/stage) written last, so callers cannot override them and the parent's live map is never mutated. One rule: a label on the workflow run is a label on its stages.

The envtest spec covers propagation plus the spoofing case (a parent attempting to set the controller-owned keys loses).

Deliberately unchanged, flag if either should follow: the AlreadyExists adoption path does not re-sync labels when the parent's labels change between reconciles (this is create-time inheritance only), and the Sandbox/Secret children of an AgentRun keep their fixed label maps.

Fixes #107

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Stage runs now inherit labels from their parent workflow runs.
  • Controller-managed labels consistently take precedence over conflicting user-supplied values.
• Documentation
  • Added release documentation for label inheritance behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

The controller copies AgentWorkflowRun labels to stage AgentRun resources. Controller-owned labels override inherited values. Integration coverage verifies propagation, precedence, resource creation, and cleanup. An unreleased bugfix note documents the behavior.

Changes

Stage label inheritance

Layer / File(s)	Summary
Label propagation implementation internal/controller/agentworkflowrun_controller.go	createAgentRunForStage copies parent labels into a new map, then applies controller-owned labels.
Integration coverage and release note internal/controller/agentworkflowrun_controller_test.go, changes/unreleased/107-stage-label-inheritance.yaml	Tests use a shared stage name and verify inherited labels and controller-owned label precedence. The release note documents the bugfix.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• konveyor/agentic-controller#36: Both changes modify stage AgentRun creation and label assignment.
• konveyor/agentic-controller#80: This change extends stage AgentRun creation logic covered by that PR.
• konveyor/agentic-controller#105: This change implements the documented parent-label propagation gap.

Suggested reviewers: djzager

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title uses the required 🐛 prefix and clearly summarizes label inheritance for Stage AgentRuns.
Description check	✅ Passed	The description explains the problem, implementation, test coverage, linked issue, and deliberate scope boundaries.
Linked Issues check	✅ Passed	The implementation propagates all parent labels, preserves the parent map, and overwrites controller-owned keys as required by #107.
Out of Scope Changes check	✅ Passed	The code and tests remain focused on Stage AgentRun label inheritance and controller-owned label precedence.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: build linters: plugin(logcheck): plugin "logcheck" not found
The command is terminated due to an error: build linters: plugin(logcheck): plugin "logcheck" not found

---

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]

🧹 Nitpick comments (1)

internal/controller/agentworkflowrun_controller_test.go (1)

378-469: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Assert that parent labels remain unchanged.

The test verifies child-label propagation and controller-owned precedence. It does not verify the non-mutation contract. An implementation that overwrites pbRun.Labels could still pass this test. Assert that the original parent retains all supplied values, including the three spoofed controller-owned labels.

Proposed test assertions

 			Expect(stageRun.Labels).To(HaveKeyWithValue(labelManagedBy, managedByLabel))
 			Expect(stageRun.Labels).To(HaveKeyWithValue(labelAgentWorkflowRun, pbRunName))
 			Expect(stageRun.Labels).To(HaveKeyWithValue(labelStage, stageAName))
 
+			Expect(pbRun.Labels).To(HaveKeyWithValue("konveyor.io/application", "42"))
+			Expect(pbRun.Labels).To(HaveKeyWithValue("custom/foo", "bar"))
+			Expect(pbRun.Labels).To(HaveKeyWithValue(labelManagedBy, "spoofed-manager"))
+			Expect(pbRun.Labels).To(HaveKeyWithValue(labelAgentWorkflowRun, "spoofed-run"))
+			Expect(pbRun.Labels).To(HaveKeyWithValue(labelStage, "spoofed-stage"))
+
 			By("cleaning up")

🤖 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 `@internal/controller/agentworkflowrun_controller_test.go` around lines 378 -
469, Extend the “propagate parent labels” test around the created
AgentWorkflowRun to fetch the parent after reconciliation and assert its Labels
still contain the original values for application, custom/foo, labelManagedBy,
labelAgentWorkflowRun, and labelStage. Keep the existing child-label precedence
assertions unchanged, ensuring controller-owned values are overridden only on
the child and the parent’s supplied labels remain intact.

🤖 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.

Nitpick comments:
In `@internal/controller/agentworkflowrun_controller_test.go`:
- Around line 378-469: Extend the “propagate parent labels” test around the
created AgentWorkflowRun to fetch the parent after reconciliation and assert its
Labels still contain the original values for application, custom/foo,
labelManagedBy, labelAgentWorkflowRun, and labelStage. Keep the existing
child-label precedence assertions unchanged, ensuring controller-owned values
are overridden only on the child and the parent’s supplied labels remain intact.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ea04be64-c8a1-48e0-9805-98c7e8da87e0

📥 Commits

Reviewing files that changed from the base of the PR and between fc93e2e and 35a8a9a.

📒 Files selected for processing (2)

• internal/controller/agentworkflowrun_controller.go
• internal/controller/agentworkflowrun_controller_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/controller/agentworkflowrun_controller.go

[djzager]

Clean, minimal fix that does exactly what #107 asks for. The core logic is correct — maps.Copy followed by controller-owned key overwrites is the right pattern. Nil-safety is handled correctly (maps.Copy(dst, nil) is a documented no-op). Good scope discipline calling out what's deliberately unchanged in the PR description. Two minor comments below.

[djzager]

This is the first usage of maps.Copy in the codebase — the rest of the controller package uses inline map[string]string{...} literals or manual assignment. maps.Copy is the idiomatic Go 1.21+ way to do this. We should file an issue to update the rest of the codebase to use this pattern.

[djzager]

The test covers propagation (caller labels appear on the stage run) and spoofing (controller-owned keys win). One gap: there's no explicit assertion for the nil-labels case — when the parent AgentWorkflowRun has no labels. The existing sequential test creates a workflow run without explicit labels, implicitly covering this path, but doesn't assert Expect(stageARun.Labels).To(HaveLen(3)) to make the contract explicit. Not blocking — maps.Copy with nil is a no-op — but an explicit assertion would be stronger.