Skip to content

OLS-3781: Give verification agent retry autonomy for convergence-dependent checks #423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
blublinsky wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OLS-3781: Give verification agent retry autonomy for convergence-dependent checks #423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions controller/agenticrun/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,10 @@ type AgentHTTPClient struct {
endpoint string
}

func NewAgentHTTPClient(endpoint string) AgentHTTPClientInterface {
func NewAgentHTTPClient(endpoint string, timeout time.Duration) AgentHTTPClientInterface {
return &AgentHTTPClient{
httpClient: &http.Client{
Timeout: 5 * time.Minute,
Timeout: timeout,
Transport: &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // internal cluster traffic
},
Expand Down
13 changes: 7 additions & 6 deletions controller/agenticrun/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net/http"
"net/http/httptest"
"testing"
"time"

agenticv1alpha1 "github.com/openshift/lightspeed-agentic-operator/api/v1alpha1"
)
Expand Down Expand Up @@ -35,7 +36,7 @@ func TestAgentHTTPClient_RunSuccess(t *testing.T) {
}))
defer server.Close()

client := NewAgentHTTPClient(server.URL)
client := NewAgentHTTPClient(server.URL, 5*time.Minute)
resp, err := client.Run(context.Background(), "You are an SRE agent", "check health", nil, nil, nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
Expand All @@ -52,15 +53,15 @@ func TestAgentHTTPClient_RunHTTPError(t *testing.T) {
}))
defer server.Close()

client := NewAgentHTTPClient(server.URL)
client := NewAgentHTTPClient(server.URL, 5*time.Minute)
_, err := client.Run(context.Background(), "", "test", nil, nil, nil)
if err == nil {
t.Fatal("expected error for HTTP 500")
}
}

func TestAgentHTTPClient_RunConnectionError(t *testing.T) {
client := NewAgentHTTPClient("http://127.0.0.1:1")
client := NewAgentHTTPClient("http://127.0.0.1:1", 5*time.Minute)
_, err := client.Run(context.Background(), "", "test", nil, nil, nil)
if err == nil {
t.Fatal("expected error for connection failure")
Expand Down Expand Up @@ -100,7 +101,7 @@ func TestAgentHTTPClient_RunWithExecutionResult(t *testing.T) {
}))
defer server.Close()

client := NewAgentHTTPClient(server.URL)
client := NewAgentHTTPClient(server.URL, 5*time.Minute)
agentCtx := &agentContext{
TargetNamespaces: []string{"production"},
ExecutionResult: &agentExecutionResult{
Expand Down Expand Up @@ -135,7 +136,7 @@ func TestAgentHTTPClient_RunWithoutExecutionResult(t *testing.T) {
}))
defer server.Close()

client := NewAgentHTTPClient(server.URL)
client := NewAgentHTTPClient(server.URL, 5*time.Minute)
agentCtx := &agentContext{
TargetNamespaces: []string{"production"},
}
Expand Down Expand Up @@ -169,7 +170,7 @@ func TestAgentHTTPClient_RunWithContext(t *testing.T) {
}))
defer server.Close()

client := NewAgentHTTPClient(server.URL)
client := NewAgentHTTPClient(server.URL, 5*time.Minute)
agentCtx := &agentContext{
TargetNamespaces: []string{"production"},
PreviousAttempts: []agentPreviousAttempt{{Attempt: 1, FailureReason: "timeout"}},
Expand Down
2 changes: 1 addition & 1 deletion controller/agenticrun/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ func newMockSandboxAgent(analysisJSON, executionJSON, verificationJSON string) (
caller := &SandboxAgentCaller{
Sandbox: sandbox,
K8sClient: fc,
ClientFactory: func(_ string) AgentHTTPClientInterface {
ClientFactory: func(_ string, _ time.Duration) AgentHTTPClientInterface {
resp := responses[callCount%len(responses)]
callCount++
httpClient.response = &agentRunResponse{Response: json.RawMessage(resp)}
Expand Down
3 changes: 3 additions & 0 deletions controller/agenticrun/revision_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ func TestBuildAnalysisQuery_FullAgenticRun(t *testing.T) {
if !strings.Contains(result, "Verification plan") {
t.Error("full run should mention verification plan")
}
if !strings.Contains(result, "read-only cluster access") {
t.Error("full run should constrain verification to read-only commands")
}
if !strings.Contains(result, "Fix the crash") {
t.Error("should contain the request text")
}
Expand Down
21 changes: 19 additions & 2 deletions controller/agenticrun/sandbox_agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ import (
const (
defaultSandboxTimeout = 5 * time.Minute

analysisStepTimeout = 10 * time.Minute
executionStepTimeout = 10 * time.Minute
verificationStepTimeout = 30 * time.Minute

ErrAnalysisAgentCall = "analysis agent call"
ErrParseAnalysisResponse = "parse analysis response"
ErrExecutionAgentCall = "execution agent call"
Expand Down Expand Up @@ -62,7 +66,7 @@ type SandboxLifecycle interface {
type SandboxAgentCaller struct {
Sandbox SandboxLifecycle
K8sClient client.Client
ClientFactory func(endpoint string) AgentHTTPClientInterface
ClientFactory func(endpoint string, timeout time.Duration) AgentHTTPClientInterface
Namespace string
Timeout time.Duration
Audit AuditLogger
Expand Down Expand Up @@ -188,6 +192,19 @@ func (s *SandboxAgentCaller) Escalate(ctx context.Context, run *agenticv1alpha1.
}, nil
}

func stepTimeout(step string) time.Duration {
switch step {
case "analysis", "escalation":
return analysisStepTimeout
case "execution":
return executionStepTimeout
case "verification":
return verificationStepTimeout
default:
return analysisStepTimeout
}
}

func (s *SandboxAgentCaller) callWithSandbox(
ctx context.Context,
run *agenticv1alpha1.AgenticRun,
Expand Down Expand Up @@ -227,7 +244,7 @@ func (s *SandboxAgentCaller) callWithSandbox(
s.Audit.InjectTraceContext(ctx, run, headers)
}

client := s.ClientFactory(agentURL)
client := s.ClientFactory(agentURL, stepTimeout(stepName)-2*time.Minute)
resp, err := client.Run(ctx, "", query, schema, agentCtx, headers)
if err != nil {
return nil, err
Expand Down
10 changes: 8 additions & 2 deletions controller/agenticrun/sandbox_agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func newTestSandboxAgentCaller(sandbox *mockSandboxProvider, httpClient *mockHTT
return &SandboxAgentCaller{
Sandbox: sandbox,
K8sClient: fc,
ClientFactory: func(_ string) AgentHTTPClientInterface { return httpClient },
ClientFactory: func(_ string, _ time.Duration) AgentHTTPClientInterface { return httpClient },
Namespace: "test-ns",
Timeout: 5 * time.Minute,
}
Expand All @@ -76,7 +76,7 @@ func newTestSandboxAgentCallerWithAgenticRun(sandbox *mockSandboxProvider, httpC
return &SandboxAgentCaller{
Sandbox: sandbox,
K8sClient: fc,
ClientFactory: func(_ string) AgentHTTPClientInterface { return httpClient },
ClientFactory: func(_ string, _ time.Duration) AgentHTTPClientInterface { return httpClient },
Namespace: "test-ns",
Timeout: 5 * time.Minute,
}
Expand Down Expand Up @@ -540,6 +540,12 @@ func TestSandboxAgentCaller_VerificationQueryFraming(t *testing.T) {
if strings.Contains(httpClient.lastQuery, "Pod crashing with OOMKilled") {
t.Error("verification query should NOT contain the original request")
}
if !strings.Contains(httpClient.lastQuery, "Convergence-dependent checks") {
t.Error("verification query should contain convergence retry guidance")
}
if !strings.Contains(httpClient.lastQuery, "wait and retry before reporting failure") {
t.Error("verification query should instruct agent to retry convergence checks")
}
Comment on lines +543 to +548

@coderabbitai coderabbitai Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

}

func TestSandboxAgentCaller_ExecutionQueryNilOption(t *testing.T) {
Expand Down
12 changes: 10 additions & 2 deletions controller/agenticrun/templates/analysis_query.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
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.

@vimalk78 vimalk78 Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why delete for OpenShift clusters ?

@blublinsky blublinsky Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cause we are running on Openshift cluster. It does not seem to add any useful info

@vimalk78 vimalk78 Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does the agent know this without encountering oc/kubectl in the prompt. isnt it better to be explicit?

You are an analysis agent. 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.

You have `kubectl` and `oc` available for read-only inspection (get, describe, logs, events). Use them to inspect the cluster BEFORE you diagnose. Do not guess from local files.

## Before diagnosing — determine the failure scope

1. **Cross-service correlation**: Check whether other workloads in the affected namespace(s) show the same or similar errors. Compare event and log timestamps across services to identify correlated failure onset. Also check recent cluster events for infrastructure changes (node conditions, certificate renewals, network policy updates, operator upgrades). If multiple services fail with the same error class at the same time, the root cause is likely a shared dependency.
2. **App vs infrastructure errors**: Distinguish application logic errors from infrastructure errors. Application: NPE, division by zero, assertion failures, panic. Infrastructure: connection refused, pool exhausted, timeout, OOM, disk pressure, certificate expired. Infrastructure errors require infrastructure-level fixes, not application rollbacks.
3. **Trace the dependency graph**: Inspect environment variables, ConfigMaps, and connection strings to discover shared backends — these may reside in other namespaces. If affected workloads share a backend (database, cache, message bus, external API), check the backend's health first.

Match your remediation and verification to the scope. Infrastructure problems need infrastructure-level fixes and checks. Application bugs need application-specific fixes and checks.

When more than one solution exists, propose multiple remediation options. For each option:

- **Diagnose** the root cause with confidence level.
Expand Down Expand Up @@ -40,7 +48,7 @@ When more than one solution exists, propose multiple remediation options. For ea
{{- end}}
{{- 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.

@vimalk78 vimalk78 Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

@blublinsky blublinsky Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{- end}}

- **Risk assessment** and reversibility.
Expand Down
2 changes: 1 addition & 1 deletion controller/agenticrun/templates/execution_query.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
You are an execution agent for OpenShift clusters. Execute the approved remediation option below. Do not re-analyze the problem. Do not propose alternative solutions.
You are an execution agent. Execute the approved remediation option below. Do not re-analyze the problem. Do not propose alternative solutions.

The approved option contains a concrete remediation script — an ordered list of exact bash commands. Follow these rules:

Expand Down
19 changes: 17 additions & 2 deletions controller/agenticrun/templates/verification_query.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,21 @@
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.

Run the verification checks from the approved option's verification plan. Compare the current cluster state against the expected outcomes. Report each check as Passed or Failed with evidence.
Run each check from the approved option's verification plan in order. Report every check as Passed or Failed with evidence.

**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

@vimalk78 vimalk78 Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Warning: Retry tier labels not anchored to VerificationStep.type field — agent must guess from free text

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.

@blublinsky blublinsky Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Some checks depend on cluster state that converges over time after a remediation. For example: alerts clearing, pods becoming ready, or metrics dropping below a threshold.

If a convergence-dependent check fails on the first attempt, wait and retry before reporting failure. Be patient — cluster state often takes minutes to converge after a remediation. Use these guidelines:

- **Alerts** (for example, alert stopped firing): wait 60 seconds between retries, up to 10 retries (~10 minutes). Alerts often have a `for` duration before they clear.
- **Pod readiness / rollout**: wait 15–30 seconds between retries, up to 10 retries (~5 minutes). Image pulls, init containers, and readiness probes all add latency.
- **Metrics** (for example, error rate below threshold): wait 60 seconds between retries, up to 10 retries (~10 minutes). Metrics windows need time to reflect the new state.
- **Instant state checks** (for example, image tag or config value): do not retry. These reflect immediately.

Do not give up early. Use the full retry budget before reporting a check as Failed. If a check passes on a later retry, report it as Passed. If the check still fails after all retries, report Failed with the last observed value.

## Approved Option

Expand Down