OLS-3712 OLS-3558: Fix RBAC discovery, execution outcome override, and prompt refinement

.ai/spec/how/reconciler.md

@@ -26,7 +26,7 @@ Audience: AI agents. Behavioral rules and phase semantics live in **what/** spec
 | File | Types / primary responsibilities | Key functions / methods |
 |------|----------------------------------|-------------------------|
 | `reconciler.go` | `AgenticRunReconciler` (embeds `client.Client`, `Agent AgentCaller`, `Log`) | `Reconcile`, `SetupWithManager` |
-| `handlers.go` | (methods on `AgenticRunReconciler`) | `handleAnalysis`, `handleRevision`, `handleExecution`, `handleVerification`, `handleEscalation`, `handleFailed`, `denyAgenticRun`, `conditionTime` |
+| `handlers.go` | (methods on `AgenticRunReconciler`) | `handleAnalysis`, `handleRevision`, `handleExecution`, `handleVerification`, `handleEscalation`, `handleFailed`, `denyAgenticRun`, `conditionTime`, `hasMutationSuccess`, `isObservationAction` |
 | `helpers.go` | `revisionData`, `analysisQuery`, `executionQuery`, `verificationQuery`, `escalationData`; embedded templates via `//go:embed templates/*.tmpl` | `renderTemplate`, `failStep`, `statusPatch`, `hasSandboxClaims`, `isTerminal`, `setVerificationSkipped`, `getLatestAnalysisResult`, `selectedOption`, `trimNonSelectedOptions`, `resetExecutionAndVerification`, `maxAttempts`, `buildEscalationRequest`, `needsRevision`, `buildRevisionContext`, `buildAnalysisQuery`, `buildExecutionQuery`, `buildVerificationQuery`, `prettyJSON` |
 | `approval.go` | — | `getApprovalPolicy`, `getAgenticRunApproval`, `ensureAgenticRunApproval`, `isStageApproved`, `isStageDenied`, `getStageOverrideAgent`, `getStageOption` |
 | `resolve.go` | `resolvedStep`, `resolvedWorkflow` | `resolveAgenticRun`, `stepAgentName` |
@@ -38,7 +38,7 @@ Audience: AI agents. Behavioral rules and phase semantics live in **what/** spec
 | `sandbox_templates.go` | `templateHashInput`; label constants (`LabelManaged`, `LabelRun`, etc.); MCP env DTOs | `EnsureAgentTemplate`, `SandboxTemplateServiceAccount`, `computeTemplateHash`, `agentTemplateName`, `gcOldTemplates`, `patchLLMCredentials`, `credentialsSecretName`, `providerURL`, `patchRequiredSecrets`, `patchMCPServers`, `patchSkillsImage`, `patchSkillsPaths`, `patchProbes`, unstructured helpers (`firstContainer`, `setEnvVar`, `addEnvFromSecret`, …) |
 | `client.go` | `AgentHTTPClientInterface`, `AgentHTTPClient`; `agentRunRequest`, `agentContext`, `agentExecutionResult`, `agentPreviousAttempt`, `agentRunResponse` | `NewAgentHTTPClient`, `(*AgentHTTPClient).Run`, `executionOutputToAgentResult` |
 | `schemas.go` | Package vars: default/minimal analysis schemas, execution/verification/escalation schemas; `defaultOutputSchemas`, `builtInPropertyJSON` | `init` (precompute property JSON), `injectBuiltInProperty`, `outputSchemaForStep` |
-| `rbac.go` | — | `ensureExecutionRBAC`, `cleanupExecutionRBAC`, `annotatedRBACNamespaces`, `deleteIfExists`, `rbacTargetNamespaces`, `truncateK8sName`, `executionRoleName`, `clusterRoleName`, `rbacLabels`, `rbacRulesToPolicyRules`, `normalizeCoreAPIGroup` |
+| `rbac.go` | `readerBindings atomic.Value` (cached CRB names) | `ensureExecutionRBAC`, `cleanupExecutionRBAC`, `resolveReaderBindings`, `addReaderSubject`, `removeReaderSubject`, `addSubjectToBinding`, `removeSubjectFromBinding`, `annotatedRBACNamespaces`, `deleteIfExists`, `rbacTargetNamespaces`, `truncateK8sName`, `executionRoleName`, `clusterRoleName`, `rbacLabels`, `rbacRulesToPolicyRules`, `normalizeCoreAPIGroup` |
 | `results.go` | `statusHolder` interface (defined; no references elsewhere in this package) | `resultCRName`, `agenticRunOwnerRef`, `resultLabels`, `executionRetryIndex`, `resultConditions`, `createAnalysisResult`, `createExecutionResult`, `createVerificationResult`, `createEscalationResult`, `createIdempotent` |
 | `templates/*.tmpl` | Text templates | Names: `analysis_query.tmpl`, `execution_query.tmpl`, `verification_query.tmpl`, `revision_context.tmpl`, `escalation_request.tmpl` |
 | `reconciler_test.go` | `testAgentCaller`, fixtures | `testScheme`, `testDefaultAgent`, `testAgenticRun`, `reconcileOnce`, `getAgenticRun`, … |
@@ -175,6 +175,8 @@ Audience: AI agents. Behavioral rules and phase semantics live in **what/** spec
 - **Creation:** `handleExecution`, when selected option has non-empty `RBAC` rules, calls `ensureExecutionRBAC(ctx, Client, proposal, &selectedOption.RBAC, defaultSandboxSA, proposal.Namespace)`. Creates namespaced `Role`/`RoleBinding` per target namespace (from `Spec.TargetNamespaces` or rule namespace fields), persists comma-joined namespaces in annotation `agentic.openshift.io/rbac-namespaces`, and cluster `ClusterRole`/`ClusterRoleBinding` when cluster rules present. Sandbox SA name constant `defaultSandboxSA` (`lightspeed-agent` in `helpers.go`).
 - **Cleanup:** `cleanupExecutionRBAC` reads annotation to delete bindings/roles; deletes cluster RBAC by derived name. Invoked on: run deletion (finalizer), `handleFailed` if annotation set, after successful escalation completion, and terminal phases via sandbox release path is separate.
 - **`normalizeCoreAPIGroup`:** Maps LLM-facing `"core"` to `""` in K8s `PolicyRule.APIGroups`.
+- **Read RBAC (multi-binding):** `resolveReaderBindings` discovers **all** `ClusterRoleBinding`s where `lightspeed-agent` is a subject (e.g. `cluster-reader`, `cluster-monitoring-view`), caches the list in `readerBindings` (`atomic.Value` storing `[]string`), and returns it. `addReaderSubject`/`removeReaderSubject` iterate all discovered bindings. Individual binding updates are in `addSubjectToBinding`/`removeSubjectFromBinding` with conflict retry loops. [OLS-3712]
+- **Execution outcome override:** In `handleExecution`, when the agent reports `success=false`, the controller calls `hasMutationSuccess(actions)` to check whether all mutating actions actually succeeded. If yes, it overrides `execResult.Success = true` and proceeds to verification (or trust-mode completion). `isObservationAction(type)` classifies non-mutating action types (`pre-check`, `post-check`, `verification`, `check`, `wait`); everything else is treated as a mutation. [OLS-3558]
 
 ---
 

.ai/spec/what/run-lifecycle.md

@@ -40,7 +40,7 @@ Behavioral specification for the `AgenticRun` resource lifecycle. **Approval gat
 15. **Success**: `Verified=True` MUST yield `Completed` once rule 9 reaches the `Verified` branch, unless an earlier branch already returned `Escalated` or `Denied` per rules 9–10.
 16. **Step failure**: Any of `Analyzed`, `Executed`, or `Verified` with status `False` and reasons that are not the dedicated retrying-execution reason MUST yield `Failed` when reached by the derivation order in rule 9 (unless superseded by `Escalated` / `Denied` per rules 9–10).
 17. **Escalation failure**: `Escalated` with status `False` MUST yield `Failed` once rule 9 evaluates the `Escalated` presence branch (non-`True`, non-`Unknown`).
-18. **Result CR linkage**: Each analysis/execution/verification/escalation attempt SHOULD append a `status.steps.*.results[]` entry naming the corresponding result resource with an outcome matching agent success/failure for that attempt.
+18. **Result CR linkage**: Each analysis/execution/verification/escalation attempt SHOULD append a `status.steps.*.results[]` entry naming the corresponding result resource with an outcome matching agent success/failure for that attempt. **Exception:** when the execution agent reports `success=false` but all mutating actions succeeded (only inline verification checks failed), the controller MUST override the outcome to `Succeeded` and proceed to the verification step. Observation action types (`pre-check`, `post-check`, `verification`, `check`, `wait`) are not considered when determining mutation success.
 19. **Observed generation**: Conditions SHOULD carry `observedGeneration` aligned with `metadata.generation` when the controller updates them for the current spec generation, except revision completion MAY pin the analyzed condition to the generation that triggered the revision, per existing reconciliation behavior.
 20. **Immutable spec (excluding revision)**: Once set, `spec.request`, `spec.targetNamespaces`, `spec.analysisOutput`, `spec.tools`, `spec.analysis`, `spec.execution`, and `spec.verification` MUST NOT change; CEL on the CRD enforces this. Only `spec.revisionFeedback` is mutable for iterative feedback.
 21. **Option trim after analysis**: When multiple remediation options exist, execution MUST use the option selected through the approval resource; non-selected options MAY be removed from the stored analysis result before execution (see `approval.md`).
@@ -72,3 +72,4 @@ Behavioral specification for the `AgenticRun` resource lifecycle. **Approval gat
 - [DONE: OLS-3018] `EmergencyStopped` phase and condition type added to run lifecycle. See `system-config.md` for full kill switch specification.
 - [PLANNED: OLS-3268] `NoActionRequired` terminal phase: when analysis returns `actionRequired=false`, the operator sets `Analyzed=True` with reason `NoActionRequired` and the run auto-completes, bypassing approval/execution/verification.
 - [DONE: OLS-3295] Renamed `Proposal` CRD kind to `AgenticRun`, `ProposalApproval` to `AgenticRunApproval`, and updated all associated API surface (labels, RBAC resources, CLI commands, audit events, OTEL spans).
+- [DONE: OLS-3558] Execution outcome override — controller no longer hard-fails when `success=false` but all mutating actions succeeded; defers outcome to the verification step. See `sandbox-execution.md` rule 21b.

.ai/spec/what/sandbox-execution.md

@@ -14,8 +14,8 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes
 8. **HTTP contract**: Each step MUST call the agent **`POST /v1/agent/run`** with JSON body carrying at least `query`, `outputSchema`, and `context`; optional `systemPrompt` and `timeout_ms` exist in the wire shape but **system prompt MUST be sent empty** in the current implementation (prompt material lives in `query` and templates).
 9. **Response handling**: HTTP success responses MUST be parsed as JSON matching the per-step schema (analysis/execution/verification/escalation). Non-success HTTP MUST fail the step with an error surfaced to run conditions.
 10. **Output schema selection**: `outputSchema` MUST be the step-specific JSON schema: 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 and encode workflow flags indicating whether execution/verification steps exist (template-rendered). The analysis prompt MUST instruct the agent to use kubectl/oc to inspect cluster state before diagnosing, produce a concrete remediation script of executable bash commands (including pre-checks, mutations, waits, and post-checks), and derive RBAC by tracing every command in the script — requiring `get`/`list`/`watch` as minimum read verbs for every resource.
-12. **Execution query payload**: The `query` MUST include JSON describing the approved remediation option, which contains a concrete remediation script (ordered bash commands). The execution prompt MUST instruct the agent to follow the script exactly, execute commands in order without substitution, and dry-run every mutation command with `--dry-run=server` before applying.
+11. **Analysis query payload**: The `query` string MUST encode the user request or revision-augmented request and encode workflow flags indicating whether execution/verification steps exist (template-rendered). The analysis prompt MUST instruct the agent to use kubectl/oc to 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). The execution prompt 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 prompt MUST NOT instruct the agent to perform inline 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 and inline verification) when available.
 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.
@@ -49,6 +49,8 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes
 19b. [PLANNED: OLS-3594] **OLSConfig / classic-operator handoff** for introspection and connection details is deferred with 19a (includes OLS-3572).
 20. **Sandbox observability patch**: Immediately after creating a claim (or Pod in `bare-pod` mode), the controller MUST patch `AgenticRun.status.steps.<step>.sandbox` with the resource name and operator namespace so consoles/CLIs can tail logs before the sandbox is ready. In `bare-pod` mode, `status.steps.<step>.sandbox.claimName` MUST be set to the Pod name (same field used for SandboxClaim names in `sandbox-claim` mode).
 21. **Execution RBAC materialization**: When the approved remediation option includes RBAC requests, before execution the controller MUST create a **per-run ServiceAccount** named `ls-exec-{run-namespace}-{run-name}` in the operator namespace (truncated to 63 chars), then create namespace-scoped `Role`+`RoleBinding` pairs in each target namespace and `ClusterRole`+`ClusterRoleBinding` for cluster-scoped rules, binding subjects to **this per-run SA** (not the shared `lightspeed-agent` SA). The per-run SA MUST NOT carry an owner reference — cross-namespace owner refs are unsupported by Kubernetes GC (the SA lives in the operator namespace while the AgenticRun may be in a different namespace). Cleanup is handled explicitly by `cleanupExecutionRBAC` via the AgenticRun's finalizer. Idempotent create MUST tolerate existing objects (`AlreadyExists` is a no-op). Only execution sandbox pods use this SA — analysis and verification pods continue using `lightspeed-agent`. The operator's `cluster-admin` privilege (external prerequisite) ensures no RBAC escalation issues when creating arbitrary Roles/SAs. See the workspace-level agentic security spec at `ols/.ai/spec/what/agentic-security.md` rules 7-15 for full specification.
+21a. **Read RBAC multi-binding resolution**: The shared `lightspeed-agent` SA may have **multiple** `ClusterRoleBinding`s granting read access (e.g. `cluster-reader` and `cluster-monitoring-view`). When granting read permissions to a per-run execution SA, the controller MUST discover **all** `ClusterRoleBinding`s where `lightspeed-agent` is a subject and add the per-run SA to every one of them — not just the first match. Discovery results MUST be cached for the lifetime of the process (the infrastructure CRBs are static). On cleanup, the per-run SA MUST be removed from all discovered bindings. [OLS-3712]
+21b. **Execution outcome override**: When the execution agent reports `success=false` but **all mutating actions succeeded**, the controller MUST NOT hard-fail the run. Instead it MUST override the execution outcome to `Succeeded` and proceed to the verification step (or trust-mode completion if verification is absent). The controller determines mutation success by inspecting `ExecutionAction` entries: observation action types (`pre-check`, `post-check`, `verification`, `check`, `wait`) are excluded; all remaining action types are considered mutations. The run MUST hard-fail only when (a) no mutating actions exist (agent never attempted the fix), or (b) any mutating action has `outcome=Failed`. [OLS-3558]
 22. **RBAC subjects namespace**: RoleBindings MUST reference the **per-run service account** in the **operator namespace** (where sandbox pods run), even when roles live in target namespaces.
 23. **RBAC tracking annotation**: The controller SHOULD persist the list of namespaces receiving namespace-scoped RBAC on the `AgenticRun` via a dedicated annotation so cleanup can run after retries or status resets.
 24. **RBAC cleanup**: The per-run SA and all associated Roles/RoleBindings/ClusterRoles/ClusterRoleBindings MUST be **explicitly deleted immediately after execution completes** (before verification starts). This ensures the execution SA does not persist into subsequent phases. On terminal outcomes (failure, escalation, deletion, `EmergencyStopped`), the controller MUST also run cleanup as a fallback (see `system-config.md` rule 14). No owner references are used (cross-namespace GC unsupported); the AgenticRun finalizer guarantees cleanup on deletion even if in-flight cleanup fails.
@@ -97,5 +99,7 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes
 - [PLANNED: OLS-3038] **TLS verification and network policy** for agent traffic may replace permissive internal TLS client behavior.
 - [PLANNED: OLS-2894] Support **multiple concurrent skills images** in template derivation beyond the first `skills` entry if product requires composite skill bundles.
 - [DONE: OLS-3295] Renamed per-run ServiceAccount from `ls-exec-{proposal-namespace}-{proposal-name}` to `ls-exec-{run-namespace}-{run-name}` and updated all associated RBAC resource naming.
+- [DONE: OLS-3712] Read RBAC multi-binding resolution — controller discovers all `ClusterRoleBinding`s for `lightspeed-agent` SA, not just the first. See rule 21a.
+- [DONE: OLS-3558] Execution outcome override — controller overrides `success=false` to `Succeeded` when all mutations passed, deferring outcome to verification. See rule 21b.
 - [PLANNED: OLS-3572] Inter-operator configuration handoff — refactor `PodSpecBuilder` to read a base `corev1.PodSpec` from the `lightspeed-sandbox-config` ConfigMap (produced by lightspeed-operator) and apply all per-run config (LLM creds, skills, per-run MCP, SA, probes, audit, reasoning) through a single overlay code path. Eliminates the current duplication between `PodSpecBuilder` (bare-pod) and `EnsureAgentTemplate` (sandbox-claim) — one function builds the complete PodSpec, then the mode determines delivery: bare-pod creates a Pod directly, sandbox-claim converts the PodSpec into a `SandboxTemplate`. The `--sandbox-mode` flag is deprecated in favor of `sandbox-mode` key in the ConfigMap (sourced from `OLSConfig.spec.agenticOLS.sandboxMode`). If the ConfigMap is missing, the agentic operator fails hard after timeout — no fallback. [PLANNED: OLS-3572 — design doc TBD].
 - [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.