Skip to content

feat(controller): add persistent sandbox workspaces - #494

Open
掌柜的 (nikawang) wants to merge 3 commits into
Azure:mainfrom
nikawang:feature/persistent-workspace
Open

feat(controller): add persistent sandbox workspaces#494
掌柜的 (nikawang) wants to merge 3 commits into
Azure:mainfrom
nikawang:feature/persistent-workspace

Conversation

@nikawang

Copy link
Copy Markdown

Summary

  • add opt-in per-sandbox PVC storage for /sandbox, including dynamic claims, explicit existing-claim recovery, Retain/Delete lifecycle safety, and StorageReady status
  • add declarative OpenClaw workspace bootstrap for AGENTS.md, SOUL.md, HEARTBEAT.md, TOOLS.md, and USER.md, with atomic writes, digest provenance, symlink defenses, and init-container credential isolation
  • expose storage/bootstrap through kars add, Helm CRD/RBAC, generated CRD validation, lifecycle docs, and security guidance while preserving emptyDir as the backward-compatible default

Related Issues

None.

Type of Change

  • Bug fix
  • New feature
  • Security hardening
  • Documentation
  • CI/CD
  • Refactor

Checklist

  • Code compiles/builds without errors
  • Tests pass (make test)
  • Linting passes for changed Rust/TypeScript surfaces
  • Documentation updated (if applicable)
  • No secrets committed
  • Helm chart updated (if K8s changes)

Testing

  • cargo test --locked -p kars-controller — 886 controller tests passed; phase taxonomy guard passed
  • cargo clippy --locked -p kars-controller --bin kars-controller --no-deps -- -D warnings -A clippy::uninlined_format_args
  • cargo fmt --all -- --check
  • npm test -- --exclude src/commands/dev.test.ts — 928 stable CLI tests passed
  • npm test -- --run src/commands/add.test.ts — 30/30 add-command tests passed
  • npm run typecheck && npm run lint && npm run build — typecheck/build passed; lint reported 0 errors (29 pre-existing warnings outside this change)
  • kubectl apply --dry-run=server -f deploy/helm/kars/templates/crd.yaml — CRD OpenAPI/CEL accepted by the API server
  • bash -n for both OpenClaw scripts, plus executable bootstrap behavior tests covering IfMissing, Always, SHA-256 state manifest, file symlink rejection, and destination/state directory symlink rejection
  • independent code review completed with no remaining Critical/Important findings

make test is intentionally left unchecked: the local repository-wide wrapper invokes unrelated Python/secret-scanner gates that currently fail on pre-existing fixtures and paths outside this diff. The component-native Rust and TypeScript suites above are the executable validation for this change.

@pallakatos

Copy link
Copy Markdown
Collaborator

Thanks for your patience, and apologies for the delay — I was out of office. I’m back now and starting a repository-wide maintenance pass, including refreshing the CI/security baseline and reviewing the open contributor PRs. I’ll review this properly as soon as possible and follow up here with substantive feedback. No action is needed from you in the meantime unless you already planned another update.

@pallakatos

Pal Lakatos-Toth (pallakatos) commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Thank you 掌柜的 (@nikawang) for the detailed workspace implementation. I completed an initial hostile-input supply-chain/security review. The supply-chain surface is clean: no dependency/lock, workflow, release, vendored/binary, obfuscated, unsafe, or new network-fetch changes were found. The bootstrap init-container hardening and symlink defenses are also thoughtfully implemented.

There are two blocking isolation issues to address before this can proceed:

1. HIGH — cross-tenant retained-PVC adoption

existingClaim accepts any same-runtime-namespace PVC by name, and validate_namespace_claim_reuse explicitly permits a claim when requested_existing_claim == claim.metadata.name, even when the recorded sandbox UID differs. Runtime namespaces are derived from the sandbox name only (kars-<name>), while the controller watches KarsSandbox cluster-wide. Two CRs named foo in different source namespaces therefore converge on the same runtime namespace.

With retention now default, a principal able to create a KarsSandbox elsewhere can request a retained victim claim and mount its /sandbox contents, including persisted OpenClaw configuration, channel/plugin credentials, memory, and mesh identity material. The current test retained_claim_requires_explicit_existing_claim_recovery codifies this cross-UID adoption.

Please bind generated runtime namespaces and retained PVCs to the owning CR namespace/identity, refuse reconciliation on owner mismatch, and require an explicit operator-controlled adoption policy rather than name-only existingClaim reuse.

2. MEDIUM — retained privileged namespace loses network isolation

The retention cleanup path deletes all NetworkPolicy objects but deliberately preserves a namespace labelled pod-security.kubernetes.io/enforce: privileged. It also removes the finalizer immediately after background deletion requests. This leaves a durable privileged namespace with no default-deny policy and can briefly remove isolation while the old pod still terminates.

Please retain/reapply deny-all networking for dormant namespaces, tighten the dormant namespace PSA posture, and drain workloads with foreground deletion or an explicit wait before removing isolation/finalizers.

Additional scope items should be separated or justified: the unrelated Hermes channel-flag validation relaxation, the global status-phase semantic change, and the RuntimePlanError::ShapeInvalid reason change. The Helm drift test should compare the full generated storage schema rather than key presence only.

This is not an approval. After the tenancy and retained-namespace controls are redesigned, I will continue the compatibility/behavior review.

@pallakatos

Copy link
Copy Markdown
Collaborator

Hi 掌柜的 (@nikawang) — a gentle follow-up on this one, and again apologies for the initially delayed response while I was out of office. We have now completed the repository maintenance/release pass and worked through the other outstanding contributor review, so this PR is back at the front of the queue.

When you have bandwidth, could you let us know whether you are planning to revise the retained-workspace design around the two isolation findings above: cross-tenant PVC adoption and preserving deny-all isolation for dormant retained namespaces? There is no need to rush, but an update on your intended direction would help us plan the next review. Once a revision is ready, we will prioritize the re-review.

Thank you again for the substantial work here and for contributing to Kars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants