✨ Add AgentPlaybook and AgentPlaybookRun controllers#36
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesAgentPlaybook orchestration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentPlaybookRunReconciler
participant AgentPlaybook
participant AgentRun
participant AgentPlaybookRun
AgentPlaybookRunReconciler->>AgentPlaybook: read readiness and stages
AgentPlaybookRunReconciler->>AgentRun: create active stage
AgentRun-->>AgentPlaybookRunReconciler: return phase
AgentPlaybookRunReconciler->>AgentPlaybookRun: update stage and overall status
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
140c3d9 to
c48e8cc
Compare
djzager
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/controller/agentplaybook_controller_test.go (1)
43-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the Agent-change watch in this test.
After asserting
AgentsNotReady, create and ready the referenced Agent, then assert the existing playbook transitions toAllAgentsReady. Otherwise, the reverse watch/index could be broken while both current scenarios still pass.🤖 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/agentplaybook_controller_test.go` around lines 43 - 65, The test should exercise the Agent-change watch after the existing AgentsNotReady assertions: create the referenced Agent named “nonexistent-agent,” make it ready using the test’s established readiness setup, then poll the same playbook until its Ready condition becomes True with reason “AllAgentsReady.” Keep the existing missing-agent assertions and cleanup intact, and use the controller’s existing Agent fixture/status symbols rather than inventing a separate readiness path.internal/controller/agentplaybookrun_controller_test.go (2)
104-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the complete execution-input forwarding contract.
The playbook sets
Guide, but the resultingKONVEYOR_PLAYBOOK_INSTRUCTIONSenvironment variable is not asserted;EnvandEnvFromare not supplied at all. Add these inputs and verify them on the created AgentRuns.🤖 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/agentplaybookrun_controller_test.go` around lines 104 - 158, Extend the AgentPlaybookRun setup and assertions around stageAgentRunName to cover execution-input forwarding: define representative Env and EnvFrom values on the playbook run, then assert the created stage AgentRun preserves the playbook Guide as KONVEYOR_PLAYBOOK_INSTRUCTIONS and forwards both Env and EnvFrom unchanged. Apply the checks to the created AgentRuns while retaining the existing stage-specific spec assertions.
51-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-ready-to-ready playbook transition test.
The missing-playbook case is terminal, while the sequential case starts after readiness. Neither exercises the indexed AgentPlaybook watch that should resume a waiting run when its playbook becomes Ready.
🤖 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/agentplaybookrun_controller_test.go` around lines 51 - 75, Add a controller test alongside the missing-playbook case that creates an AgentPlaybookRun referencing an existing but initially non-ready AgentPlaybook, verifies the run remains waiting, then marks the playbook Ready and asserts the run resumes successfully through the indexed AgentPlaybook watch. Use the existing test helpers, status condition symbols, and cleanup pattern, and preserve the terminal missing-playbook test unchanged.
🤖 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.
Inline comments:
In `@internal/controller/agentplaybookrun_controller.go`:
- Around line 323-328: Update the AlreadyExists branch in the AgentRun creation
flow to fetch the existing object and validate that its controller-owner UID and
expected stage labels match the current run before returning agentRunName. If
ownership or labels do not match, return a collision error instead of accepting
the object; preserve the existing deterministic-name behavior for valid prior
creations.
- Around line 102-112: Pin the playbook revision and stage set when execution
begins, then reject any later playbook generation or stage changes before
initializing or advancing stage state. In the readiness check around
playbookReady, require ObservedGeneration to equal playbook.Generation; update
the initialization and findCurrentStageIndex flow to use the pinned stages and
prevent indexing beyond pbRun.Status.Stages.
- Around line 87-96: Set pbRun.Status.CompletionTime in the
errors.IsNotFound(err) branch of the reconciliation flow before calling
patchRunStatus, alongside the Failed phase and Ready condition updates, so this
terminal missing-playbook path records completion immediately.
- Around line 299-307: Update the AgentRun construction in the agent playbook
run reconciliation to use Kubernetes label-safe, hashed/truncated values for
pbRun.Name and stage.Name in labelAgentPlaybookRun and labelStage. Preserve the
full raw names in annotations on the AgentRun so they remain available without
violating label constraints.
---
Nitpick comments:
In `@internal/controller/agentplaybook_controller_test.go`:
- Around line 43-65: The test should exercise the Agent-change watch after the
existing AgentsNotReady assertions: create the referenced Agent named
“nonexistent-agent,” make it ready using the test’s established readiness setup,
then poll the same playbook until its Ready condition becomes True with reason
“AllAgentsReady.” Keep the existing missing-agent assertions and cleanup intact,
and use the controller’s existing Agent fixture/status symbols rather than
inventing a separate readiness path.
In `@internal/controller/agentplaybookrun_controller_test.go`:
- Around line 104-158: Extend the AgentPlaybookRun setup and assertions around
stageAgentRunName to cover execution-input forwarding: define representative Env
and EnvFrom values on the playbook run, then assert the created stage AgentRun
preserves the playbook Guide as KONVEYOR_PLAYBOOK_INSTRUCTIONS and forwards both
Env and EnvFrom unchanged. Apply the checks to the created AgentRuns while
retaining the existing stage-specific spec assertions.
- Around line 51-75: Add a controller test alongside the missing-playbook case
that creates an AgentPlaybookRun referencing an existing but initially non-ready
AgentPlaybook, verifies the run remains waiting, then marks the playbook Ready
and asserts the run resumes successfully through the indexed AgentPlaybook
watch. Use the existing test helpers, status condition symbols, and cleanup
pattern, and preserve the terminal missing-playbook test unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7b7678b-d105-411a-8a45-4139cad6663a
📒 Files selected for processing (10)
changes/unreleased/agentplaybook-controllers.yamlcmd/main.goconfig/rbac/role.yamlinternal/controller/agentplaybook_controller.gointernal/controller/agentplaybook_controller_test.gointernal/controller/agentplaybookrun_controller.gointernal/controller/agentplaybookrun_controller_test.gointernal/controller/agentrun_controller.gointernal/controller/doc.gointernal/controller/suite_test.go
| agentRun := &konveyoriov1alpha1.AgentRun{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: agentRunName, | ||
| Namespace: pbRun.Namespace, | ||
| Labels: map[string]string{ | ||
| labelManagedBy: managedByLabel, | ||
| labelAgentPlaybookRun: pbRun.Name, | ||
| labelStage: stage.Name, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 \
'labelAgentPlaybookRun|labelStage|MaxLength|Pattern|AgentPlaybookStage' \
internal/controller/agentplaybookrun_controller.go \
api/v1alpha1/agentplaybook_types.goRepository: konveyor/agentic-controller
Length of output: 2377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '260,340p' internal/controller/agentplaybookrun_controller.go
printf '\n---\n'
sed -n '1,140p' api/v1alpha1/agentplaybook_types.go
printf '\n---\n'
rg -n -C3 'labelAgentPlaybookRun|labelStage|annotations|Annotation|labels' internal/controller/agentplaybookrun_controller.go api/v1alpha1/agentplaybook_types.goRepository: konveyor/agentic-controller
Length of output: 7103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'labelAgentPlaybookRun|labelStage|labelManagedBy|managedByLabel|sanitizeVolumeName|Annotations|annotation' internal apiRepository: konveyor/agentic-controller
Length of output: 6358
Use label-safe values for AgentRun labels. pbRun.Name and stage.Name can exceed Kubernetes label limits or include invalid characters, which can make AgentRun creation fail for otherwise valid playbooks. Hash/truncate the label values and keep the raw names in annotations.
🤖 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/agentplaybookrun_controller.go` around lines 299 - 307,
Update the AgentRun construction in the agent playbook run reconciliation to use
Kubernetes label-safe, hashed/truncated values for pbRun.Name and stage.Name in
labelAgentPlaybookRun and labelStage. Preserve the full raw names in annotations
on the AgentRun so they remain available without violating label constraints.
There was a problem hiding this comment.
Fixed in 626eaf7 — added +kubebuilder:validation:MaxLength=63 and +kubebuilder:validation:Pattern to AgentPlaybookStage.Name so it's constrained to valid Kubernetes label values at admission time. CRD regenerated.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '286,325p' internal/controller/agentplaybookrun_controller.go
printf '\n--- AgentPlaybookStage validation ---\n'
rg -n -A12 -B4 'type AgentPlaybookStage struct|Name\s+string' api/v1alpha1/agentplaybook_types.go
printf '\n--- AgentPlaybookRun naming/schema validation ---\n'
rg -n -A20 -B4 'type AgentPlaybookRun struct|type AgentPlaybookRunSpec struct|metadata:|maxLength: 63|pattern:' \
api/v1alpha1/agentplaybookrun_types.go config/crd/bases/konveyor.io_agentplaybookruns.yaml 2>/dev/null || trueLength of output: 20157
@djzager, the stage-name validation resolves the labelStage half of this issue—thanks.
labelAgentPlaybookRun still uses the raw pbRun.Name, though. AgentPlaybookRun.metadata.name is not limited to 63 characters, so a valid long resource name can still cause child AgentRun creation to fail label validation. Please also derive a label-safe, truncated/hashed value for pbRun.Name (retaining the full name in an annotation if it is needed for lookup).
There was a problem hiding this comment.
Valid point about pbRun.Name exceeding 63 chars. In practice K8s object names are max 253 chars but label values are max 63 — same issue exists for labelAgentRun in the existing AgentRun controller. Accepting this as a known limitation for the POC rather than adding truncation/hashing across all controllers in this PR.
There was a problem hiding this comment.
@djzager, understood—thanks for clarifying the POC scope. I’ll leave this as an accepted known limitation for this PR, given the same raw-name label pattern already exists in the AgentRun controller.
Would you like me to open a follow-up GitHub issue to standardize label-safe naming across both controllers?
✏️ Learnings added
Learnt from: djzager
Repo: konveyor/agentic-controller PR: 36
File: internal/controller/agentplaybookrun_controller.go:301-309
Timestamp: 2026-07-16T15:42:37.054Z
Learning: For the AgentPlaybook orchestration POC, raw Kubernetes resource names may be used as label values even though names can exceed the 63-character label-value limit. This known limitation also exists in the existing AgentRun controller (`labelAgentRun`) and is intentionally deferred rather than addressed within PR `#36`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@api/v1alpha1/agentplaybook_types.go`:
- Around line 27-32: The stage-name validation pattern in
api/v1alpha1/agentplaybook_types.go must match the intended Kubernetes
label-value contract: either broaden it to permit uppercase letters and
underscores, or explicitly document and retain the lowercase-only policy.
Regenerate the CRD so the corresponding validation in
config/crd/bases/konveyor.io_agentplaybooks.yaml is synchronized with the source
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff47920f-5349-406a-bae7-905e63eeb1ee
📒 Files selected for processing (3)
api/v1alpha1/agentplaybook_types.goconfig/crd/bases/konveyor.io_agentplaybooks.yamlinternal/controller/agentplaybookrun_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/agentplaybookrun_controller.go
626eaf7 to
acda72a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/controller/agentplaybookrun_controller_test.go (3)
64-71: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the missing-playbook completion timestamp.
This test does not cover the newly added terminal timestamp, so a regression could pass unnoticed.
g.Expect(fetched.Status.Phase).To(Equal(konveyoriov1alpha1.AgentRunPhaseFailed)) +g.Expect(fetched.Status.CompletionTime).NotTo(BeNil())🤖 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/agentplaybookrun_controller_test.go` around lines 64 - 71, Extend the Eventually assertions for the missing-playbook case around fetched.Status to verify the terminal completion timestamp is populated. Keep the existing failed phase, Ready condition, and PlaybookNotFound reason assertions unchanged, and use the status timestamp field introduced by the controller.
133-143: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover deterministic-name collisions with a foreign AgentRun.
Pre-create
expectedStageANamewithout the playbook run’s controller owner reference, then assert the controller’s documented rejection behavior. This protects the new ownership check from regressions that could adopt an unrelated resource.🤖 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/agentplaybookrun_controller_test.go` around lines 133 - 143, Extend the test around stageAgentRunName and the playbook-run reconciliation to pre-create an AgentRun named expectedStageAName without the playbook run’s controller owner reference, then assert the controller rejects the collision according to its documented behavior instead of adopting the foreign resource. Keep the existing deterministic-name assertions for the non-collision path intact.
121-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the complete run-configuration forwarding contract.
Add
EnvandEnvFrominputs and assert them onstageARun; also verify the model’sProviderandModel, not only its role. Otherwise those fields can be dropped or miswired without failing this test.🤖 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/agentplaybookrun_controller_test.go` around lines 121 - 158, Add Env and EnvFrom values to the AgentPlaybookRun Spec in the test setup, then extend the stageARun assertions to verify both fields are forwarded unchanged. Also assert stageARun.Spec.Models[0].Provider and Model alongside the existing Role check, covering the complete run-configuration forwarding contract.
🤖 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.
Inline comments:
In `@internal/controller/agentplaybookrun_controller_test.go`:
- Around line 219-223: Update the successful cleanup path in the test to list
and delete all labeled child AgentRuns before deleting pbRun, reusing the helper
already used by the failure test. Keep the existing playbook and agent cleanup
afterward.
- Around line 170-176: Replace the direct AgentRun status updates at
internal/controller/agentplaybookrun_controller_test.go:170-176, 197-204, and
283-294 with a shared small helper that re-fetches the latest object and retries
Status().Update on conflicts. Use the helper for all three stage success/failure
transitions while preserving their existing status mutations and conditions.
---
Nitpick comments:
In `@internal/controller/agentplaybookrun_controller_test.go`:
- Around line 64-71: Extend the Eventually assertions for the missing-playbook
case around fetched.Status to verify the terminal completion timestamp is
populated. Keep the existing failed phase, Ready condition, and PlaybookNotFound
reason assertions unchanged, and use the status timestamp field introduced by
the controller.
- Around line 133-143: Extend the test around stageAgentRunName and the
playbook-run reconciliation to pre-create an AgentRun named expectedStageAName
without the playbook run’s controller owner reference, then assert the
controller rejects the collision according to its documented behavior instead of
adopting the foreign resource. Keep the existing deterministic-name assertions
for the non-collision path intact.
- Around line 121-158: Add Env and EnvFrom values to the AgentPlaybookRun Spec
in the test setup, then extend the stageARun assertions to verify both fields
are forwarded unchanged. Also assert stageARun.Spec.Models[0].Provider and Model
alongside the existing Role check, covering the complete run-configuration
forwarding contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ada0dc8-a2a4-4266-8bd9-f127a183d355
📒 Files selected for processing (12)
api/v1alpha1/agentplaybook_types.gochanges/unreleased/agentplaybook-controllers.yamlcmd/main.goconfig/crd/bases/konveyor.io_agentplaybooks.yamlconfig/rbac/role.yamlinternal/controller/agentplaybook_controller.gointernal/controller/agentplaybook_controller_test.gointernal/controller/agentplaybookrun_controller.gointernal/controller/agentplaybookrun_controller_test.gointernal/controller/agentrun_controller.gointernal/controller/doc.gointernal/controller/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/controller/doc.go
- changes/unreleased/agentplaybook-controllers.yaml
- cmd/main.go
- config/rbac/role.yaml
- api/v1alpha1/agentplaybook_types.go
- config/crd/bases/konveyor.io_agentplaybooks.yaml
- internal/controller/agentplaybook_controller.go
- internal/controller/agentrun_controller.go
- internal/controller/suite_test.go
- internal/controller/agentplaybookrun_controller.go
97b416c to
370aa56
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/controller/agentplaybookrun_controller_test.go (2)
170-176: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake these AgentRun status writes retry on conflict.
A one-shot
Status().Updatecan race with the live reconciler inenvtest, causing these stage success/failure transitions to flake. Introduce a small helper that re-fetches the latest object and retries the status update, and apply it at all three sites.
internal/controller/agentplaybookrun_controller_test.go#L170-L176: Replace the one-shot status update forstageARunwith a retryable helper orEventuallyblock.internal/controller/agentplaybookrun_controller_test.go#L197-L204: Apply the same retryable update forstageBRun.internal/controller/agentplaybookrun_controller_test.go#L283-L294: Apply the same retryable update forstageRunin the failure path.🛠️ Proposed helper and usage
You can define a small helper function at the package or suite level, or inside the test, for example:
func updateAgentRunStatus(name string, updateFn func(*konveyoriov1alpha1.AgentRun)) { EventuallyWithOffset(1, func(g Gomega) { var run konveyoriov1alpha1.AgentRun g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: name, Namespace: testNamespace}, &run)).To(Succeed()) updateFn(&run) g.Expect(k8sClient.Status().Update(ctx, &run)).To(Succeed()) }, timeout, interval).Should(Succeed()) }Then replace the direct updates with calls to this helper. For example, for stage A:
By("simulating stage-a AgentRun success") updateAgentRunStatus(stageARunName, func(run *konveyoriov1alpha1.AgentRun) { run.Status.Phase = konveyoriov1alpha1.AgentRunPhaseSucceeded meta.SetStatusCondition(&run.Status.Conditions, metav1.Condition{ Type: ConditionTypeReady, Status: metav1.ConditionTrue, Reason: reasonSucceeded, }) })🤖 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/agentplaybookrun_controller_test.go` around lines 170 - 176, Make AgentRun status updates retryable by adding a small helper that fetches the latest object, applies the status mutation, and retries Status().Update until it succeeds. Replace the direct updates for stageARun at internal/controller/agentplaybookrun_controller_test.go:170-176, stageBRun at internal/controller/agentplaybookrun_controller_test.go:197-204, and stageRun at internal/controller/agentplaybookrun_controller_test.go:283-294 with this helper, preserving each success or failure condition.
219-223: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDelete owned AgentRuns on the successful path too.
The failure test explicitly removes child AgentRuns to avoid GC issues, but this successful execution path leaves both stage runs behind. Reusing the list-and-delete logic prevents potential resource leakage or cross-test pollution in envtest.
🧹 Proposed fix
By("cleaning up") + var runList konveyoriov1alpha1.AgentRunList + Expect(k8sClient.List(ctx, &runList, + client.InNamespace(testNamespace), + client.MatchingLabels{labelAgentPlaybookRun: pbRunName}, + )).To(Succeed()) + for i := range runList.Items { + Expect(k8sClient.Delete(ctx, &runList.Items[i])).To(Succeed()) + } Expect(k8sClient.Delete(ctx, pbRun)).To(Succeed()) Expect(k8sClient.Delete(ctx, playbook)).To(Succeed()) Expect(k8sClient.Delete(ctx, agent)).To(Succeed())🤖 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/agentplaybookrun_controller_test.go` around lines 219 - 223, Update the successful cleanup block in the relevant AgentPlaybookRun test to list and delete all owned AgentRuns, reusing the failure test’s existing list-and-delete logic before deleting pbRun, playbook, and agent. Ensure both stage runs are removed to prevent leftover resources between tests.
🤖 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.
Duplicate comments:
In `@internal/controller/agentplaybookrun_controller_test.go`:
- Around line 170-176: Make AgentRun status updates retryable by adding a small
helper that fetches the latest object, applies the status mutation, and retries
Status().Update until it succeeds. Replace the direct updates for stageARun at
internal/controller/agentplaybookrun_controller_test.go:170-176, stageBRun at
internal/controller/agentplaybookrun_controller_test.go:197-204, and stageRun at
internal/controller/agentplaybookrun_controller_test.go:283-294 with this
helper, preserving each success or failure condition.
- Around line 219-223: Update the successful cleanup block in the relevant
AgentPlaybookRun test to list and delete all owned AgentRuns, reusing the
failure test’s existing list-and-delete logic before deleting pbRun, playbook,
and agent. Ensure both stage runs are removed to prevent leftover resources
between tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30cb9395-9955-4f66-8cb1-3611b1b5956c
📒 Files selected for processing (12)
api/v1alpha1/agentplaybook_types.gochanges/unreleased/agentplaybook-controllers.yamlcmd/main.goconfig/crd/bases/konveyor.io_agentplaybooks.yamlconfig/rbac/role.yamlinternal/controller/agentplaybook_controller.gointernal/controller/agentplaybook_controller_test.gointernal/controller/agentplaybookrun_controller.gointernal/controller/agentplaybookrun_controller_test.gointernal/controller/agentrun_controller.gointernal/controller/doc.gointernal/controller/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- changes/unreleased/agentplaybook-controllers.yaml
- internal/controller/doc.go
- config/rbac/role.yaml
- config/crd/bases/konveyor.io_agentplaybooks.yaml
- api/v1alpha1/agentplaybook_types.go
- internal/controller/suite_test.go
- cmd/main.go
- internal/controller/agentplaybook_controller.go
- internal/controller/agentrun_controller.go
- internal/controller/agentplaybookrun_controller.go
Implement the two remaining controllers from ADR 0001: AgentPlaybook controller: - Validates all stages' referenced Agents exist and are Ready - Watches Agent changes via field index for re-reconciliation - Sets Ready condition on the playbook AgentPlaybookRun controller: - Sequential stage execution: creates one AgentRun per stage - Uses generateName for AgentRun naming with labels for discoverability - AlreadyExists handling for idempotency on status-patch conflicts - Forwards params, models, env, envFrom from playbook run to each stage's AgentRun - Passes playbook Guide as KONVEYOR_PLAYBOOK_INSTRUCTIONS env var (harness decides how to present it to the agent runtime) - Watches owned AgentRuns for status changes - Watches AgentPlaybook for readiness changes (unblocks runs waiting on PlaybookNotReady) - Tracks per-stage status with phase and AgentRun name - Uses snapshotted stages in status as source of truth to prevent out-of-bounds panics if playbook stages are modified mid-run - Fails the entire playbook run if any stage fails Also improves the sandboxFinishedReasonSucceeded constant comment to note it must match Agent Sandbox's SandboxReasonPodSucceeded. Signed-off-by: David Zager <david.j.zager@gmail.com>
370aa56 to
2075299
Compare
ibolton336
left a comment
ibolton336
left a comment
There was a problem hiding this comment.
Nice work — the controller code is idiomatic and follows the repo's existing patterns (patch-from-deep-copy status updates, field-index reverse lookups, deterministic child naming with AlreadyExists + owner-UID verification). The watch wiring looks correct: Owns(AgentRun) drives stage advancement, and the playbook watch unblocks runs stuck on PlaybookNotReady. Requesting changes for one correctness gap; the rest are suggestions.
Blocking
Duplicate stage names aren't rejected, and they break execution silently. AgentPlaybookSpec.Stages only has MinItems=1 — nothing enforces the "unique within the playbook" doc comment on AgentPlaybookStage.Name. With two stages named build, both map to the same deterministic AgentRun name (<run>-build); the second stage's create hits AlreadyExists, passes the ownership check (same owner UID), and instantly "succeeds" by adopting the first stage's AgentRun — so the second stage never actually runs. A CEL rule on the stage list (or +listType=map with name as the key) closes this.
Related edge case: stage names a.b and a-b are distinct per the validation pattern but sanitize to the same AgentRun name, with the same silent-adoption result.
Suggestions
-
The "snapshot" comment overpromises. The comment in
Reconcilesays the run "executes the stages that were captured at initialization time," but only stage names and order are snapshotted into status —agentRefandinstructionsare re-read live from the playbook on every reconcile, so editing a playbook mid-run changes what later stages execute. Either snapshot the full stage spec into status or adjust the comment to document the actual behavior. -
Cross-run name collisions loop forever instead of failing terminally. PlaybookRun
foowith stagebar-bazand playbook runfoo-barwith stagebazboth produce AgentRunfoo-bar-baz. The not-owned branch increateAgentRunForStagereturns an error, which requeues with backoff indefinitely — the losing run is permanently wedged inAgentRunCreationFailedbut never reachesPhase=Failed. Since the name is deterministic, the error can never self-heal; it should be a terminal failure rather than a retry. -
The description's headline bugfix isn't in the diff.
mainalready hassandboxFinishedReasonSucceeded = "PodSucceeded"— this branch only changes the comment above it. Worth dropping the claim from the PR description and the changelog fragment so the release notes don't advertise a fix that shipped elsewhere. -
Guideenv var is never asserted in tests. Both tests setGuide, and the sequential test inspects the stage AgentRun spec in detail (params, models, labels), but nothing checks thatKONVEYOR_PLAYBOOK_INSTRUCTIONSlanded inSpec.Env. One-line assertion worth adding given it's a headline feature. (Also untested: thePlaybookNotReady→ Ready unblock path,StageNotFound, andAgentRunDeleted.)
Minor
- If
pbRun.Spec.Envalso definesKONVEYOR_PLAYBOOK_INSTRUCTIONS, both entries end up in the AgentRun env (user's entry last). Probably fine as an intentional override hook, but worth a comment. - When the playbook goes not-Ready mid-run, the
PlaybookNotReadycondition overwritesStageRunningwhile the current stage's AgentRun is still executing — pause/resume works via the watch, but the condition momentarily misrepresents an in-flight stage. Cosmetic.
Read-only slice of konveyor#50. Shim serves agentplaybooks/agentplaybookruns through the generic READ_ONLY dispatch; agentic-client gains the AgentPlaybookRun contract types (no duration field — computed from timestamps; Ready=False/StageRunning documented as the healthy running state) and list/get methods. UI adds an Agent runs / Playbook runs toggle, a playbook-runs list (phase, x/y stages, computed duration), and a detail page with a ProgressStepper stage ladder where each stage links into the existing RunDetailPage via status.stages[].agentRunName (never recomputed — names hash-truncate past 63 chars). Stage-owned AgentRuns in the flat runs list get a parent-playbook badge and a disabled Delete (deleting one terminally fails the parent, konveyor#36). Verified against a live 3-stage playbook run (upgrade-run-1) on minikube with the merged konveyor#36 controllers. Refs konveyor#50. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Implements the two remaining controllers from ADR 0001 for sequential multi-stage agent orchestration.
What this does
AgentPlaybook controller — validates that all stages' referenced Agents exist and are Ready. Watches Agent changes via field index for automatic re-reconciliation. Sets a
Readycondition on the playbook.AgentPlaybookRun controller — orchestrates sequential stage execution:
<playbookrun>-<stage>) withAlreadyExistshandling for idempotency on status-patch conflicts (follows the Tekton pattern)params,models,env,envFromfrom the playbook run spec to each stage's AgentRunGuidefield asKONVEYOR_PLAYBOOK_INSTRUCTIONSenv var — the harness decides how to present it alongside the Agent prompt and stage instructionsPlaybookNotReady)Pending,Running,Succeeded,Failed) with the AgentRun nameBugfix included
Fixes the Sandbox finished-reason constant:
"Succeeded"→"PodSucceeded"to match Agent Sandbox v0.5.0'sSandboxReasonPodSucceeded. Without this fix, successful AgentRuns would be incorrectly marked as failed.Known gaps (intentional for POC)
Testing
Ready=False; all Agents Ready →Ready=TruePhase=Failed; sequential two-stage execution with simulated success → advances stages →Phase=Succeeded; stage failure → entire run fails, subsequent stages never startedmake test: 78% coverage,go vetcleanDesign context
This controller is deliberately simple — sequential execution with opaque param forwarding. The architecture preserves optionality for:
Closes #42
Summary by CodeRabbit
AgentPlaybookreadiness tracking based on referenced stage Agents.AgentPlaybookRunorchestration for ordered, stage-by-stage execution with forwarded models/params/env/envFrom and playbook guidance.AgentRunnaming and end-to-end reporting of run/stage status.PodSucceededas success for compatibility.AgentPlaybookCRD validation for stage names (length + label-compatible pattern).