OLS-3712 OLS-3558: Fix RBAC discovery, execution outcome override, and prompt refinement#379
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@blublinsky: This pull request references OLS-3712 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 bug to target the "5.0.0" version, but no target version was set. This pull request references OLS-3558 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 bug 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. |
|
[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 |
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe controller now classifies execution actions by mutation versus observation, defers eligible failures to verification, manages execution ServiceAccount subjects across all matching cached ClusterRoleBindings, and retries deletion cleanup before removing the RBAC finalizer. ChangesExecution and RBAC lifecycle
Sequence Diagram(s)sequenceDiagram
participant ExecutionAgent
participant AgenticRunReconciler
participant VerificationPhase
ExecutionAgent->>AgenticRunReconciler: report action outcomes
AgenticRunReconciler->>AgenticRunReconciler: classify mutation and observation actions
AgenticRunReconciler->>VerificationPhase: defer outcome when mutations succeeded
VerificationPhase-->>AgenticRunReconciler: return final verification result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/rbac.go`:
- Around line 158-159: In controller/agenticrun/rbac.go, update both retry paths
at lines 158-159 and 209-210 to retain the last conflict returned by c.Update
and wrap it in the final error using the %w format with the existing
ErrAddReaderSubject context, preserving Kubernetes error classification and
diagnostics.
- Around line 166-169: The error handling after resolveReaderBindings in the
cleanup flow must not treat every discovery failure as success. Suppress only
the typed “no bindings remain” condition, while propagating forbidden, timeout,
and other errors; retain removeSubjectFromBinding’s existing IsNotFound handling
for bindings deleted during cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e16719ab-cc84-419a-9a1c-8328fe2544d5
📒 Files selected for processing (10)
.ai/spec/how/reconciler.md.ai/spec/what/run-lifecycle.md.ai/spec/what/sandbox-execution.mdcontroller/agenticrun/handlers.gocontroller/agenticrun/handlers_test.gocontroller/agenticrun/rbac.gocontroller/agenticrun/rbac_test.gocontroller/agenticrun/state_machine_test.gocontroller/agenticrun/templates/analysis_query.tmplcontroller/agenticrun/templates/execution_query.tmpl
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
| } | ||
| return fmt.Errorf("%s: conflict after retries", ErrAddReaderSubject) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve the final conflict cause after retry exhaustion.
Store and wrap the last conflict error so callers retain Kubernetes error classification and diagnostics.
controller/agenticrun/rbac.go#L158-L159: retain the lastc.Updateconflict and wrap it with%w.controller/agenticrun/rbac.go#L209-L210: retain the lastc.Updateconflict and wrap it with%w.
As per coding guidelines, errors should be wrapped with fmt.Errorf("%s: %w", …).
📍 Affects 1 file
controller/agenticrun/rbac.go#L158-L159(this comment)controller/agenticrun/rbac.go#L209-L210
🤖 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 158 - 159, In
controller/agenticrun/rbac.go, update both retry paths at lines 158-159 and
209-210 to retain the last conflict returned by c.Update and wrap it in the
final error using the %w format with the existing ErrAddReaderSubject context,
preserving Kubernetes error classification and diagnostics.
Source: Coding guidelines
217d762 to
3bf255a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controller/agenticrun/state_machine_test.go (1)
628-641: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the mutation-aware execution branches.
This test only verifies the no-mutation hard-failure path. Add cases for
Success=falsewith a successful mutation, a failed mutation, and failed observation actions; assert the resulting phase and status outcome. Run the unit suite withmake test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/agenticrun/state_machine_test.go` around lines 628 - 641, The test coverage around TestManualApproval_ExecutionSuccessFalse_NoMutations_Fails only exercises the no-mutation hard-failure path. Add cases for execution Success=false with a successful mutation, a failed mutation, and failed observation actions, asserting each resulting phase and status outcome according to the state machine behavior. Run the unit suite with make test.
🤖 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.go`:
- Around line 337-341: Update the execution-result handling around
hasMutationSuccess so only mutations with ActionOutcomeSucceeded can override
execResult.Success=false and proceed to verification. Replace the broad
non-failed outcome check with an explicit ActionOutcomeSucceeded comparison, and
apply the same explicit comparison in the mutation scan site to exclude skipped,
pending, or unknown outcomes.
- Around line 778-785: Update isObservationAction and the parse/status boundary
used by hasMutationSuccess to validate ExecutionAction.Type against an explicit
allowlist of recognized observation and mutation categories. Ensure unknown or
misspelled action types are rejected or ignored and cannot fall through as
mutating actions that override workflow status; preserve existing behavior for
recognized types.
---
Nitpick comments:
In `@controller/agenticrun/state_machine_test.go`:
- Around line 628-641: The test coverage around
TestManualApproval_ExecutionSuccessFalse_NoMutations_Fails only exercises the
no-mutation hard-failure path. Add cases for execution Success=false with a
successful mutation, a failed mutation, and failed observation actions,
asserting each resulting phase and status outcome according to the state machine
behavior. Run the unit suite with make test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f604f6f4-2b80-41f5-894c-e14eee469acc
📒 Files selected for processing (12)
.ai/spec/how/reconciler.md.ai/spec/what/run-lifecycle.md.ai/spec/what/sandbox-execution.mdcontroller/agenticrun/handlers.gocontroller/agenticrun/handlers_test.gocontroller/agenticrun/helpers.gocontroller/agenticrun/rbac.gocontroller/agenticrun/rbac_test.gocontroller/agenticrun/reconciler.gocontroller/agenticrun/state_machine_test.gocontroller/agenticrun/templates/analysis_query.tmplcontroller/agenticrun/templates/execution_query.tmpl
🔗 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/templates/execution_query.tmpl
- .ai/spec/what/sandbox-execution.md
- controller/agenticrun/templates/analysis_query.tmpl
- controller/agenticrun/handlers_test.go
- .ai/spec/what/run-lifecycle.md
- .ai/spec/how/reconciler.md
- controller/agenticrun/rbac.go
- controller/agenticrun/rbac_test.go
| func isObservationAction(actionType string) bool { | ||
| switch actionType { | ||
| case "pre-check", "post-check", "verification", "check", "wait": | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate handlers.go and relevant symbols =="
fd -a 'handlers\.go$' . | sed 's#^\./##'
echo
echo "== File outline =="
ast-grep outline controller/agenticrun/handlers.go || true
echo
echo "== Relevant isObservationAction and hasMutationSuccess usages =="
rg -n "isObservationAction|hasMutationSuccess|handleExecution|ActionType|Observation|mutation|Success" controller/agenticrun -C 3
echo
echo "== Read relevant handlers.go sections =="
sed -n '720,830p' controller/agenticrun/handlers.go
echo
sed -n '830,960p' controller/agenticrun/handlers.goRepository: openshift/lightspeed-agentic-operator
Length of output: 50393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handlers.go relevant implementation =="
sed -n '759,800p' controller/agenticrun/handlers.go
echo
echo "== all ExecutionAction Type usages =="
rg -n "ExecutionAction|ExecutionPlan|MutationPlan|proposed action|ActionType|propose|type.*action|mutation|check|verification|wait|pre-check|post-check" --glob '*.go' api controller | sed -n '1,240p'
echo
echo "== schema constants around action types =="
sed -n '1,100p' controller/agenticrun/schemas.go
echoRepository: openshift/lightspeed-agentic-operator
Length of output: 35662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== executionResult schema/type definitions =="
sed -n '40,105p' api/v1alpha1/agenticrun_status_types.go
sed -n '280,340p' api/v1alpha1/agenticrun_analysis_types.go
echo
echo "== execution query/action type mentions =="
rg -n -C 3 "'mutation'|\"mutation|mutation|ActionOutcome|ActionOutcomeFromBool" controller/agenticrun templates -g '*.go' -g '*.tmpl'
echo
echo "== deterministic behavior probe for isObservationAction logic =="
python3 - <<'PY'
observation = {"pre-check", "post-check", "verification", "check", "wait"}
def is_observation(action_type: str) -> bool:
return action_type in observation
def has_mutation_success(actions):
found = False
for action in actions:
if is_observation(action["type"]):
continue
if action["outcome"] == "Failed":
return False
found = True
return found
cases = [
("unrecognized action", [{"type": "dry-run", "outcome": "Succeeded"}]),
("typo observation", [{"type": "pre_check", "outcome": "Failed"}]),
("unknown succeeded only", [{"type": "unknown", "outcome": "Succeeded"}]),
("unknown failed only", [{"type": "unknown", "outcome": "Failed"}]),
]
for name, actions in cases:
print(name, "hasMutationSuccess=", has_mutation_success(actions))
PYRepository: openshift/lightspeed-agentic-operator
Length of output: 40415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== execution action type schema enum/values if present =="
python3 - <<'PY'
import json, re, pathlib
for path in pathlib.Path('controller/agenticrun').glob('*.go'):
text = path.read_text()
# Extract JSONRawMessage values containing ExecutionAction type schemas/CRD snippets
for m in re.finditer(r'json\.RawMessage\s*\(`(.*?)`\)', text, flags=re.S):
s = m.group(1)
if '"type"' in s and ('executionresult' in str(path).lower() or 'ExecutionAction' in str(path).lower() or 'actionsTaken' in s):
print('---', path)
# Print the compacted action object schema if present
data = json.loads(s)
for k,v in data.get('properties', {}).items():
if k == 'commands' or k == 'actions' or k == 'actionsTaken' or k == 'steps':
print(k, v)
PY
echo
echo "== read only action constants/type validation =="
rg -n "enum=|ValidationEnum|ProposedAction|ExecutionAction|\"mutation\"|'mutation'|pre-check|post-check|verification|wait|check" -g '*.go' api/v1alpha1 controller/agenticrun | sed -n '1,260p'
echo
echo "== deterministic behavior probe for isObservationAction logic =="
python3 - <<'PY'
observation = {"pre-check", "post-check", "verification", "check", "wait"}
def is_observation(action_type: str) -> bool:
return action_type in observation
def has_mutation_success(actions):
found = False
for action in actions:
if is_observation(action["type"]):
continue
if action["outcome"] == "Failed":
return False
found = True
return found
cases = [
("unrecognized action", [{"type": "dry-run", "outcome": "Succeeded"}]),
("typo observation", [{"type": "pre_check", "outcome": "Failed"}]),
("unknown succeeded only", [{"type": "unknown", "outcome": "Succeeded"}]),
("unknown failed only", [{"type": "unknown", "outcome": "Failed"}]),
]
for name, actions in cases:
print(name, "hasMutationSuccess=", has_mutation_success(actions))
PYRepository: openshift/lightspeed-agentic-operator
Length of output: 30993
Fail closed for unvalidated action types.
ExecutionAction.Type is a free-form string, but hasMutationSuccess() treats any type outside the observation allowlist as a mutating action. An unrecognized or misspelled action can make success=false execution result count as mutation success and override the workflow status. Validate execution action types at parse/status boundary with an allowlist of mutation observation categories, or reject/ignore unknown types.
🤖 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.go` around lines 778 - 785, Update
isObservationAction and the parse/status boundary used by hasMutationSuccess to
validate ExecutionAction.Type against an explicit allowlist of recognized
observation and mutation categories. Ensure unknown or misspelled action types
are rejected or ignored and cannot fall through as mutating actions that
override workflow status; preserve existing behavior for recognized types.
Source: Path instructions
3bf255a to
31b3496
Compare
…d prompt refinement
31b3496 to
3c22cb3
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
OLS-3712: Fix read RBAC multi-binding resolution —
discoverReaderBindingonly stored the firstClusterRoleBindingmatching thelightspeed-agentSA, silently dropping others (e.g.cluster-monitoring-view). Replaced withresolveReaderBindingswhich discovers all matching CRBs, caches them, and adds/removes the per-run SA to every one.OLS-3558: Fix execution false failure — when the execution agent reports
success=falsebut all mutating actions succeeded (only pre-checks or inline verification failed), the controller now overrides the outcome toSucceededand proceeds to the verification step instead of hard-failing.OLS-3558 (prompts): Remove pre-checks and post-checks from the analysis remediation script (analysis already inspected the cluster; verification is a separate step). Remove inline verification instruction from the execution prompt. Scope RBAC derivation to mutations and subresource access only. Add syntax error recovery and non-blocking failure handling as separate execution rules.
Changes
rbac.go,rbac_test.gohandlers.go,handlers_test.go,state_machine_test.goanalysis_query.tmpl,execution_query.tmplsandbox-execution.md,run-lifecycle.md,how/reconciler.mdTest plan
make test— all unit tests passcluster-readerandcluster-monitoring-viewCRBs