refactor(k8s): replace per-user dynamic PVCs with shared PVC + subPath by jacoblee-io · Pull Request #105 · scitix/siclaw

jacoblee-io · 2026-03-13T13:46:55Z

Summary

Replace per-user dynamically provisioned PVCs with a single shared PVC (siclaw-data)
Gateway creates per-user subdirectories (users/{userId}/{workspaceId}/) on the shared volume
AgentBox pods mount via subPath for isolation — no dynamic provisioning or StorageClass required
Sanitize userId/workspaceId in path segments to prevent traversal and invalid directory names

Details

Before: Each user got a dedicated PVC (siclaw-user-{userId}) created on demand via K8s API, requiring a StorageClass that supports dynamic provisioning.

After: Operator pre-creates one shared ReadWriteMany PVC. Gateway mounts it and creates subdirectories at spawn time; AgentBox pods mount the user-specific subdirectory via subPath.

Changes

k8s-spawner.ts: Remove ensureUserPvc()/userPvcName(), add ensureUserDir()/sanitizePathSegment(), use shared PVC claimName + subPath
gateway-main.ts: Replace storageClass/accessMode/size env vars with claimName/mountPath
Helm values: Simplify persistence config to enabled + claimName
Helm gateway-deployment: Mount shared PVC on gateway, pass SICLAW_PERSISTENCE_MOUNT_PATH
Helm RBAC: Remove PVC create verb (only get/list needed)
siclaw-data-pvc.yaml: Update to ReadWriteMany, 10Gi, updated comments

Test plan

Deploy with persistence.enabled=true, verify gateway creates users/{userId}/{workspaceId}/ on the shared PVC
Spawn AgentBox pod, verify it mounts the correct subPath and user data persists across pod restarts
Verify special characters in userId/workspaceId are sanitized (no path traversal)
Deploy with persistence.enabled=false (default), verify emptyDir fallback still works
Verify RBAC: gateway no longer needs PVC create permission

jacoblee-io

Good refactor — dynamic PVC provisioning per user was unnecessarily complex. Shared PVC + subPath is the right call. A few issues below, one of which could cause silent bugs.

jacoblee-io · 2026-03-13T13:51:42Z

src/gateway/agentbox/k8s-spawner.ts

-        throw err;
-      }
+  private ensureUserDir(userId: string, workspaceId: string): void {
+    const mountPath = this.config.persistence?.mountPath || ".siclaw/shared-data";


mountPath default is a relative path — fragile.

The Helm template always passes an absolute path (/app/.siclaw/shared-data), but the code defaults here and in gateway-main.ts are relative (.siclaw/shared-data). If someone runs without Helm or the env var isn't set, path.resolve() resolves against CWD, which depends on container WORKDIR and can silently break.

Suggest making the default absolute:

const mountPath = this.config.persistence?.mountPath || "/app/.siclaw/shared-data";

And same in gateway-main.ts:31.

jacoblee-io · 2026-03-13T13:51:42Z

src/gateway/agentbox/k8s-spawner.ts

-      await this.ensureUserPvc(userId);
+      const subDir = `users/${safeUserId}/${safeWorkspaceId}`;
+      console.log(`[k8s-spawner] Persistence enabled: shared PVC "${this.config.persistence.claimName}", subPath "${subDir}"`);
+      this.ensureUserDir(userId, workspaceId);


Nit: double sanitization.

safeUserId/safeWorkspaceId are computed at lines 183-184, but ensureUserDir() takes the raw userId/workspaceId and sanitizes internally again. Both produce the same result, so it's correct, but the pattern is confusing — a reader might wonder if they diverge.

Consider either:

Passing safeUserId/safeWorkspaceId and removing sanitizePathSegment from ensureUserDir, or

Having ensureUserDir return the safe values so the caller doesn't need to compute them separately.

jacoblee-io · 2026-03-13T13:51:42Z

helm/siclaw/templates/gateway-deployment.yaml

+            - name: SICLAW_PERSISTENCE_CLAIM_NAME
+              value: {{ .Values.agentbox.persistence.claimName | quote }}
+            - name: SICLAW_PERSISTENCE_MOUNT_PATH
+              value: "/app/.siclaw/shared-data"


Hardcoded mount path not configurable via values.yaml.

The SICLAW_PERSISTENCE_MOUNT_PATH env var is hardcoded to /app/.siclaw/shared-data, and the volumeMount.mountPath on line 83 is also hardcoded to the same value. If someone changes one but not the other, they'll silently diverge.

Consider either:

Adding mountPath to values.yaml and templating both, or

Adding a comment noting these two must stay in sync.

jacoblee-io

Previous comments all addressed — nice. Two small nits remaining, neither is a blocker.

jacoblee-io · 2026-03-13T13:57:18Z

src/gateway/agentbox/k8s-spawner.ts

-    size: string;        // e.g. "1Gi"
+    /** Name of the pre-existing shared PVC (e.g. "siclaw-data") */
+    claimName: string;
+    /** Local mount path of the shared PVC on the gateway (default: ".siclaw/shared-data") */


Nit: stale JSDoc default.

The comment says (default: ".siclaw/shared-data") but the actual default in ensureUserDir (line 352) is now "/app/.siclaw/user-data". Should be:

/** Local mount path of the shared PVC on the gateway (default: "/app/.siclaw/user-data") */

jacoblee-io · 2026-03-13T13:57:18Z

k8s/agentbox-template.yaml

    - name: skills-pv
      mountPath: /app/.siclaw/user-data
-      subPath: user/${USER_ID}/agent-data
+      subPath: users/${USER_ID}/${WORKSPACE_ID}


Nit: users/ vs user/ prefix inconsistency on the same PVC.

User data subPath is now users/{USER_ID}/{WORKSPACE_ID} (plural), but three lines below (line 114), credentials still use user/{USER_ID}/... (singular). Skills also use user/ (lines 107-108). Having both users/ and user/ as top-level directories on the same PVC is a readability trap — someone will inevitably confuse them.

Consider aligning all to one convention (either users/ everywhere or user/ everywhere) if there's no hard reason to keep them separate.

jacoblee-io

Re: stale JSDoc — Fixed, now reads "/app/.siclaw/user-data".

Re: users/ vs user/ prefix — Intentional. user/ is the skills PVC directory layout (skills, credentials, kube configs), while users/ is the shared data PVC layout (memory, sessions). These are different volumes in the actual K8s spawner; the template co-locates them on one PVC for reference only. Renaming user/ would break existing skills sync paths in local-spawner.ts and resource-handlers.ts.

Instead of dynamically creating a PVC per user (requiring StorageClass with dynamic provisioning), use a single pre-existing shared PVC. Gateway creates per-user subdirectories (users/{userId}/{workspaceId}/); AgentBox pods mount via subPath for isolation. - Replace persistence config (storageClass/accessMode/size) with claimName - Remove ensureUserPvc() / userPvcName() — no more K8s PVC creation - Add ensureUserDir() with sanitizePathSegment() for safe directory names - Mount shared PVC on gateway for directory management - Template mountPath via values.yaml (env + volumeMount stay in sync) - Remove PVC "create" verb from RBAC (only get/list needed) - Update all K8s manifests and Helm templates for consistency

jacoblee-io

All previous comments addressed — JSDoc default fixed, double sanitization cleaned up, mountPath templated via values.yaml. No remaining issues. LGTM, ready to merge.

0.1.2

3df1f29

jacoblee-io commented Mar 13, 2026

View reviewed changes

jacoblee-io force-pushed the refactor/shared-pvc branch from 5000d0c to e406a5c Compare March 13, 2026 13:56

jacoblee-io commented Mar 13, 2026

View reviewed changes

jacoblee-io force-pushed the refactor/shared-pvc branch from e406a5c to 7d9a32a Compare March 13, 2026 14:00

jacoblee-io commented Mar 13, 2026

View reviewed changes

fred-scitix self-requested a review March 13, 2026 14:03

fred-scitix approved these changes Mar 13, 2026

View reviewed changes

fred-scitix merged commit 7819024 into main Mar 13, 2026
2 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(k8s): replace per-user dynamic PVCs with shared PVC + subPath#105

refactor(k8s): replace per-user dynamic PVCs with shared PVC + subPath#105
fred-scitix merged 2 commits intomainfrom
refactor/shared-pvc

jacoblee-io commented Mar 13, 2026

Uh oh!

jacoblee-io left a comment

Uh oh!

jacoblee-io Mar 13, 2026

Uh oh!

jacoblee-io Mar 13, 2026

Uh oh!

jacoblee-io Mar 13, 2026

Uh oh!

jacoblee-io left a comment

Uh oh!

jacoblee-io Mar 13, 2026

Uh oh!

jacoblee-io Mar 13, 2026

Uh oh!

jacoblee-io left a comment

Uh oh!

jacoblee-io left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jacoblee-io commented Mar 13, 2026

Summary

Details

Changes

Test plan

Uh oh!

jacoblee-io left a comment

Choose a reason for hiding this comment

Uh oh!

jacoblee-io Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

jacoblee-io Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

jacoblee-io Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

jacoblee-io left a comment

Choose a reason for hiding this comment

Uh oh!

jacoblee-io Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

jacoblee-io Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

jacoblee-io left a comment

Choose a reason for hiding this comment

Uh oh!

jacoblee-io left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants