Repository navigation
feat(openclaw): skill save/delete workflow templates - #17
Conversation
…l-manager protocol
Two new ClusterWorkflowTemplates for owner-initiated backup of OpenClaw
skills to the gitops repo:
- openclaw-skill-save: writes
platform-gitops/services/{team}/openclaw/skills/{name}.md from a
base64-encoded payload. Overwrites if present. Commits with the
triggering user recorded as actor.
- openclaw-skill-delete: removes the same file. No-op if absent.
Both validate team_name and skill_name (defense-in-depth; mctl-api will
re-validate before submission), contain paths within the tenant's
services dir, and push with the same rebase-retry pattern used by
tpl-git-commit. Identity: argo-workflows[bot], same as every other
gitops automation.
Adds mctl-skill-manager SKILL.md for admins that documents the
workspace/gitops split and the set of mctl_skill_* MCP tools (landing
in a separate mctl-api PR).
No runtime pod changes; templates are inert until the mctl-api tools
and corresponding Operation registry entries are deployed.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c95d312a6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| TEAM="{{inputs.parameters.team_name}}" | ||
| SKILL="{{inputs.parameters.skill_name}}" | ||
| ACTOR="{{inputs.parameters.actor}}" |
There was a problem hiding this comment.
TEAM, SKILL, and ACTOR are injected directly into the shell script source via "{{inputs.parameters.*}}"; in sh, payloads containing command substitution (for example $(...)) are executed while parsing these assignment lines, before the regex checks run. If this template is submitted directly (Argo UI/CLI with user-supplied params), that enables arbitrary command execution in a pod that mounts mctl-gitops-deploy-key, which can be abused to tamper with gitops state. Pass parameters as opaque env/args (or safely escape them) before any validation logic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds Argo ClusterWorkflowTemplates and tenant admin documentation to support owner-triggered backup/cleanup of OpenClaw SKILL.md content into the gitops repo (one commit per save/delete), to be invoked later by companion mctl-api tooling.
Changes:
- Introduces
openclaw-skill-saveworkflow to write a base64 payload toplatform-gitops/services/{team}/openclaw/skills/{name}.mdand push a commit. - Introduces
openclaw-skill-deleteworkflow to remove the same file path idempotently and push a commit. - Adds an admins-facing
mctl-skill-managerprotocol doc describing runtime vs gitops skill storage and operator actions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| platform-gitops/argo-workflows/cluster-templates/wft-openclaw-skill-save.yaml | New workflow template for saving a skill file into gitops with push-rebase retry. |
| platform-gitops/argo-workflows/cluster-templates/wft-openclaw-skill-delete.yaml | New workflow template for deleting a skill file from gitops idempotently. |
| platform-gitops/services/admins/openclaw/skills/mctl-skill-manager.md | Admin/agent doc describing the two-layer skill model and operational workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ACTOR="{{inputs.parameters.actor}}" | ||
|
|
||
| # ── Validate names (defense-in-depth; mctl-api validates first) ──── | ||
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,30}$'; then |
There was a problem hiding this comment.
team_name validation is capped at 31 chars ({0,30}), which will reject valid tenant names created elsewhere (e.g., create-tenant allows 2–63 chars; tpl-validate-tenant allows ≤63). Align this regex with the platform tenant naming rules so existing longer team names can save skills.
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,30}$'; then | |
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,62}$'; then |
| SKILL="{{inputs.parameters.skill_name}}" | ||
| ACTOR="{{inputs.parameters.actor}}" | ||
|
|
||
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,30}$'; then |
There was a problem hiding this comment.
team_name validation is capped at 31 chars ({0,30}), which will reject valid tenant names created elsewhere (e.g., create-tenant allows 2–63 chars; tpl-validate-tenant allows ≤63). Align this regex with the platform tenant naming rules so existing longer team names can delete skills.
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,30}$'; then | |
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$'; then |
| git commit \ | ||
| -m "feat(${TEAM}/openclaw): save skill ${SKILL}" \ | ||
| -m "Triggered by: ${ACTOR}" |
There was a problem hiding this comment.
actor is interpolated directly into the git commit message without any validation/sanitization. To keep the audit trail trustworthy (and avoid newline/control-character injection into commit metadata), validate actor against a strict pattern (e.g., numeric ID / UUID) or strip newlines before using it in git commit -m.
| git commit \ | |
| -m "feat(${TEAM}/openclaw): save skill ${SKILL}" \ | |
| -m "Triggered by: ${ACTOR}" | |
| SAFE_ACTOR=$(printf '%s' "$ACTOR" | tr -d '\r\n' | tr -d '[:cntrl:]') | |
| git commit \ | |
| -m "feat(${TEAM}/openclaw): save skill ${SKILL}" \ | |
| -m "Triggered by: ${SAFE_ACTOR}" |
| git commit \ | ||
| -m "chore(${TEAM}/openclaw): remove skill ${SKILL}" \ | ||
| -m "Triggered by: ${ACTOR}" |
There was a problem hiding this comment.
actor is interpolated directly into the git commit message without any validation/sanitization. To keep the audit trail trustworthy (and avoid newline/control-character injection into commit metadata), validate actor against a strict pattern (e.g., numeric ID / UUID) or strip newlines before using it in git commit -m.
|
|
||
| ## Rules | ||
|
|
||
| - `skill_name` must be kebab-case: lowercase letters, digits, hyphens; starts and ends with alphanumeric; 1–64 chars. Must match `^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$`. |
There was a problem hiding this comment.
The stated skill_name length ("1–64 chars") doesn’t match the regex shown (^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$), which requires at least 2 characters. Either update the text to 2–64 chars or adjust the regex/text so they’re consistent.
| - `skill_name` must be kebab-case: lowercase letters, digits, hyphens; starts and ends with alphanumeric; 1–64 chars. Must match `^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$`. | |
| - `skill_name` must be kebab-case: lowercase letters, digits, hyphens; starts and ends with alphanumeric; 2–64 chars. Must match `^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$`. |
Adds four owner-scoped MCP tools for managing OpenClaw SKILL.md files
backed up in the gitops repo:
- mctl_list_openclaw_skills / GET /api/v1/openclaw/{team}/skills
- mctl_read_openclaw_skill / GET /api/v1/openclaw/{team}/skills/{name}
- mctl_save_openclaw_skill / POST /api/v1/openclaw/{team}/skills
- mctl_delete_openclaw_skill / DELETE /api/v1/openclaw/{team}/skills/{name}
Read endpoints (list/read) hit gitops.Reader directly and are instant.
Write endpoints (save/delete) submit openclaw-skill-save /
openclaw-skill-delete ClusterWorkflowTemplates (landing in
mctlhq/mctl-gitops#17) with the caller's user ID recorded as actor in
the commit body. Content is base64-encoded before being handed to
Argo to keep newlines safe. Save is capped at 100 KB per file; names
must match ^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$.
All four endpoints gate on the existing requireOpenClawOwner —
non-owners get 403. Admin bypass is preserved.
Runtime workspace (S3) is not touched by these endpoints; restoring a
skill into the workspace is a two-step flow driven by the agent in
chat (read from gitops here → write to workspace via its own tools).
Tool count in server_test.go bumped from 39 to 43.
- [codex P1] Pass team_name/skill_name/content_b64/actor as Argo env vars
instead of interpolating them into the shell source. Container runtime
injects env values opaquely; this avoids shell parsing of \$(…) or \`\`…\`\`
inside user-controlled params before our regex validation runs. Prior
flow was safe when invoked via mctl-api (input validated there first),
but a direct Argo UI/CLI submission with crafted params would have
executed arbitrary shell as the argo-workflow-sa — real concern given
the pod mounts mctl-gitops-deploy-key.
- [copilot] Align team_name regex with platform rules: allow 2-63 chars
matching ^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$ (was capped at 31), so
longer tenant names like mctl-reviewer can save/delete skills.
- [copilot] Sanitize actor before using it in git commit trailer
(strip \\r, \\n, and other control chars) so a crafted actor can't
forge commit metadata.
- [copilot] Fix doc inconsistency in mctl-skill-manager.md: regex
requires 2-64 chars, not 1-64.
|
Addressed all review findings in db942a1:
@codex review |
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db942a14e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$'; then | ||
| echo "❌ Invalid team name: $TEAM" | ||
| exit 1 | ||
| fi | ||
| if ! printf '%s' "$SKILL" | grep -Eq '^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$'; then |
There was a problem hiding this comment.
The TEAM/SKILL validation can be bypassed with embedded newlines because grep -Eq '^...$' succeeds if any line matches; for example, a value like admins\nfoo/bar passes even though it contains /. That defeats the intended kebab-case gate and lets callers create/delete unexpected nested paths (including newline-containing filenames) under gitops. Please reject newline/control characters (or otherwise validate the entire string atomically) before these regex checks; the same pattern is duplicated in the delete template as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| source: | | ||
| set -e | ||
|
|
||
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$'; then |
There was a problem hiding this comment.
The team name validation regex here differs from the platform’s existing tenant/team naming validation (e.g., tpl-validate-tenant.yaml:68). If upstream validation accepts a different pattern, this workflow can fail post-submission for some team_names. Aligning on a single shared regex (or reusing a common validation template) would avoid inconsistent behavior.
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$'; then | |
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?$'; then |
| set -e | ||
|
|
||
| # ── Validate names (defense-in-depth; mctl-api validates first) ──── | ||
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$'; then |
There was a problem hiding this comment.
The team name regex here is stricter/different than the tenant naming validation used elsewhere (e.g., platform-gitops/argo-workflows/cluster-templates/tpl-validate-tenant.yaml:68 uses ^[a-z0-9][a-z0-9-]{0,62}$). If mctl-api/tooling accepts the broader pattern, this workflow will fail after submission for some team_names. Consider centralizing/reusing the same validation rule across workflows (or at least matching the existing platform regex) so callers don’t see inconsistent acceptance criteria.
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$'; then | |
| if ! printf '%s' "$TEAM" | grep -Eq '^[a-z0-9][a-z0-9-]{0,62}$'; then |
| apk add --no-cache git openssh-client ca-certificates > /dev/null | ||
|
|
||
| mkdir -p ~/.ssh | ||
| cp /secrets/deploy-key/ssh-privatekey ~/.ssh/id_ed25519 | ||
| chmod 600 ~/.ssh/id_ed25519 | ||
| printf 'Host github.com\n Hostname ssh.github.com\n Port 443\n IdentityFile ~/.ssh/id_ed25519\n StrictHostKeyChecking no\n' > ~/.ssh/config | ||
| chmod 600 ~/.ssh/config | ||
|
|
||
| git clone --depth=1 "git@github.com:${GITOPS_ORG}/${GITOPS_REPO}.git" /tmp/mctl-gitops | ||
| cd /tmp/mctl-gitops | ||
| git config user.name "argo-workflows[bot]" | ||
| git config user.email "argo-workflows[bot]@users.noreply.github.com" | ||
|
|
There was a problem hiding this comment.
This delete workflow duplicates the same SSH setup / git clone / push-with-rebase-retry logic as the save workflow. Duplicated git plumbing tends to drift over time (e.g., future changes to auth, host key handling, retry behavior). Consider extracting the shared pieces into a reusable template (similar in spirit to tpl-git-commit) and have both workflows call it with an operation-specific script/body.
| - `skill_name` must be kebab-case: lowercase letters, digits, hyphens; starts and ends with alphanumeric; 2–64 chars. Must match `^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$`. | ||
| - Skill content must be valid Markdown with a YAML frontmatter block whose `name:` matches the file name, and a `description:` field so other agents can find it. | ||
| - Only a tenant owner can save or delete in gitops. Non-owners get a 403. Do not retry on auth failures. | ||
| - Gitops writes return a `workflow_name`. Verify the workflow succeeds at `https://workflows.mctl.ai/workflows/{team}/{workflow_name}` before reporting success to the operator. |
There was a problem hiding this comment.
The workflow status URL placeholder uses {team} here, but the general platform skill doc uses {namespace} for the same URL shape (platform-gitops/services/admins/openclaw/skills/mctl-platform.md:12). To avoid confusion when the namespace and team concepts diverge, consider using {namespace} (or explicitly noting that the namespace equals the team name).
| - Gitops writes return a `workflow_name`. Verify the workflow succeeds at `https://workflows.mctl.ai/workflows/{team}/{workflow_name}` before reporting success to the operator. | |
| - Gitops writes return a `workflow_name`. Verify the workflow succeeds at `https://workflows.mctl.ai/workflows/{namespace}/{workflow_name}` before reporting success to the operator. |
Codex P2 round 2 on PR #17: `grep -Eq '^…$'` against a multi-line payload succeeds when ANY single line matches, so a team/skill name like `admins\nfoo/bar` bypasses the kebab-case gate and would let a crafted workflow submission create/delete unexpected nested paths. Fix: prepend an atomic character-set check with `tr -d 'a-z0-9-'` — if anything remains (newlines, control chars, `/`, `.`, etc.), reject before the regex runs. Applied to both save and delete templates. Copilot: team-name regex now matches upstream tpl-validate-tenant.yaml (`^[a-z0-9][a-z0-9-]{0,62}$`) so this workflow accepts exactly the same names as tenant creation; eliminates the surprising rejection of valid tenant names with length 33-63. Copilot doc fix: mctl-skill-manager.md uses `{namespace}` in the workflow URL (consistent with mctl-platform.md) with a note that namespace equals the team name in the common case. Deferred: extracting shared SSH/clone/push plumbing out of both templates. ~70 LOC of reuse, unlike tpl-git-commit which centralises 500+ LOC. Filed as follow-up rather than in-flight scope creep.
|
Round 2 findings addressed in f54d191:
Deferred: extracting shared SSH/clone/push plumbing into a reusable template. ~70 LOC of duplication, versus tpl-git-commit which earns its existence by centralising 500+ LOC. Filed as follow-up rather than blocking this PR on another round trip. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Atomic character-set check rejects embedded newlines / control | ||
| # chars (otherwise `grep -Eq '^…$'` would pass if ANY line matched). | ||
| if [ -n "$(printf '%s' "$TEAM" | tr -d 'a-z0-9-')" ]; then | ||
| echo "❌ Invalid team name: contains disallowed characters" | ||
| exit 1 | ||
| fi | ||
| if [ -n "$(printf '%s' "$SKILL" | tr -d 'a-z0-9-')" ]; then |
There was a problem hiding this comment.
The team_name validation regex here differs from the repo’s established tenant validation and may reject names accepted elsewhere. tpl-validate-tenant.yaml uses ^[a-z0-9][a-z0-9-]{0,62}$ (platform-gitops/argo-workflows/cluster-templates/tpl-validate-tenant.yaml:68), but this workflow requires an alphanumeric final character and at least 2 chars. Aligning these avoids inconsistent behavior between workflows.
| if ! printf '%s' "$SKILL" | grep -Eq '^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$'; then | ||
| echo "❌ Invalid skill name: $SKILL" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
SSH is configured with StrictHostKeyChecking no, which weakens protection against MITM during git operations. Since other workflows in this repo already use ssh-keyscan to populate known_hosts (e.g., wft-preview-deploy.yaml:83), consider switching to that approach and enabling strict host key checking here too.
| - Only a tenant owner can save or delete in gitops. Non-owners get a 403. Do not retry on auth failures. | ||
| - Gitops writes return a `workflow_name`. Verify the workflow succeeds at `https://workflows.mctl.ai/workflows/{namespace}/{workflow_name}` (namespace usually equals the team name) before reporting success to the operator. | ||
| - Never `rm -rf skills/*` in bulk without confirming with the operator first. |
There was a problem hiding this comment.
This doc uses https://workflows.mctl.ai/workflows/{team}/{workflow_name} for the workflow URL, but the existing platform doc in the same directory specifies {namespace} (mctl-platform.md:12). Consider using the same placeholder terminology here (or explicitly stating that the namespace equals the team name) to avoid confusing operators.
| # Reject embedded newlines / control chars / any byte outside the | ||
| # allowed kebab-case alphabet atomically — otherwise `grep -Eq '^…$'` | ||
| # below would succeed when ANY single line of a multi-line input | ||
| # matches, letting callers sneak `/` or nested paths past validation. | ||
| if [ -n "$(printf '%s' "$TEAM" | tr -d 'a-z0-9-')" ]; then | ||
| echo "❌ Invalid team name: contains disallowed characters" |
There was a problem hiding this comment.
The team_name validation regex here is stricter than the repo’s established tenant validation and may reject team names that other workflows accept. For example, tpl-validate-tenant.yaml validates with ^[a-z0-9][a-z0-9-]{0,62}$ (platform-gitops/argo-workflows/cluster-templates/tpl-validate-tenant.yaml:68), but this workflow requires an alphanumeric final character and at least 2 chars. Consider reusing the same regex (or centralizing it) so naming rules are consistent across workflows.
| # ── Set up SSH ──────────────────────────────────────────────────── | ||
| mkdir -p ~/.ssh | ||
| cp /secrets/deploy-key/ssh-privatekey ~/.ssh/id_ed25519 | ||
| chmod 600 ~/.ssh/id_ed25519 | ||
| printf 'Host github.com\n Hostname ssh.github.com\n Port 443\n IdentityFile ~/.ssh/id_ed25519\n StrictHostKeyChecking no\n' > ~/.ssh/config |
There was a problem hiding this comment.
This workflow will create platform-gitops/services/${TEAM}/openclaw/skills even if the team (or openclaw service directory) doesn’t exist, which can lead to committing stray tenant/service directories on typos or misrouted requests. Consider failing fast unless platform-gitops/services/${TEAM}/openclaw (or at least platform-gitops/services/${TEAM}) already exists in the repo before writing/committing.
| if ! printf '%s' "$SKILL" | grep -Eq '^[a-z0-9][a-z0-9-]{0,62}[a-z0-9]$'; then | ||
| echo "❌ Invalid skill name: $SKILL" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
SSH is configured with StrictHostKeyChecking no, which makes git operations vulnerable to MITM if DNS or routing is compromised. Other workflows in this repo use ssh-keyscan -p 443 ssh.github.com >> ~/.ssh/known_hosts (e.g., wft-preview-deploy.yaml:83) instead. Consider pinning/initializing known_hosts and enabling strict host key checking here as well.
Summary
openclaw-skill-saveandopenclaw-skill-deleteClusterWorkflowTemplates for owner-initiated backup of OpenClawSKILL.mdfiles to the gitops repo.mctl-skill-managerSKILL.md for admins describing the two-layer (S3 workspace + gitops) skill model.mctl_skill_*MCP tools + Operation registry entries.Design
Earlier today the openclaw service template stopped force-seeding skills, so skills are tenant-managed in the pod's S3-backed workspace. This PR adds the first half of the gitops backup path — explicit, owner-triggered, one commit per save.
mctl-gitops-deploy-keyevery other gitops automation uses. No new identity, no silent cron.mctl_skill_save/mctl_skill_delete) that mctl-api will gate withrequireOpenClawOwner.Files
Follow-up
mctl-api PR adds:
Test plan