support multiple controllers + optional agent fields (chart 0.3.0)

[bborbe]

Extends the agent chart (0.2.0 → 0.3.0) so it can model the real quant deployment, which the 0.2.0 single-controller block couldn't express. Prep for migrating quant's controllers + agents off kubectl-managed manifests onto Helm.

Changes

• BREAKING: controller: → controllers: list. The chart ran exactly one agent-task-controller; quant runs two (per Obsidian vault: openclaw, personal, differing only in vaultName/gitRestUrl/taskDir). Each list entry renders agent-task-controller-<name> StatefulSet + Service + Secret, mirroring the agents: list pattern ($ root, leading ---). Nested fields (image, sentry, storage) are nil-safe so a minimal entry works.
• Optional agent fields. volumeMountPath/volumeClaim/PVC and resources are now optional per agent (a stateless agent — e.g. a backtest runner — declares no volumeMountPath and gets no PVC), plus an optional secretName override (for agents whose existing Secret name differs from the agent name). This lets a cluster reference existing (e.g. teamvault-managed) Secrets by leaving secretEnv empty.
• recurring-task-creator overrides. recurringTaskCreator.affinity + recurringTaskCreator.pullSecrets (fall back to the globals) — it often pins a different node pool / pull secret than the executor + controllers.

Verification

• helm lint clean. helm template renders valid YAML (all parse via yaml.safe_load_all) for a full quant-shaped values set: 2 named controllers, a stateful agent (PVC) + a stateless agent (no PVC/volumeClaim/resources, overridden secretName), recurring with node_type=agent affinity + docker-quant pull secret. Executor-only (controllers: []) still renders 0 controllers.
• make precommit passes.

Chart consumers pin --version 0.3.0.

🤖 Generated with Claude Code

[ben-s-pull-request-reviewer]

PR Review: support multiple controllers + optional agent fields (chart 0.3.0)

Scope

Files changed: CHANGELOG.md, helm/Chart.yaml, helm/templates/_helpers.tpl, helm/templates/agents.yaml, helm/templates/controller-secret.yaml, helm/templates/controller-service.yaml, helm/templates/controller-statefulset.yaml, helm/templates/recurring-task-creator-statefulset.yaml, helm/values.yaml

Mode: selector (helm template changes — no ast-grep mechanical funnel applicable)

---

Must Fix (Critical)

None identified. The PR is a Helm chart change; the pre-commit lint step is appropriate for CI rather than reviewer re-run.

---

Should Fix (Important)

1. storageClassName added to agents but missing from controller BoltDB PVC (helm/templates/controller-statefulset.yaml:135-138)

• agents.yaml:64 gained storageClassName for agent PVCs, but the controller's BoltDB PVC at controller-statefulset.yaml:135-138 still only uses ($c.storage | default dict).storageClassName — inconsistency between the two PVC emission sites. A cluster that sets storageClassName on agents (via storageClassName: in the agent values block) cannot set it on the controller's datadir volume without the same field being plumbed through the controller's storage: sub-key.

2. Hard-coded vault-obsidian-<vaultName> service name still renders silently wrong (helm/templates/controller-statefulset.yaml:85)

• value: {{ $c.gitRestUrl | default (printf "http://vault-obsidian-%s:9090" (required "controllers[].vaultName is required" $c.vaultName)) | quote }}
• If gitRestUrl is empty AND vaultName is set to a non-obsidian name, the controller deploys but silently points at a service that does not exist. The comment in values.yaml acknowledges this but there is no Helm validation to catch the mismatch at install time.

3. volumeClaim emitted into Config spec even when no PVC exists (helm/templates/agents.yaml:19-21)

• When volumeMountPath is set, volumeClaim is correctly emitted into the Config CR's spec.volumeClaim. However, if volumeMountPath is omitted (stateless agent), the PVC is not created but volumeClaim is also omitted from the Config spec — this is correct behavior. No action needed. (Reviewing the concern raised in the plan — on re-examination the conditional {{- if $agent.volumeMountPath }} at line 19 correctly gates both volumeClaim and volumeMountPath, so this concern is resolved.)

4. Inconsistent template helper removal (helm/templates/_helpers.tpl removed agent.controller.image and agent.controller.gitRestUrl)

• These helpers were used only by the old single-controller templates. Removing them is correct and leaves no orphaned references. No action needed.

---

Nice to Have (Optional)

1. CHANGELOG.md unreleased section correctly structured — proper semantic versioning prefixes (BREAKING, helm) and brief descriptions. ✓

2. Chart version bumped 0.2.0 → 0.3.0 — matches CHANGELOG "Chart 0.2.0 → 0.3.0" notation. ✓

3. LICENSE file present at repo root. ✓

4. helm lint equivalent not re-run by reviewer — this is a template-heavy change; running helm template smoke against the new values.yaml example block would have caught the controllers: [] empty-list issue (no controllers rendered by default). Recommend CI verify helm template output is non-empty for a controller-enabled values override.

---

Concerns from Plan — Assessed

Concern	File	Assessment
controller: → controllers: breaking change, no migration path	controller-statefulset.yaml	Confirmed. controllers: [] defaults to empty; users with controller.enabled=true in 0.2.0 values get 0 controllers in 0.3.0. No helm lint warning.
kafkaBrokers now per-controller required	controller-statefulset.yaml:83	Acknowledged. required call present. The 0.2.0 controller.kafkaBrokers: "" was equally invalid at runtime — the new structure makes the requirement explicit per entry.
Secret name change agent-task-controller → agent-task-controller-<name>	controller-secret.yaml:12	Confirmed breaking. External RBAC / network policies referencing the old name silently break.
vault-obsidian-<vaultName> hard-coded derivation	controller-statefulset.yaml:85	Still present. Comment acknowledges quant-specific naming but no install-time validation added.
PVC conditional on volumeMountPath	agents.yaml:19-21, 51-70	Correctly gated. Both volumeClaim and volumeMountPath are inside {{- if $agent.volumeMountPath }}.
Optional secretName override + secretEnv coexistence	agents.yaml:18	No conflict. secretName sets Config.spec.secretName (which secret the agent pod mounts), secretEnv controls whether a new Secret is created with those env values. They serve different purposes.
recurringTaskCreator.affinity fallback to global	recurring-task-creator-statefulset.yaml:111	Correctly implemented as `(.Values.recurringTaskCreator.affinity

---

Summary

The PR delivers the stated goal (multi-controller support, optional agent PVC/resources, recurringTaskCreator affinity/pullSecrets overrides) with accurate CHANGELOG and version bump. The primary reviewer concern is the breaking change from controller: to controllers:[] — existing 0.2.0 users who set controller.enabled=true will silently get zero controllers after upgrading. A values migration note in the CHANGELOG or a helm lint check for the empty-list case would help. The secret naming change and obsidian service derivation are known trade-offs documented in comments but carry silent-break risk for non-quant adopters.

{
  "verdict": "request-changes",
  "summary": "PR delivers multi-controller support and optional agent fields correctly, but introduces a silent breaking change: existing 0.2.0 installs using `controller.enabled=true` will render zero controllers with `controllers:[]` (empty by default). Additionally, the hardcoded `vault-obsidian-<vaultName>` GIT_REST_URL derivation is acknowledged but not validated at install time, and the secret name change from `agent-task-controller` to `agent-task-controller-<name>` will silently break external RBAC/network policies referencing the old name.",
  "comments": [
    {
      "file": "helm/values.yaml",
      "line": 87,
      "severity": "major",
      "message": "controllers: [] defaults to empty — breaking change from 0.2.0 where `controller.enabled=true` rendered a controller. Existing users upgrading from chart 0.2.0 will get 0 controllers with no helm lint error. Recommend: add a comment explaining the migration path, or consider a note in CHANGELOG under the BREAKING bullet explaining the required values transformation."
    },
    {
      "file": "helm/templates/controller-statefulset.yaml",
      "line": 85,
      "severity": "major",
      "message": "GIT_REST_URL default `http://vault-obsidian-<vaultName>:9090` silently points at a non-existent service for non-quant clusters. The comment in values.yaml acknowledges this but there is no Helm-level validation. Consider adding a comment in the rendered template warning when gitRestUrl derivation is used, or document the migration path prominently."
    },
    {
      "file": "helm/templates/controller-secret.yaml",
      "line": 12,
      "severity": "major",
      "message": "Secret name changed from `agent-task-controller` to `agent-task-controller-<name>`. Existing clusters with RBAC RoleBindings, NetworkPolicies, or external tooling referencing `agent-task-controller` by name will silently break. No helm template error; runtime failure only. Breaking change should be called out explicitly in CHANGELOG."
    },
    {
      "file": "helm/templates/controller-statefulset.yaml",
      "line": 135,
      "severity": "minor",
      "message": "Agent PVC gained `storageClassName` field (agents.yaml:64) but the controller's BoltDB volumeClaimTemplate does not surface `storageClassName` through the `storage:` sub-key in the same way. Clusters using a non-default StorageClass for the controller's datadir must set it via the controller's `storage:` block — verify this is equally possible or document the gap."
    }
  ],
  "concerns_addressed": [
    "correctness: controller: single-block replaced by controllers: list — confirmed breaking change, no migration path documented",
    "correctness: kafkaBrokers per-controller required — required call present at line 83, explicit per-entry requirement documented in values.yaml comments",
    "security: Secret name agent-task-controller → agent-task-controller-<name> — confirmed, will break existing RBAC/policies silently",
    "correctness: vault-obsidian-<vaultName> hard-coded derivation — still present at line 85, acknowledged in comments but no install-time validation",
    "correctness: PVC conditional on volumeMountPath — correctly gated in agents.yaml:19-21 and 51-70",
    "security: Optional secretName override + secretEnv coexistence — no conflict, different purposes (pod secret mount vs chart-managed Secret creation)",
    "correctness: recurringTaskCreator.affinity fallback to global — correctly implemented at recurring-task-creator-statefulset.yaml:111"
  ]
}

[bborbe]

Addressed in 4437748 — the findings are all about documenting the (intentional) breaking change; done:

• controllers[] empty default / migration — added a Migration (0.2.0 → 0.3.0) block to the CHANGELOG BREAKING bullet with a before/after values diff (controller: {...} → controllers: [{name: main, ...}]), and called out that controllers: [] renders zero controllers until migrated.
• agent-task-controller → agent-task-controller-<name> rename — explicitly documented in the CHANGELOG (external RBAC/NetworkPolicies must update to the per-name form; the quant crossover uses --take-ownership and the live objects already carry per-vault names).
• gitRestUrl derivation — added a template-level comment above the GIT_REST_URL env pointing at the controllers[].gitRestUrl override (the values.yaml already carries a prominent warning).

On finding #4 (minor — controller storageClassName): that's a false positive. The controller's volumeClaimTemplates already surfaces it via {{- with ($c.storage | default dict).storageClassName }} (controller-statefulset.yaml ~line 135), exactly parallel to the agent PVC. A cluster sets it per controller via controllers[].storage.storageClassName (quant uses local-path). No gap.

superseded by new automated review

[ben-s-pull-request-reviewer]

{
  "verdict": "approve",
  "summary": "Helm chart refactor from single controller to controllers list is well-executed. All nil-safe patterns are correct, the breaking change migration path is clearly documented, PVC conditional logic is sound, secretName override is intentionally flexible (documented as referencing existing secrets when secretEnv is omitted), and the gitRestUrl derivation caveat is documented in both values.yaml and inline in the template. No template logic tests exist, but helm lint/template was validated manually. No Must/Should fixes identified.",
  "comments": [],
  "concerns_addressed": [
    "security: secretName override is intentionally designed for existing secret reference; secretEnv omitted + secretName set = references existing (no new Secret created); secretEnv set = always creates new Secret (secretName only affects Config.spec.secretName, not the created Secret name). No exfiltration path.",
    "correctness: BREAKING change from controller: to controllers: list is documented in CHANGELOG with explicit migration codeblock; controllers: [] default is intentional — existing installs will render zero controllers until migrated, which is the desired fail-safe behavior.",
    "correctness: PVC creation is gated on $agent.volumeMountPath presence (agents.yaml:19-22,51-70); the volumeClaim name uses volumeClaim field with default to name, storageClassName uses nil-safe with block that correctly omits when empty.",
    "correctness: nil-safe nested fields verified: (c.image | default dict).repository, ($c.sentry | default dict).dsn, ($c.storage | default dict).storageClassName, ($c.storage | default dict).size all handle nil correctly.",
    "tests: no helm template unit tests (ct/unittest) — PR description mentions helm lint + helm template manual validation only; flagged as nice-to-have for future."
  ]
}