OLS-3794: Operator batch sandbox execution model - #435
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@blublinsky: This pull request references OLS-3794 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe controller replaces HTTP agent calls with batch sandboxes. ChangesBatch sandbox execution
Sequence Diagram(s)sequenceDiagram
participant AgentCaller
participant SandboxManager
participant SandboxPod
participant ResultCR
participant PodEventHandler
AgentCaller->>SandboxManager: Create run step with query and context
SandboxManager->>SandboxPod: Create pod with input ConfigMap
SandboxPod->>ResultCR: Write completed result
SandboxPod->>PodEventHandler: Emit pod event
PodEventHandler->>ResultCR: Read completed result
PodEventHandler->>SandboxManager: Release run and step
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
|
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
controller/agenticrun/sandbox_step_fsm_test.go (1)
123-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for
PodSucceededpast the timeout.The table has "running past timeout" but no case where the pod finished before the deadline passed. That gap hides the ordering defect flagged in
controller/agenticrun/sandbox_step_fsm.golines 144-167. Add cases forPodSucceededandPodFailedwithPodCreatedAtolder thanTimeout, and assert the grace and termination-message outcomes.🤖 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 `@controller/agenticrun/sandbox_step_fsm_test.go` around lines 123 - 135, Extend the sandbox step FSM table tests near the existing “pod running past timeout” case with PodSucceeded and PodFailed inputs whose PodCreatedAt precedes the configured Timeout, asserting the expected grace and termination-message actions/reasons. Use the existing sandboxStepInput, sandboxStepTimeout, and termination-message outcome symbols to verify completed pods follow the intended ordering.
🤖 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 @.ai/spec/how/reconciler.md:
- Line 110: Update the Release behavior description to state that the input
ConfigMap named after the AgenticRun UID is deleted during cleanup for each of
the four steps, preventing payload reuse across steps; alternatively, explicitly
document that ConfigMap creation performs an upsert.
In @.ai/spec/what/sandbox-execution.md:
- Line 13: The ConfigMap placement and ownership requirements in rule 7 conflict
with the cross-namespace constraints in rules 2 and 21. Resolve this by choosing
one consistent model: place the ConfigMap in the AgenticRun namespace so its
owner reference is valid, or keep it in the operator namespace and replace the
owner reference with labels plus explicit finalizer-driven cleanup; update rule
7 and any dependent cleanup requirements accordingly.
In `@controller/agenticrun/input_configmap.go`:
- Around line 43-52: Remove the cross-namespace OwnerReferences assignment in
input_configmap.go’s ConfigMap construction and rely on labels with explicit
cleanup, matching the per-run ServiceAccount pattern. Update
.ai/spec/what/sandbox-execution.md rule 7 to permit this approach and align with
rules 2 and 21. In reconciler.go, replace Owns calls for Pods and ConfigMaps
with Watches mappings that read the run name and namespace labels and enqueue
the corresponding AgenticRun.
- Around line 43-52: Remove the OwnerReferences assignment from
buildInputConfigMap so the ConfigMap does not reference a potentially
cross-namespace AgenticRun. Retain the existing labels and metadata, relying on
labels and explicit cleanup as with the per-run ServiceAccount pattern.
In `@controller/agenticrun/sandbox_agent.go`:
- Around line 251-257: Ensure each step replaces the run-scoped input ConfigMap
before sandbox creation, rather than reusing stale data. Update the step cleanup
flow around buildInputConfigMap and SandboxManager.createInputConfigMap to
delete the existing ConfigMap before the next step, or update its contents when
AlreadyExists occurs, while preserving the current step-specific query,
output-schema, and result-template values.
In `@controller/agenticrun/sandbox_manager.go`:
- Around line 116-123: Use an immutable input ConfigMap name in
createInputConfigMap that incorporates the run UID, step, and retry index, so
AlreadyExists cannot reuse another step’s payload; ensure final cleanup deletes
all run-owned input ConfigMaps. Update
controller/agenticrun/input_configmap_test.go:27-33 to expect the
step/retry-qualified name, and update
controller/agenticrun/sandbox_manager_test.go:204-220 with a regression test
verifying distinct step inputs reference distinct ConfigMaps.
- Around line 432-435: Update the cleanup flow around releaseErr and
releaseInputConfigMap in the sandbox release method so input ConfigMap deletion
runs only when sandbox release succeeds. Return releaseErr immediately when it
is non-nil, and invoke releaseInputConfigMap only afterward, preserving its
existing error handling for successful releases.
In `@controller/agenticrun/sandbox_step_fsm.go`:
- Around line 144-167: Move the timedOut(in) check out of the pre-switch path
and into the switch default arm in the sandbox step decision function. Ensure
PodSucceeded continues through decideSucceededGrace and PodFailed preserves its
termination message or exit-code message, while timeout handling applies only to
non-terminal phases such as Pending, Running, or Unknown.
- Around line 223-231: Update the container-status check around the existing
Waiting-state switch to iterate pod.Status.ContainerStatuses and
pod.Status.InitContainerStatuses in separate loops, removing the append
expression so cached Pod status slices are never mutated. Preserve the existing
nil Waiting-state filtering and ImagePullBackOff/ErrImagePull return behavior in
both iterations.
---
Nitpick comments:
In `@controller/agenticrun/sandbox_step_fsm_test.go`:
- Around line 123-135: Extend the sandbox step FSM table tests near the existing
“pod running past timeout” case with PodSucceeded and PodFailed inputs whose
PodCreatedAt precedes the configured Timeout, asserting the expected grace and
termination-message actions/reasons. Use the existing sandboxStepInput,
sandboxStepTimeout, and termination-message outcome symbols to verify completed
pods follow the intended ordering.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c9c746d-1e91-4abd-a65b-f06785dd6e96
📒 Files selected for processing (13)
.ai/spec/how/reconciler.md.ai/spec/what/sandbox-execution.mdcontroller/agenticrun/input_configmap.gocontroller/agenticrun/input_configmap_test.gocontroller/agenticrun/podspec_builder.gocontroller/agenticrun/podspec_builder_test.gocontroller/agenticrun/reconciler.gocontroller/agenticrun/sandbox_agent.gocontroller/agenticrun/sandbox_agent_test.gocontroller/agenticrun/sandbox_manager.gocontroller/agenticrun/sandbox_manager_test.gocontroller/agenticrun/sandbox_step_fsm.gocontroller/agenticrun/sandbox_step_fsm_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
| if timedOut(in) { | ||
| return sandboxStepDecision{ | ||
| Action: sandboxStepTimeout, | ||
| Reason: ReasonSandboxTimeout, | ||
| Message: fmt.Sprintf("sandbox exceeded timeout %s", in.Timeout), | ||
| } | ||
| } | ||
|
|
||
| switch in.PodPhase { | ||
| case corev1.PodSucceeded: | ||
| return decideSucceededGrace(in) | ||
| case corev1.PodFailed: | ||
| msg := in.TerminatedMessage | ||
| if msg == "" { | ||
| msg = "sandbox pod failed" | ||
| if in.TerminatedExitCode != nil { | ||
| msg = fmt.Sprintf("sandbox pod failed (exit %d)", *in.TerminatedExitCode) | ||
| } | ||
| } | ||
| return sandboxStepDecision{ | ||
| Action: sandboxStepFail, | ||
| Reason: ReasonSandboxFailed, | ||
| Message: msg, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move the timeout check after the terminal Pod phases.
timedOut(in) runs before the PodSucceeded and PodFailed arms. A pod that already finished loses its real outcome once PodCreatedAt + Timeout passes:
PodSucceededwith a late Result CR returnssandboxStepTimeoutinstead of entering the grace path (rules 42c, 43d).PodFailedreturns"sandbox exceeded timeout"instead of the termination message fromTerminatedMessage/TerminatedExitCode(rule 43e).
Spec rule 42 states that earlier failure signals take precedence over the full per-step timeout. Evaluate the timeout only for non-terminal phases.
🐛 Proposed fix: check the timeout only for Pending/Running/Unknown
- if timedOut(in) {
- return sandboxStepDecision{
- Action: sandboxStepTimeout,
- Reason: ReasonSandboxTimeout,
- Message: fmt.Sprintf("sandbox exceeded timeout %s", in.Timeout),
- }
- }
-
switch in.PodPhase {
case corev1.PodSucceeded:
return decideSucceededGrace(in)
case corev1.PodFailed:
msg := in.TerminatedMessage
if msg == "" {
msg = "sandbox pod failed"
if in.TerminatedExitCode != nil {
msg = fmt.Sprintf("sandbox pod failed (exit %d)", *in.TerminatedExitCode)
}
}
return sandboxStepDecision{
Action: sandboxStepFail,
Reason: ReasonSandboxFailed,
Message: msg,
}
case corev1.PodPending, corev1.PodRunning, corev1.PodUnknown, "":
+ if timedOut(in) {
+ return sandboxStepDecision{
+ Action: sandboxStepTimeout,
+ Reason: ReasonSandboxTimeout,
+ Message: fmt.Sprintf("sandbox exceeded timeout %s", in.Timeout),
+ }
+ }
reason := ReasonRunningAdd the same guard to the default arm.
🤖 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 `@controller/agenticrun/sandbox_step_fsm.go` around lines 144 - 167, Move the
timedOut(in) check out of the pre-switch path and into the switch default arm in
the sandbox step decision function. Ensure PodSucceeded continues through
decideSucceededGrace and PodFailed preserves its termination message or
exit-code message, while timeout handling applies only to non-terminal phases
such as Pending, Running, or Unknown.
| for _, st := range append(pod.Status.ContainerStatuses, pod.Status.InitContainerStatuses...) { | ||
| if st.State.Waiting == nil { | ||
| continue | ||
| } | ||
| switch st.State.Waiting.Reason { | ||
| case "ImagePullBackOff", "ErrImagePull": | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not append to a slice owned by the cached Pod status.
append(pod.Status.ContainerStatuses, pod.Status.InitContainerStatuses...) writes into the ContainerStatuses backing array whenever that slice has spare capacity. pod normally comes from the controller-runtime informer cache, and mutation of cached objects corrupts state for every other reader.
Iterate the two slices separately.
🔒 Proposed fix: iterate both slices without appending
- for _, st := range append(pod.Status.ContainerStatuses, pod.Status.InitContainerStatuses...) {
- if st.State.Waiting == nil {
- continue
- }
- switch st.State.Waiting.Reason {
- case "ImagePullBackOff", "ErrImagePull":
- return true
- }
- }
- return false
+ for _, group := range [][]corev1.ContainerStatus{
+ pod.Status.ContainerStatuses,
+ pod.Status.InitContainerStatuses,
+ } {
+ for _, st := range group {
+ if st.State.Waiting == nil {
+ continue
+ }
+ switch st.State.Waiting.Reason {
+ case "ImagePullBackOff", "ErrImagePull":
+ return true
+ }
+ }
+ }
+ return false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, st := range append(pod.Status.ContainerStatuses, pod.Status.InitContainerStatuses...) { | |
| if st.State.Waiting == nil { | |
| continue | |
| } | |
| switch st.State.Waiting.Reason { | |
| case "ImagePullBackOff", "ErrImagePull": | |
| return true | |
| } | |
| } | |
| for _, group := range [][]corev1.ContainerStatus{ | |
| pod.Status.ContainerStatuses, | |
| pod.Status.InitContainerStatuses, | |
| } { | |
| for _, st := range group { | |
| if st.State.Waiting == nil { | |
| continue | |
| } | |
| switch st.State.Waiting.Reason { | |
| case "ImagePullBackOff", "ErrImagePull": | |
| return true | |
| } | |
| } | |
| } | |
| return false |
🤖 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 `@controller/agenticrun/sandbox_step_fsm.go` around lines 223 - 231, Update the
container-status check around the existing Waiting-state switch to iterate
pod.Status.ContainerStatuses and pod.Status.InitContainerStatuses in separate
loops, removing the append expression so cached Pod status slices are never
mutated. Preserve the existing nil Waiting-state filtering and
ImagePullBackOff/ErrImagePull return behavior in both iterations.
fd3f368 to
3ab4a9c
Compare
openshift-ci
Bot
added
the
needs-rebase
|
/remove hold |
3ab4a9c to
0d5ba09
Compare
openshift-ci
Bot
removed
the
needs-rebase
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
docs/architecture-redesign-spec.html-641-643 (1)
641-643: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the collapsed lists in the Markdown source, then regenerate this HTML.
Several bullet and numbered lists render as one paragraph with literal
-and digit characters instead of<ul>/<ol>:
- Line 641-643: the "At scale" proposal-count list.
- Lines 918-921: the gateway crash behavior list.
- Lines 1140-1147: the multi-SDK divergence list.
- Lines 1226-1231: the "Relationship to batch model" list.
- Lines 1288-1299: the Phase A and Phase B numbered steps.
- Lines 1401-1403: the approval experience list.
The cause is in
docs/architecture-redesign-spec.md: each of these lists starts on the line immediately after a paragraph with no blank line between. Pandoc treats the whole block as one paragraph. Add a blank line before each list in the Markdown, then regenerate the HTML.📝 Example fix in the Markdown source
-At scale: -- 100 proposals: 5–20 MB (negligible) +At scale: + +- 100 proposals: 5–20 MB (negligible) - 1,000 proposals: 50–200 MB (noticeable on shared clusters) - 5,000 proposals: 250 MB – 1 GB (approaching etcd default 2 GB limit)Apply the same blank-line insertion at
docs/architecture-redesign-spec.mdlines 371-374, 527-535, 565-569, 601-616, and 663-666.Also applies to: 918-921, 1140-1147, 1226-1231, 1288-1299
🤖 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 `@docs/architecture-redesign-spec.html` around lines 641 - 643, Insert a blank line before each affected bullet or numbered list in the Markdown source, including the “At scale” list and the additional locations identified in the review, so Pandoc recognizes them as lists. Then regenerate the HTML from the corrected architecture-redesign-spec.md, preserving the list content and ordering.docs/rbac.md-154-154 (1)
154-154: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the double negative.
"no shared SA prevents cross-step permission bleed" reads as the opposite of the intent. In a security boundaries table this inversion matters.
📝 Proposed fix
-| Per-step SA isolation | Each step gets a unique SA; no shared SA prevents cross-step permission bleed | +| Per-step SA isolation | Each step gets a unique SA; removing the shared SA prevents cross-step permission bleed |🤖 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 `@docs/rbac.md` at line 154, Update the “Per-step SA isolation” row in the security boundaries table to remove the double negative and state positively that unique, non-shared service accounts prevent cross-step permission bleed.docs/inter-operator-handoff-design.md-90-91 (1)
90-91: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate the audit and templog specs, which still mandate startup blocking.
Line 90 removes the startup
WaitForand starts telemetry disabled. Two specs still require the opposite:
.ai/spec/what/audit-logging.mdrule 26: the operator blocks at startup untillightspeed-otel-collector-clientexists, 5 minute timeout, fatal on expiry..ai/spec/what/templog.mdrule 4: same blocking requirement, and a missing ConfigMap is fatal.Both specs also still name
lightspeed-otel-collector-clientas the source, while line 55 of this document moves OTEL keys intolightspeed-agentic-configuration. Update those spec rules so the behavioral specs match the implemented handoff.🤖 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 `@docs/inter-operator-handoff-design.md` around lines 90 - 91, Update the audit-logging rule 26 and templog rule 4 to remove startup blocking, timeout, and fatal behavior for the old OTEL ConfigMap. Reference lightspeed-agentic-configuration as the OTEL configuration source, and specify that telemetry starts disabled and activates only after the cache contains valid OTEL endpoints and the CA Secret is available; otherwise it remains disabled.docs/architecture-redesign-spec.md-699-712 (1)
699-712: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the dry-run contradiction in §9.
Lines 593-616 reject server-side dry-run and specify Phase B as deterministic 403 collection with no LLM, where "the agent never sees the 403 errors". These lines then contradict that:
- Line 699: the output field is named
dryRunValidated.- Line 708: actions are "validated via dry-run".
- Line 709: permissions are "discovered by the agent during dry-run".
- Line 710: risk is "informed by actual dry-run validation".
- Line 712: "the same agent session that planned the actions also discovered the permissions" — Phase B has no agent session.
- Line 733: "the dry-run that discovered the permissions".
Rename the field and rewrite these statements to reference Phase B empirical 403 collection.
📝 Proposed fix for the terminology
- "dryRunValidated": true, + "permissionsDiscovered": true, @@ -- **Concrete actions** — exact commands that will be executed (validated via dry-run) -- **Exact RBAC** — permissions discovered by the agent during dry-run, not estimated -- **Risk level** — informed by actual dry-run validation +- **Concrete actions** — exact commands that will be executed (attempted during Phase B) +- **Exact RBAC** — permissions discovered empirically via 403 collection in Phase B, not estimated +- **Risk level** — informed by the Phase B attempt results -User approves one option → operator creates execution RBAC from `requiredPermissions` → execution pod runs with accurate, user-approved permissions. RBAC mismatches are eliminated in the happy path because the same agent session that planned the actions also discovered the permissions. +User approves one option → operator creates execution RBAC from `requiredPermissions` → execution pod runs with accurate, user-approved permissions. RBAC mismatches are eliminated in the happy path because Phase B attempted the exact script that execution will replay.And at line 733:
-Users can only select from options produced by analysis. No free-form alternatives. This guarantees RBAC accuracy — the dry-run that discovered the permissions is the same plan that will execute. +Users can only select from options produced by analysis. No free-form alternatives. This guarantees RBAC accuracy — the Phase B attempt that discovered the permissions ran the same plan that will execute.Also applies to: 733-733
🤖 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 `@docs/architecture-redesign-spec.md` around lines 699 - 712, Resolve the terminology contradiction in §9 by renaming the output field dryRunValidated and rewriting the Approval experience statements to describe Phase B empirical 403 collection rather than dry-run validation or agent-discovered permissions. Update the references to concrete actions, RBAC, risk, the same agent session, and the statement at “the dry-run that discovered the permissions” so they accurately reflect deterministic collection with no LLM or agent session.docs/architecture-redesign-spec.md-240-249 (1)
240-249: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the
k8s_proposalstable reference.Line 242 queries a table named
k8s_proposals, but the schema at lines 325-337 defines onlyproposals, and that table holds queued records withstatus = 'queued'— never'Completed','Failed', or'Denied'. The promotion loop at line 343 obtains the active count from the Kubernetes API, not from SQL.As written, this table tells an implementer that cluster-wide active-count gating is a SQL predicate. It is not. Line 249 repeats that claim.
📝 Proposed fix for the gating table entry
-| Total active proposals | `SELECT count(*) FROM k8s_proposals WHERE phase NOT IN ('Completed','Failed','Denied')` | +| Total active proposals | Count Proposal CRs via the Kubernetes API using the phase label selector (see §6) — not a SQL predicate |Then adjust line 249 to state that the remaining dimensions are SQL predicates on the
proposalsqueue table, while the active-count gate reads Kubernetes.🤖 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 `@docs/architecture-redesign-spec.md` around lines 240 - 249, Correct the gating table’s “Total active proposals” entry to reference the Kubernetes API rather than the nonexistent k8s_proposals table or SQL status values. Update the “Rate limiting” or surrounding summary at the table’s closing statement to clarify that only the remaining dimensions use SQL predicates on the queued proposals table, while active-count gating is obtained from Kubernetes.docs/architecture-redesign-spec.md-61-65 (1)
61-65: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNote the divergence between this output contract and the implemented flow.
This document argues against direct Result CR writes (lines 61-65) and specifies that the operator reads the output ConfigMap and then creates the typed Result CR (lines 92-93). The PR implements the opposite direction for output: sandbox pods create Result CRs and exit, and the operator inspects completed Result CRs on pod events. The input ConfigMap contract matches; the output contract does not.
The file is marked
Status: DraftwithJira: TBD, so a target-state divergence may be intentional. Add a short note in §1 stating which parts are implemented and which remain target state. Otherwise readers will implement against the output-ConfigMap contract.Also applies to: 91-95
🤖 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 `@docs/architecture-redesign-spec.md` around lines 61 - 65, Add a short implementation-status note in §1 of the architecture redesign specification, explicitly distinguishing the implemented output flow—sandbox pods create Result CRs and the operator processes them—from the target-state output-ConfigMap flow described in the document. Preserve the existing input ConfigMap contract and clarify that the output contract remains unimplemented, so readers do not treat it as current behavior.docs/architecture-redesign-spec.md-796-804 (1)
796-804: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale section numbers in the Jira traceability table, in both the Markdown and its generated HTML. The table references Processing Gating as §5 and RBAC Accuracy as §6, but the actual headings number them §7 and §9. The HTML is pandoc output of the Markdown, so both copies carry the same wrong numbers from one source.
docs/architecture-redesign-spec.md#L796-L804: correct OLS-3066 to§7 Processing Gating, OLS-3283 to§9, OLS-3294 to§9, and resolve the ambiguous§3, §5entry for OLS-3296.docs/architecture-redesign-spec.html#L1640-L1690: regenerate this file from the corrected Markdown with pandoc; do not hand-edit the generated output.🤖 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 `@docs/architecture-redesign-spec.md` around lines 796 - 804, The Jira traceability table contains stale section references. In docs/architecture-redesign-spec.md:796-804, update OLS-3066 to §7 Processing Gating, OLS-3283 and OLS-3294 to §9, and resolve OLS-3296’s ambiguous §3, §5 reference using the current headings; then regenerate docs/architecture-redesign-spec.html:1640-1690 from the corrected Markdown with pandoc, without hand-editing the generated HTML.docs/unifying-approach-to-rbac-generation.md-84-98 (1)
84-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe example's derived RBAC does not cover the commands shown.
kubectl set image deployment/web-appreads the Deployment before patching it, so it needsgetondeployments.kubectl rollout status deployment/web-appneedsgetandwatchondeployments. The derived RBAC grants onlypatchondeploymentsplusget/listonreplicasets. Running this script with exactly these rules returns 403.This is the reference example for agent RBAC derivation, so the gap propagates into prompt behavior.
📝 Proposed fix for the derived RBAC
- apiGroups: ["apps"] resources: ["deployments"] - verbs: ["patch"] + verbs: ["get", "watch", "patch"] resourceNames: ["web-app"] +- apiGroups: ["apps"] + resources: ["deployments"] + verbs: ["list"] - apiGroups: ["apps"] resources: ["replicasets"] verbs: ["get", "list"]Note that
listandwatchon a collection cannot be restricted byresourceNames, which is why it needs a separate rule. That constraint is worth stating in the mapping rules.🤖 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 `@docs/unifying-approach-to-rbac-generation.md` around lines 84 - 98, Update the derived RBAC example to grant get on deployments for kubectl set image and rollout status, plus watch on deployments for rollout status, while retaining patch on the named deployment and the existing ReplicaSet permissions. Add a separate deployment rule without resourceNames for collection-level list/watch permissions, and state this resourceNames limitation in the mapping rules.docs/inter-operator-handoff-design.md-42-42 (1)
42-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the missing-configuration behavior with
AgenticRunReconciler.When
Cache.Available()is false, the reconciler logs and returns without updating theAgenticRunstatus. Update lines 42 and 104 to describe this skip behavior, or document where the actionable error is set.🤖 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 `@docs/inter-operator-handoff-design.md` at line 42, Update the missing-configuration descriptions in the design document to match AgenticRunReconciler behavior: when Cache.Available() is false, the reconciler logs and skips processing without updating AgenticRun status. If an actionable error is set elsewhere, identify that location instead; otherwise remove the claim that the run fails with that error..ai/spec/how/reconciler.md-123-123 (1)
123-123: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe
RequeueAfter(30s)claim does not match the handlers.
launchSandboxreturnsnil, andhandleAnalysis,handleExecution,handleVerificationreturnctrl.Result{}, nilafter the launch. No handler setsRequeueAfter. Re-entry depends only on pod and Result CR watch events plus the one-minute timeout loop inpod_handler.go. Either add the safety-net requeue in the handlers or remove the claim from this document.Also applies to: 133-134
🤖 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 @.ai/spec/how/reconciler.md at line 123, Correct the Batch flow documentation for launchSandbox and the Analyze/Execute/Verify handlers: remove the claim that handlers return RequeueAfter(30s), and state that re-entry relies on pod and Result CR watch events plus the one-minute timeout loop in pod_handler.go. Apply the same correction to the Escalate-related wording at the referenced section..ai/spec/how/reconciler.md-34-34 (1)
34-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFunction names in the
pod_handler.gorow do not exist.
controller/agenticrun/pod_handler.godefineshandlePodEvent,patchStepCondition,releaseSandbox,runTimeoutLoop, andhandleTimeEvent. It does not defineprocessPodTerminationorcheckStepTimeout.📝 Proposed fix
-| `pod_handler.go` | `PodEventHandler` (pod watch handler); timeout background goroutine | `handlePodEvent`, `processPodTermination`, `releaseSandbox`, `runTimeoutLoop`, `checkStepTimeout` | +| `pod_handler.go` | pod watch handler (methods on `AgenticRunReconciler`); timeout background goroutine | `handlePodEvent`, `patchStepCondition`, `releaseSandbox`, `runTimeoutLoop`, `handleTimeEvent`, `stepConditionType`, `sandboxClaimName`, `fetchResultCR`, `podFailMessage` |🤖 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 @.ai/spec/how/reconciler.md at line 34, Update the pod_handler.go row in the reconciler documentation to replace the nonexistent processPodTermination and checkStepTimeout references with the actual handleTimeEvent and patchStepCondition symbols, while preserving the existing valid function names.controller/agenticrun/reconciler.go-274-276 (1)
274-276: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace
time.Sleepwith a context-aware wait inrunTimeoutLoop. The method matchesmanager.RunnableFuncand checksctx.Err(), buttime.Sleep(sandboxTimeoutCheckInterval)delays manager shutdown by up to one minute. Use a timer with aselectonctx.Done()so cancellation returns immediately.🤖 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 `@controller/agenticrun/reconciler.go` around lines 274 - 276, Update runTimeoutLoop to replace time.Sleep(sandboxTimeoutCheckInterval) with a context-aware timer wait, selecting between the timer channel and ctx.Done(). Stop or clean up the timer as needed, and return immediately when the context is canceled while preserving the existing periodic timeout-check behavior.
🧹 Nitpick comments (13)
docs/architecture-redesign-spec.md (1)
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd languages to the fenced code blocks.
markdownlint reports MD040 on these five blocks. Use
textfor the ASCII diagrams (lines 71, 139, 274, 747) andtextorpseudocodefor the promotion loop (line 341). This keeps the docs lint clean.Also applies to: 139-139, 274-274, 341-341, 747-747
🤖 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 `@docs/architecture-redesign-spec.md` at line 71, Add explicit language identifiers to the five fenced code blocks in the architecture redesign specification: use text for the ASCII diagrams and text or pseudocode for the promotion loop. Preserve each block’s existing content while updating only the fence declarations to satisfy markdownlint MD040.Source: Linters/SAST tools
docs/architecture-redesign-spec.html (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm how the generated HTML stays in sync.
This file is pandoc output committed next to its Markdown source. Any future edit to
docs/architecture-redesign-spec.mdsilently leaves this file stale. The same applies todocs/unifying-approach-to-rbac-generation.html.Add a
maketarget or a CI check that regenerates and diffs the HTML, or drop the HTML from the repository and generate it at publish time.🤖 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 `@docs/architecture-redesign-spec.html` around lines 1 - 8, Add synchronization for the generated HTML files `architecture-redesign-spec.html` and `unifying-approach-to-rbac-generation.html`: either add a Make target or CI check that regenerates both from their Markdown sources and fails on differences, or remove the committed HTML and generate it during publishing.docs/rbac.md (2)
120-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one placeholder name for the run.
Line 120 writes
ls-execution-{namespace}-{name}, line 130 refers tosandboxSAName(run, step), and lines 33 and 126 writels-{step}-{namespace}-{runName}. The per-phase table at lines 134-137 uses{ns}and{name}. Use{namespace}and{runName}consistently so readers do not read{name}as the step name.Also applies to: 130-130
🤖 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 `@docs/rbac.md` at line 120, Standardize the run placeholder in the RBAC documentation by replacing `{name}` and `{ns}` with `{runName}` and `{namespace}` throughout the referenced service-account descriptions and per-phase table. Preserve `{step}` for the step placeholder and keep all examples consistent with `sandboxSAName(run, step)`.
33-33: 🩺 Stability & Availability | 🔵 TrivialConsider documenting a reconciliation sweep for stale reader subjects.
The operator adds per-step ServiceAccount subjects to admin-owned ClusterRoleBindings and relies on
Releaseand the AgenticRun finalizer to remove them. If the operator is force-deleted, or a finalizer is removed manually, stale subjects remain in a binding that grants cluster-wide read. Those subject names reference deleted ServiceAccounts, so they grant nothing immediately, but a later run with a colliding truncated name would inherit the binding.Document a periodic sweep that removes reader subjects whose ServiceAccount no longer exists, or state explicitly that stale subjects are accepted and why.
🤖 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 `@docs/rbac.md` at line 33, Update the RBAC documentation near the reconciliation flow to specify how stale per-step ServiceAccount subjects are handled: document a periodic sweep that removes subjects whose ServiceAccounts no longer exist, or explicitly state that stale subjects are intentionally accepted and explain the safety rationale.docs/unifying-approach-to-rbac-generation.md (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the Jira reference.
docs/inter-operator-handoff-design.mdcarries its Jira IDs on line 3, anddocs/architecture-redesign-spec.mdcarries a front-matter block withDate,Status, andJira. This document has neither. Add at least the tracking Jira and a status so readers can tell whether it is a draft or an accepted design.🤖 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 `@docs/unifying-approach-to-rbac-generation.md` around lines 1 - 2, Add document metadata at the top of “Unifying Approach to RBAC Generation,” including the tracking Jira reference and a status indicating whether the design is a draft or accepted. Follow the existing metadata style used by the referenced documentation, preserving the document title and content.controller/agenticrun/sandbox_manager.go (1)
158-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
smClaimGVK.Versioninstead of the literal"v1alpha1".The
Groupcomes from the GVK constant while the version is hardcoded. A version bump to the constant would leave this owner reference stale.♻️ Proposed refactor
ownerRef = metav1.OwnerReference{ - APIVersion: smClaimGVK.Group + "/v1alpha1", + APIVersion: smClaimGVK.GroupVersion().String(), Kind: smClaimGVK.Kind,🤖 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 `@controller/agenticrun/sandbox_manager.go` around lines 158 - 163, Update the OwnerReference construction in the ownerRef assignment to build APIVersion using smClaimGVK.Group and smClaimGVK.Version, removing the hardcoded "v1alpha1" while preserving the existing Kind, Name, and UID fields.controller/agenticrun/sandbox_manager_test.go (1)
145-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an assertion for the input ConfigMap owner references.
TestCreate_BarePodverifies the ConfigMap data and the mount but not its owner references.SandboxManager.CreatecallssetInputConfigMapOwner, which currently replaces theAgenticRuncontroller reference with a plain Pod reference. An assertion here would catch that regression and pin the intended ownership contract.💚 Proposed test addition
if cm.Data[inputConfigMapKeyQuery] != "test query" { t.Errorf("ConfigMap query = %q", cm.Data[inputConfigMapKeyQuery]) } + var hasRunOwner, hasPodOwner bool + for _, o := range cm.OwnerReferences { + if o.Kind == "AgenticRun" && o.Controller != nil && *o.Controller { + hasRunOwner = true + } + if o.Kind == "Pod" && o.Name == name { + hasPodOwner = true + } + } + if !hasRunOwner { + t.Error("input ConfigMap missing AgenticRun controller owner reference") + } + if !hasPodOwner { + t.Error("input ConfigMap missing pod owner reference for GC") + }🤖 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 `@controller/agenticrun/sandbox_manager_test.go` around lines 145 - 162, Add an assertion in TestCreate_BarePod after retrieving the input ConfigMap to verify its ownerReferences contain the AgenticRun controller reference and do not replace it with a plain Pod reference. Reuse the existing owner-reference helpers or identifiers used by SandboxManager.Create and setInputConfigMapOwner, while preserving the current data and volume-mount assertions.controller/agenticrun/pod_handler_test.go (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe "sandbox exited without creating result" string exists in two places.
podFailMessagereturns this literal for a succeeded pod.handlePodEventincontroller/agenticrun/pod_handler.goalso passes the same literal directly topatchStepConditionon theresultCR == nilbranch. The two copies can drift, and this test only pins one of them. Promote the string to a constant and use it in both places.🤖 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 `@controller/agenticrun/pod_handler_test.go` around lines 18 - 22, Define a shared constant for the “sandbox exited without creating result” message, then update podFailMessage and the resultCR == nil branch in handlePodEvent to use it instead of duplicate literals. Keep the existing behavior and test expectation unchanged.controller/agenticrun/handlers_test.go (1)
905-905: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo RBAC tests no longer test RBAC.
TestReconcile_ExecutionRBACCreatedOnApprovalasserts no RBAC object.TestReconcile_ExecutionRBACCleanedOnFailureasserts no cleanup and no failure. Both now assert only the phase pathProposed → Verifying → Completed, which duplicatesTestReconcile_HappyPath_FullLifecycle.Take one of these actions:
- Restore the assertions against the new owner of per-step RBAC, and keep the names.
- Delete both tests and rename nothing, because the coverage moved to
rbac_test.goandsandbox_manager_test.go.Keeping the current names hides the fact that execution RBAC creation and deletion are no longer verified end to end.
Also applies to: 944-963
🤖 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 `@controller/agenticrun/handlers_test.go` at line 905, Remove TestReconcile_ExecutionRBACCreatedOnApproval and TestReconcile_ExecutionRBACCleanedOnFailure because their RBAC assertions were removed and their remaining phase-path checks duplicate TestReconcile_HappyPath_FullLifecycle; retain the moved coverage in rbac_test.go and sandbox_manager_test.go without renaming replacement tests.controller/agenticrun/audit.go (2)
443-467: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated step-span cleanup loop.
CleanupandCleanupDeletedboth iterate the same hardcoded step list and end stored spans. Extract one helper that takes atypes.UID. This also keeps the step list in one place if a fifth step is added.♻️ Proposed helper extraction
+var auditSteps = []string{"analysis", "execution", "verification", "escalation"} + +func (l *ProductionAuditLogger) endActiveSpans(uid types.UID) { + for _, step := range auditSteps { + if val, ok := l.activeSpans.LoadAndDelete(activeSpanKey(uid, step)); ok { + if span, ok := val.(trace.Span); ok { + span.End() + } + } + } +} + func (l *ProductionAuditLogger) Cleanup(run *agenticv1alpha1.AgenticRun) { l.priorPhase.Delete(run.UID) l.emittedApproval.Delete(run.UID) - for _, step := range []string{"analysis", "execution", "verification", "escalation"} { - if val, ok := l.activeSpans.LoadAndDelete(activeSpanKey(run.UID, step)); ok { - if span, ok := val.(trace.Span); ok { - span.End() - } - } - } + l.endActiveSpans(run.UID) }🤖 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 `@controller/agenticrun/audit.go` around lines 443 - 467, Extract the duplicated active-span cleanup loop from Cleanup and CleanupDeleted into a shared helper that accepts a types.UID, preserving the existing activeSpanKey lookup, trace.Span type assertion, and End behavior. Replace both inline loops with calls to the helper and keep the step list centralized there.
254-271: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose replaced spans and identify them by sandbox instance.
Storecan replace an active span without ending it.sync.Map.Swapis supported by the Go 1.25.7 toolchain. However,CompleteStepkeys only by run UID and step, so a late event from the old pod can end the new span. Include the pod UID or retry index in the key and completion call.🤖 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 `@controller/agenticrun/audit.go` around lines 254 - 271, Update ProductionAuditLogger.BeginStep and CompleteStep to key active spans by run UID plus sandbox instance identifier (pod UID or retry index), and pass that same identifier through completion. When storing a new span, atomically replace any existing span with sync.Map.Swap and end the replaced span before retaining the new one, preventing stale spans and late events from affecting the current sandbox span.controller/agenticrun/audit_test.go (1)
498-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
BeginStepandCompleteStep.The removed tests covered the synchronous completion events. The new
BeginStep/CompleteSteppair is the replacement contract, and no test in this file exercises it. Add a test that callsBeginStep, asserts the span has not ended, then callsCompleteStepwith a completed Result CR and asserts the span ended with theagenticrun.<step>.completedevent. Add a second case whereCompleteStepruns without a priorBeginStepand asserts no span is produced.Do you want me to generate these tests?
🤖 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 `@controller/agenticrun/audit_test.go` around lines 498 - 503, Add coverage in the audit tests for the BeginStep/CompleteStep contract: verify BeginStep creates an active span, then CompleteStep with a completed Result CR ends it and records agenticrun.<step>.completed; also verify CompleteStep without a preceding BeginStep produces no span.controller/agenticrun/reconciler_test.go (1)
134-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEvery fake-client call discards its error, which hides setup failures.
_ = ta.fc.Get(...)on line 139 is the worst case. If theGetfails,freshstays a zero-valueAgenticRun. The followingCreateandStatus().Patchthen run against an empty name and namespace and also fail silently. The test then fails much later on a phase mismatch, and the real cause is invisible. The same pattern repeats incompleteExecution,completeVerification, andcompleteEscalation.Record the first error on the struct and assert it from the tests, or pass
*testing.TintowithClientand fail immediately.♻️ Proposed error capture
type testAgentCaller struct { analyzeErr error executeErr error verifyErr error escalateErr error + + // simErr records the first fake-client failure during inline + // sandbox-completion simulation. + simErr errorvar fresh agenticv1alpha1.AgenticRun - _ = ta.fc.Get(ctx, client.ObjectKeyFromObject(run), &fresh) + if err := ta.fc.Get(ctx, client.ObjectKeyFromObject(run), &fresh); err != nil { + ta.record(err) + return + }As per path instructions for
**/*.go: "Never ignore error returns". Based on learnings, the blank-identifier exception in this repository applies only to theAddToSchemescheme-registration pattern, so it does not cover these client calls.🤖 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 `@controller/agenticrun/reconciler_test.go` around lines 134 - 162, Handle every fake-client error in testAgentCaller helpers instead of discarding it: update completeAnalysis, completeExecution, completeVerification, and completeEscalation to record the first error on the struct (or fail immediately through testing.T), and stop or avoid subsequent client operations after an error. Ensure the associated tests assert the captured error so Get, Create, and status updates cannot fail silently.Sources: Path instructions, Learnings
🤖 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 @.ai/spec/what/sandbox-execution.md:
- Line 14: Resolve the conflicting retryIndex contract between rules 7a/8 and
rule 27 in the sandbox-execution specification. Confirm the current CRD
behavior, then update the outdated rule so result-template requirements and the
stated ExecutionResult/VerificationResult schema consistently either include or
remove spec.retryIndex.
In `@controller/agenticrun/pod_handler.go`:
- Around line 131-139: Update AgenticRunReconciler.runTimeoutLoop to wait on a
cancellable timer or ticker using ctx.Done() instead of time.Sleep, returning
immediately when the context is canceled. Trigger the first handleTimeEvent
check without the initial one-minute delay, while preserving the periodic
timeout-check interval thereafter.
- Around line 150-164: Update handleTimeEvent to skip pods in terminal
PodSucceeded or PodFailed phases before evaluating overallTimedOut, while
preserving the existing start-timeout handling for non-terminal pods. Ensure
terminal pods cannot reach patchStepCondition with SandboxTimeout and overwrite
a successful step condition.
- Around line 68-82: The successful-pod handling around fetchResultCR must
resolve Result CRs independently of status.steps results. Update fetchResultCR
to list Result CRs using LabelRun and LabelStep, select the Completed=True
result, and populate the corresponding status reference before evaluating
success, while preserving the existing failure condition when no completed
Result CR exists.
In `@controller/agenticrun/rbac.go`:
- Around line 345-387: Update controller/agenticrun/rbac.go lines 345-387 in
ensureResultRBAC to create the result Role and RoleBinding in run.Namespace
while keeping their ServiceAccount subject in operatorNS; explicitly delete
these cross-namespace RBAC objects during SandboxManager.Release. In
controller/agenticrun/pod_handler.go lines 41-45 and 166-169, resolve the
AgenticRun namespace from the pod label rather than pod.Namespace so
handlePodEvent and handleTimeEvent locate runs across namespaces and perform
condition updates and sandbox release.
In `@controller/agenticrun/reconciler_test.go`:
- Around line 258-299: The production reconciliation path that handles completed
VerificationResult objects must evaluate verifyResult.Success and every check
result before setting AgenticRunConditionVerified. Move the retry-count updates,
execution-condition removal, retry/exhaustion reasons, and escalation condition
handling from the test double’s completion logic into the production handler,
preserving the existing max-attempt behavior; reduce completeVerification to
only creating the completed result and recording its reference.
In `@controller/agenticrun/reconciler.go`:
- Around line 82-89: Before removing rbacCleanupFinalizer in the deletion path,
call cleanupExecutionRBAC for the AgenticRun and handle any returned error
consistently with the existing cleanup flow. Keep sandbox release intact, and
only remove the finalizer after execution RBAC cleanup succeeds.
- Line 285: Update the controller manager cache configuration used by the
reconciler to add cache.Options.ByObject for &corev1.Pod{}, restricting it to
the sandbox label selector and operator namespace. Keep handlePodEvent
predicates for event filtering, but ensure Pods are scoped before entering the
cache rather than relying on the existing Watches call.
In `@controller/agenticrun/sandbox_agent.go`:
- Around line 153-161: Change patchSandboxInfo to return the status-patch error
instead of only logging it, and update launchSandbox to propagate that error
after sandbox creation. Ensure the failure path preserves the existing claim
error context while allowing cleanup to proceed even when sandbox.claimName was
not persisted.
In `@controller/agenticrun/sandbox_manager.go`:
- Around line 152-177: The owner-reference update in setInputConfigMapOwner must
preserve the existing AgenticRun reference and append the Pod or SandboxClaim
reference instead of replacing all references. Mark the AgenticRun owner
reference with Controller and BlockOwnerDeletion set to true, and apply the same
ownership flags to the appended child reference as required.
In `@docs/inter-operator-handoff-design.md`:
- Line 77: The Create flow’s sandboxSAName generation must remain unique for
long run identities instead of relying only on 63-character truncation. Update
the sandboxSAName construction used by Create to derive a collision-resistant
hash suffix from the full identity, preserve the Kubernetes name limit, and
ensure the resulting name remains valid for ServiceAccount naming while keeping
existing ensureSA and Release behavior intact.
In `@docs/rbac.md`:
- Line 126: Update the “Resolved: per-step SA isolation” note to remove the
claim that the operator requires cluster-admin. Reference the scoped
agentic-operator-escalation ClusterRole prerequisite instead, preserving that it
permits creating ServiceAccounts and Roles as required while directing readers
to the documented narrower scope.
In `@docs/unifying-approach-to-rbac-generation.md`:
- Line 27: Update the document’s MCP authentication assumptions and “Scope and
Limitations” section to explicitly require cluster-facing MCP servers to forward
or impersonate the sandbox’s ServiceAccount token. State that MCP servers
authenticating with their own ServiceAccount identity are out of scope, and
apply this constraint consistently to the related statements around the MCP tool
model, permission derivation, and limitations.
- Line 46: Reconcile the RBAC derivation flow described in the document with
architecture-redesign-spec.md §9 by explicitly identifying the current source of
permissions or stating that this document only covers MCP layering. Update the
flow around the command-to-API mapping and MCP-rewritten script execution so
RBAC is derived from the exact script that will run, or add an explicit
validation step that blocks execution when rewrite fidelity is not guaranteed;
align the claims near the rewrite behavior and known limitations accordingly.
---
Minor comments:
In @.ai/spec/how/reconciler.md:
- Line 123: Correct the Batch flow documentation for launchSandbox and the
Analyze/Execute/Verify handlers: remove the claim that handlers return
RequeueAfter(30s), and state that re-entry relies on pod and Result CR watch
events plus the one-minute timeout loop in pod_handler.go. Apply the same
correction to the Escalate-related wording at the referenced section.
- Line 34: Update the pod_handler.go row in the reconciler documentation to
replace the nonexistent processPodTermination and checkStepTimeout references
with the actual handleTimeEvent and patchStepCondition symbols, while preserving
the existing valid function names.
In `@controller/agenticrun/reconciler.go`:
- Around line 274-276: Update runTimeoutLoop to replace
time.Sleep(sandboxTimeoutCheckInterval) with a context-aware timer wait,
selecting between the timer channel and ctx.Done(). Stop or clean up the timer
as needed, and return immediately when the context is canceled while preserving
the existing periodic timeout-check behavior.
In `@docs/architecture-redesign-spec.html`:
- Around line 641-643: Insert a blank line before each affected bullet or
numbered list in the Markdown source, including the “At scale” list and the
additional locations identified in the review, so Pandoc recognizes them as
lists. Then regenerate the HTML from the corrected
architecture-redesign-spec.md, preserving the list content and ordering.
In `@docs/architecture-redesign-spec.md`:
- Around line 699-712: Resolve the terminology contradiction in §9 by renaming
the output field dryRunValidated and rewriting the Approval experience
statements to describe Phase B empirical 403 collection rather than dry-run
validation or agent-discovered permissions. Update the references to concrete
actions, RBAC, risk, the same agent session, and the statement at “the dry-run
that discovered the permissions” so they accurately reflect deterministic
collection with no LLM or agent session.
- Around line 240-249: Correct the gating table’s “Total active proposals” entry
to reference the Kubernetes API rather than the nonexistent k8s_proposals table
or SQL status values. Update the “Rate limiting” or surrounding summary at the
table’s closing statement to clarify that only the remaining dimensions use SQL
predicates on the queued proposals table, while active-count gating is obtained
from Kubernetes.
- Around line 61-65: Add a short implementation-status note in §1 of the
architecture redesign specification, explicitly distinguishing the implemented
output flow—sandbox pods create Result CRs and the operator processes them—from
the target-state output-ConfigMap flow described in the document. Preserve the
existing input ConfigMap contract and clarify that the output contract remains
unimplemented, so readers do not treat it as current behavior.
- Around line 796-804: The Jira traceability table contains stale section
references. In docs/architecture-redesign-spec.md:796-804, update OLS-3066 to §7
Processing Gating, OLS-3283 and OLS-3294 to §9, and resolve OLS-3296’s ambiguous
§3, §5 reference using the current headings; then regenerate
docs/architecture-redesign-spec.html:1640-1690 from the corrected Markdown with
pandoc, without hand-editing the generated HTML.
In `@docs/inter-operator-handoff-design.md`:
- Around line 90-91: Update the audit-logging rule 26 and templog rule 4 to
remove startup blocking, timeout, and fatal behavior for the old OTEL ConfigMap.
Reference lightspeed-agentic-configuration as the OTEL configuration source, and
specify that telemetry starts disabled and activates only after the cache
contains valid OTEL endpoints and the CA Secret is available; otherwise it
remains disabled.
- Line 42: Update the missing-configuration descriptions in the design document
to match AgenticRunReconciler behavior: when Cache.Available() is false, the
reconciler logs and skips processing without updating AgenticRun status. If an
actionable error is set elsewhere, identify that location instead; otherwise
remove the claim that the run fails with that error.
In `@docs/rbac.md`:
- Line 154: Update the “Per-step SA isolation” row in the security boundaries
table to remove the double negative and state positively that unique, non-shared
service accounts prevent cross-step permission bleed.
In `@docs/unifying-approach-to-rbac-generation.md`:
- Around line 84-98: Update the derived RBAC example to grant get on deployments
for kubectl set image and rollout status, plus watch on deployments for rollout
status, while retaining patch on the named deployment and the existing
ReplicaSet permissions. Add a separate deployment rule without resourceNames for
collection-level list/watch permissions, and state this resourceNames limitation
in the mapping rules.
---
Nitpick comments:
In `@controller/agenticrun/audit_test.go`:
- Around line 498-503: Add coverage in the audit tests for the
BeginStep/CompleteStep contract: verify BeginStep creates an active span, then
CompleteStep with a completed Result CR ends it and records
agenticrun.<step>.completed; also verify CompleteStep without a preceding
BeginStep produces no span.
In `@controller/agenticrun/audit.go`:
- Around line 443-467: Extract the duplicated active-span cleanup loop from
Cleanup and CleanupDeleted into a shared helper that accepts a types.UID,
preserving the existing activeSpanKey lookup, trace.Span type assertion, and End
behavior. Replace both inline loops with calls to the helper and keep the step
list centralized there.
- Around line 254-271: Update ProductionAuditLogger.BeginStep and CompleteStep
to key active spans by run UID plus sandbox instance identifier (pod UID or
retry index), and pass that same identifier through completion. When storing a
new span, atomically replace any existing span with sync.Map.Swap and end the
replaced span before retaining the new one, preventing stale spans and late
events from affecting the current sandbox span.
In `@controller/agenticrun/handlers_test.go`:
- Line 905: Remove TestReconcile_ExecutionRBACCreatedOnApproval and
TestReconcile_ExecutionRBACCleanedOnFailure because their RBAC assertions were
removed and their remaining phase-path checks duplicate
TestReconcile_HappyPath_FullLifecycle; retain the moved coverage in rbac_test.go
and sandbox_manager_test.go without renaming replacement tests.
In `@controller/agenticrun/pod_handler_test.go`:
- Around line 18-22: Define a shared constant for the “sandbox exited without
creating result” message, then update podFailMessage and the resultCR == nil
branch in handlePodEvent to use it instead of duplicate literals. Keep the
existing behavior and test expectation unchanged.
In `@controller/agenticrun/reconciler_test.go`:
- Around line 134-162: Handle every fake-client error in testAgentCaller helpers
instead of discarding it: update completeAnalysis, completeExecution,
completeVerification, and completeEscalation to record the first error on the
struct (or fail immediately through testing.T), and stop or avoid subsequent
client operations after an error. Ensure the associated tests assert the
captured error so Get, Create, and status updates cannot fail silently.
In `@controller/agenticrun/sandbox_manager_test.go`:
- Around line 145-162: Add an assertion in TestCreate_BarePod after retrieving
the input ConfigMap to verify its ownerReferences contain the AgenticRun
controller reference and do not replace it with a plain Pod reference. Reuse the
existing owner-reference helpers or identifiers used by SandboxManager.Create
and setInputConfigMapOwner, while preserving the current data and volume-mount
assertions.
In `@controller/agenticrun/sandbox_manager.go`:
- Around line 158-163: Update the OwnerReference construction in the ownerRef
assignment to build APIVersion using smClaimGVK.Group and smClaimGVK.Version,
removing the hardcoded "v1alpha1" while preserving the existing Kind, Name, and
UID fields.
In `@docs/architecture-redesign-spec.html`:
- Around line 1-8: Add synchronization for the generated HTML files
`architecture-redesign-spec.html` and
`unifying-approach-to-rbac-generation.html`: either add a Make target or CI
check that regenerates both from their Markdown sources and fails on
differences, or remove the committed HTML and generate it during publishing.
In `@docs/architecture-redesign-spec.md`:
- Line 71: Add explicit language identifiers to the five fenced code blocks in
the architecture redesign specification: use text for the ASCII diagrams and
text or pseudocode for the promotion loop. Preserve each block’s existing
content while updating only the fence declarations to satisfy markdownlint
MD040.
In `@docs/rbac.md`:
- Line 120: Standardize the run placeholder in the RBAC documentation by
replacing `{name}` and `{ns}` with `{runName}` and `{namespace}` throughout the
referenced service-account descriptions and per-phase table. Preserve `{step}`
for the step placeholder and keep all examples consistent with
`sandboxSAName(run, step)`.
- Line 33: Update the RBAC documentation near the reconciliation flow to specify
how stale per-step ServiceAccount subjects are handled: document a periodic
sweep that removes subjects whose ServiceAccounts no longer exist, or explicitly
state that stale subjects are intentionally accepted and explain the safety
rationale.
In `@docs/unifying-approach-to-rbac-generation.md`:
- Around line 1-2: Add document metadata at the top of “Unifying Approach to
RBAC Generation,” including the tracking Jira reference and a status indicating
whether the design is a draft or accepted. Follow the existing metadata style
used by the referenced documentation, preserving the document title and content.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 02218aed-6e2d-41ff-a635-c640eb56bf64
📒 Files selected for processing (32)
.ai/spec/how/project-structure.md.ai/spec/how/reconciler.md.ai/spec/what/audit-logging.md.ai/spec/what/sandbox-execution.md.ai/spec/what/system-config.md.ai/spec/what/templog.mdcmd/main.gocontroller/agenticrun/agent.gocontroller/agenticrun/audit.gocontroller/agenticrun/audit_test.gocontroller/agenticrun/client.gocontroller/agenticrun/client_test.gocontroller/agenticrun/handlers.gocontroller/agenticrun/handlers_test.gocontroller/agenticrun/helpers.gocontroller/agenticrun/pod_handler.gocontroller/agenticrun/pod_handler_test.gocontroller/agenticrun/rbac.gocontroller/agenticrun/rbac_test.gocontroller/agenticrun/reconciler.gocontroller/agenticrun/reconciler_test.gocontroller/agenticrun/sandbox_agent.gocontroller/agenticrun/sandbox_agent_test.gocontroller/agenticrun/sandbox_manager.gocontroller/agenticrun/sandbox_manager_test.gocontroller/agenticrun/state_machine_test.godocs/architecture-redesign-spec.htmldocs/architecture-redesign-spec.mddocs/inter-operator-handoff-design.mddocs/rbac.mddocs/unifying-approach-to-rbac-generation.htmldocs/unifying-approach-to-rbac-generation.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
💤 Files with no reviewable changes (2)
- controller/agenticrun/client_test.go
- controller/agenticrun/client.go
| now := time.Now() | ||
| for i := range pods.Items { | ||
| pod := &pods.Items[i] | ||
| step := pod.Labels[LabelStep] | ||
| runName := pod.Labels[LabelRun] | ||
|
|
||
| created := pod.CreationTimestamp.Time | ||
| var message string | ||
| if startTimedOut(pod.Status.Phase, created, now, podStartTimeout) { | ||
| message = fmt.Sprintf("sandbox pod did not start within %s", podStartTimeout) | ||
| } else if overallTimedOut(created, now, stepTimeout(step)) { | ||
| message = fmt.Sprintf("sandbox exceeded timeout %s", stepTimeout(step)) | ||
| } else { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
handleTimeEvent applies the overall timeout to pods that already terminated.
startTimedOut excludes PodRunning, PodSucceeded, and PodFailed. overallTimedOut checks only creationTimestamp. A Succeeded or Failed pod that survives past the step timeout — for example when releaseSandbox failed, or when the run reached a terminal phase — still matches. patchStepCondition then overwrites a True condition with False and reason SandboxTimeout, which flips the derived phase to Failed after a successful step.
Skip terminal pod phases before the overall timeout check.
🔧 Proposed fix
created := pod.CreationTimestamp.Time
+ if pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed {
+ // Terminal pods are handled by handlePodEvent; releasing them is
+ // separate from timeout enforcement.
+ continue
+ }
var message string
if startTimedOut(pod.Status.Phase, created, now, podStartTimeout) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| now := time.Now() | |
| for i := range pods.Items { | |
| pod := &pods.Items[i] | |
| step := pod.Labels[LabelStep] | |
| runName := pod.Labels[LabelRun] | |
| created := pod.CreationTimestamp.Time | |
| var message string | |
| if startTimedOut(pod.Status.Phase, created, now, podStartTimeout) { | |
| message = fmt.Sprintf("sandbox pod did not start within %s", podStartTimeout) | |
| } else if overallTimedOut(created, now, stepTimeout(step)) { | |
| message = fmt.Sprintf("sandbox exceeded timeout %s", stepTimeout(step)) | |
| } else { | |
| continue | |
| } | |
| now := time.Now() | |
| for i := range pods.Items { | |
| pod := &pods.Items[i] | |
| step := pod.Labels[LabelStep] | |
| runName := pod.Labels[LabelRun] | |
| created := pod.CreationTimestamp.Time | |
| if pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed { | |
| // Terminal pods are handled by handlePodEvent; releasing them is | |
| // separate from timeout enforcement. | |
| continue | |
| } | |
| var message string | |
| if startTimedOut(pod.Status.Phase, created, now, podStartTimeout) { | |
| message = fmt.Sprintf("sandbox pod did not start within %s", podStartTimeout) | |
| } else if overallTimedOut(created, now, stepTimeout(step)) { | |
| message = fmt.Sprintf("sandbox exceeded timeout %s", stepTimeout(step)) | |
| } else { | |
| continue | |
| } |
🤖 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 `@controller/agenticrun/pod_handler.go` around lines 150 - 164, Update
handleTimeEvent to skip pods in terminal PodSucceeded or PodFailed phases before
evaluating overallTimedOut, while preserving the existing start-timeout handling
for non-terminal pods. Ensure terminal pods cannot reach patchStepCondition with
SandboxTimeout and overwrite a successful step condition.
| func ensureResultRBAC(ctx context.Context, c client.Client, run *agenticv1alpha1.AgenticRun, step, serviceAccount, operatorNS string) error { | ||
| resource, ok := stepResultResource[step] | ||
| if !ok { | ||
| return fmt.Errorf("unknown step %q for result RBAC", step) | ||
| } | ||
|
|
||
| roleName := resultRoleName(string(run.UID), step) | ||
| labels := rbacLabels(run.Name, "result-rbac") | ||
|
|
||
| role := &rbacv1.Role{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: roleName, Namespace: operatorNS, Labels: labels}, | ||
| Rules: []rbacv1.PolicyRule{ | ||
| { | ||
| APIGroups: []string{"agentic.openshift.io"}, | ||
| Resources: []string{resource}, | ||
| Verbs: []string{"create", "get"}, | ||
| }, | ||
| { | ||
| APIGroups: []string{"agentic.openshift.io"}, | ||
| Resources: []string{resource + "/status"}, | ||
| Verbs: []string{"get", "patch", "update"}, | ||
| }, | ||
| }, | ||
| } | ||
| if err := c.Create(ctx, role); err != nil && !apierrors.IsAlreadyExists(err) { | ||
| return fmt.Errorf("create result Role %s: %w", roleName, err) | ||
| } | ||
|
|
||
| binding := &rbacv1.RoleBinding{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: roleName, Namespace: operatorNS, Labels: labels}, | ||
| RoleRef: rbacv1.RoleRef{APIGroup: rbacv1.GroupName, Kind: "Role", Name: roleName}, | ||
| Subjects: []rbacv1.Subject{{ | ||
| Kind: rbacv1.ServiceAccountKind, | ||
| Name: serviceAccount, | ||
| Namespace: operatorNS, | ||
| }}, | ||
| } | ||
| if err := c.Create(ctx, binding); err != nil && !apierrors.IsAlreadyExists(err) { | ||
| return fmt.Errorf("create result RoleBinding %s: %w", roleName, err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Cross-namespace AgenticRun handling is broken in the result RBAC and the pod handler. Both sites assume the AgenticRun lives in the operator namespace. .ai/spec/what/sandbox-execution.md rule 2 states that it does not. Result CRs are created in run.Namespace, while pods and RBAC are created in the operator namespace.
controller/agenticrun/rbac.go#L345-L387: create the resultRoleandRoleBindinginrun.Namespaceinstead ofoperatorNS, keep the subject inoperatorNS, and clean these objects up explicitly inSandboxManager.Releasebecause a cross-namespace owner reference to the pod is not honored by garbage collection.controller/agenticrun/pod_handler.go#L41-L45: resolve the run namespace from a pod label instead ofpod.NamespacesohandlePodEventfinds the run, patches the step condition, and releases the sandbox.controller/agenticrun/pod_handler.go#L166-L169: apply the same run-namespace resolution sohandleTimeEventdoes not skip every pod whose run is in another namespace.
📍 Affects 2 files
controller/agenticrun/rbac.go#L345-L387(this comment)controller/agenticrun/pod_handler.go#L41-L45controller/agenticrun/pod_handler.go#L166-L169
🤖 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 `@controller/agenticrun/rbac.go` around lines 345 - 387, Update
controller/agenticrun/rbac.go lines 345-387 in ensureResultRBAC to create the
result Role and RoleBinding in run.Namespace while keeping their ServiceAccount
subject in operatorNS; explicitly delete these cross-namespace RBAC objects
during SandboxManager.Release. In controller/agenticrun/pod_handler.go lines
41-45 and 166-169, resolve the AgenticRun namespace from the pod label rather
than pod.Namespace so handlePodEvent and handleTimeEvent locate runs across
namespaces and perform condition updates and sandbox release.
0d5ba09 to
4bf20a2
Compare
openshift-ci
Bot
added
the
needs-rebase
4bf20a2 to
12e357c
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.ai/spec/what/sandbox-execution.md (1)
107-107: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe configuration surface still assigns Result CRs to the
AgenticRunnamespace.Line 107 reads
AgenticRun.metadata.namespace(secrets + result CRs). This PR moves Result CR creation to the operator namespace incontroller/agenticrun/results.goand inbuildResultTemplateincontroller/agenticrun/input_configmap.go. Rule 7a at line 14 and rule 123 also now describe operator-namespace placement for the template while rule 123 keeps "in the AgenticRun namespace" for the per-step RBAC. Update line 107 and rule 123 to one namespace model.🤖 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 @.ai/spec/what/sandbox-execution.md at line 107, Update the namespace model in `.ai/spec/what/sandbox-execution.md`: change the configuration-surface statement at line 107 so Result CRs use the operator namespace while secrets remain in the AgenticRun namespace, and revise rule 123 consistently while preserving its per-step RBAC namespace requirement.
♻️ Duplicate comments (2)
controller/agenticrun/reconciler.go (1)
81-90: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFinalizer removal ignores sandbox release failure, so execution RBAC can leak permanently.
ReleaseSandboxesfailure is logged only. The code then removesrbacCleanupFinalizerand patches.SandboxManager.Releaseperforms the explicit cross-namespaceRole/ClusterRolecleanup, and it returns early whensandboxClaimName(run, step)is empty. Two paths therefore leave granted mutate permissions in the cluster after the run is deleted:
ReleaseSandboxesreturns an error and the finalizer is dropped anyway.status.steps.execution.sandbox.claimNameis empty while execution RBAC already exists.Keep the finalizer and requeue when release fails, with a bounded attempt count as
handleTemplogCleanupdoes.🛡️ Proposed fix
if controllerutil.ContainsFinalizer(&run, rbacCleanupFinalizer) { if err := r.Agent.ReleaseSandboxes(ctx, &run); err != nil { - log.Error(err, "sandbox release failed during deletion") + log.Error(err, "sandbox release failed during deletion, will retry") + return ctrl.Result{RequeueAfter: templogCleanupRequeueAfter}, nil } original := run.DeepCopy()🤖 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 `@controller/agenticrun/reconciler.go` around lines 81 - 90, Update the deletion cleanup around ReleaseSandboxes to retain rbacCleanupFinalizer and requeue when sandbox release fails, using the same bounded retry/attempt-count behavior as handleTemplogCleanup; only remove and patch the finalizer after successful cleanup. Also ensure cleanup still runs when sandboxClaimName is empty so existing execution RBAC is removed rather than returning early.controller/agenticrun/pod_handler.go (1)
226-239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
overallTimedOutstill applies to pods that already terminated.
startTimedOutat line 228 excludesPodRunning,PodSucceeded, andPodFailed.overallTimedOutat line 230 checks onlycreationTimestamp. The retry branch at line 220 only continues whenisStepInProgressis true. ASucceededpod whose step condition is alreadyTruetherefore reaches line 237, andpatchStepConditionoverwritesTruewithFalseand reasonReasonSandboxTimeout.DerivePhasethen reportsFailedfor a step that succeeded. This happens wheneverreleaseSandboxfailed and the pod outlivesstepTimeout(step).🔧 Proposed fix
created := pod.CreationTimestamp.Time + if phase == corev1.PodSucceeded || phase == corev1.PodFailed { + // Terminal pods are handled above; timeout enforcement does not apply. + continue + } var message string if startTimedOut(phase, created, now, podStartTimeout) {🤖 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 `@controller/agenticrun/pod_handler.go` around lines 226 - 239, Restrict the overall timeout branch in the timeout handling flow to non-terminal pod phases, matching the exclusions used by startTimedOut. Update the logic around overallTimedOut so PodSucceeded and PodFailed pods cannot reach patchStepCondition or be changed from a successful condition to ReasonSandboxTimeout; retain timeout handling for eligible active pods.
🧹 Nitpick comments (2)
controller/agenticrun/handlers_test.go (1)
428-431: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueState
Successexplicitly in thisVerificationOutput.The literal omits
Success, so the test depends on the zero value to express failure.TestReconcile_VerificationObjectiveFailure_Failedat line 320 setsSuccess: falseexplicitly. Match that form.♻️ Proposed change
agent.verifyResult = &VerificationOutput{ + Success: false, Checks: []agenticv1alpha1.VerifyCheck{{Name: "pod-running", Source: "oc", Value: "CrashLoopBackOff", Result: agenticv1alpha1.CheckResultFailed}}, Summary: "Pod still crashing", }🤖 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 `@controller/agenticrun/handlers_test.go` around lines 428 - 431, Update the VerificationOutput literal in TestReconcile_VerificationObjectiveFailure_Failed to set Success explicitly to false, matching the existing test pattern while preserving the current Checks and Summary values.controller/agenticrun/reconciler_test.go (1)
130-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test double writes Result CRs to the run namespace, so the namespace change is untested.
Line 142 uses
Namespace: fresh.Namespace. Production code incontroller/agenticrun/results.goandcontroller/agenticrun/input_configmap.gonow uses the operator namespace. Every test sets both values to"default", so no test detects a divergence. Line 139 also computes the index aslen(...)while production useslen(...)+1andnextResultIndex.Use
ta.nsfor the namespace and the production index helper, then add one case where the run namespace differs fromta.ns.completeExecution,completeVerification, andcompleteEscalationhave the same two issues.🤖 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 `@controller/agenticrun/reconciler_test.go` around lines 130 - 158, Update test-double methods completeAnalysis, completeExecution, completeVerification, and completeEscalation to use ta.ns for result CR namespaces and the production nextResultIndex helper instead of len(...). Add a test case with a run namespace different from ta.ns, while preserving existing behavior for matching namespaces.
🤖 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 @.ai/spec/what/sandbox-execution.md:
- Around line 14-15: Update rule 27 in the sandbox execution specification to
reflect that retryIndex remains present on ExecutionResult and
VerificationResult and is populated in the result template. Ensure the rule no
longer states that retryIndex is removed, while preserving the surrounding
result-template and output-delivery requirements.
- Line 18: Update the sandbox-execution specification rules 9 and 43a to match
the current reconciler behavior: remove requirements for SetupWithManager to Own
Pods, ConfigMaps, and Result CRs where unsupported, remove the mandatory
RequeueAfter(30s) requirement, and document the runTimeoutLoop-based timeout
behavior. Ensure completion detection remains covered by an appropriate watch or
explicitly record the needed implementation change if the specification requires
sandbox completion updates to trigger reconciliation.
In `@controller/agenticrun/pod_handler.go`:
- Around line 35-45: Update pod identity handling in handlePodEvent and
handleTimeEvent to persist the full AgenticRun name and namespace in pod
annotations when creating or managing pods, rather than relying on the truncated
LabelRun and pod namespace. During lookup, read those annotations and use the
full name and stored namespace in client.ObjectKey, preserving the existing
missing-run behavior.
- Around line 284-327: Preserve the Result CR name generated for the pod
template when launch setup begins, rather than recomputing it with
nextResultIndex in fetchResultCR. Update the launch/completeStep flow and
fetchResultCR to pass or retrieve that stored name, so a same-step result
appended by failStep does not change the lookup target and a completed pod still
resolves its successful Result CR.
In `@controller/agenticrun/results.go`:
- Around line 95-101: Use a single namespace/ownership model for all child
resources: place the four Result CRs in run.Namespace while retaining
agenticRunOwnerRef(run), updating the result constructions at
controller/agenticrun/results.go lines 95-101, 153, 205, and 258. Apply the same
namespace correction to buildResultTemplate in
controller/agenticrun/input_configmap.go and AgenticRunApproval in
controller/agenticrun/approval.go lines 86-98; keep the reconciler Owns watch
unchanged because the owner remains valid in the child namespace.
In `@controller/agenticrun/state_machine_test.go`:
- Around line 767-787: Update the production verification flow, specifically
handleVerification, to set the Escalated condition to Unknown when verification
retries are exhausted so DerivePhase can enter AgenticRunPhaseEscalating and
reconciler.go can invoke handleEscalation. Replace the driveToEscalating test
setup with a reconciliation-driven scenario that exhausts verification retries
and asserts the Escalating transition.
In `@docs/unifying-approach-to-rbac-generation.md`:
- Around line 89-98: Add get, list, and watch verbs to the deployments entry in
the Derived RBAC example while retaining patch and resourceNames: ["web-app"],
so rollout status requests for the named deployment are authorized.
---
Outside diff comments:
In @.ai/spec/what/sandbox-execution.md:
- Line 107: Update the namespace model in `.ai/spec/what/sandbox-execution.md`:
change the configuration-surface statement at line 107 so Result CRs use the
operator namespace while secrets remain in the AgenticRun namespace, and revise
rule 123 consistently while preserving its per-step RBAC namespace requirement.
---
Duplicate comments:
In `@controller/agenticrun/pod_handler.go`:
- Around line 226-239: Restrict the overall timeout branch in the timeout
handling flow to non-terminal pod phases, matching the exclusions used by
startTimedOut. Update the logic around overallTimedOut so PodSucceeded and
PodFailed pods cannot reach patchStepCondition or be changed from a successful
condition to ReasonSandboxTimeout; retain timeout handling for eligible active
pods.
In `@controller/agenticrun/reconciler.go`:
- Around line 81-90: Update the deletion cleanup around ReleaseSandboxes to
retain rbacCleanupFinalizer and requeue when sandbox release fails, using the
same bounded retry/attempt-count behavior as handleTemplogCleanup; only remove
and patch the finalizer after successful cleanup. Also ensure cleanup still runs
when sandboxClaimName is empty so existing execution RBAC is removed rather than
returning early.
---
Nitpick comments:
In `@controller/agenticrun/handlers_test.go`:
- Around line 428-431: Update the VerificationOutput literal in
TestReconcile_VerificationObjectiveFailure_Failed to set Success explicitly to
false, matching the existing test pattern while preserving the current Checks
and Summary values.
In `@controller/agenticrun/reconciler_test.go`:
- Around line 130-158: Update test-double methods completeAnalysis,
completeExecution, completeVerification, and completeEscalation to use ta.ns for
result CR namespaces and the production nextResultIndex helper instead of
len(...). Add a test case with a run namespace different from ta.ns, while
preserving existing behavior for matching namespaces.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ec4ce50-a777-4c02-b40b-0e03199a74aa
📒 Files selected for processing (22)
.ai/spec/how/reconciler.md.ai/spec/what/sandbox-execution.mdcontroller/agenticrun/approval.gocontroller/agenticrun/approval_test.gocontroller/agenticrun/handlers.gocontroller/agenticrun/handlers_test.gocontroller/agenticrun/helpers.gocontroller/agenticrun/input_configmap.gocontroller/agenticrun/input_configmap_test.gocontroller/agenticrun/pod_handler.gocontroller/agenticrun/rbac.gocontroller/agenticrun/rbac_test.gocontroller/agenticrun/reconciler.gocontroller/agenticrun/reconciler_test.gocontroller/agenticrun/results.gocontroller/agenticrun/sandbox_agent.gocontroller/agenticrun/sandbox_agent_test.gocontroller/agenticrun/sandbox_manager.gocontroller/agenticrun/state_machine_test.godocs/inter-operator-handoff-design.mddocs/rbac.mddocs/unifying-approach-to-rbac-generation.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
🚧 Files skipped from review as they are similar to previous changes (11)
- controller/agenticrun/rbac_test.go
- controller/agenticrun/input_configmap.go
- docs/rbac.md
- controller/agenticrun/input_configmap_test.go
- controller/agenticrun/sandbox_manager.go
- .ai/spec/how/reconciler.md
- controller/agenticrun/sandbox_agent.go
- controller/agenticrun/helpers.go
- controller/agenticrun/handlers.go
- docs/inter-operator-handoff-design.md
- controller/agenticrun/rbac.go
| runName := pod.Labels[LabelRun] | ||
| step := pod.Labels[LabelStep] | ||
| if runName == "" || step == "" { | ||
| return nil | ||
| } | ||
|
|
||
| // Look up the owning AgenticRun. Gone → nothing to do. | ||
| var run agenticv1alpha1.AgenticRun | ||
| if err := r.Get(ctx, client.ObjectKey{Name: runName, Namespace: pod.Namespace}, &run); err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the run label value with the AgenticRun name, and check pod vs run namespace.
set -eu
echo '--- truncateK8sName definition ---'
ast-grep run --pattern 'func truncateK8sName($$$) $_ { $$$ }' --lang go controller
echo '--- pod label and namespace assignment ---'
rg -nP --type=go -C10 'LabelRun:\s*' controller/agenticrun/sandbox_manager.go
echo '--- every AgenticRun lookup keyed by pod namespace ---'
rg -nP --type=go -C4 'client\.ObjectKey\{Name:\s*runName' controller/agenticrun
echo '--- AgenticRun name length validation ---'
rg -nP -C3 'MaxLength' api/v1alpha1/agenticrun_types.go | head -40Repository: openshift/lightspeed-agentic-operator
Length of output: 5382
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- pod handler ---'
sed -n '1,270p' controller/agenticrun/pod_handler.go
echo '--- run name schema and controller wiring ---'
rg -n -C5 'type AgenticRun|metadata.name|namespace|NewControllerManagedBy|Owns|Pod' api controller .ai/spec/what/sandbox-execution.md | head -240
echo '--- namespace and pod manager construction ---'
rg -n -C6 'NewSandboxManager|namespace:|m\.namespace|AgenticRun.*Namespace|TargetNamespaces' controller/agenticrun
echo '--- tests for pod handler and long names ---'
rg -n -C5 'pod_handler|LabelRun|truncateK8sName|AgenticRun.*253|long name|namespace' --glob '*_test.go' .Repository: openshift/lightspeed-agentic-operator
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- reconciler namespace and pod watch setup ---'
rg -n -C8 'type AgenticRunReconciler|Namespace string|SetupWithManager|Owns\(' controller/agenticrun --glob '*.go'
echo '--- sandbox manager namespace assignment ---'
rg -n -C8 'NewSandboxManager|SandboxManager\{|namespace:' controller/agenticrun --glob '*.go'
echo '--- AgenticRun name validation ---'
rg -n -C5 'AgenticRun.*metadata|Name.*MaxLength|MaxLength=253|dns1123Subdomain' api config --glob '*.go' --glob '*.yaml' --glob '*.yml' || true
echo '--- focused tests ---'
rg -n -C6 'handlePodEvent|handleTimeEvent|LabelRun|OwnerReferences|Namespace.*operator|truncateK8sName' controller/agenticrun --glob '*_test.go' || trueRepository: openshift/lightspeed-agentic-operator
Length of output: 50393
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
def truncate_k8s_name(name):
if len(name) > 63:
name = name[:63].rstrip("-._")
return name
for n in (63, 64, 253):
name = "a" * n
label = truncate_k8s_name(name)
print(f"length={n} label_length={len(label)} label_equals_name={label == name}")
PY
echo '--- reconciler namespace initialization ---'
rg -n -C8 'AgenticRunReconciler\{|NewSandboxManager\(' --glob '*.go' .
echo '--- AgenticRun resource scope and metadata name constraints ---'
rg -n -C3 'kind: AgenticRun|scope: Namespaced|metadata.name|name.*DNS|DNS.*253' config/crd/bases/agentic.openshift.io_agenticruns.yaml api/v1alpha1 --glob '*.yaml' --glob '*.go' | head -100
echo '--- full owner reference construction ---'
sed -n '255,280p' controller/agenticrun/sandbox_manager.goRepository: openshift/lightspeed-agentic-operator
Length of output: 50393
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- reconcile paths that inspect sandbox pods or terminal steps ---'
rg -n -C8 'PodList|Get\(ctx,.*Pod|sandbox\.ClaimName|ClaimName|PodSucceeded|PodFailed|releaseSandbox|completeStep|RequeueAfter' controller/agenticrun --glob '*.go' | head -260
echo '--- exact AgenticRun definition and generated resource scope ---'
rg -n -C8 'type AgenticRun struct|kubebuilder:resource:scope|AgenticRun represents' api/v1alpha1/agenticrun_types.go config/crd/bases/agentic.openshift.io_agenticruns.yamlRepository: openshift/lightspeed-agentic-operator
Length of output: 25043
Resolve pods with the full AgenticRun identity
LabelRun contains truncateK8sName(run.Name), so any valid AgenticRun name longer than 63 characters cannot be found. Pods also run in the configured operator namespace, while AgenticRun is namespaced and can exist elsewhere. Both handlePodEvent and handleTimeEvent therefore skip status updates and sandbox release without logging.
Persist the full run name and namespace in pod annotations and use them for lookup. The owner reference contains the full name and UID but not the namespace.
🤖 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 `@controller/agenticrun/pod_handler.go` around lines 35 - 45, Update pod
identity handling in handlePodEvent and handleTimeEvent to persist the full
AgenticRun name and namespace in pod annotations when creating or managing pods,
rather than relying on the truncated LabelRun and pod namespace. During lookup,
read those annotations and use the full name and stored namespace in
client.ObjectKey, preserving the existing missing-run behavior.
12e357c to
381ee8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
controller/agenticrun/sandbox_manager_test.go (1)
414-434: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe two release-idempotency tests exercise the same path.
Neither test sets
run.Status.Steps.Analysis.Sandbox.ClaimName, so both callReleasewith an empty claim name. Thesandbox-claimmode inTestRelease_SandboxClaim_Idempotentnever reaches claim deletion. Set a non-existent claim name in each test so the missing-resource path is actually covered per mode.♻️ Proposed change
func TestRelease_SandboxClaim_Idempotent(t *testing.T) { cache := testCache(t, "sandbox-claim") run := testSMRun() + run.Status.Steps.Analysis.Sandbox.ClaimName = "ls-does-not-exist" fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(testReaderCRB()).Build()🤖 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 `@controller/agenticrun/sandbox_manager_test.go` around lines 414 - 434, Update TestRelease_BarePod_Idempotent and TestRelease_SandboxClaim_Idempotent to assign distinct non-existent claim names through run.Status.Steps.Analysis.Sandbox.ClaimName before calling Release, ensuring each mode exercises its missing-resource deletion path rather than the empty-claim early path.
🤖 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 @.ai/spec/what/sandbox-execution.md:
- Line 13: The specification’s owner-reference requirements conflict with
cross-namespace AgenticRun handling because Kubernetes cannot garbage-collect
resources across namespaces. Update rules 7, 24, and 32, including the ConfigMap
requirements here, to use same-namespace dependents or replace cross-namespace
owner references with identifying labels and explicit finalizer-based cleanup.
- Line 13: The ConfigMap naming and idempotent creation flow must prevent a
subsequent step from reusing a previous step’s payload during asynchronous
deletion. Update the per-step ConfigMap creation logic to use a step-specific
name, or alternatively wait for deletion and validate the existing object’s step
identity before accepting AlreadyExists; ensure each step mounts only its own
query, context, and result-template.
- Line 97: Separate SandboxTemplate cleanup from per-run SandboxManager.Release:
do not delete a reusable template as part of releasing one sandbox. Track
template references and remove a template only after no SandboxClaims reference
it, or alternatively make templates run-scoped so each release owns its template
exclusively; preserve the existing reuse behavior unless intentionally changing
the lifecycle model.
In `@controller/agenticrun/pod_handler.go`:
- Around line 150-170: Update patchStepResult to acquire stepCondMu and check
the existing condition with the same terminal-condition guard used by
patchStepCondition before appending or patching. Return without changes when the
condition is already resolved, ensuring repeated PodSucceeded events and timeout
retries do not duplicate StepResultRef entries or advance nextResultIndex.
In `@docs/unifying-approach-to-rbac-generation.md`:
- Around line 60-63: Update the RBAC generation workflow around the MCP rewrite
guidance to define an explicit allow-list of permitted MCP tools, including each
tool’s declared API footprint, and validate the final rewritten artifact against
that allow-list before granting RBAC or execution. Ensure validation rejects
rewrites that add ungranted mutations, target different resources, or retain
unnecessary permissions, rather than relying solely on the pre-rewrite kubectl
commands.
- Around line 56-63: Update the MCP rewrite flow and its documentation so the
final query remains an ordered executable bash remediation script, as required
by the sandbox batch contract. Either extend and validate the batch contract and
sandbox runtime to execute ordered MCP calls, or compile MCP calls back into the
required bash format before writing query; preserve ordered execution and update
the related guidance around the MCP rewrite and final artifact.
---
Nitpick comments:
In `@controller/agenticrun/sandbox_manager_test.go`:
- Around line 414-434: Update TestRelease_BarePod_Idempotent and
TestRelease_SandboxClaim_Idempotent to assign distinct non-existent claim names
through run.Status.Steps.Analysis.Sandbox.ClaimName before calling Release,
ensuring each mode exercises its missing-resource deletion path rather than the
empty-claim early path.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a5a12a42-50bf-4fa3-998a-a38403f01b3b
📒 Files selected for processing (15)
.ai/spec/how/reconciler.md.ai/spec/what/sandbox-execution.md.ai/spec/what/system-config.mdcontroller/agenticrun/audit.gocontroller/agenticrun/audit_test.gocontroller/agenticrun/handlers_test.gocontroller/agenticrun/pod_handler.gocontroller/agenticrun/pod_handler_test.gocontroller/agenticrun/reconciler_test.gocontroller/agenticrun/sandbox_manager.gocontroller/agenticrun/sandbox_manager_test.gocontroller/agenticrun/state_machine_test.godocs/inter-operator-handoff-design.mddocs/rbac.mddocs/unifying-approach-to-rbac-generation.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
🚧 Files skipped from review as they are similar to previous changes (9)
- controller/agenticrun/pod_handler_test.go
- docs/rbac.md
- controller/agenticrun/state_machine_test.go
- .ai/spec/how/reconciler.md
- controller/agenticrun/audit.go
- controller/agenticrun/handlers_test.go
- docs/inter-operator-handoff-design.md
- controller/agenticrun/sandbox_manager.go
- .ai/spec/what/system-config.md
| 5. **Claim naming**: Claim names MUST be derived from run name and step label, truncated to valid Kubernetes name length limits. | ||
| 6. **[OLS-3066] Batch execution model**: Sandbox pods are **batch executors** — the operator does NOT call the sandbox over HTTP. Instead: (a) the operator creates an input ConfigMap with the step payload, (b) mounts it read-only into the sandbox pod, (c) the sandbox runs the agent autonomously, (d) the sandbox creates the Result CR via `oc`, and (e) the sandbox exits. The operator watches for the Result CR to appear. There is no HTTP server in the sandbox; rules 7–9 below replace the former HTTP contract. | ||
| 7. **[OLS-3066] Input delivery — ConfigMap**: For each step invocation, the operator MUST create a namespaced `ConfigMap` in the operator namespace with owner reference to the `AgenticRun` (`controller: true`, `blockOwnerDeletion: true`). Name pattern `ls-input-{step}-{run}` truncated to 63 chars. The ConfigMap MUST contain keys: `query` (step **input** text only — see rules 11–13 and OLS-3491), `system-prompt` [PLANNED: OLS-3491] (step **system instructions** from materialized `spec.<step>.instructions` or escalation resolution), `output-schema` (JSON schema computed by `outputSchemaForStep`), `context` (JSON object with targetNamespaces, previousAttempts, approvedOption, executionResult as applicable), and `result-template` (pre-filled Result CR JSON — see rule 7a). The ConfigMap MUST be mounted read-only into the sandbox pod at `/input/`. Creation MUST be idempotent (`AlreadyExists` = no-op). Until OLS-3491 is implemented, `query` MAY still contain role text from legacy step templates and `system-prompt` MAY be absent/empty. | ||
| 7. **[OLS-3066] Input delivery — ConfigMap**: For each step invocation, the operator MUST create a namespaced `ConfigMap` in the operator namespace with owner reference to the `AgenticRun` (`controller: true`, `blockOwnerDeletion: true`). Name is the AgenticRun UID. Steps are sequential and the ConfigMap is deleted on step cleanup, so one name per run is sufficient. The ConfigMap MUST contain keys: `query` (step **input** text only — see rules 11–13 and OLS-3491), `system-prompt` [PLANNED: OLS-3491] (step **system instructions** from materialized `spec.<step>.instructions` or escalation resolution), `output-schema` (JSON schema computed by `outputSchemaForStep`), `context` (JSON object with targetNamespaces, previousAttempts, approvedOption, executionResult as applicable), and `result-template` (pre-filled Result CR JSON — see rule 7a). The ConfigMap MUST be mounted read-only into the sandbox pod at `/input/`. Creation MUST be idempotent (`AlreadyExists` = no-op). Until OLS-3491 is implemented, `query` MAY still contain role text from legacy step templates and `system-prompt` MAY be absent/empty. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve the cross-namespace owner-reference contract.
Rule 2 allows the AgenticRun namespace to differ from the operator namespace. Rules 7 and 32 then require operator-namespace dependents to reference the AgenticRun. Kubernetes does not provide valid cross-namespace owner-reference garbage collection. The same conflict affects the result RBAC cleanup in rule 24. Use same-namespace dependents, or replace these references with labels and explicit finalizer cleanup.
Also applies to: 97-97
🤖 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 @.ai/spec/what/sandbox-execution.md at line 13, The specification’s
owner-reference requirements conflict with cross-namespace AgenticRun handling
because Kubernetes cannot garbage-collect resources across namespaces. Update
rules 7, 24, and 32, including the ConfigMap requirements here, to use
same-namespace dependents or replace cross-namespace owner references with
identifying labels and explicit finalizer-based cleanup.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not reuse one ConfigMap name for step-specific payloads.
If step cleanup issues an asynchronous delete, the next step can observe AlreadyExists and mount the previous step's query, context, and result-template. Use a step-specific ConfigMap name, or wait for deletion and validate the existing object's step identity before treating AlreadyExists as success.
🤖 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 @.ai/spec/what/sandbox-execution.md at line 13, The ConfigMap naming and
idempotent creation flow must prevent a subsequent step from reusing a previous
step’s payload during asynchronous deletion. Update the per-step ConfigMap
creation logic to use a step-specific name, or alternatively wait for deletion
and validate the existing object’s step identity before accepting AlreadyExists;
ensure each step mounts only its own query, context, and result-template.
|
|
||
| 31. **Sandbox mode selection**: The sandbox mode (`bare-pod` or `sandbox-claim`) is read from the `sandbox-mode` key in the `lightspeed-agentic-configuration` ConfigMap (produced by lightspeed-operator). When the key is omitted or empty, the operator MUST default to `bare-pod` mode. There is no CLI flag — the ConfigMap is the single source of truth. | ||
| 32. **Unified sandbox lifecycle**: For each step invocation, the `SandboxManager` MUST read the base PodSpec from the config cache, overlay agent-specific configuration via `PodSpecBuilder.Build`, and then create either a bare Pod or a SandboxClaim+SandboxTemplate depending on the configured mode. Resource name MUST follow the pattern `ls-{step}-{agenticRunName}` truncated to 63 characters — both modes use the same `ls-` prefix for consistency with the existing naming convention. `Release` dispatches to the correct backend by reading `cfg.Sandbox.Mode` from the config cache. Both resource types MUST carry controller `ownerReferences` to their `AgenticRun` (`controller: true`, `blockOwnerDeletion: true`). [OLS-3066] `WaitReady` is removed — the operator does not poll for pod readiness; it watches for pod completion and Result CR creation. | ||
| 32. **Unified sandbox lifecycle**: `SandboxManager.Create` fully encapsulates sandbox setup for every step: (a) creates a per-step ServiceAccount (`ls-{step}-{namespace}-{runUID}`) with owner reference to the pod/claim, (b) adds the per-step SA to all reader ClusterRoleBindings, (c) for execution: creates cross-namespace Roles/ClusterRoles and persists the RBAC namespaces annotation, (d) builds and creates the input ConfigMap with owner reference, (e) reads the base PodSpec from the config cache, overlays agent-specific configuration via `PodSpecBuilder.Build`, and creates either a bare Pod or a SandboxClaim+SandboxTemplate depending on the configured mode. Resource name MUST follow the pattern `ls-{step}-{agenticRunName}` truncated to 63 characters. `Release` encapsulates sandbox teardown: deletes the pod/claim (GC cascades to SA, ConfigMap, result RBAC via owner refs), removes the per-step SA from reader CRBs, and for execution: explicitly cleans up cross-namespace Roles/ClusterRoles. Both resource types MUST carry controller `ownerReferences` to their `AgenticRun` (`controller: true`, `blockOwnerDeletion: true`). [OLS-3066] `WaitReady` is removed — the operator does not poll for pod readiness; it watches for pod completion and Result CR creation. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Separate shared SandboxTemplate lifecycle from per-run release.
Rules 3–4 allow derived templates to be reused. Rule 35 deletes a SandboxTemplate during one sandbox release. One run can therefore delete a template still referenced by another SandboxClaim. Keep template cleanup separate and remove templates only after no claims reference them, or make templates run-scoped and remove reuse.
🤖 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 @.ai/spec/what/sandbox-execution.md at line 97, Separate SandboxTemplate
cleanup from per-run SandboxManager.Release: do not delete a reusable template
as part of releasing one sandbox. Track template references and remove a
template only after no SandboxClaims reference it, or alternatively make
templates run-scoped so each release owns its template exclusively; preserve the
existing reuse behavior unless intentionally changing the lifecycle model.
| If MCP tools are available, the analysis agent rewrites the remediation script using available MCP tools where they provide a clearer or more reliable execution path. | ||
|
|
||
| Rules for rewriting: | ||
|
|
||
| - **Prefer MCP over raw commands.** If MCP tools are available, rewrite the remediation script preferring MCP tool calls over raw oc/kubectl commands. If a kubectl command has no MCP equivalent, keep it as-is. | ||
| - **RBAC is already complete.** The RBAC derived in step 1 covers all underlying API calls. The MCP rewrite does not add or remove RBAC rules. | ||
|
|
||
| The rewritten script is returned for execution. The purely kubectl-based script is an intermediate representation required for defining RBAC and creating the final script. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align the MCP rewrite with the batch execution contract.
.ai/spec/what/sandbox-execution.md rules 11–12 require query to contain an ordered executable bash remediation script. Rule 21 requires the execution agent to run those commands in order. This document changes the final artifact to JSON MCP tool calls. Update the batch contract and sandbox runtime to support and validate MCP calls, or compile them into the required format before writing query.
Also applies to: 77-79
🤖 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 `@docs/unifying-approach-to-rbac-generation.md` around lines 56 - 63, Update
the MCP rewrite flow and its documentation so the final query remains an ordered
executable bash remediation script, as required by the sandbox batch contract.
Either extend and validate the batch contract and sandbox runtime to execute
ordered MCP calls, or compile MCP calls back into the required bash format
before writing query; preserve ordered execution and update the related guidance
around the MCP rewrite and final artifact.
| - **Prefer MCP over raw commands.** If MCP tools are available, rewrite the remediation script preferring MCP tool calls over raw oc/kubectl commands. If a kubectl command has no MCP equivalent, keep it as-is. | ||
| - **RBAC is already complete.** The RBAC derived in step 1 covers all underlying API calls. The MCP rewrite does not add or remove RBAC rules. | ||
|
|
||
| The rewritten script is returned for execution. The purely kubectl-based script is an intermediate representation required for defining RBAC and creating the final script. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Validate the final MCP artifact before granting RBAC.
The document derives RBAC from pre-rewrite kubectl commands, then executes a different artifact. Tool descriptions do not prove that an MCP call has the same API footprint. An unfaithful rewrite can add an ungranted mutation, target a different resource, or leave excessive permissions. Add a tool allow-list with declared API footprints and validate the final artifact before execution.
Also applies to: 141-143
🤖 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 `@docs/unifying-approach-to-rbac-generation.md` around lines 60 - 63, Update
the RBAC generation workflow around the MCP rewrite guidance to define an
explicit allow-list of permitted MCP tools, including each tool’s declared API
footprint, and validate the final rewritten artifact against that allow-list
before granting RBAC or execution. Ensure validation rejects rewrites that add
ungranted mutations, target different resources, or retain unnecessary
permissions, rather than relying solely on the pre-rewrite kubectl commands.
381ee8e to
98b05a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
controller/agenticrun/reconciler_test.go (3)
130-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe release double is a silent no-op, so release regressions pass.
This layer releases sandboxes on completion and on timeout. Both methods return
nilwithout recording the call. A reconciler change that skips release still passes every test in this file.Record the calls so tests can assert release behavior.
♻️ Proposed refactor
+ releaseAllCount int + releasedSteps []string + func (ta *testAgentCaller) ReleaseSandboxes(_ context.Context, _ *agenticv1alpha1.AgenticRun) error { + ta.releaseAllCount++ return nil } -func (ta *testAgentCaller) ReleaseSandbox(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ string) error { +func (ta *testAgentCaller) ReleaseSandbox(_ context.Context, _ *agenticv1alpha1.AgenticRun, step string) error { + ta.releasedSteps = append(ta.releasedSteps, step) return nil }🤖 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 `@controller/agenticrun/reconciler_test.go` around lines 130 - 136, Update testAgentCaller.ReleaseSandboxes and ReleaseSandbox to record each invocation, including the relevant run or sandbox information, while preserving their successful return behavior. Expose or reuse recorded-call state so reconciler tests can assert releases occur on completion and timeout.
46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
recorddoes not stop the simulation, so a failedGetproduces cascading junk errors.
recordcallst.Errorf, which is non-fatal. Each completion helper continues after a failedGetwith a zero-valuedfresh. The helper then creates a Result CR with an empty name and namespace, and patches a non-existent object. The reported failures then hide the original cause.Make
recordreturn a boolean and stop the helper on the first real error.♻️ Proposed refactor
-func (ta *testAgentCaller) record(err error) { +// record reports err and returns false when the simulation must stop. +func (ta *testAgentCaller) record(err error) bool { if err == nil || ta.t == nil || apierrors.IsAlreadyExists(err) || apierrors.IsConflict(err) { - return + return err == nil || apierrors.IsAlreadyExists(err) || apierrors.IsConflict(err) } ta.t.Helper() ta.t.Errorf("testAgentCaller simulation error: %v", err) + return false }Then guard each
GetincompleteAnalysis,completeExecution,completeVerification, andcompleteEscalation:var fresh agenticv1alpha1.AgenticRun if !ta.record(ta.fc.Get(ctx, client.ObjectKeyFromObject(run), &fresh)) { return }🤖 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 `@controller/agenticrun/reconciler_test.go` around lines 46 - 52, Change testAgentCaller.record to return false for nil/ignored errors and after reporting a real error, otherwise return true; update completeAnalysis, completeExecution, completeVerification, and completeEscalation to guard each Get with record’s boolean result and return immediately when it is false, preventing subsequent operations on an invalid fresh object.
891-915: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMigrate
Requeueusage as a coordinated refactor
Requeueremains supported but is deprecated in controller-runtime v0.23.3. This assertion still matches current behavior. Update the productionctrl.Result{Requeue: true}path and related test assertions together.🤖 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 `@controller/agenticrun/reconciler_test.go` around lines 891 - 915, Migrate the reconcile result handling from deprecated Requeue usage to RequeueAfter in the production reconcile path, preserving the existing requeue behavior. Update the related assertions in this test around reconcileOnce and result to validate the new field consistently, including the expected no-requeue case.controller/agenticrun/rbac_test.go (1)
471-488: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the new result RBAC functions.
ensureResultRBAC,resultRoleName, andsetResultRBACOwnerare new incontroller/agenticrun/rbac.goand have no coverage in this file. They control what a sandbox may create and patch, so the rules and the owner-reference handling should be pinned by tests. Cover at least: the granted verbs and resources per step, the unknown-step error, idempotent repeat calls, and the owner reference set on the Role and RoleBinding.Also note that this fixture omits
UID.cleanupExecutionRBACnow derives every name fromrun.UID, so the test exercises an empty UID suffix. Add aUIDto keep the fixture representative.Run unit tests with
make test.I can generate these tests. Do you want me to open an issue to track it?
🤖 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 `@controller/agenticrun/rbac_test.go` around lines 471 - 488, Expand rbac_test.go with coverage for ensureResultRBAC, resultRoleName, and setResultRBACOwner: verify each step’s granted verbs and resources, reject unknown steps, allow repeated ensure calls idempotently, and assert the Role and RoleBinding owner references. Set a representative UID on the AgenticRun fixture so cleanupExecutionRBAC derives non-empty names from run.UID, then run make test.Source: Coding guidelines
🤖 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 `@controller/agenticrun/handlers_test.go`:
- Around line 371-373: Update every reconcileOnce call in the affected tests,
including the calls near approveAgenticRun and the other referenced locations,
to check its returned error and fail the test immediately when reconciliation
fails. Preserve the existing reconciliation order and assertions while ensuring
no reconcileOnce error is discarded.
In `@controller/agenticrun/rbac_test.go`:
- Line 98: Update both service-account name assertions in the
ensureExecutionRBAC tests to pass the operator namespace supplied to
ensureExecutionRBAC, rather than run.Namespace, when calling sandboxSAName. Keep
the assertions focused on the operator-namespace subject value so they validate
cross-namespace behavior.
In `@controller/agenticrun/rbac.go`:
- Around line 356-367: Update the RBAC rules in the role construction to scope
result access to the deterministic name from resultCRName(run.Name, step,
nextResultIndex(run, step)). Keep create in its existing separate rule without
resourceNames, and add that resourceNames restriction to the get rule for the
result resource and the get, patch, update rule for its status subresource.
In `@controller/agenticrun/sandbox_manager.go`:
- Around line 195-207: Update ensureSA to pass string(run.UID) as the first
argument to rbacLabels when constructing the ServiceAccount labels, matching the
UID-based labeling used by other resources and avoiding invalid long label
values.
- Around line 428-453: Update SandboxManager.Release to attempt every cleanup
operation even when an earlier step fails: run releaseBarePod,
releaseSandboxClaim, removeReaderSubject, and execution-only
cleanupExecutionRBAC without returning immediately on errors. Track and return
the first encountered error after all applicable cleanup steps complete,
preserving the existing claim-name early return and execution-step condition.
- Around line 151-190: Update launchSandbox and the related sandbox lifecycle
helpers to derive resource names and labels from the execution attempt index as
well as step and run. Propagate this attempt-specific identity through
SandboxInfo, release handling, and status lookups so retries create and resolve
distinct pods or claims instead of reusing prior resources; preserve the
existing owner-reference setup.
---
Nitpick comments:
In `@controller/agenticrun/rbac_test.go`:
- Around line 471-488: Expand rbac_test.go with coverage for ensureResultRBAC,
resultRoleName, and setResultRBACOwner: verify each step’s granted verbs and
resources, reject unknown steps, allow repeated ensure calls idempotently, and
assert the Role and RoleBinding owner references. Set a representative UID on
the AgenticRun fixture so cleanupExecutionRBAC derives non-empty names from
run.UID, then run make test.
In `@controller/agenticrun/reconciler_test.go`:
- Around line 130-136: Update testAgentCaller.ReleaseSandboxes and
ReleaseSandbox to record each invocation, including the relevant run or sandbox
information, while preserving their successful return behavior. Expose or reuse
recorded-call state so reconciler tests can assert releases occur on completion
and timeout.
- Around line 46-52: Change testAgentCaller.record to return false for
nil/ignored errors and after reporting a real error, otherwise return true;
update completeAnalysis, completeExecution, completeVerification, and
completeEscalation to guard each Get with record’s boolean result and return
immediately when it is false, preventing subsequent operations on an invalid
fresh object.
- Around line 891-915: Migrate the reconcile result handling from deprecated
Requeue usage to RequeueAfter in the production reconcile path, preserving the
existing requeue behavior. Update the related assertions in this test around
reconcileOnce and result to validate the new field consistently, including the
expected no-requeue case.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6126d964-5d1f-4d9a-a7f9-ac942873c261
📒 Files selected for processing (17)
controller/agenticrun/approval.gocontroller/agenticrun/approval_test.gocontroller/agenticrun/handlers.gocontroller/agenticrun/handlers_test.gocontroller/agenticrun/helpers.gocontroller/agenticrun/helpers_test.gocontroller/agenticrun/input_configmap.gocontroller/agenticrun/input_configmap_test.gocontroller/agenticrun/pod_handler.gocontroller/agenticrun/podspec_builder.gocontroller/agenticrun/rbac.gocontroller/agenticrun/rbac_test.gocontroller/agenticrun/reconciler_test.gocontroller/agenticrun/results.gocontroller/agenticrun/results_test.gocontroller/agenticrun/sandbox_agent.gocontroller/agenticrun/sandbox_manager.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
🚧 Files skipped from review as they are similar to previous changes (8)
- controller/agenticrun/input_configmap_test.go
- controller/agenticrun/approval.go
- controller/agenticrun/helpers.go
- controller/agenticrun/input_configmap.go
- controller/agenticrun/approval_test.go
- controller/agenticrun/pod_handler.go
- controller/agenticrun/handlers.go
- controller/agenticrun/sandbox_agent.go
67caacd to
ace1884
Compare
ace1884 to
784febe
Compare
|
@blublinsky: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Replace the synchronous HTTP operator↔sandbox contract with a batch execution model where sandbox pods run autonomously, create Result CRs, and exit. The operator stays watch-driven and under the 30s wall-clock SLO.
Net result: −1,744 lines (2,947 deletions, 1,203 insertions) — 37% smaller across touched files.
Key changes
SandboxManager encapsulation
SandboxManageris now the single owner of the entire sandbox lifecycle. All concerns are encapsulated behindCreate/Release:Callers (
SandboxAgentCaller, handlers, reconciler) never touch SA, RBAC, ConfigMap, or audit spans directly.Per-step RBAC isolation
Each sandbox step gets its own ServiceAccount (
ls-{step}-{ns}-{runUID}), scoped to create/patch only its specific Result CRD. Owner references on the pod/claim cascade deletion to SA, ConfigMap, and result RBAC — no explicit cleanup needed for same-namespace resources.Async pod handler
Two event sources in
pod_handler.go:handlePodEventviaWatches): patches step condition on terminal pod phases, fetches Result CR for rich audit eventsrunTimeoutLoopviamgr.Add): background goroutine checks start/overall timeoutsAudit span lifecycle
Spans are managed by
SandboxManager, not handlers:Create→BeginStep(opens span) + intermediate span events (SA, ConfigMap, RBAC, pod created)Release→CompleteStep(ends span, safety net)pod_handlercallsCompleteStepwith the Result CR before Release for rich audit dataCreatefailure prevents orphaned spansMock agent batch rewrite
Rewrote
test/agentfrom an HTTP server (POST /v1/agent/run) to a batch CLI:/input/ConfigMap files (query, output-schema, context, result-template)controller-runtimeclientoc/kubectlneededE2E test fixes for batch model
All e2e happy-path tests updated for the new contract:
LabelRunnow storesrun.UID— allMatchingLabelsqueries updatedls-exec-{uid},ls-execution-{ns}-{uid})Batch pod bug fix
PodSpecBuildernow setsRestartPolicy=Never— batch pods must not restart on exit or they never reachSucceededphase.Dead code cleanup
client.go/client_test.go— entire HTTP client (−318 lines)AgentHTTPClient,callWithSandbox,WaitReady— sync interaction modelEmit*Completed,InjectTraceContext— dead audit methodscontroller/sandbox/package — bootstrap SandboxTemplate never referenced by any SandboxClaimtest/agent/cmd/schemadump/— HTTP-era curl helperbootstrap.goandsandboxtemplate.yamlPod handler design
Status patches on AgenticRun trigger reconcile via the existing
For(&AgenticRun{})watch. The reconciler's phase routing has guards (condition already Unknown/True → skip re-launch).Test plan
make fmt— cleanmake vet— cleanmake test— all unit tests passmake api-lint— 0 issuesmake test-e2e— happy path (analysis → proposed → executing → verifying → completed)