From 965177dac666bcdbacf95083964a8b8adbb0cc88 Mon Sep 17 00:00:00 2001 From: blublinsky Date: Wed, 2 Sep 2026 15:03:32 +0100 Subject: [PATCH] OLS-4098 Add spec.instructions to Agent CRD to externalize system and user prompts per step --- .ai/spec/what/crd-api.md | 16 +- .ai/spec/what/sandbox-execution.md | 14 +- api/v1alpha1/agent_types.go | 48 ++++ cmd/main.go | 3 + .../bases/agentic.openshift.io_agents.yaml | 100 ++++++++ config/webhook/manifests.yaml | 25 ++ controller/agenticrun/agent.go | 8 +- controller/agenticrun/handlers.go | 10 +- controller/agenticrun/helpers.go | 78 ++++-- controller/agenticrun/input_configmap.go | 110 +++++++- controller/agenticrun/input_configmap_test.go | 241 +++++++++++++++++- controller/agenticrun/reconciler_test.go | 4 +- controller/agenticrun/revision_test.go | 25 +- controller/agenticrun/sandbox_agent.go | 31 +-- controller/agenticrun/sandbox_agent_test.go | 18 +- controller/agenticrun/sandbox_manager.go | 3 +- controller/agenticrun/sandbox_manager_test.go | 29 ++- controller/agenticrun/templates_test.go | 7 +- .../{approval_webhook.go => webhooks.go} | 55 ++++ ...roval_webhook_test.go => webhooks_test.go} | 148 ++++++++++- 20 files changed, 863 insertions(+), 110 deletions(-) rename controller/agenticrun/{approval_webhook.go => webhooks.go} (56%) rename controller/agenticrun/{approval_webhook_test.go => webhooks_test.go} (64%) diff --git a/.ai/spec/what/crd-api.md b/.ai/spec/what/crd-api.md index 290e974d..2ef65642 100644 --- a/.ai/spec/what/crd-api.md +++ b/.ai/spec/what/crd-api.md @@ -21,11 +21,11 @@ Kubernetes API surface for the agentic operator. **Lifecycle and gates** are in 9. **AgenticRun — `spec.tools`**: Default `ToolsSpec` for all steps; immutable once set. Per-step `tools` on `spec.analysis` / `spec.execution` / `spec.verification` replaces the default for that step only when non-zero. 10. **AgenticRun — `spec.analysis|execution|verification`**: Immutable `AgenticRunStep` records after set. Each non-zero step MAY name `agent` (DNS subdomain) defaulting to `default` when empty; MAY carry per-step `tools`. 10a–10g. ~~[SUPERSEDED by OLS-3491 redesign]~~ Rules 10a–10g removed and replaced. Per-step instructions now live on the `Agent` CR, not on `AgenticRunStep` or `OLSConfig`. See rules 10h–10l below. -10h. [PLANNED: OLS-3491] **Agent — `spec.instructions`**: Optional `AgentInstructions` struct with per-step string fields: `analysis`, `execution`, `verification`, `escalation`. Each field is optional (MaxLength=32768). When a field is non-empty, it is a **full replacement** of the product built-in system instructions for that step. When absent or empty, the product built-in is used. The Agent becomes a complete "compute + behavior" entity. -10i. [PLANNED: OLS-3491] **Instruction resolution at sandbox setup**: For each step, the operator resolves the step's Agent CR and reads `Agent.spec.instructions.`. If non-empty, use it; otherwise render the product built-in Go template. The resolved instructions are written to the input ConfigMap `system-prompt` key. There is no create-time materialization on the AgenticRun and no per-run instruction override. -10j. [PLANNED: OLS-3491] **Channel split**: Step **system instructions** travel on the system channel (`/input/system-prompt`). Step **input** travels on the query channel (`/input/query`): analysis uses `spec.request` (plus existing revision suffix); execution uses approved option JSON; verification uses option + execution output JSON; escalation uses its dynamic payload (run metadata, request, result refs). Role text MUST NOT be embedded in `query`. -10k. [PLANNED: OLS-3491] **Revision feedback**: `spec.revisionFeedback` / revision context template remain query-side append behavior; not part of `instructions`. -10l. [PLANNED: OLS-3491] **Precedence**: `Agent.spec.instructions.` (when non-empty) > product built-in. Two layers only. Different Agents carry different instructions for different use cases (alerts remediation, security audit, etc.); the adapter selects the appropriate Agent when creating an AgenticRun. +10h. [DONE: OLS-4098] **Agent — `spec.instructions`**: Optional `AgentInstructions` struct with per-step `StepInstructions` fields: `analysis`, `execution`, `verification`, `escalation`. Each `StepInstructions` has two optional strings (MaxLength=32768): `systemPrompt` (LLM system message → `/input/system-prompt`; default when empty: sandbox built-in `"You are an AI agent."`) and `userPrompt` (Go template replacing built-in query template → `/input/query`; supports the same template variables as the built-in templates, e.g. `{{.Request}}`, `{{.HasExecution}}`, `{{.HasVerification}}` for analysis, `{{.OptionJSON}}` for execution, `{{.OptionJSON}}`/`{{.ExecutionJSON}}` for verification; default when empty: built-in templates in `controller/agenticrun/templates/*.tmpl`). When absent or empty, product built-in defaults are used. The Agent becomes a complete "compute + behavior" entity. +10i. [DONE: OLS-4098] **Instruction resolution at sandbox setup**: `buildInputConfigMap` resolves both prompts via `resolvePrompts`. For `systemPrompt`: Agent CR value or empty (sandbox defaults to `"You are an AI agent."`). For `userPrompt`: Agent CR Go template or built-in template file (`templates/*.tmpl`). Both rendered with the same template data as built-in templates (e.g. `{{.Request}}`, `{{.HasExecution}}`). The `system-prompt` key is only included in the ConfigMap when non-empty. +10j. [DONE: OLS-4098] **Channel split**: `systemPrompt` travels on `/input/system-prompt`. `userPrompt` (rendered) travels on `/input/query`. Analysis query uses `spec.request`; execution uses approved option JSON; verification uses option + execution output JSON; escalation uses run metadata, request, and result refs. Revision feedback appends to query-side via `buildRevisionContext`. +10k. [DONE: OLS-4098] **Revision feedback**: `spec.revisionFeedback` / revision context template remain query-side append behavior; not part of `instructions`. +10l. [DONE: OLS-4098] **Precedence**: `Agent.spec.instructions..{systemPrompt,userPrompt}` (when non-empty) > product built-in. Two layers only. Different Agents carry different instructions for different use cases. 11. **AgenticRun — `status`**: Observed-only. `status.conditions` holds map-merge conditions (types include `Analyzed`, `Executed`, `Verified`, `Denied`, `Escalated`, `EmergencyStopped`). `status.steps` holds per-step sandbox info and result refs. 12. **Phase display types**: `AgenticRunPhase` and `StepPhase` string enums in the API describe display labels only; they are not stored fields on `AgenticRun` (phase is derived — see `run-lifecycle.md`). `AgenticRunPhase` values include `EmergencyStopped` (terminal, set by kill switch — see `system-config.md`). When analysis determines no remediation is needed, the run derives as `Completed` with `Analyzed` condition reason `NoActionRequired`. `StepPhase` values include `PendingApproval`, `Running`, `Completed`, `Failed`, `Skipped`. 13. **Sandbox step enum**: `SandboxStep` values `Analysis`, `Execution`, `Verification`, `Escalation` identify workflow steps for approvals, sandbox labels, and policies. @@ -77,7 +77,7 @@ Kubernetes API surface for the agentic operator. **Lifecycle and gates** are in - `status.conditions`, `status.steps.analysis|execution|verification|escalation.*`, `status.terminalTime`, `status.tokenUsage` [PLANNED: OLS-3661] ### Agent -- `metadata.name`, `spec.llmProvider.name`, `spec.model`, `spec.reasoningConfig`, `spec.timeouts.*`, `spec.maxTurns`, `spec.instructions.*` [PLANNED: OLS-3491], `status.conditions` +- `metadata.name`, `spec.llmProvider.name`, `spec.model`, `spec.reasoningConfig`, `spec.timeouts.*`, `spec.maxTurns`, `spec.instructions.{analysis,execution,verification,escalation}.{systemPrompt,userPrompt}` [OLS-4098], `status.conditions` ### LLMProvider - `metadata.name`, `spec.type`, `spec.anthropic.*`, `spec.googleCloudVertex.*`, `spec.openAI.*`, `spec.azureOpenAI.*`, `spec.awsBedrock.*` @@ -100,7 +100,7 @@ Kubernetes API surface for the agentic operator. **Lifecycle and gates** are in ### Shared / embedded types - `AgenticRunStep`: `agent`, `tools` -- `AgentInstructions`: `analysis`, `execution`, `verification`, `escalation` [PLANNED: OLS-3491] +- `AgentInstructions`: `analysis`, `execution`, `verification`, `escalation` (each with `systemPrompt`, `userPrompt`) [DONE: OLS-4098] - `ToolsSpec`: `skills[]`, `mcpServers[]`, `requiredSecrets[]` (`disableDefaultMCP` deferred — see rule 37a / OLS-3594) - `SkillsSource`: `image`, `paths[]` - `SecretRequirement`: `name`, `description`, `mountAs.*` @@ -116,7 +116,7 @@ Kubernetes API surface for the agentic operator. **Lifecycle and gates** are in ## Planned Changes - [PLANNED: OLS-2940] Autonomous workflow CRD migrations may rename or reshape fields; specs MUST be updated when `v1alpha1` changes. -- [PLANNED: OLS-3491] Configurable per-step `instructions` on `Agent` CR (`spec.instructions.analysis|execution|verification|escalation`). Two-layer precedence: Agent instructions > product built-in. No per-run overrides, no `OLSConfig` involvement. See rules 10h–10l and `sandbox-execution.md`. Design spec: `docs/superpowers/specs/2026-09-01-configurable-instructions-design.md`. +- [DONE: OLS-4098] Configurable per-step `instructions` on `Agent` CR with `systemPrompt` and `userPrompt` per step. Two-layer precedence: Agent instructions > product built-in. `systemPrompt` → `/input/system-prompt`; `userPrompt` (Go template) → `/input/query`. See rules 10h–10l. - [OLS-3328] Add `spec.templog` to `AgenticOLSConfig` CRD for temporary audit log storage. - [DONE: OLS-3295] Renamed `Proposal` → `AgenticRun`, `ProposalApproval` → `AgenticRunApproval` CRD kinds and all associated field names, RBAC resources, and label keys. - [PLANNED: OLS-3594] Optional `disableDefaultMCP` (and related auto-injection) — deferred; blocked by OLS-3526 and OLS-3572. Not near-term. diff --git a/.ai/spec/what/sandbox-execution.md b/.ai/spec/what/sandbox-execution.md index 016b00af..f72688c4 100644 --- a/.ai/spec/what/sandbox-execution.md +++ b/.ai/spec/what/sandbox-execution.md @@ -10,19 +10,19 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes 4. **[sandbox-claim mode only] Template immutability & GC**: If a derived template for the same agent + step + hash already exists, creation MUST be a no-op. Older derived templates for the same agent+step with different names SHOULD be garbage-collected separately from per-run release, and only after the operator confirms that no live SandboxClaim or sandbox workload references them. 5. **Claim naming**: Claim names MUST be derived from run name and step label, truncated to valid Kubernetes name length limits. 6. **[OLS-3066] Batch execution model**: Sandbox pods are **batch executors** — the operator does NOT call the sandbox over HTTP. Instead: (a) the operator creates an input ConfigMap with the step payload, (b) mounts it read-only into the sandbox pod, (c) the sandbox runs the agent autonomously, (d) the sandbox creates the Result CR via `oc`, and (e) the sandbox exits. The operator watches for the Result CR to appear. There is no HTTP server in the sandbox; rules 7–9 below replace the former HTTP contract. -7. **[OLS-3066] Input delivery — ConfigMap**: For each step invocation, the operator MUST create a namespaced `ConfigMap` in the operator namespace. Name is `ls-{step}-{uid}` (unique per step, same pattern as pods and SAs). Initial owner reference is the `AgenticRun`; after the sandbox pod or SandboxClaim is created, the owner is replaced with that resource so GC cleans up the ConfigMap when the pod/claim is released. Per-step naming prevents GC of one step's ConfigMap from affecting the next. The ConfigMap MUST contain keys: `query` (step **input** text only — see rules 11–13 and OLS-3491), `system-prompt` [PLANNED: OLS-3491] (step **system instructions** from materialized `spec..instructions` or escalation resolution), `output-schema` (JSON schema computed by `outputSchemaForStep`), `context` (JSON object with targetNamespaces, previousAttempts, approvedOption, executionResult as applicable), and `result-template` (pre-filled Result CR JSON — see rule 7a). The ConfigMap MUST be mounted read-only into the sandbox pod at `/input/`. Creation MUST be idempotent (`AlreadyExists` = no-op). Until OLS-3491 is implemented, `query` MAY still contain role text from legacy step templates and `system-prompt` MAY be absent/empty. +7. **[OLS-3066] Input delivery — ConfigMap**: For each step invocation, the operator MUST create a namespaced `ConfigMap` in the operator namespace. Name is `ls-{step}-{uid}` (unique per step, same pattern as pods and SAs). Initial owner reference is the `AgenticRun`; after the sandbox pod or SandboxClaim is created, the owner is replaced with that resource so GC cleans up the ConfigMap when the pod/claim is released. Per-step naming prevents GC of one step's ConfigMap from affecting the next. The ConfigMap MUST contain keys: `query` (rendered from Agent CR `userPrompt` template or built-in template — see rules 11–13 and 13a), `system-prompt` (optional — from Agent CR `systemPrompt` when non-empty; omitted otherwise and sandbox defaults to `"You are an AI agent."`), `output-schema` (JSON schema computed by `outputSchemaForStep`), `context` (JSON object with targetNamespaces, previousAttempts, approvedOption, executionResult as applicable), and `result-template` (pre-filled Result CR JSON — see rule 7a). The ConfigMap MUST be mounted read-only into the sandbox pod at `/input/`. Creation MUST be idempotent (`AlreadyExists` = no-op). 7a. **[OLS-3066] Result template**: The `result-template` key MUST contain a JSON object with complete `apiVersion`, `kind`, `metadata` (name, namespace, labels, ownerReferences including AgenticRun UID), and `spec` (agenticRunName). The operator pre-computes all metadata — the sandbox only fills in `status` fields. CR naming MUST use the existing `resultCRName` convention. Owner references MUST use the current AgenticRun UID. 8. **[OLS-3066] Output delivery — Result CR via `oc`**: The sandbox MUST create the Result CR in two steps: (a) `oc create -f ` using the pre-filled template with `spec` fields, then (b) `oc patch --type=merge --subresource=status` with the agent output in `status` fields (options, diagnosis, actionRequired, actionsTaken, checks, conditions, failureReason as applicable per step). The Result CR `status.conditions` MUST include a `Completed` condition set to `True` as part of the status patch — this is the operator's readiness signal (see rule 8b). On agent failure, the sandbox MUST still create the Result CR with `status.failureReason` populated and exit 0 (the sandbox succeeded; the agent failed). [PLANNED: OLS-3743] A cooperative timeout MUST use `Completed=True` with reason `AgentTimeout`; other agent failures use reason `Failed`. The operator maps these reasons to distinct step conditions. On sandbox failure (cannot read input, `oc create` fails, etc.), the sandbox MUST write an error message to `/dev/termination-log` (max 4096 bytes) and exit non-zero. 8a. **[OLS-3066] Sandbox RBAC for Result CRs**: Each step gets its own per-step ServiceAccount (`ls-{step}-{namespace}-{runUID}`). The per-step SA MUST have `create` and `patch` (with `status` subresource) permissions on only its specific Result CRD (e.g. analysis SA can only create `AnalysisResult`). The execution SA additionally receives execution-specific Roles/ClusterRoles for the approved remediation. 8b. **[OLS-3066] Result CR readiness signal**: The operator MUST only process a Result CR when its `status.conditions` includes `Completed=True`. A Result CR without this condition indicates the sandbox has called `oc create` but has not yet patched the status — the operator MUST wait for the status update (which triggers another `Owns()` watch event). This guards against the race between `oc create` and `oc patch --subresource=status`. 9. **[OLS-3066] Watch-driven async**: The controller MUST use watch-based event delivery instead of synchronous polling. `SetupWithManager` MUST `Owns()` Pods (bare-pod mode), SandboxClaims (sandbox-claim mode), ConfigMaps, and all Result CR types (AnalysisResult, ExecutionResult, VerificationResult, EscalationResult). Pod watches are for **failure detection only** (Pod `Failed`, `ImagePullBackOff`). Result CR watches are for **completion detection** (Result CR created with `Completed` condition). Every in-progress step MUST return `RequeueAfter(30s)` as a safety net for missed watch events. 10. **Output schema selection**: The `output-schema` key in the input ConfigMap MUST be the step-specific JSON schema computed by the operator: analysis schema depends on `spec.analysisOutput.mode`, whether execution/verification steps exist in the run, and optional injected `components` sub-schema from `spec.analysisOutput.schema`; other steps use fixed schemas for their response shapes. -11. **Analysis query payload**: The `query` string MUST encode the user request or revision-augmented request. [PLANNED: OLS-3491] Workflow flags and role/rules (prefer mounted skill when matching, fall back to kubectl/oc, inspect before diagnosing, remediation script shape, RBAC derivation) MUST live in `system-prompt` / `instructions`, not in `query`. Until OLS-3491, those instructions MAY still be template-rendered into `query`. The analysis instructions MUST instruct the agent to prefer a mounted skill when one matches the investigation and fall back to kubectl/oc for read-only inspection when no skill applies, inspect cluster state before diagnosing, produce a concrete remediation script of executable bash commands (mutations and waits only — pre-checks and post-checks are excluded because analysis already inspected the cluster and verification is a separate step), and derive RBAC for mutations and subresource access only (the execution environment already has cluster-wide read access to standard resources). -12. **Execution query payload**: The `query` MUST include JSON describing the approved remediation option, which contains a concrete remediation script (ordered bash commands). [PLANNED: OLS-3491] Execution role/rules MUST live in `system-prompt` / `instructions`. Until OLS-3491, those MAY still be template-rendered into `query`. The execution instructions MUST instruct the agent to follow the script exactly, execute commands in order without substitution, dry-run every mutation command with `--dry-run=server` before applying, and fix syntax errors only (no semantic changes). The execution instructions MUST NOT instruct the agent to perform verification — that is the verification step's responsibility. -13. **Verification query payload**: The `query` MUST include the approved option JSON and a JSON description of the latest execution output (actions taken and success status) when available. [PLANNED: OLS-3491] Verification role/rules MUST live in `system-prompt` / `instructions`. -13a. [PLANNED: OLS-3491] **System instructions resolution**: See `crd-api.md` rules 10h–10l. The operator resolves instructions from the step's Agent CR and writes them to the `system-prompt` ConfigMap key. The operator MUST NOT send an empty system prompt after OLS-3491 (sandbox default persona is a fallback only). +11. **Analysis query payload**: The `query` string is rendered from the Agent CR's `userPrompt` template (or the built-in `analysis_query.tmpl`). The analysis instructions MUST instruct the agent to prefer a mounted skill when one matches the investigation and fall back to kubectl/oc for read-only inspection when no skill applies, inspect cluster state before diagnosing, produce a concrete remediation script of executable bash commands (mutations and waits only — pre-checks and post-checks are excluded because analysis already inspected the cluster and verification is a separate step), and derive RBAC for mutations and subresource access only (the execution environment already has cluster-wide read access to standard resources). +12. **Execution query payload**: The `query` is rendered from the Agent CR's `userPrompt` template (or the built-in `execution_query.tmpl`) with the approved remediation option JSON. The execution instructions MUST instruct the agent to follow the script exactly, execute commands in order without substitution, dry-run every mutation command with `--dry-run=server` before applying, and fix syntax errors only (no semantic changes). The execution instructions MUST NOT instruct the agent to perform verification — that is the verification step's responsibility. +13. **Verification query payload**: The `query` is rendered from the Agent CR's `userPrompt` template (or the built-in `verification_query.tmpl`) with the approved option JSON and execution output JSON. +13a. [DONE: OLS-4098] **Prompt resolution**: See `crd-api.md` rules 10h–10l. `buildInputConfigMap` resolves both prompts via `resolvePrompts`: `systemPrompt` from `Agent.spec.instructions..systemPrompt` (or empty — sandbox defaults to `"You are an AI agent."`); query from `Agent.spec.instructions..userPrompt` Go template (or built-in `templates/*.tmpl`). The `system-prompt` ConfigMap key is only included when non-empty. 13b. ~~[REMOVED]~~ HTTP path (pre-OLS-3066) no longer applies — batch execution model only. -13c. [PLANNED: OLS-3491] **Escalation query payload**: The escalation `query` / `/input/query` MUST carry the dynamic step input only: run metadata (name, namespace), original `spec.request`, and references to prior Analysis/Execution/Verification result CRs (as today). Escalation role/rules MUST NOT be embedded in `query`; they belong only in `system-prompt` / `/input/system-prompt` via rule 13a. +13c. [DONE: OLS-4098] **Escalation query payload**: The escalation `query` carries run metadata (name, namespace), original `spec.request`, and references to prior Analysis/Execution/Verification result CRs. Rendered from the escalation template (built-in or custom `Agent.spec.instructions.escalation.userPrompt`). 14. **Context envelope**: The `context` object MUST include `targetNamespaces` from `spec.targetNamespaces`, synthesized `previousAttempts` from failed prior `status.steps.*.results` entries, `approvedOption` when executing/verifying, and `executionResult` when verifying. Note: the sandbox context prefix formatter (see sandbox `run-api.md`) only expands `targetNamespaces`, `attempt`, `previousAttempts`, and `approvedOption` into the model prompt; `executionResult` is carried in `context` for tracing but verification execution details are primarily conveyed to the model via the `query` body (rendered from the verification template). 15. **Secrets — run** `spec.tools.requiredSecrets` / per-step tools: Secret objects MUST live in the **same namespace as the `AgenticRun`**. Mounting into sandbox MUST honor `SecretMountSpec`: environment variable injection OR file mount at configured absolute path. 16. **Secrets — LLM credentials**: LLM provider credentials MUST be loaded from secret names declared on the `LLMProvider` and wired into the derived template via `envFrom` (all secret keys as env vars) AND a read-only volume mount at `/var/run/secrets/llm-credentials/`. Both mounts MUST be unconditional regardless of provider type — the sandbox uses whichever form its SDK requires. The operator MUST NOT set individual credential env vars by name. Because the mount is unconditional and key-agnostic, additional credential keys flow to the sandbox unchanged with no operator wiring change: Azure Entra ID service-principal keys (`client_id`, `tenant_id`, `client_secret`) [OLS-3050] and the AWS Bedrock `role_arn` (alongside `aws_access_key_id` / `aws_secret_access_key`) [OLS-4092]. Validation of the Azure credential-key set is per `crd-api.md` rule 21a and of the Bedrock set per rule 21b; the sandbox resolves Azure Entra-vs-key mode per `lightspeed-agentic-sandbox/.ai/spec/what/configuration.md` rule 9a and Bedrock static-vs-assume-role mode per rule 9b. @@ -144,7 +144,7 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes - [DONE: OLS-3558] Execution outcome override — controller overrides `success=false` to `Succeeded` when all mutations passed, deferring outcome to verification. See rule 21b. - [DONE: OLS-3685/OLS-3686] Inter-operator configuration handoff — `PodSpecBuilder` reads a base `corev1.PodSpec` from the `lightspeed-agentic-configuration` ConfigMap (produced by lightspeed-operator) and applies all per-run config through a single overlay code path. Eliminated duplication between `PodSpecBuilder` (bare-pod) and `EnsureAgentTemplate` (sandbox-claim) — unified `SandboxManager` builds the complete PodSpec, then the mode determines delivery. CLI flags `--sandbox-mode`, `--agentic-sandbox-image`, `--image-pull-policy` replaced by ConfigMap keys. No-blocking startup; graceful degradation when ConfigMap is absent. See `docs/inter-operator-handoff-design.md`. - [PLANNED: OLS-3594] Optional default ocp-mcp auto-injection into sandbox pods — deferred; product value unconfirmed. Blocked by OLS-3526 (standalone HTTPS ocp-mcp) and OLS-3572 (inter-operator config handoff). Not near-term. -- [PLANNED: OLS-3491] Configurable step `instructions` (system channel) separate from step `query` input. Instructions resolved from Agent CR (`spec.instructions.`) or product built-in — see `crd-api.md` rules 10h–10l. Design spec: `docs/superpowers/specs/2026-09-01-configurable-instructions-design.md`. +- [DONE: OLS-4098] Configurable per-step `systemPrompt` and `userPrompt` on Agent CR. Resolved at sandbox setup in `buildInputConfigMap` via `resolvePrompts`. See `crd-api.md` rules 10h–10l. - [PLANNED: OLS-3661] Token usage aggregation — operator reads `status.tokenUsage` from completed Result CRs and accumulates into `AgenticRun.status.tokenUsage`. See rule 43.1 and `crd-api.md` rules 6c–6e. - [PLANNED: OLS-3743] Layer Agent-configured cooperative execution budgets under fixed operator sandbox startup and hard running deadlines; wire `maxTurns`; distinguish timeout sources in status. - [PLANNED: OLS-3298, OLS-4018] Shared hard-stop cleanup: zero-grace Pod deletion, SandboxClaim/backing-workload deletion, sandbox access revocation, dual resource discovery, idempotency, and retries after terminal status. See `agentic-run-termination.md`. diff --git a/api/v1alpha1/agent_types.go b/api/v1alpha1/agent_types.go index faaae166..58eb3805 100644 --- a/api/v1alpha1/agent_types.go +++ b/api/v1alpha1/agent_types.go @@ -51,6 +51,47 @@ type AgentTimeouts struct { ChatSeconds int32 `json:"chatSeconds,omitempty"` } +// StepInstructions holds the system and user prompt instructions for a single step. +type StepInstructions struct { + // systemPrompt is the LLM system message for this step. + // Replaces the product built-in system prompt when non-empty. + // Written to /input/system-prompt in the sandbox. + // Default (when empty): "You are an AI agent." (sandbox built-in). + // +optional + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=32768 + SystemPrompt string `json:"systemPrompt,omitempty"` + + // userPrompt is a Go template that replaces the product built-in query + // template for this step when non-empty. Supports the same template + // variables as the built-in (e.g. {{.Request}}, {{.HasExecution}}). + // Written to /input/query in the sandbox after rendering. + // Default (when empty): built-in templates in controller/agenticrun/templates/*.tmpl. + // +optional + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=32768 + UserPrompt string `json:"userPrompt,omitempty"` +} + +// AgentInstructions provides optional per-step system and user instructions. +type AgentInstructions struct { + // analysis instructions for the analysis step. + // +optional + Analysis *StepInstructions `json:"analysis,omitzero"` //nolint:kubeapilinter // all fields optional; empty means use defaults + + // execution instructions for the execution step. + // +optional + Execution *StepInstructions `json:"execution,omitzero"` //nolint:kubeapilinter // all fields optional; empty means use defaults + + // verification instructions for the verification step. + // +optional + Verification *StepInstructions `json:"verification,omitzero"` //nolint:kubeapilinter // all fields optional; empty means use defaults + + // escalation instructions for the escalation step. + // +optional + Escalation *StepInstructions `json:"escalation,omitzero"` //nolint:kubeapilinter // all fields optional; empty means use defaults +} + // AgentSpec defines the desired state of Agent. type AgentSpec struct { // llmProvider references a cluster-scoped LLMProvider CR that supplies the @@ -83,6 +124,13 @@ type AgentSpec struct { // +kubebuilder:validation:Maximum=500 MaxTurns int32 `json:"maxTurns,omitempty"` + // instructions provides optional per-step system and user instructions. + // When a field is non-empty it fully replaces the product built-in + // instructions for that step. Omitted or empty fields fall back to + // built-in defaults. + // +optional + Instructions *AgentInstructions `json:"instructions,omitzero"` //nolint:kubeapilinter // all fields optional; empty means use defaults + // reasoningConfig is a freeform map of provider- and model-specific // reasoning parameters. The exact keys and values depend on the provider // and model — consult the provider's SDK documentation for supported diff --git a/cmd/main.go b/cmd/main.go index f80e040a..883a5a27 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -174,6 +174,9 @@ func main() { mgr.GetWebhookServer().Register("/mutate-agenticrunapproval", &admission.Webhook{ Handler: &agenticrun.AgenticRunApprovalMutator{}, }) + mgr.GetWebhookServer().Register("/validate-agent", &admission.Webhook{ + Handler: &agenticrun.AgentValidator{}, + }) if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { log.Error(err, "unable to set up health check") diff --git a/config/crd/bases/agentic.openshift.io_agents.yaml b/config/crd/bases/agentic.openshift.io_agents.yaml index 07f73dac..7c98351e 100644 --- a/config/crd/bases/agentic.openshift.io_agents.yaml +++ b/config/crd/bases/agentic.openshift.io_agents.yaml @@ -66,6 +66,106 @@ spec: spec: description: spec defines the desired state of Agent. properties: + instructions: + description: |- + instructions provides optional per-step system and user instructions. + When a field is non-empty it fully replaces the product built-in + instructions for that step. Omitted or empty fields fall back to + built-in defaults. + properties: + analysis: + description: analysis instructions for the analysis step. + properties: + systemPrompt: + description: |- + systemPrompt is the LLM system message for this step. + Replaces the product built-in system prompt when non-empty. + Written to /input/system-prompt in the sandbox. + Default (when empty): "You are an AI agent." (sandbox built-in). + maxLength: 32768 + minLength: 1 + type: string + userPrompt: + description: |- + userPrompt is a Go template that replaces the product built-in query + template for this step when non-empty. Supports the same template + variables as the built-in (e.g. {{.Request}}, {{.HasExecution}}). + Written to /input/query in the sandbox after rendering. + Default (when empty): built-in templates in controller/agenticrun/templates/*.tmpl. + maxLength: 32768 + minLength: 1 + type: string + type: object + escalation: + description: escalation instructions for the escalation step. + properties: + systemPrompt: + description: |- + systemPrompt is the LLM system message for this step. + Replaces the product built-in system prompt when non-empty. + Written to /input/system-prompt in the sandbox. + Default (when empty): "You are an AI agent." (sandbox built-in). + maxLength: 32768 + minLength: 1 + type: string + userPrompt: + description: |- + userPrompt is a Go template that replaces the product built-in query + template for this step when non-empty. Supports the same template + variables as the built-in (e.g. {{.Request}}, {{.HasExecution}}). + Written to /input/query in the sandbox after rendering. + Default (when empty): built-in templates in controller/agenticrun/templates/*.tmpl. + maxLength: 32768 + minLength: 1 + type: string + type: object + execution: + description: execution instructions for the execution step. + properties: + systemPrompt: + description: |- + systemPrompt is the LLM system message for this step. + Replaces the product built-in system prompt when non-empty. + Written to /input/system-prompt in the sandbox. + Default (when empty): "You are an AI agent." (sandbox built-in). + maxLength: 32768 + minLength: 1 + type: string + userPrompt: + description: |- + userPrompt is a Go template that replaces the product built-in query + template for this step when non-empty. Supports the same template + variables as the built-in (e.g. {{.Request}}, {{.HasExecution}}). + Written to /input/query in the sandbox after rendering. + Default (when empty): built-in templates in controller/agenticrun/templates/*.tmpl. + maxLength: 32768 + minLength: 1 + type: string + type: object + verification: + description: verification instructions for the verification step. + properties: + systemPrompt: + description: |- + systemPrompt is the LLM system message for this step. + Replaces the product built-in system prompt when non-empty. + Written to /input/system-prompt in the sandbox. + Default (when empty): "You are an AI agent." (sandbox built-in). + maxLength: 32768 + minLength: 1 + type: string + userPrompt: + description: |- + userPrompt is a Go template that replaces the product built-in query + template for this step when non-empty. Supports the same template + variables as the built-in (e.g. {{.Request}}, {{.HasExecution}}). + Written to /input/query in the sandbox after rendering. + Default (when empty): built-in templates in controller/agenticrun/templates/*.tmpl. + maxLength: 32768 + minLength: 1 + type: string + type: object + type: object llmProvider: description: |- llmProvider references a cluster-scoped LLMProvider CR that supplies the diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 08be186e..1982ea9f 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -22,3 +22,28 @@ webhooks: failurePolicy: Fail sideEffects: None admissionReviewVersions: ["v1"] +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingWebhookConfiguration +metadata: + name: agentic-operator-validating-webhook + annotations: + service.beta.openshift.io/inject-cabundle: "true" +webhooks: + - name: agent-validator.agentic.openshift.io + namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: __OPERATOR_NAMESPACE__ + clientConfig: + service: + name: agentic-operator-webhook-service + namespace: __OPERATOR_NAMESPACE__ + path: /validate-agent + rules: + - operations: ["CREATE", "UPDATE"] + apiGroups: ["agentic.openshift.io"] + apiVersions: ["v1alpha1"] + resources: ["agents"] + failurePolicy: Fail + sideEffects: None + admissionReviewVersions: ["v1"] diff --git a/controller/agenticrun/agent.go b/controller/agenticrun/agent.go index 174dcd7a..29722f5e 100644 --- a/controller/agenticrun/agent.go +++ b/controller/agenticrun/agent.go @@ -49,10 +49,10 @@ type EscalationOutput struct { // runs autonomously, creates the Result CR, and exits. The pod // handler watches for completion and patches the step condition. type AgentCaller interface { - Analyze(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep, requestText string) error + Analyze(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep) error Execute(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep, option *agenticv1alpha1.RemediationOption) error Verify(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep, option *agenticv1alpha1.RemediationOption, exec *ExecutionOutput) error - Escalate(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep, requestText string) error + Escalate(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep) error ReleaseSandboxes(ctx context.Context, run *agenticv1alpha1.AgenticRun) error ReleaseSandbox(ctx context.Context, run *agenticv1alpha1.AgenticRun, step string) error } @@ -60,7 +60,7 @@ type AgentCaller interface { // StubAgentCaller is a no-op implementation for testing. type StubAgentCaller struct{} -func (s *StubAgentCaller) Analyze(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ resolvedStep, _ string) error { +func (s *StubAgentCaller) Analyze(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ resolvedStep) error { return nil } @@ -72,7 +72,7 @@ func (s *StubAgentCaller) Verify(_ context.Context, _ *agenticv1alpha1.AgenticRu return nil } -func (s *StubAgentCaller) Escalate(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ resolvedStep, _ string) error { +func (s *StubAgentCaller) Escalate(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ resolvedStep) error { return nil } diff --git a/controller/agenticrun/handlers.go b/controller/agenticrun/handlers.go index 519f56b1..e83c233b 100644 --- a/controller/agenticrun/handlers.go +++ b/controller/agenticrun/handlers.go @@ -74,7 +74,7 @@ func (r *AgenticRunReconciler) handleAnalysis( return ctrl.Result{}, fmt.Errorf("%s: %w", ErrUpdateToAnalyzing, err) } - if err := r.Agent.Analyze(ctx, run, resolved.Analysis, run.Spec.Request); err != nil { + if err := r.Agent.Analyze(ctx, run, resolved.Analysis); err != nil { return r.failStep(ctx, run, agenticv1alpha1.AgenticRunConditionAnalyzed, err) } @@ -120,10 +120,7 @@ func (r *AgenticRunReconciler) handleRevision( return ctrl.Result{}, fmt.Errorf("%s: %w", ErrUpdateToAnalyzingRevision, err) } - revisionSuffix := buildRevisionContext(run) - requestWithRevision := run.Spec.Request + "\n\n" + revisionSuffix - - if err := r.Agent.Analyze(ctx, run, resolved.Analysis, requestWithRevision); err != nil { + if err := r.Agent.Analyze(ctx, run, resolved.Analysis); err != nil { return r.failStep(ctx, run, agenticv1alpha1.AgenticRunConditionAnalyzed, err) } @@ -413,8 +410,7 @@ func (r *AgenticRunReconciler) handleEscalation( return ctrl.Result{}, fmt.Errorf("%s: %w", ErrUpdateToEscalating, err) } - escalationText := buildEscalationRequest(run, r.Namespace) - if err := r.Agent.Escalate(ctx, run, step, escalationText); err != nil { + if err := r.Agent.Escalate(ctx, run, step); err != nil { return r.failStep(ctx, run, agenticv1alpha1.AgenticRunConditionEscalated, err) } diff --git a/controller/agenticrun/helpers.go b/controller/agenticrun/helpers.go index 6474c3c1..06406085 100644 --- a/controller/agenticrun/helpers.go +++ b/controller/agenticrun/helpers.go @@ -26,14 +26,45 @@ import ( //go:embed templates/*.tmpl var templateFS embed.FS -var templates = template.Must(template.ParseFS(templateFS, "templates/*.tmpl")) +// maxRenderedTemplateSize allows up to 32,768 Unicode characters encoded as UTF-8 (worst case: 4 bytes each). +const maxRenderedTemplateSize = 32768 * 4 // 131 KiB byte bound on rendered output -func renderTemplate(name string, data any) string { - var buf bytes.Buffer - if err := templates.ExecuteTemplate(&buf, name, data); err != nil { - return fmt.Sprintf("(template %q error: %v)", name, err) +type limitedWriter struct { + buf bytes.Buffer + limit int64 +} + +func (w *limitedWriter) Write(p []byte) (int, error) { + if int64(w.buf.Len())+int64(len(p)) > int64(w.limit) { + return 0, fmt.Errorf("rendered template exceeds %d bytes", w.limit) + } + return w.buf.Write(p) +} + +func (w *limitedWriter) String() string { + return w.buf.String() +} + +func renderTemplate(tmpl string, data any) (string, error) { + t, err := template.New("custom").Parse(tmpl) + if err != nil { + return "", fmt.Errorf("template parse: %w", err) + } + t = t.Funcs(template.FuncMap{}) // Restrict to default safe functions only + w := &limitedWriter{limit: maxRenderedTemplateSize} + if err := t.Execute(w, data); err != nil { + return "", fmt.Errorf("template exec: %w", err) + } + return w.String(), nil +} + +// readBuiltinTemplate reads a built-in template file from the embedded FS. +func readBuiltinTemplate(name string) (string, error) { + content, err := templateFS.ReadFile(name) + if err != nil { + return "", fmt.Errorf("read built-in template %s: %w", name, err) } - return buf.String() + return string(content), nil } const ( @@ -310,7 +341,11 @@ type escalationData struct { VerificationResults []agenticv1alpha1.StepResultRef } -func buildEscalationRequest(run *agenticv1alpha1.AgenticRun, resultNamespace string) string { +func buildEscalationRequest(run *agenticv1alpha1.AgenticRun, resultNamespace string) (string, error) { + tmpl, err := readBuiltinTemplate("templates/escalation_request.tmpl") + if err != nil { + return "", err + } data := escalationData{ Name: run.Name, Namespace: run.Namespace, @@ -320,7 +355,7 @@ func buildEscalationRequest(run *agenticv1alpha1.AgenticRun, resultNamespace str ExecutionResults: run.Status.Steps.Execution.Results, VerificationResults: run.Status.Steps.Verification.Results, } - return renderTemplate("escalation_request.tmpl", data) + return renderTemplate(tmpl, data) } func needsRevision(run *agenticv1alpha1.AgenticRun) bool { @@ -341,14 +376,18 @@ type revisionData struct { Feedback string } -func buildRevisionContext(run *agenticv1alpha1.AgenticRun) string { +func buildRevisionContext(run *agenticv1alpha1.AgenticRun) (string, error) { + tmpl, err := readBuiltinTemplate("templates/revision_context.tmpl") + if err != nil { + return "", err + } data := revisionData{ Generation: run.Generation, AgenticRunName: run.Name, Namespace: run.Namespace, Feedback: run.Spec.RevisionFeedback, } - return renderTemplate("revision_context.tmpl", data) + return renderTemplate(tmpl, data) } func prettyJSON(v interface{}) string { @@ -372,8 +411,12 @@ type analysisQuery struct { HasVerification bool } -func buildAnalysisQuery(requestText string, run *agenticv1alpha1.AgenticRun) string { - return renderTemplate("analysis_query.tmpl", analysisQuery{ +func buildAnalysisQuery(requestText string, run *agenticv1alpha1.AgenticRun) (string, error) { + tmpl, err := readBuiltinTemplate("templates/analysis_query.tmpl") + if err != nil { + return "", err + } + return renderTemplate(tmpl, analysisQuery{ Request: requestText, HasExecution: !run.Spec.Execution.IsZero(), HasVerification: !run.Spec.Verification.IsZero(), @@ -384,18 +427,7 @@ type executionQuery struct { OptionJSON string } -func buildExecutionQuery(option *agenticv1alpha1.RemediationOption) string { - return renderTemplate("execution_query.tmpl", executionQuery{OptionJSON: prettyJSON(option)}) -} - type verificationQuery struct { OptionJSON string ExecutionJSON string } - -func buildVerificationQuery(option *agenticv1alpha1.RemediationOption, exec *ExecutionOutput) string { - return renderTemplate("verification_query.tmpl", verificationQuery{ - OptionJSON: prettyJSON(option), - ExecutionJSON: prettyJSON(executionOutputToAgentResult(exec)), - }) -} diff --git a/controller/agenticrun/input_configmap.go b/controller/agenticrun/input_configmap.go index f4b00ad5..06d15f92 100644 --- a/controller/agenticrun/input_configmap.go +++ b/controller/agenticrun/input_configmap.go @@ -18,6 +18,98 @@ const ( ErrUnknownStep = "unknown step" ) +// resolvePrompts returns the system prompt and rendered query for the step. +// For each step: system prompt comes from Agent CR or empty (sandbox default). +// Query is rendered from Agent CR's custom template or the built-in template. +// Returns an error if template resolution or rendering fails. +func resolvePrompts(agent *agenticv1alpha1.Agent, step, operatorNamespace string, run *agenticv1alpha1.AgenticRun, agentCtx *agentContext) (systemPrompt, query string, err error) { + var tmpl string + switch step { + case "analysis": + systemPrompt, tmpl, err = resolveStepPrompts(agent, step, "templates/analysis_query.tmpl") + if err != nil { + return "", "", err + } + request := run.Spec.Request + if run.Spec.RevisionFeedback != "" { + revCtx, revErr := buildRevisionContext(run) + if revErr != nil { + return "", "", revErr + } + request += "\n\n" + revCtx + } + query, err = renderTemplate(tmpl, analysisQuery{ + Request: request, + HasExecution: !run.Spec.Execution.IsZero(), + HasVerification: !run.Spec.Verification.IsZero(), + }) + case "execution": + systemPrompt, tmpl, err = resolveStepPrompts(agent, step, "templates/execution_query.tmpl") + if err != nil { + return "", "", err + } + query, err = renderTemplate(tmpl, executionQuery{ + OptionJSON: prettyJSON(agentCtx.ApprovedOption), + }) + case "verification": + systemPrompt, tmpl, err = resolveStepPrompts(agent, step, "templates/verification_query.tmpl") + if err != nil { + return "", "", err + } + query, err = renderTemplate(tmpl, verificationQuery{ + OptionJSON: prettyJSON(agentCtx.ApprovedOption), + ExecutionJSON: prettyJSON(agentCtx.ExecutionResult), + }) + case "escalation": + systemPrompt, tmpl, err = resolveStepPrompts(agent, step, "templates/escalation_request.tmpl") + if err != nil { + return "", "", err + } + query, err = renderTemplate(tmpl, escalationData{ + Name: run.Name, + Namespace: run.Namespace, + ResultNamespace: operatorNamespace, + Request: run.Spec.Request, + AnalysisResults: run.Status.Steps.Analysis.Results, + ExecutionResults: run.Status.Steps.Execution.Results, + VerificationResults: run.Status.Steps.Verification.Results, + }) + } + return +} + +// resolveStepPrompts returns the system prompt and user prompt template +// for the step. System prompt is from Agent CR or empty. User prompt +// template is from Agent CR or the built-in default. +func resolveStepPrompts(agent *agenticv1alpha1.Agent, step, builtinTemplate string) (systemPrompt, userPromptTemplate string, err error) { + var si *agenticv1alpha1.StepInstructions + if agent != nil && agent.Spec.Instructions != nil { + switch step { + case "analysis": + si = agent.Spec.Instructions.Analysis + case "execution": + si = agent.Spec.Instructions.Execution + case "verification": + si = agent.Spec.Instructions.Verification + case "escalation": + si = agent.Spec.Instructions.Escalation + } + } + + if si != nil { + systemPrompt = si.SystemPrompt + if si.UserPrompt != "" { + return systemPrompt, si.UserPrompt, nil + } + } + + tmpl, err := readBuiltinTemplate(builtinTemplate) + if err != nil { + return "", "", err + } + return systemPrompt, tmpl, nil +} + // inputConfigMapName returns the per-step ConfigMap name: ls-{step}-{uid}. func inputConfigMapName(step string, uid string) string { return fmt.Sprintf("ls-%s-%s", step, uid) @@ -25,11 +117,13 @@ func inputConfigMapName(step string, uid string) string { // buildInputConfigMap builds the batch input ConfigMap for a step (rule 7). // Name is ls-{step}-{uid}, unique per step to prevent GC conflicts. +// Resolves both system-prompt and query internally from the Agent CR +// (custom templates) or built-in defaults. func buildInputConfigMap( operatorNamespace string, run *agenticv1alpha1.AgenticRun, step string, - query string, + agent *agenticv1alpha1.Agent, schema json.RawMessage, agentCtx *agentContext, ) (*corev1.ConfigMap, error) { @@ -44,7 +138,13 @@ func buildInputConfigMap( if err != nil { return nil, fmt.Errorf("%s: %w", ErrBuildInputConfigMap, err) } - return &corev1.ConfigMap{ + + systemPrompt, query, err := resolvePrompts(agent, step, operatorNamespace, run, agentCtx) + if err != nil { + return nil, fmt.Errorf("resolve prompts for step %s: %w", step, err) + } + + cm := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: inputConfigMapName(step, string(run.UID)), Namespace: operatorNamespace, @@ -60,7 +160,11 @@ func buildInputConfigMap( inputConfigMapKeyCtx: string(ctxJSON), inputConfigMapKeyTmpl: tmpl, }, - }, nil + } + if systemPrompt != "" { + cm.Data[inputConfigMapKeySystemPrompt] = systemPrompt + } + return cm, nil } // buildResultTemplate returns JSON for result-template (rule 7a): apiVersion, diff --git a/controller/agenticrun/input_configmap_test.go b/controller/agenticrun/input_configmap_test.go index 5a879115..1eda546a 100644 --- a/controller/agenticrun/input_configmap_test.go +++ b/controller/agenticrun/input_configmap_test.go @@ -2,6 +2,7 @@ package agenticrun import ( "encoding/json" + "strings" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -18,13 +19,14 @@ func TestBuildInputConfigMap(t *testing.T) { UID: types.UID("uid-aaaa-bbbb"), }, Spec: agenticv1alpha1.AgenticRunSpec{ + Request: "analyze this", TargetNamespaces: []string{"payments"}, }, } schema := json.RawMessage(`{"type":"object"}`) agentCtx := &agentContext{TargetNamespaces: []string{"payments"}} - cm, err := buildInputConfigMap("op-ns", run, "analysis", "analyze this", schema, agentCtx) + cm, err := buildInputConfigMap("op-ns", run, "analysis", nil, schema, agentCtx) if err != nil { t.Fatalf("buildInputConfigMap: %v", err) } @@ -45,8 +47,8 @@ func TestBuildInputConfigMap(t *testing.T) { t.Errorf("missing data key %q", key) } } - if cm.Data[inputConfigMapKeyQuery] != "analyze this" { - t.Errorf("query = %q", cm.Data[inputConfigMapKeyQuery]) + if !strings.Contains(cm.Data[inputConfigMapKeyQuery], "analyze this") { + t.Errorf("query should contain request text, got %q", cm.Data[inputConfigMapKeyQuery]) } if cm.Data[inputConfigMapKeySchema] != string(schema) { t.Errorf("schema = %q", cm.Data[inputConfigMapKeySchema]) @@ -71,6 +73,239 @@ func TestBuildInputConfigMap(t *testing.T) { } } +func TestResolvePrompts_DefaultBuiltinTemplates(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}, + Spec: agenticv1alpha1.AgenticRunSpec{Request: "fix the crash"}, + } + agentCtx := &agentContext{} + + systemPrompt, query, err := resolvePrompts(nil, "analysis", "ns", run, agentCtx) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if systemPrompt != "" { + t.Errorf("expected empty system prompt for nil agent, got %q", systemPrompt) + } + if !strings.Contains(query, "fix the crash") { + t.Errorf("query should contain request text, got %q", query) + } + if !strings.Contains(query, "analysis agent") { + t.Errorf("query should contain built-in template text, got %q", query) + } +} + +func TestResolvePrompts_CustomSystemPromptWithCustomUser(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}, + Spec: agenticv1alpha1.AgenticRunSpec{Request: "fix it"}, + } + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + SystemPrompt: "You are a security auditor.", + UserPrompt: "Audit: {{.Request}}", + }, + }, + }, + } + + systemPrompt, query, err := resolvePrompts(agent, "analysis", "ns", run, &agentContext{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if systemPrompt != "You are a security auditor." { + t.Errorf("systemPrompt = %q, want custom", systemPrompt) + } + if query != "Audit: fix it" { + t.Errorf("query = %q, want rendered custom template", query) + } +} + +func TestResolvePrompts_CustomUserPrompt(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}, + Spec: agenticv1alpha1.AgenticRunSpec{Request: "pod is crashing"}, + } + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + UserPrompt: "Custom instructions.\n\n## Request\n\n{{.Request}}", + }, + }, + }, + } + + systemPrompt, query, err := resolvePrompts(agent, "analysis", "ns", run, &agentContext{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if systemPrompt != "" { + t.Errorf("expected empty system prompt, got %q", systemPrompt) + } + if !strings.Contains(query, "Custom instructions.") { + t.Errorf("query should contain custom template, got %q", query) + } + if !strings.Contains(query, "pod is crashing") { + t.Errorf("query should contain rendered request, got %q", query) + } +} + +func TestResolvePrompts_BothCustom(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}, + Spec: agenticv1alpha1.AgenticRunSpec{Request: "check certs"}, + } + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + SystemPrompt: "You are a cert auditor.", + UserPrompt: "Audit certs for: {{.Request}}", + }, + }, + }, + } + + systemPrompt, query, err := resolvePrompts(agent, "analysis", "ns", run, &agentContext{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if systemPrompt != "You are a cert auditor." { + t.Errorf("systemPrompt = %q", systemPrompt) + } + if query != "Audit certs for: check certs" { + t.Errorf("query = %q", query) + } +} + +func TestResolvePrompts_SystemPromptReturnedWithBuiltinTemplate(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}, + Spec: agenticv1alpha1.AgenticRunSpec{Request: "fix it"}, + } + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + SystemPrompt: "custom system", + // UserPrompt empty — uses built-in template + }, + }, + }, + } + + systemPrompt, _, err := resolvePrompts(agent, "analysis", "ns", run, &agentContext{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if systemPrompt != "custom system" { + t.Errorf("expected custom system prompt even with built-in user template, got %q", systemPrompt) + } +} + +func TestResolvePrompts_NoSystemPromptWhenNoneConfigured(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}, + Spec: agenticv1alpha1.AgenticRunSpec{Request: "fix it"}, + } + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{}, + } + + systemPrompt, _, err := resolvePrompts(agent, "analysis", "ns", run, &agentContext{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if systemPrompt != "" { + t.Errorf("expected empty system prompt when none configured, got %q", systemPrompt) + } +} + +func TestResolvePrompts_InvalidCustomTemplate(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}, + Spec: agenticv1alpha1.AgenticRunSpec{Request: "fix it"}, + } + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + UserPrompt: "{{.InvalidField}}", + }, + }, + }, + } + + _, _, err := resolvePrompts(agent, "analysis", "ns", run, &agentContext{}) + if err == nil { + t.Fatal("expected error for invalid custom template, got nil") + } + if !strings.Contains(err.Error(), "template exec") { + t.Errorf("expected template exec error, got: %v", err) + } +} + +func TestResolvePrompts_MalformedCustomTemplate(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}, + Spec: agenticv1alpha1.AgenticRunSpec{Request: "fix it"}, + } + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + UserPrompt: "{{.Unclosed", + }, + }, + }, + } + + _, _, err := resolvePrompts(agent, "analysis", "ns", run, &agentContext{}) + if err == nil { + t.Fatal("expected error for malformed template syntax, got nil") + } + if !strings.Contains(err.Error(), "template parse") { + t.Errorf("expected template parse error, got: %v", err) + } +} + +func TestBuildInputConfigMap_SystemPromptConditional(t *testing.T) { + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}, + Spec: agenticv1alpha1.AgenticRunSpec{Request: "fix"}, + } + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + SystemPrompt: "custom", + UserPrompt: "Do: {{.Request}}", + }, + }, + }, + } + + cm, err := buildInputConfigMap("ns", run, "analysis", agent, json.RawMessage(`{}`), nil) + if err != nil { + t.Fatalf("buildInputConfigMap: %v", err) + } + if cm.Data[inputConfigMapKeySystemPrompt] != "custom" { + t.Errorf("system-prompt = %q, want 'custom'", cm.Data[inputConfigMapKeySystemPrompt]) + } + + // Without custom instructions — system-prompt key should be absent + cm2, err := buildInputConfigMap("ns", run, "analysis", nil, json.RawMessage(`{}`), nil) + if err != nil { + t.Fatalf("buildInputConfigMap: %v", err) + } + if _, ok := cm2.Data[inputConfigMapKeySystemPrompt]; ok { + t.Errorf("system-prompt key should be absent when not configured, got %q", cm2.Data[inputConfigMapKeySystemPrompt]) + } +} + func TestBuildResultTemplate_UnknownStep(t *testing.T) { run := &agenticv1alpha1.AgenticRun{ObjectMeta: metav1.ObjectMeta{Name: "r", Namespace: "ns", UID: "u"}} _, err := buildResultTemplate(run, "nope", run.Namespace) diff --git a/controller/agenticrun/reconciler_test.go b/controller/agenticrun/reconciler_test.go index 3c870b91..b7fd2343 100644 --- a/controller/agenticrun/reconciler_test.go +++ b/controller/agenticrun/reconciler_test.go @@ -101,7 +101,7 @@ func (ta *testAgentCaller) withClient(t *testing.T, fc client.Client, ns string) return ta } -func (ta *testAgentCaller) Analyze(ctx context.Context, run *agenticv1alpha1.AgenticRun, _ resolvedStep, _ string) error { +func (ta *testAgentCaller) Analyze(ctx context.Context, run *agenticv1alpha1.AgenticRun, _ resolvedStep) error { if ta.analyzeErr != nil { return ta.analyzeErr } @@ -125,7 +125,7 @@ func (ta *testAgentCaller) Verify(ctx context.Context, run *agenticv1alpha1.Agen return nil } -func (ta *testAgentCaller) Escalate(ctx context.Context, run *agenticv1alpha1.AgenticRun, _ resolvedStep, _ string) error { +func (ta *testAgentCaller) Escalate(ctx context.Context, run *agenticv1alpha1.AgenticRun, _ resolvedStep) error { if ta.escalateErr != nil { return ta.escalateErr } diff --git a/controller/agenticrun/revision_test.go b/controller/agenticrun/revision_test.go index 8d411ddf..adbf5a61 100644 --- a/controller/agenticrun/revision_test.go +++ b/controller/agenticrun/revision_test.go @@ -55,7 +55,10 @@ func TestBuildRevisionContext_WithFeedback(t *testing.T) { RevisionFeedback: "Please focus on the memory issue, not CPU", }, } - result := buildRevisionContext(run) + result, err := buildRevisionContext(run) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if !strings.Contains(result, "Please focus on the memory issue, not CPU") { t.Errorf("expected feedback in revision context, got: %s", result) } @@ -69,7 +72,10 @@ func TestBuildRevisionContext_WithoutFeedback(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test-run", Namespace: "default", Generation: 2}, Spec: agenticv1alpha1.AgenticRunSpec{}, } - result := buildRevisionContext(run) + result, err := buildRevisionContext(run) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if strings.Contains(result, "## User Feedback") { t.Errorf("expected no User Feedback header when feedback is empty, got: %s", result) } @@ -85,7 +91,10 @@ func TestBuildAnalysisQuery_FullAgenticRun(t *testing.T) { Verification: agenticv1alpha1.AgenticRunStep{Agent: "default"}, }, } - result := buildAnalysisQuery("Fix the crash", run) + result, err := buildAnalysisQuery("Fix the crash", run) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if !strings.Contains(result, "Derive RBAC") { t.Error("full run should mention RBAC derivation") } @@ -118,7 +127,10 @@ func TestBuildAnalysisQuery_TrustMode(t *testing.T) { Execution: agenticv1alpha1.AgenticRunStep{Agent: "default"}, }, } - result := buildAnalysisQuery("Fix the crash", run) + result, err := buildAnalysisQuery("Fix the crash", run) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if !strings.Contains(result, "Derive RBAC") { t.Error("execution run should mention RBAC derivation") } @@ -129,7 +141,10 @@ func TestBuildAnalysisQuery_TrustMode(t *testing.T) { func TestBuildAnalysisQuery_Advisory(t *testing.T) { run := &agenticv1alpha1.AgenticRun{} - result := buildAnalysisQuery("What is 2+2?", run) + result, err := buildAnalysisQuery("What is 2+2?", run) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if strings.Contains(result, "Derive RBAC") { t.Error("advisory run should NOT mention RBAC derivation") } diff --git a/controller/agenticrun/sandbox_agent.go b/controller/agenticrun/sandbox_agent.go index 4c06806e..22ed8664 100644 --- a/controller/agenticrun/sandbox_agent.go +++ b/controller/agenticrun/sandbox_agent.go @@ -35,11 +35,12 @@ const ( podStartTimeout = 5 * time.Minute // Input ConfigMap (sandbox-execution.md rule 7). - inputConfigMapMountPath = "/input" - inputConfigMapKeyQuery = "query" - inputConfigMapKeySchema = "output-schema" - inputConfigMapKeyCtx = "context" - inputConfigMapKeyTmpl = "result-template" + inputConfigMapMountPath = "/input" + inputConfigMapKeyQuery = "query" + inputConfigMapKeySystemPrompt = "system-prompt" + inputConfigMapKeySchema = "output-schema" + inputConfigMapKeyCtx = "context" + inputConfigMapKeyTmpl = "result-template" // CRD maxLength limits for analysis option fields, injected into // the LLM output schema so the model respects CRD constraints. @@ -109,7 +110,7 @@ type agentPreviousAttempt struct { // Release handles all cleanup: pod deletion (GC handles children) plus // explicit cross-namespace/cluster-scoped RBAC teardown. type SandboxLifecycle interface { - Create(ctx context.Context, run *agenticv1alpha1.AgenticRun, step string, agent *agenticv1alpha1.Agent, llm *agenticv1alpha1.LLMProvider, tools *agenticv1alpha1.ToolsSpec, deadline time.Duration, query string, agentCtx *agentContext) (string, error) + Create(ctx context.Context, run *agenticv1alpha1.AgenticRun, step string, agent *agenticv1alpha1.Agent, llm *agenticv1alpha1.LLMProvider, tools *agenticv1alpha1.ToolsSpec, deadline time.Duration, agentCtx *agentContext) (string, error) Release(ctx context.Context, run *agenticv1alpha1.AgenticRun, step string) error } @@ -127,9 +128,8 @@ func stepString(step agenticv1alpha1.SandboxStep) string { return strings.ToLower(string(step)) } -func (s *SandboxAgentCaller) Analyze(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep, requestText string) error { - query := buildAnalysisQuery(requestText, run) - return s.launchSandbox(ctx, run, stepString(agenticv1alpha1.SandboxStepAnalysis), step, query, buildAgentContext(run)) +func (s *SandboxAgentCaller) Analyze(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep) error { + return s.launchSandbox(ctx, run, stepString(agenticv1alpha1.SandboxStepAnalysis), step, buildAgentContext(run)) } func (s *SandboxAgentCaller) Execute(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep, option *agenticv1alpha1.RemediationOption) error { @@ -137,8 +137,7 @@ func (s *SandboxAgentCaller) Execute(ctx context.Context, run *agenticv1alpha1.A if option != nil { agentCtx.ApprovedOption = option } - query := buildExecutionQuery(option) - return s.launchSandbox(ctx, run, stepString(agenticv1alpha1.SandboxStepExecution), step, query, agentCtx) + return s.launchSandbox(ctx, run, stepString(agenticv1alpha1.SandboxStepExecution), step, agentCtx) } func (s *SandboxAgentCaller) Verify(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep, option *agenticv1alpha1.RemediationOption, exec *ExecutionOutput) error { @@ -147,12 +146,11 @@ func (s *SandboxAgentCaller) Verify(ctx context.Context, run *agenticv1alpha1.Ag agentCtx.ApprovedOption = option } agentCtx.ExecutionResult = executionOutputToAgentResult(exec) - query := buildVerificationQuery(option, exec) - return s.launchSandbox(ctx, run, stepString(agenticv1alpha1.SandboxStepVerification), step, query, agentCtx) + return s.launchSandbox(ctx, run, stepString(agenticv1alpha1.SandboxStepVerification), step, agentCtx) } -func (s *SandboxAgentCaller) Escalate(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep, requestText string) error { - return s.launchSandbox(ctx, run, stepString(agenticv1alpha1.SandboxStepEscalation), step, requestText, buildAgentContext(run)) +func (s *SandboxAgentCaller) Escalate(ctx context.Context, run *agenticv1alpha1.AgenticRun, step resolvedStep) error { + return s.launchSandbox(ctx, run, stepString(agenticv1alpha1.SandboxStepEscalation), step, buildAgentContext(run)) } // launchSandbox delegates to SandboxLifecycle.Create which handles all setup @@ -162,14 +160,13 @@ func (s *SandboxAgentCaller) launchSandbox( run *agenticv1alpha1.AgenticRun, stepName string, step resolvedStep, - query string, agentCtx *agentContext, ) error { podDeadline := stepTimeout(stepName) + defaultSandboxTimeout var name string if err := retryOnTransient(ctx, func() error { var createErr error - name, createErr = s.Sandbox.Create(ctx, run, stepName, step.Agent, step.LLM, step.Tools, podDeadline, query, agentCtx) + name, createErr = s.Sandbox.Create(ctx, run, stepName, step.Agent, step.LLM, step.Tools, podDeadline, agentCtx) return createErr }); err != nil { return fmt.Errorf("%s: %w", ErrClaimSandbox, err) diff --git a/controller/agenticrun/sandbox_agent_test.go b/controller/agenticrun/sandbox_agent_test.go index 9c873f86..7652d1bf 100644 --- a/controller/agenticrun/sandbox_agent_test.go +++ b/controller/agenticrun/sandbox_agent_test.go @@ -26,7 +26,7 @@ type mockSandboxProvider struct { releaseCalls int } -func (m *mockSandboxProvider) Create(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ string, _ *agenticv1alpha1.Agent, _ *agenticv1alpha1.LLMProvider, _ *agenticv1alpha1.ToolsSpec, _ time.Duration, _ string, _ *agentContext) (string, error) { +func (m *mockSandboxProvider) Create(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ string, _ *agenticv1alpha1.Agent, _ *agenticv1alpha1.LLMProvider, _ *agenticv1alpha1.ToolsSpec, _ time.Duration, _ *agentContext) (string, error) { m.claimCalls++ if len(m.claimErrors) > 0 { idx := m.claimCalls - 1 @@ -82,7 +82,7 @@ func TestSandboxAgentCaller_Analyze_CreatesSandbox(t *testing.T) { sandbox := &mockSandboxProvider{claimName: "ls-analysis-fix-crash"} caller := newTestSandboxAgentCaller(sandbox) - err := caller.Analyze(context.Background(), testSandboxAgenticRun(), testSandboxStep(), "Pod crashing") + err := caller.Analyze(context.Background(), testSandboxAgenticRun(), testSandboxStep()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -128,7 +128,7 @@ func TestSandboxAgentCaller_Escalate_CreatesSandbox(t *testing.T) { sandbox := &mockSandboxProvider{claimName: "ls-escalation-fix-crash"} caller := newTestSandboxAgentCaller(sandbox) - err := caller.Escalate(context.Background(), testSandboxAgenticRun(), testSandboxStep(), "Pod crashing") + err := caller.Escalate(context.Background(), testSandboxAgenticRun(), testSandboxStep()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -141,7 +141,7 @@ func TestSandboxAgentCaller_Analyze_CreateError(t *testing.T) { sandbox := &mockSandboxProvider{claimErr: fmt.Errorf("sandbox unavailable")} caller := newTestSandboxAgentCaller(sandbox) - err := caller.Analyze(context.Background(), testSandboxAgenticRun(), testSandboxStep(), "Pod crashing") + err := caller.Analyze(context.Background(), testSandboxAgenticRun(), testSandboxStep()) if err == nil { t.Fatal("expected error on sandbox create failure") } @@ -162,7 +162,7 @@ func TestSandboxAgentCaller_PatchesSandboxInfo(t *testing.T) { run := testSandboxAgenticRun() caller := newTestSandboxAgentCallerWithAgenticRun(sandbox, run) - err := caller.Analyze(context.Background(), run, testSandboxStep(), "Pod crashing") + err := caller.Analyze(context.Background(), run, testSandboxStep()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -270,7 +270,7 @@ type trackingMockSandbox struct { errOnClaim string } -func (m *trackingMockSandbox) Create(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ string, _ *agenticv1alpha1.Agent, _ *agenticv1alpha1.LLMProvider, _ *agenticv1alpha1.ToolsSpec, _ time.Duration, _ string, _ *agentContext) (string, error) { +func (m *trackingMockSandbox) Create(_ context.Context, _ *agenticv1alpha1.AgenticRun, _ string, _ *agenticv1alpha1.Agent, _ *agenticv1alpha1.LLMProvider, _ *agenticv1alpha1.ToolsSpec, _ time.Duration, _ *agentContext) (string, error) { return "", nil } func (m *trackingMockSandbox) Release(_ context.Context, run *agenticv1alpha1.AgenticRun, step string) error { @@ -297,7 +297,7 @@ func TestSandboxAgentCaller_TransientRetryThenSuccess(t *testing.T) { run := testSandboxAgenticRun() caller := newTestSandboxAgentCallerWithAgenticRun(sandbox, run) - err := caller.Analyze(context.Background(), run, testSandboxStep(), "Pod crashing") + err := caller.Analyze(context.Background(), run, testSandboxStep()) if err != nil { t.Fatalf("expected success after transient retry, got: %v", err) } @@ -313,7 +313,7 @@ func TestSandboxAgentCaller_PermanentErrorNoRetry(t *testing.T) { } caller := newTestSandboxAgentCaller(sandbox) - err := caller.Analyze(context.Background(), testSandboxAgenticRun(), testSandboxStep(), "Pod crashing") + err := caller.Analyze(context.Background(), testSandboxAgenticRun(), testSandboxStep()) if err == nil { t.Fatal("expected error for permanent failure") } @@ -330,7 +330,7 @@ func TestSandboxAgentCaller_TransientExhaustsRetries(t *testing.T) { } caller := newTestSandboxAgentCaller(sandbox) - err := caller.Analyze(context.Background(), testSandboxAgenticRun(), testSandboxStep(), "Pod crashing") + err := caller.Analyze(context.Background(), testSandboxAgenticRun(), testSandboxStep()) if err == nil { t.Fatal("expected error after exhausting retries") } diff --git a/controller/agenticrun/sandbox_manager.go b/controller/agenticrun/sandbox_manager.go index f42162c1..44655a40 100644 --- a/controller/agenticrun/sandbox_manager.go +++ b/controller/agenticrun/sandbox_manager.go @@ -83,7 +83,6 @@ func (m *SandboxManager) Create( llm *agenticv1alpha1.LLMProvider, tools *agenticv1alpha1.ToolsSpec, deadline time.Duration, - query string, agentCtx *agentContext, ) (name string, retErr error) { if m.audit != nil { @@ -136,7 +135,7 @@ func (m *SandboxManager) Create( } schema := outputSchemaForStep(step, run) - inputCM, err := buildInputConfigMap(m.namespace, run, step, query, schema, agentCtx) + inputCM, err := buildInputConfigMap(m.namespace, run, step, agent, schema, agentCtx) if err != nil { createErr = err return "", err diff --git a/controller/agenticrun/sandbox_manager_test.go b/controller/agenticrun/sandbox_manager_test.go index 2be19ae0..bfc14a41 100644 --- a/controller/agenticrun/sandbox_manager_test.go +++ b/controller/agenticrun/sandbox_manager_test.go @@ -2,6 +2,7 @@ package agenticrun import ( "context" + "strings" "testing" "time" @@ -114,7 +115,7 @@ func TestCreate_BarePod(t *testing.T) { fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(testReaderCRB()).Build() mgr := newTestSandboxManager(fc, cache) - name, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("Create failed: %v", err) } @@ -147,8 +148,8 @@ func TestCreate_BarePod(t *testing.T) { if err := fc.Get(context.Background(), types.NamespacedName{Name: inputConfigMapName("analysis", string(run.UID)), Namespace: "test-ns"}, &cm); err != nil { t.Fatalf("input ConfigMap not found: %v", err) } - if cm.Data[inputConfigMapKeyQuery] != "test query" { - t.Errorf("ConfigMap query = %q", cm.Data[inputConfigMapKeyQuery]) + if !strings.Contains(cm.Data[inputConfigMapKeyQuery], "fix it") { + t.Errorf("ConfigMap query should contain request text, got %q", cm.Data[inputConfigMapKeyQuery]) } if len(cm.OwnerReferences) == 0 { t.Fatal("expected OwnerReferences on input ConfigMap") @@ -173,7 +174,7 @@ func TestCreate_SandboxClaim(t *testing.T) { fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(testReaderCRB()).Build() mgr := newTestSandboxManager(fc, cache) - name, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("Create failed: %v", err) } @@ -202,7 +203,7 @@ func TestCreate_ConfigNotAvailable(t *testing.T) { fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(testReaderCRB()).Build() mgr := newTestSandboxManager(fc, cache) - _, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + _, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err == nil { t.Fatal("expected error when config is not available") } @@ -213,11 +214,11 @@ func TestCreate_Idempotent_BarePod(t *testing.T) { fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(testReaderCRB()).Build() mgr := newTestSandboxManager(fc, cache) - name1, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name1, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("first Create failed: %v", err) } - name2, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name2, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("second Create failed: %v", err) } @@ -231,11 +232,11 @@ func TestCreate_Idempotent_SandboxClaim(t *testing.T) { fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(testReaderCRB()).Build() mgr := newTestSandboxManager(fc, cache) - name1, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name1, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("first Create failed: %v", err) } - name2, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name2, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("second Create failed: %v", err) } @@ -250,7 +251,7 @@ func TestCreate_OTELEnvVars(t *testing.T) { mgr := newTestSandboxManager(fc, cache) run := testSMRun() - name, err := mgr.Create(context.Background(), run, "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name, err := mgr.Create(context.Background(), run, "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("Create failed: %v", err) } @@ -307,7 +308,7 @@ func TestCreate_NoOTEL_NoEnvVars(t *testing.T) { fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(testReaderCRB()).Build() mgr := newTestSandboxManager(fc, cache) - name, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("Create failed: %v", err) } @@ -330,7 +331,7 @@ func TestCreate_OTELEnvVars_SandboxClaim(t *testing.T) { mgr := newTestSandboxManager(fc, cache) run := testSMRun() - name, err := mgr.Create(context.Background(), run, "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name, err := mgr.Create(context.Background(), run, "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("Create failed: %v", err) } @@ -436,7 +437,7 @@ func TestNamePrefix_LSPrefix(t *testing.T) { fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(testReaderCRB()).Build() mgr := newTestSandboxManager(fc, cache) - name, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name, err := mgr.Create(context.Background(), testSMRun(), "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("Create failed: %v", err) } @@ -455,7 +456,7 @@ func TestNamePrefix_LongNameTruncated(t *testing.T) { longRun := testSMRun() longRun.Name = "a-very-long-run-name-that-exceeds-sixty-three-characters-in-total-length" - name, err := mgr.Create(context.Background(), longRun, "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, "test query", nil) + name, err := mgr.Create(context.Background(), longRun, "analysis", testSMAgent(), testLLMForManager(), nil, 15*time.Minute, nil) if err != nil { t.Fatalf("Create failed: %v", err) } diff --git a/controller/agenticrun/templates_test.go b/controller/agenticrun/templates_test.go index 9f0f25b9..7ef20cb1 100644 --- a/controller/agenticrun/templates_test.go +++ b/controller/agenticrun/templates_test.go @@ -38,10 +38,9 @@ func TestBuildEscalationRequest_UsesOutcome(t *testing.T) { }, } - result := buildEscalationRequest(run, "openshift-lightspeed") - - if strings.Contains(result, "template") && strings.Contains(result, "error") { - t.Fatalf("template rendering failed: %s", result) + result, err := buildEscalationRequest(run, "openshift-lightspeed") + if err != nil { + t.Fatalf("template rendering failed: %v", err) } if !strings.Contains(result, "AnalysisResult: analysis-1 (outcome=Succeeded)") { diff --git a/controller/agenticrun/approval_webhook.go b/controller/agenticrun/webhooks.go similarity index 56% rename from controller/agenticrun/approval_webhook.go rename to controller/agenticrun/webhooks.go index 316d9e39..244b0a64 100644 --- a/controller/agenticrun/approval_webhook.go +++ b/controller/agenticrun/webhooks.go @@ -3,8 +3,11 @@ package agenticrun import ( "context" "encoding/json" + "fmt" "net/http" + "text/template" "time" + "unicode/utf8" "gomodules.xyz/jsonpatch/v2" admissionv1 "k8s.io/api/admission/v1" @@ -13,6 +16,8 @@ import ( agenticv1alpha1 "github.com/openshift/lightspeed-agentic-operator/api/v1alpha1" ) +// --- AgenticRunApproval mutating webhook --- + // AgenticRunApprovalMutator is a MutatingAdmissionWebhook handler that injects // the authenticated user's identity into spec.approver on every UPDATE to a // AgenticRunApproval, overwriting any client-submitted values. @@ -73,3 +78,53 @@ func (m *AgenticRunApprovalMutator) Handle(_ context.Context, req admission.Requ return admission.Patched("injected spec.approver", patches...) } + +// --- Agent validating webhook --- + +const maxPromptLength = 32768 + +// AgentValidator is a ValidatingAdmissionWebhook handler that rejects +// Agent create/update when any prompt is too long or contains invalid Go +// template syntax. +type AgentValidator struct{} + +func (v *AgentValidator) Handle(_ context.Context, req admission.Request) admission.Response { + var agent agenticv1alpha1.Agent + if err := json.Unmarshal(req.Object.Raw, &agent); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + if agent.Spec.Instructions == nil { + return admission.Allowed("no instructions configured") + } + + steps := map[string]*agenticv1alpha1.StepInstructions{ + "analysis": agent.Spec.Instructions.Analysis, + "execution": agent.Spec.Instructions.Execution, + "verification": agent.Spec.Instructions.Verification, + "escalation": agent.Spec.Instructions.Escalation, + } + + for step, si := range steps { + if si == nil { + continue + } + if utf8.RuneCountInString(si.SystemPrompt) > maxPromptLength { + return admission.Denied(fmt.Sprintf( + "spec.instructions.%s.systemPrompt: must not exceed %d characters", step, maxPromptLength)) + } + if si.UserPrompt == "" { + continue + } + if utf8.RuneCountInString(si.UserPrompt) > maxPromptLength { + return admission.Denied(fmt.Sprintf( + "spec.instructions.%s.userPrompt: must not exceed %d characters", step, maxPromptLength)) + } + if _, err := template.New(step).Parse(si.UserPrompt); err != nil { + return admission.Denied(fmt.Sprintf( + "spec.instructions.%s.userPrompt: invalid Go template: %v", step, err)) + } + } + + return admission.Allowed("templates valid") +} diff --git a/controller/agenticrun/approval_webhook_test.go b/controller/agenticrun/webhooks_test.go similarity index 64% rename from controller/agenticrun/approval_webhook_test.go rename to controller/agenticrun/webhooks_test.go index 3fcb4cfe..e95bdade 100644 --- a/controller/agenticrun/approval_webhook_test.go +++ b/controller/agenticrun/webhooks_test.go @@ -3,6 +3,7 @@ package agenticrun import ( "context" "encoding/json" + "strings" "testing" admissionv1 "k8s.io/api/admission/v1" @@ -14,6 +15,8 @@ import ( agenticv1alpha1 "github.com/openshift/lightspeed-agentic-operator/api/v1alpha1" ) +// --- Helpers --- + func makeApprovalJSON(t *testing.T, approval *agenticv1alpha1.AgenticRunApproval) []byte { t.Helper() raw, err := json.Marshal(approval) @@ -23,6 +26,31 @@ func makeApprovalJSON(t *testing.T, approval *agenticv1alpha1.AgenticRunApproval return raw } +func makeAgentJSON(t *testing.T, agent *agenticv1alpha1.Agent) []byte { + t.Helper() + raw, err := json.Marshal(agent) + if err != nil { + t.Fatalf("marshal Agent: %v", err) + } + return raw +} + +func agentRequest(t *testing.T, agent *agenticv1alpha1.Agent) admission.Request { + t.Helper() + return admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: makeAgentJSON(t, agent)}, + }, + } +} + +func int32Ptr(i int32) *int32 { + return &i +} + +// --- AgenticRunApproval mutator tests --- + func TestApprovalWebhook_InjectsApproverOnUpdate(t *testing.T) { approval := &agenticv1alpha1.AgenticRunApproval{ ObjectMeta: metav1.ObjectMeta{ @@ -283,6 +311,122 @@ func TestApprovalWebhook_MissingSpec(t *testing.T) { t.Error("expected add /spec patch when spec is missing") } -func int32Ptr(i int32) *int32 { - return &i +// --- Agent validator tests --- + +func TestAgentValidator_NoInstructions(t *testing.T) { + agent := &agenticv1alpha1.Agent{} + resp := (&AgentValidator{}).Handle(context.Background(), agentRequest(t, agent)) + if !resp.Allowed { + t.Fatalf("expected allowed, got denied: %s", resp.Result.Message) + } +} + +func TestAgentValidator_ValidTemplates(t *testing.T) { + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + SystemPrompt: "You are an auditor.", + UserPrompt: "Audit: {{.Request}}", + }, + Execution: &agenticv1alpha1.StepInstructions{ + UserPrompt: "Execute option:\n{{.OptionJSON}}", + }, + }, + }, + } + resp := (&AgentValidator{}).Handle(context.Background(), agentRequest(t, agent)) + if !resp.Allowed { + t.Fatalf("expected allowed, got denied: %s", resp.Result.Message) + } +} + +func TestAgentValidator_PromptLength(t *testing.T) { + for _, tc := range []struct { + name string + field string + length int + allow bool + }{ + {name: "systemPrompt at maximum", field: "systemPrompt", length: maxPromptLength, allow: true}, + {name: "systemPrompt over maximum", field: "systemPrompt", length: maxPromptLength + 1}, + {name: "userPrompt at maximum", field: "userPrompt", length: maxPromptLength, allow: true}, + {name: "userPrompt over maximum", field: "userPrompt", length: maxPromptLength + 1}, + } { + t.Run(tc.name, func(t *testing.T) { + instructions := &agenticv1alpha1.StepInstructions{} + if tc.field == "systemPrompt" { + instructions.SystemPrompt = strings.Repeat("x", tc.length) + } else { + instructions.UserPrompt = strings.Repeat("x", tc.length) + } + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{Analysis: instructions}, + }, + } + + resp := (&AgentValidator{}).Handle(context.Background(), agentRequest(t, agent)) + if resp.Allowed != tc.allow { + t.Fatalf("allowed = %v, want %v: %s", resp.Allowed, tc.allow, resp.Result.Message) + } + }) + } +} + +func TestAgentValidator_MalformedTemplate(t *testing.T) { + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + UserPrompt: "{{.Unclosed", + }, + }, + }, + } + resp := (&AgentValidator{}).Handle(context.Background(), agentRequest(t, agent)) + if resp.Allowed { + t.Fatal("expected denied for malformed template, got allowed") + } + if !strings.Contains(resp.Result.Message, "spec.instructions.analysis.userPrompt") { + t.Errorf("expected field path in message, got: %s", resp.Result.Message) + } +} + +func TestAgentValidator_InvalidFieldInOneStep(t *testing.T) { + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + UserPrompt: "Valid: {{.Request}}", + }, + Verification: &agenticv1alpha1.StepInstructions{ + UserPrompt: "Bad: {{.Unclosed", + }, + }, + }, + } + resp := (&AgentValidator{}).Handle(context.Background(), agentRequest(t, agent)) + if resp.Allowed { + t.Fatal("expected denied when one step has invalid template") + } + if !strings.Contains(resp.Result.Message, "verification") { + t.Errorf("expected verification step in message, got: %s", resp.Result.Message) + } +} + +func TestAgentValidator_SystemPromptOnly_Allowed(t *testing.T) { + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Instructions: &agenticv1alpha1.AgentInstructions{ + Analysis: &agenticv1alpha1.StepInstructions{ + SystemPrompt: "You are a security agent.", + }, + }, + }, + } + resp := (&AgentValidator{}).Handle(context.Background(), agentRequest(t, agent)) + if !resp.Allowed { + t.Fatalf("expected allowed when only systemPrompt is set, got denied: %s", resp.Result.Message) + } }