CNTRLPLANE-3339: Add promptfoo eval framework for SME agents

.claude/agents/api-sme.md

@@ -13,10 +13,10 @@ You are an API subject matter expert system architect specializing in HCP.
 - Basic security patterns (auth, rate limiting)
 
 ## Approach
-1. Follow OpenShift dev guides from https://github.com/openshift/enhancements/tree/master/dev-guide
-2. Apply best practices from https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md
-3. Consider any API stable, running in production and ensure any API change is backward compatible
-4. Keep it simple - avoid premature optimization
+
+**MANDATORY**: Before writing any review, you MUST run `make api-lint-fix` and include its output in your review. Do not skip this step. The linter is the authoritative source for convention violations. Your review must start with the linter findings, then add your own analysis on top.
+
+Stick to ../api/AGENTS.md
 
 ## Output
 - API definitions that align with OpenShift and Kubernetes best practices

Makefile

@@ -619,6 +619,19 @@ cpo-container-sync:
 karpenter-upstream-e2e:
 	./karpenter-operator/e2e/upstream-e2e.sh
 
+EVAL_REPEAT ?= 1
+EVAL_PASS_RATE_THRESHOLD ?= 100
+PROMPTFOO_VERSION ?= 0.121.9
+
+.PHONY: eval-agents
+eval-agents: ## Run agent evals with promptfoo
+	cd test/eval && PROMPTFOO_PASS_RATE_THRESHOLD=$(EVAL_PASS_RATE_THRESHOLD) \
+		npx promptfoo@$(PROMPTFOO_VERSION) eval \
+		$(if $(EVAL_FILTER),--filter-pattern "$(EVAL_FILTER)") \
+		$(if $(EVAL_OUTPUT),--output "$(EVAL_OUTPUT)") \
+		--repeat $(EVAL_REPEAT) \
+		--no-cache
+
 ## --------------------------------------
 ## Tooling Binaries
 ## --------------------------------------

api/AGENTS.md

@@ -15,24 +15,6 @@ For conventions, always trust the kube-api-linter (`make api-lint-fix`). Do not
 - Use feature gates for experimental functionality
 - CRD generation via controller-gen with OpenShift-specific tooling
 
-Key make targets for API work:
-
-```bash
-make api               # Regenerate all CRDs, deepcopy, clients
-make api-lint-fix      # Run API linter and auto-fix violations
-make verify-api-deps   # Verify API dependencies
-make verify            # Full verification (includes api, fmt, vet, lint)
-make update            # Full update (api-deps, workspace-sync, deps, api, api-docs, clients)
-ENVTEST_OCP_K8S_VERSIONS=1.35.0 make test-envtest-ocp # Run envtest for CEL validations
-```
-
-### API Dependencies
-
-It is imperative that the imported dependencies are kept minimal. Use `make verify-api-deps` to verify that the dependencies are allowed.
-New dependencies must be approved by API reviewers and added to `api/.imports_allowed`.
-
-To avoid introducing new dependencies, do not add utils or methods to the API types.
-
 ### Serialization
 
 - **Always set `omitempty` or `omitzero` on every field, regardless of whether it is `+required` or `+optional`.** `omitempty`/`omitzero` tags control serialization, not validation. `+required` is a schema constraint enforced at admission time; the serialization tag controls what goes on the wire. Without a tag, a zero-value field serializes as an explicit value (e.g., `"pullSecret": {"name": ""}`), which makes the API server unable to distinguish "not set" from "explicitly set to empty." This breaks defaulting, server-side apply field ownership, and strategic merge patch — all of which rely on field absence to mean "don't touch this." Additionally, without omission a structured client serializes the empty object, which passes the `+required` check (based on key presence) without validating the value — so a user can forget to set a required field, it passes admission, and the reader sees a required field with an unexpected empty value.
@@ -57,6 +39,16 @@ To avoid introducing new dependencies, do not add utils or methods to the API ty
 
 ## API Type Change Guidelines
 
+### Best Practices and Patterns
+
+Use api/karpenter/v1beta1/karpenter_types.go and api/hypershift/v1beta1/etcdbackup_types.go as examples of best practices and patterns.
+
+Don't use the other existing APIs as examples as they might have many legacy constraints.
+
+### Field Grouping
+
+**When multiple fields on a spec share a common prefix or relate to the same feature, they MUST be grouped into a dedicated struct.** Top-level specs like HostedClusterSpec and NodePoolSpec should only contain fields that are independently meaningful. If removing one field would make another field meaningless, they belong together in a sub-struct. A common signal is fields that share a name prefix (e.g., `BarEndpoint`, `BarConfig`, `BarID` all relate to "Bar" and should be a single `Bar` field with a `BarSpec` struct).
+
 ### N-1 and N+1 Compatibility
 
 Every change to an API type must be safe for both:
@@ -85,3 +77,14 @@ See `api/hypershift/v1beta1/nodepool_types_test.go` for an example of this patte
 
 All API CEL validations must be covered with envtests, see test/envtest/README.md for details
 
+#### Key make targets for API work:
+
+```bash
+make api               # Regenerate all CRDs, deepcopy, clients
+make api-lint-fix      # Run API linter and auto-fix violations
+make verify            # Full verification (includes api, fmt, vet, lint)
+make update            # Full update (api-deps, workspace-sync, deps, api, api-docs, clients)
+ENVTEST_OCP_K8S_VERSIONS=1.35.0 make test-envtest-ocp # Run envtest for CEL validations
+```
+
+All these must pass for any change before creating a PR

test/eval/README.md

@@ -0,0 +1,46 @@
+# Agent Evals
+
+Eval framework using [promptfoo](https://github.com/promptfoo/promptfoo)
+for testing SME agent definitions and AGENTS.md conventions.
+
+## Prerequisites
+
+- `claude` CLI installed and authenticated
+- Node.js (npx)
+- python3
+
+## Usage
+
+```bash
+# Run all scenarios
+make eval-agents
+
+# Run a specific test
+make eval-agents EVAL_FILTER=api-sme
+
+# View results in browser
+cd test/eval && npx promptfoo@0.121.9 view
+
+# Output JUnit XML for CI
+make eval-agents EVAL_OUTPUT=results.xml
+```
+
+## How It Works
+
+- **Test scenarios** are defined inline in `promptfooconfig.yaml` with prompts and `llm-rubric` assertions
+- **Patch-based tests** use `beforeEach`/`afterEach` hooks to create a
+  temporary git worktree, apply the patch there, and clean up after the test
+- **Per-assertion judging**: each expected issue is a separate `llm-rubric`
+  assertion graded by an LLM judge
+- **Parallel execution**: configurable via `maxConcurrency` in the config
+- **Web UI**: `npx promptfoo@0.121.9 view` shows results in a browser with
+  side-by-side comparison for iterating on prompts
+
+## Configuration
+
+| Env Var | Default | Description |
+|---------|---------|-------------|
+| `EVAL_MODEL` | `claude-opus-4-6` | Model for agent invocation |
+| `EVAL_FILTER` | (all) | Filter tests by description pattern |
+| `EVAL_OUTPUT` | (none) | Output file (.json, .xml, .html) |
+| `ANTHROPIC_VERTEX_PROJECT_ID` | - | GCP project for Vertex AI auth |

test/eval/hooks.js

@@ -0,0 +1,47 @@
+const { execFileSync } = require('child_process');
+const crypto = require('crypto');
+const fs = require('fs');
+const path = require('path');
+
+const repoRoot = path.resolve(__dirname, '../..');
+
+module.exports = async function extensionHook(hookName, context) {
+  if (hookName === 'beforeEach') {
+    const patchPath = context.test?.vars?.patchFile;
+    if (patchPath) {
+      const fullPath = path.resolve(__dirname, patchPath);
+      if (fs.existsSync(fullPath)) {
+        const worktreeName = `eval-${Date.now()}-${crypto.randomUUID()}`;
+        const worktreeDir = path.join(require('os').tmpdir(), 'hypershift-eval', worktreeName);
+        let worktreeCreated = false;
+        try {
+          execFileSync('git', ['worktree', 'add', worktreeDir, 'HEAD'], { cwd: repoRoot, stdio: 'pipe' });
+          worktreeCreated = true;
+          execFileSync('git', ['apply', fullPath], { cwd: worktreeDir, stdio: 'pipe' });
+          console.log(`Created worktree and applied patch: ${worktreeDir}`);
+          context.test.vars.worktreePath = worktreeDir;
+        } catch (e) {
+          if (worktreeCreated) {
+            try {
+              execFileSync('git', ['worktree', 'remove', worktreeDir, '--force'], { cwd: repoRoot, stdio: 'pipe' });
+            } catch (_) {}
+          }
+          console.error(`Failed to create worktree or apply patch: ${e.message}`);
+        }
+      }
+    }
+    return context;
+  }
+
+  if (hookName === 'afterEach') {
+    const worktreeDir = context.test?.vars?.worktreePath;
+    if (worktreeDir) {
+      try {
+        execFileSync('git', ['worktree', 'remove', worktreeDir, '--force'], { cwd: repoRoot, stdio: 'pipe' });
+        console.log(`Removed worktree: ${worktreeDir}`);
+      } catch (e) {
+        console.error(`Failed to remove worktree: ${e.message}`);
+      }
+    }
+  }
+};

test/eval/promptfooconfig.yaml

@@ -0,0 +1,160 @@
+description: "HyperShift agent and convention evals"
+
+extensions:
+  - file://hooks.js:extensionHook
+
+providers:
+  - id: "exec: ./run-agent.sh"
+    label: claude
+
+prompts:
+  - "{{prompt}}"
+
+defaultTest:
+  options:
+    provider:
+      id: vertex:claude-opus-4-6
+      config:
+        projectId: "{{ env.ANTHROPIC_VERTEX_PROJECT_ID }}"
+        region: global
+        temperature: 0
+
+tests:
+  # --- api-sme ---
+  - description: "api-sme/01-api-design-review"
+    vars:
+      agent: api-sme
+      tools: "Bash,Read,Grep,Glob"
+      patchFile: "testdata/sme-agents/api-sme/01-api-design-review/patch.diff"
+      prompt: |
+        I've added new foo fields to HostedClusterSpec in
+        api/hypershift/v1beta1/hostedcluster_types.go. The code is already
+        on disk. Please review the change.
+    assert:
+      - type: llm-rubric
+        value: "The output identifies that Foo_IP should use Go PascalCase naming (no underscores)"
+      - type: llm-rubric
+        value: "The output identifies that JSON tags must use lowerCamelCase (not snake_case or PascalCase)"
+      - type: llm-rubric
+        value: "The output identifies missing omitempty or omitzero on every field"
+      - type: llm-rubric
+        value: "The output identifies missing IP address format validation (CEL or kubebuilder)"
+      - type: llm-rubric
+        value: "The output identifies that FooConfig should not be a pointer — use value type with omitzero instead"
+      - type: llm-rubric
+        value: "The output identifies missing +listType marker on slice field for server-side apply"
+      - type: llm-rubric
+        value: "The output identifies that FooID immutability rule is incomplete — self == oldSelf either blocks initial set or allows remove-then-set bypass on optional fields"
+      - type: llm-rubric
+        value: "The output identifies missing +optional or +required markers on fields"
+      - type: llm-rubric
+        value: "The output identifies that fields sharing a common prefix should be consolidated into a single struct rather than scattered on the parent spec"
+
+  # --- cloud-provider-sme ---
+  - description: "cloud-provider-sme/01-kms-integration"
+    vars:
+      agent: cloud-provider-sme
+      prompt: |
+        We want to implement customer-managed encryption key support for
+        etcd data at rest in hosted control planes. The feature should work
+        across AWS and Azure. How should we design this in HyperShift?
+        What API changes and controller logic are needed?
+    assert:
+      - type: llm-rubric
+        value: "The output mentions platform-specific KMS services (AWS KMS and Azure Key Vault)"
+      - type: llm-rubric
+        value: "The output proposes an API-level abstraction for cross-platform KMS configuration"
+      - type: llm-rubric
+        value: "The output addresses IAM or credential requirements for KMS access"
+      - type: llm-rubric
+        value: "The output references Kubernetes EncryptionConfiguration or etcd encryption provider mechanism"
+
+  # --- control-plane-sme ---
+  - description: "control-plane-sme/01-ho-cpo-version-skew"
+    vars:
+      agent: control-plane-sme
+      prompt: |
+        We want to add a new control plane component called "policy-engine"
+        that enforces admission policies on the hosted cluster. The
+        component needs to behave differently depending on the OCP version
+        of the hosted control plane — in 4.18+ it should use
+        ValidatingAdmissionPolicy (native K8s), but in 4.17 and below it
+        should fall back to a webhook-based approach.
+
+        The HyperShift Operator needs to know which variant to configure
+        when reconciling the HostedCluster, and the CPO needs to deploy
+        the right version of the component.
+
+        How should we implement this considering HyperShift's versioning
+        model and the HO/CPO version skew constraints?
+    assert:
+      - type: llm-rubric
+        value: "The output references the cpov2 or controlplane-component framework for deploying the component"
+      - type: llm-rubric
+        value: "The output states that version-dependent behavior should be decided in the CPO based on the hosted cluster release version, not in the HO"
+      - type: llm-rubric
+        value: "The output explains that HO and CPO can run different versions and the HO must not assume which CPO version is running"
+      - type: llm-rubric
+        value: "The output states that the CPO image is part of the OCP release payload and matches the hosted cluster version"
+      - type: llm-rubric
+        value: "The output considers impact on control plane resource footprint (CPU, memory)"
+
+  # --- data-plane-sme ---
+  - description: "data-plane-sme/01-spot-instance-lifecycle"
+    vars:
+      agent: data-plane-sme
+      prompt: |
+        We want to improve spot/preemptible instance support in NodePools.
+        Currently users can request spot instances on AWS, but we want to
+        ensure consistent behavior across platforms. How should the NodePool
+        API and controllers handle instance interruption events, and what
+        changes are needed for the data plane upgrade flow to account for
+        spot instance characteristics?
+    assert:
+      - type: llm-rubric
+        value: "The output discusses NodePool API abstraction for spot across platforms (AWS Spot, Azure Spot VMs, GCP Preemptible/Spot)"
+      - type: llm-rubric
+        value: "The output addresses instance interruption lifecycle (node drain, workload rescheduling, machine replacement)"
+      - type: llm-rubric
+        value: "The output considers impact of spot instances on rolling upgrade strategy"
+      - type: llm-rubric
+        value: "The output references ClusterAPI (CAPI) resources or controllers (MachineSet, MachineDeployment, Machine)"
+
+  # --- hcp-architect-sme ---
+  - description: "hcp-architect-sme/01-architectural-review"
+    vars:
+      agent: hcp-architect-sme
+      prompt: |
+        We are considering a design where the hosted cluster's worker
+        nodes send status updates directly to the hypershift-operator in
+        the management cluster via a webhook. The worker node would call
+        a REST endpoint on the hypershift-operator to report node health
+        metrics. This way we get real-time health data without polling.
+
+        What do you think of this approach?
+    assert:
+      - type: llm-rubric
+        value: "The output flags violation of unidirectional communication principle (management to hosted, never reverse)"
+      - type: llm-rubric
+        value: "The output raises security or tenant isolation concerns"
+      - type: llm-rubric
+        value: "The output suggests an alternative architecture that respects unidirectional communication"
+
+  # --- conventions ---
+  - description: "conventions/01-go-test-style"
+    vars:
+      prompt: |
+        Write a unit test for a function called ParseMaintenanceWindow that
+        takes a cron string and duration in minutes, and returns a
+        MaintenanceWindow struct or an error. It should reject empty cron
+        strings, durations less than 30 minutes, and durations greater than
+        480 minutes. It should accept valid inputs like "0 2 * * 6" with
+        duration 120. Just write the test, not the function itself.
+    assert:
+      - type: llm-rubric
+        value: "The generated test code uses Gherkin syntax with 'When... it should...' pattern in test names"
+      - type: llm-rubric
+        value: "The generated test code uses gomega matchers for assertions (Expect, BeTrue, BeFalse, HaveOccurred, etc.) rather than standard testing package assertions"
+
+evaluateOptions:
+  maxConcurrency: 6

test/eval/run-agent.sh

@@ -0,0 +1,33 @@
+#!/bin/bash
+# Wrapper for promptfoo exec: provider
+# $1 = prompt, $2 = options JSON, $3 = context JSON
+set -euo pipefail
+
+PROMPT="$1"
+CONTEXT="${3:-"{}"}"
+
+# Extract agent, tools, and worktree path from context vars
+AGENT=$(echo "$CONTEXT" | python3 -c "import sys,json; v=json.load(sys.stdin).get('vars',{}); print(v.get('agent',''))" 2>/dev/null)
+TOOLS=$(echo "$CONTEXT" | python3 -c "import sys,json; v=json.load(sys.stdin).get('vars',{}); print(v.get('tools','Read,Grep,Glob'))" 2>/dev/null)
+WORKDIR=$(echo "$CONTEXT" | python3 -c "import sys,json; v=json.load(sys.stdin).get('vars',{}); print(v.get('worktreePath',''))" 2>/dev/null)
+
+ARGS=(
+  --print
+  --model "${EVAL_MODEL:-claude-opus-4-6}"
+  --allowed-tools "$TOOLS"
+  --no-session-persistence
+  --output-format text
+)
+
+if [ -n "$AGENT" ]; then
+  ARGS+=(--agent "$AGENT")
+fi
+
+# Use worktree if available, otherwise repo root
+if [ -n "$WORKDIR" ] && [ -d "$WORKDIR" ]; then
+  cd "$WORKDIR"
+else
+  cd "$(dirname "$0")/../.." || exit 1
+fi
+
+printf '%s' "$PROMPT" | exec claude "${ARGS[@]}"