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

Author: sakshiep1

.ai/spec/how/reconciler.md

@@ -170,7 +170,7 @@ Audience: AI agents. Behavioral rules and phase semantics live in **what/** spec
 - **`SandboxProvider`:** Swappable claim/wait/release (tests can fake). Implementations: `SandboxManager` (sandbox-claim mode), `BarePodManager` (bare-pod mode). `SetStep` provides resolved step config before each `Claim` call.
 - **`PodSpecBuilder`:** Shared pod-spec assembly. Produces typed `corev1.PodSpec` from image + resolved step config. Used directly by `BarePodManager`; shared helper functions also used by `EnsureAgentTemplate` (unstructured path).
 - **`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, and MCP JSON env. 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 == 0
 }
 
 // DataSource references a pre-existing PersistentVolumeClaim containing
@@ -277,7 +289,11 @@ type DataSource struct {
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=253
 	// +kubebuilder:validation:XValidation:rule="!format.dns1123Subdomain().validate(self).hasValue()",message="must be a valid DNS subdomain"
-	ClaimName string `json:"claimName"`
+	ClaimName string `json:"claimName,omitempty"`
+}
+
+func (d DataSource) IsZero() bool {
+	return d.ClaimName == ""
 }
 
 // ProposalSpec defines the desired state of Proposal.
@@ -291,10 +307,9 @@ type DataSource struct {
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.analysisOutput) || (has(self.analysisOutput) && self.analysisOutput == oldSelf.analysisOutput)",message="analysisOutput is immutable once set"
 // +kubebuilder:validation:XValidation:rule="!has(self.analysisOutput) || self.analysisOutput.mode != 'Minimal' || (!has(self.execution) && !has(self.verification))",message="analysisOutput mode Minimal is only allowed for analysis-only proposals (no execution or verification steps)"
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.tools) || (has(self.tools) && self.tools == oldSelf.tools)",message="tools is immutable once set"
-// +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"
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.analysis) || (has(self.analysis) && self.analysis.agent == oldSelf.analysis.agent && self.analysis.tools == oldSelf.analysis.tools)",message="analysis agent and tools are immutable once set"
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.execution) || (has(self.execution) && self.execution.agent == oldSelf.execution.agent && self.execution.tools == oldSelf.execution.tools)",message="execution agent and tools are immutable once set"
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.verification) || (has(self.verification) && self.verification.agent == oldSelf.verification.agent && self.verification.tools == oldSelf.verification.tools)",message="verification agent and tools are 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 +357,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 +389,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
 }