fix: per-step timeoutMinutes, tools.dataSource, and review follow-ups

Author: sakshiep1

.ai/spec/how/reconciler.md

@@ -146,7 +146,7 @@ Audience: AI agents. Behavioral rules and phase semantics live in **what/** spec
 - **`AgentCaller`:** Boundary between reconciler and runtime (stub vs sandbox+HTTP). Methods mirror workflow steps plus `ReleaseSandboxes`.
 - **`SandboxProvider`:** Swappable claim/wait/release (tests can fake).
 - **`resolveProposal`:** Produces `resolvedWorkflow` with cached `Agent` + `LLMProvider` per name; applies per-stage agent overrides from `ProposalApproval` via `getStageOverrideAgent`; `Execution`/`Verification` steps nil when corresponding spec sections are zero.
-- **`EnsureAgentTemplate`:** Deterministic derived `SandboxTemplate` name from hash of LLM spec, model, skills, MCP servers, required secrets, step, and **base template resourceVersion**. Patches pod template env/volumes for credentials, Vertex/Bedrock/Azure extras, skills image/paths, MCP JSON env, `LIGHTSPEED_MODE`. GC older templates labeled for same agent+step.
+- **`EnsureAgentTemplate`:** Deterministic derived `SandboxTemplate` name from hash of LLM spec, model, skills, MCP servers, required secrets, dataSource PVC, step, and **base template resourceVersion**. `dataSource` is extracted from `tools.DataSource` (set in `ToolsSpec`). Patches pod template env/volumes for credentials, Vertex/Bedrock/Azure extras, skills image/paths, MCP JSON env, `LIGHTSPEED_MODE`, and optional `/data/input` PVC mount. GC older templates labeled for same agent+step.
 
 ---
 

api/v1alpha1/proposal_types.go

@@ -258,10 +258,22 @@ type ProposalStep struct {
 	// for this step. Use this when different steps need different skills.
 	// +optional
 	Tools ToolsSpec `json:"tools,omitzero"`
+
+	// timeoutMinutes sets the timeout for this step's sandbox agent call.
+	// This controls how long the operator waits for the sandbox pod to
+	// become ready and for the agent to complete its work. Increase this
+	// for long-running tools (e.g., IntelliAide RCA takes 10-30 minutes).
+	// Defaults to 5 minutes when omitted.
+	//
+	// Mutable: can be adjusted before approving a step.
+	// +optional
+	// +kubebuilder:validation:Minimum=1
+	// +kubebuilder:validation:Maximum=60
+	TimeoutMinutes *int32 `json:"timeoutMinutes,omitempty"`
 }
 
 func (s ProposalStep) IsZero() bool {
-	return s.Agent == "" && s.Tools.IsZero()
+	return s.Agent == "" && s.Tools.IsZero() && s.TimeoutMinutes == nil
 }
 
 // DataSource references a pre-existing PersistentVolumeClaim containing
@@ -294,7 +306,6 @@ type DataSource struct {
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.analysis) || (has(self.analysis) && self.analysis == oldSelf.analysis)",message="analysis is immutable once set"
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.execution) || (has(self.execution) && self.execution == oldSelf.execution)",message="execution is immutable once set"
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.verification) || (has(self.verification) && self.verification == oldSelf.verification)",message="verification is immutable once set"
-// +kubebuilder:validation:XValidation:rule="!has(oldSelf.dataSource) || (has(self.dataSource) && self.dataSource == oldSelf.dataSource)",message="dataSource is immutable once set"
 type ProposalSpec struct {
 	// request is the user's original request, alert description, or a
 	// description of what triggered this proposal. This text is passed to
@@ -342,25 +353,17 @@ type ProposalSpec struct {
 	AnalysisOutput AnalysisOutput `json:"analysisOutput,omitzero"`
 
 	// tools defines the default tools for all steps: skills images,
-	// MCP servers, and required secrets. Per-step tools
-	// (analysis.tools, execution.tools, verification.tools) replace
-	// this default for individual steps.
+	// MCP servers, required secrets, and an optional dataSource PVC.
+	// Per-step tools (analysis.tools, execution.tools, verification.tools)
+	// replace this default for individual steps, so a dataSource set in
+	// spec.analysis.tools is mounted only in the analysis sandbox.
 	//
 	// Immutable: the skills and secrets available to the agent are
 	// fixed at creation. Changing tools mid-flight could violate the
 	// assumptions of an in-progress analysis or execution.
 	// +optional
 	Tools ToolsSpec `json:"tools,omitzero"`
 
-	// dataSource references a PVC containing pre-populated input data
-	// (e.g., must-gather bundles, diagnostic data). The operator mounts
-	// it read-only at /data/input in the sandbox pod. Skills discover
-	// input data at this standard location.
-	//
-	// Immutable: input data source is fixed at creation.
-	// +optional
-	DataSource *DataSource `json:"dataSource,omitzero"`
-
 	// analysis defines per-step configuration for the analysis step,
 	// including which agent handles it and any per-step tools.
 	//
@@ -382,18 +385,6 @@ type ProposalSpec struct {
 	// +optional
 	Verification ProposalStep `json:"verification,omitzero"`
 
-	// timeoutMinutes sets the per-step timeout for sandbox agent calls.
-	// This controls how long the operator waits for the sandbox pod to
-	// become ready and for the agent to complete its work. Increase this
-	// for long-running tools (e.g., IntelliAide RCA takes 10-30 minutes).
-	// Defaults to 5 minutes when omitted.
-	//
-	// Mutable: can be adjusted before approving a step.
-	// +optional
-	// +kubebuilder:validation:Minimum=1
-	// +kubebuilder:validation:Maximum=60
-	TimeoutMinutes *int32 `json:"timeoutMinutes,omitempty"`
-
 	// revisionFeedback is the user's free-text feedback requesting changes
 	// to the analysis. Patching this field bumps metadata.generation, which
 	// the operator detects (generation > observedGeneration) and triggers

api/v1alpha1/tools_types.go

@@ -109,12 +109,15 @@ type SecretRequirement struct {
 }
 
 // ToolsSpec defines the tools available to an agent in its sandbox pod.
-// This includes skills images, MCP servers, and required secrets.
+// This includes skills images, MCP servers, required secrets, and an
+// optional data source PVC.
 //
 // ToolsSpec is specified on a Proposal either as a shared default
 // (spec.tools) or per-step (spec.analysis.tools, spec.execution.tools,
 // spec.verification.tools). Per-step tools replace the shared default
-// for that step.
+// for that step, so a dataSource set in spec.analysis.tools is mounted
+// only in the analysis sandbox, while one in spec.tools is mounted in
+// every step that does not override tools.
 //
 // +kubebuilder:validation:MinProperties=1
 type ToolsSpec struct {
@@ -147,8 +150,16 @@ type ToolsSpec struct {
 	// +kubebuilder:validation:MinItems=1
 	// +kubebuilder:validation:MaxItems=20
 	RequiredSecrets []SecretRequirement `json:"requiredSecrets,omitempty"`
+
+	// dataSource references a pre-existing PersistentVolumeClaim containing
+	// input data for this step (e.g., must-gather bundles, diagnostic data).
+	// The PVC must already exist in the same namespace as the Proposal and be
+	// pre-populated with data before the Proposal is created. The operator
+	// mounts it read-only at /data/input in the sandbox pod.
+	// +optional
+	DataSource *DataSource `json:"dataSource,omitempty"`
 }
 
 func (t ToolsSpec) IsZero() bool {
-	return len(t.Skills) == 0 && len(t.MCPServers) == 0 && len(t.RequiredSecrets) == 0
+	return len(t.Skills) == 0 && len(t.MCPServers) == 0 && len(t.RequiredSecrets) == 0 && t.DataSource == nil
 }

api/v1alpha1/zz_generated.deepcopy.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
 	agenticv1alpha1 "github.com/openshift/lightspeed-agentic-operator/api/v1alpha1"
 )
@@ -179,3 +180,64 @@ func TestAgentHTTPClient_RunWithContext(t *testing.T) {
 		t.Fatalf("unexpected error: %v", err)
 	}
 }
+
+// TestAgentHTTPClient_TimeoutMs_CustomTimeout verifies that a positive timeout
+// is serialized as timeout_ms in the request body sent to the agent.
+func TestAgentHTTPClient_TimeoutMs_CustomTimeout(t *testing.T) {
+	const customTimeout = 10 * time.Second
+	var gotTimeoutMs *int64
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		var req agentRunRequest
+		if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+			t.Fatalf("decode request: %v", err)
+		}
+		gotTimeoutMs = req.TimeoutMs
+		w.Header().Set("Content-Type", "application/json")
+		w.Write([]byte(`{"success": true}`))
+	}))
+	defer server.Close()
+
+	c := NewAgentHTTPClient(server.URL, customTimeout)
+	if _, err := c.Run(context.Background(), "", "ping", nil, nil); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if gotTimeoutMs == nil {
+		t.Fatal("timeout_ms was not sent in request")
+	}
+	wantMs := int64(customTimeout / time.Millisecond)
+	if *gotTimeoutMs != wantMs {
+		t.Errorf("timeout_ms = %d, want %d", *gotTimeoutMs, wantMs)
+	}
+}
+
+// TestAgentHTTPClient_TimeoutMs_ZeroFallsBackToDefault verifies that timeout <= 0
+// falls back to defaultSandboxTimeout and is reflected in timeout_ms.
+func TestAgentHTTPClient_TimeoutMs_ZeroFallsBackToDefault(t *testing.T) {
+	var gotTimeoutMs *int64
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		var req agentRunRequest
+		if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+			t.Fatalf("decode request: %v", err)
+		}
+		gotTimeoutMs = req.TimeoutMs
+		w.Header().Set("Content-Type", "application/json")
+		w.Write([]byte(`{"success": true}`))
+	}))
+	defer server.Close()
+
+	c := NewAgentHTTPClient(server.URL, 0) // zero should fall back to defaultSandboxTimeout
+	if _, err := c.Run(context.Background(), "", "ping", nil, nil); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if gotTimeoutMs == nil {
+		t.Fatal("timeout_ms was not sent in request")
+	}
+	wantMs := int64(defaultSandboxTimeout / time.Millisecond)
+	if *gotTimeoutMs != wantMs {
+		t.Errorf("timeout_ms = %d, want %d (defaultSandboxTimeout)", *gotTimeoutMs, wantMs)
+	}
+}

controller/proposal/client_test.go

@@ -58,7 +58,7 @@ func (r *ProposalReconciler) handleAnalysis(
 		return ctrl.Result{}, fmt.Errorf("update to Analyzing: %w", err)
 	}
 
-	timeout := proposalTimeout(proposal)
+	timeout := stepTimeout(resolved.Analysis)
 	analysisResult, err := r.Agent.Analyze(ctx, proposal, resolved.Analysis, proposal.Spec.Request, timeout)
 	if err != nil {
 		return r.failStep(ctx, log, proposal, agenticv1alpha1.ProposalConditionAnalyzed, err)
@@ -125,7 +125,7 @@ func (r *ProposalReconciler) handleRevision(
 	revisionSuffix := buildRevisionContext(proposal)
 	requestWithRevision := proposal.Spec.Request + "\n\n" + revisionSuffix
 
-	timeout := proposalTimeout(proposal)
+	timeout := stepTimeout(resolved.Analysis)
 	analysisResult, err := r.Agent.Analyze(ctx, proposal, resolved.Analysis, requestWithRevision, timeout)
 	if err != nil {
 		return r.failStep(ctx, log, proposal, agenticv1alpha1.ProposalConditionAnalyzed, err)
@@ -242,7 +242,7 @@ func (r *ProposalReconciler) handleExecution(
 		return ctrl.Result{}, fmt.Errorf("update to Executing: %w", err)
 	}
 
-	timeout := proposalTimeout(proposal)
+	timeout := stepTimeout(*resolved.Execution)
 	execResult, err := r.Agent.Execute(ctx, proposal, *resolved.Execution, selectedOption, timeout)
 	if err != nil {
 		return r.failStep(ctx, log, proposal, agenticv1alpha1.ProposalConditionExecuted, err)
@@ -346,7 +346,7 @@ func (r *ProposalReconciler) handleVerification(
 		}
 	}
 
-	timeout := proposalTimeout(proposal)
+	timeout := stepTimeout(*resolved.Verification)
 	verifyResult, err := r.Agent.Verify(ctx, proposal, *resolved.Verification, selectedOption, execOutput, timeout)
 	if err != nil {
 		return r.failStep(ctx, log, proposal, agenticv1alpha1.ProposalConditionVerified, err)
@@ -507,7 +507,7 @@ func (r *ProposalReconciler) handleEscalation(
 	}
 
 	escalationText := buildEscalationRequest(proposal)
-	timeout := proposalTimeout(proposal)
+	timeout := stepTimeout(step)
 	escalationResult, err := r.Agent.Escalate(ctx, proposal, step, escalationText, timeout)
 	if err != nil {
 		return r.failStep(ctx, log, proposal, agenticv1alpha1.ProposalConditionEscalated, err)

controller/proposal/handlers.go

@@ -32,6 +32,11 @@ type testAgentCaller struct {
 	executeResult  *ExecutionOutput
 	verifyResult   *VerificationOutput
 	escalateResult *EscalationOutput
+
+	lastAnalyzeTimeout  time.Duration
+	lastExecuteTimeout  time.Duration
+	lastVerifyTimeout   time.Duration
+	lastEscalateTimeout time.Duration
 }
 
 func newTestAgentCaller() *testAgentCaller {
@@ -43,25 +48,29 @@ func newTestAgentCaller() *testAgentCaller {
 	return &testAgentCaller{analyzeResult: a, executeResult: e, verifyResult: v, escalateResult: esc}
 }
 
-func (ta *testAgentCaller) Analyze(_ context.Context, _ *agenticv1alpha1.Proposal, _ resolvedStep, _ string, _ time.Duration) (*AnalysisOutput, error) {
+func (ta *testAgentCaller) Analyze(_ context.Context, _ *agenticv1alpha1.Proposal, _ resolvedStep, _ string, timeout time.Duration) (*AnalysisOutput, error) {
+	ta.lastAnalyzeTimeout = timeout
 	if ta.analyzeErr != nil {
 		return nil, ta.analyzeErr
 	}
 	return ta.analyzeResult, nil
 }
-func (ta *testAgentCaller) Execute(_ context.Context, _ *agenticv1alpha1.Proposal, _ resolvedStep, _ *agenticv1alpha1.RemediationOption, _ time.Duration) (*ExecutionOutput, error) {
+func (ta *testAgentCaller) Execute(_ context.Context, _ *agenticv1alpha1.Proposal, _ resolvedStep, _ *agenticv1alpha1.RemediationOption, timeout time.Duration) (*ExecutionOutput, error) {
+	ta.lastExecuteTimeout = timeout
 	if ta.executeErr != nil {
 		return nil, ta.executeErr
 	}
 	return ta.executeResult, nil
 }
-func (ta *testAgentCaller) Verify(_ context.Context, _ *agenticv1alpha1.Proposal, _ resolvedStep, _ *agenticv1alpha1.RemediationOption, _ *ExecutionOutput, _ time.Duration) (*VerificationOutput, error) {
+func (ta *testAgentCaller) Verify(_ context.Context, _ *agenticv1alpha1.Proposal, _ resolvedStep, _ *agenticv1alpha1.RemediationOption, _ *ExecutionOutput, timeout time.Duration) (*VerificationOutput, error) {
+	ta.lastVerifyTimeout = timeout
 	if ta.verifyErr != nil {
 		return nil, ta.verifyErr
 	}
 	return ta.verifyResult, nil
 }
-func (ta *testAgentCaller) Escalate(_ context.Context, _ *agenticv1alpha1.Proposal, _ resolvedStep, _ string, _ time.Duration) (*EscalationOutput, error) {
+func (ta *testAgentCaller) Escalate(_ context.Context, _ *agenticv1alpha1.Proposal, _ resolvedStep, _ string, timeout time.Duration) (*EscalationOutput, error) {
+	ta.lastEscalateTimeout = timeout
 	if ta.escalateErr != nil {
 		return nil, ta.escalateErr
 	}
@@ -319,3 +328,40 @@ func TestReconcile_Denied_Terminal(t *testing.T) {
 		t.Fatalf("expected Denied, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions))
 	}
 }
+
+// TestReconcile_PropagatesStepTimeout verifies that timeoutMinutes set on a
+// ProposalStep is resolved and forwarded to the AgentCaller as time.Duration.
+func TestReconcile_PropagatesStepTimeout(t *testing.T) {
+	const wantMinutes int32 = 30
+
+	scheme := testScheme()
+	proposal := &agenticv1alpha1.Proposal{
+		ObjectMeta: metav1.ObjectMeta{Name: "timeout-check", Namespace: "default"},
+		Spec: agenticv1alpha1.ProposalSpec{
+			Request: "Pod crashing",
+			Tools:   testTools(),
+			Analysis: agenticv1alpha1.ProposalStep{
+				Agent:          "default",
+				TimeoutMinutes: &[]int32{wantMinutes}[0],
+			},
+			Execution:    agenticv1alpha1.ProposalStep{Agent: "default"},
+			Verification: agenticv1alpha1.ProposalStep{Agent: "default"},
+		},
+	}
+
+	objs := append([]client.Object{proposal}, defaultObjects()...)
+	fc := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).
+		WithStatusSubresource(proposal, &agenticv1alpha1.AnalysisResult{}, &agenticv1alpha1.ExecutionResult{}, &agenticv1alpha1.VerificationResult{}, &agenticv1alpha1.EscalationResult{}).Build()
+
+	caller := newTestAgentCaller()
+	r := &ProposalReconciler{Client: fc, Log: logr.Discard(), Agent: caller, Namespace: "default"}
+
+	if _, err := reconcileOnce(r, "timeout-check"); err != nil {
+		t.Fatalf("reconcile: %v", err)
+	}
+
+	want := time.Duration(wantMinutes) * time.Minute
+	if caller.lastAnalyzeTimeout != want {
+		t.Errorf("Analyze timeout = %v, want %v", caller.lastAnalyzeTimeout, want)
+	}
+}