OLS-3781: Give verification agent retry autonomy for convergence-dependent checks - #423
OLS-3781: Give verification agent retry autonomy for convergence-dependent checks#423blublinsky wants to merge 1 commit into
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (4)
📝 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. ChangesAgent prompt contracts
🚥 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 |
|
[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: 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)
| if !strings.Contains(httpClient.lastQuery, "Convergence-dependent checks") { | ||
| t.Error("verification query should contain convergence retry guidance") | ||
| } | ||
| if !strings.Contains(httpClient.lastQuery, "wait an appropriate interval and retry") { | ||
| t.Error("verification query should instruct agent to retry convergence checks") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the bounded retry rules.
These assertions only require a heading and a generic retry phrase.
They pass if the wait ranges, five-retry cap, or instant-check exclusion are removed.
Assert each retry interval, the retry cap, and the no-retry rule for instant state checks.
Based on the PR summary, tests must assert retry guidance and the required wait interval.
🤖 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_agent_test.go` around lines 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.
| Some checks depend on cluster state that takes time to converge after a remediation (e.g., alerts clearing, pods becoming ready, metrics dropping below a threshold, conditions stabilizing). For these checks: | ||
|
|
||
| - If the check fails on the first attempt, **wait an appropriate interval and retry** before reporting failure. | ||
| - Use your judgment on wait duration and retry count based on the check type: | ||
| - **Alerts** (e.g., alert stopped firing): wait 30–60 seconds between retries, up to 5 retries. | ||
| - **Pod readiness / rollout**: wait 10–20 seconds between retries, up to 5 retries. | ||
| - **Metrics** (e.g., error rate below threshold): wait 30–60 seconds between retries, up to 5 retries. | ||
| - **Instant state checks** (e.g., image tag matches, config value set): no retry needed — these reflect immediately. | ||
| - A check that **passes on a later retry** should be reported as **Passed**, not Failed. | ||
| - Do not retry indefinitely — if the check still fails after retries, report it as Failed with the last observed value. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set a total retry deadline.
The per-check limits do not limit total wait time across the verification plan.
With several convergence-dependent checks, the agent can exceed the sandbox timeout before it reports results.
Set a deadline from the remaining sandbox time. Stop retries when the remaining time cannot support another attempt.
Based on PR objectives, retries must stay within the existing sandbox timeout.
🤖 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/templates/verification_query.tmpl` around lines 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.
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. |
|
@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. |
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 |
3 participants
Prompt-only change to the verification query template.
The verification agent now has autonomy to handle convergence-dependent checks (alerts clearing, pods becoming ready, metrics stabilizing) by waiting and retrying within its sandbox session, bounded by the existing sandbox timeout.
No schema, API, or controller logic changes.