fix(skills): accept owner-prefixed clawhub slugs (#71767)#71857
fix(skills): accept owner-prefixed clawhub slugs (#71767)#71857MkDev11 wants to merge 15 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes Confidence Score: 4/5Safe to merge — the change is narrowly scoped to slug parsing, security invariants (path-traversal guard, homograph rejection) are preserved per-segment, and the install/lockfile layers are untouched. No P0 or P1 issues found. The parser correctly strips the owner prefix before any registry or filesystem call, both validation modes are tested end-to-end, and the fallback in resolveRequestedUpdateSlug handles all update-path combinations correctly. No files require special attention. Reviews (1): Last reviewed commit: "fix(skills): accept owner-prefixed clawh..." | Re-trigger Greptile |
19e299b to
1251fd1
Compare
3e2c20e to
5c5fa26
Compare
|
Codex review: needs maintainer review before merge. Summary Reproducibility: yes. Current main reproduces from source: Next step before merge Security Review detailsBest possible solution: Keep the parser and regression tests, keep the changelog entry under current Unreleased Fixes after rebase, and land once the draft state and exact-head checks are resolved. Do we have a high-confidence way to reproduce the issue? Yes. Current main reproduces from source: Is this the best way to solve the issue? Yes. The PR fixes the shared install/update boundary instead of duplicating CLI or RPC parsing, validates both owner and slug segments, and canonicalizes before registry/filesystem use. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 5e1529c48bf1. |
9cfd629 to
655bb27
Compare
47b3cd6 to
e52e602
Compare
e52e602 to
4e26893
Compare
The install snippet rendered on clawhub.ai is openclaw skills install <owner>/<slug>, but the CLI rejected anything with a slash. Parse the owner prefix off and use the bare slug for the registry call, install dir, and lockfile; preserve the existing ASCII guard on each segment so PR openclaw#53206's homograph protection still applies.
33f034d to
b786f18
Compare
|
the ci test failures are not related to my pr |
…-prefixed-skill-slug # Conflicts: # CHANGELOG.md
…er-prefixed-skill-slug # Conflicts: # extensions/deepinfra/index.test.ts
…er-prefixed-skill-slug
|
@greptileai pls review again |
…er-prefixed-skill-slug # Conflicts: # CHANGELOG.md
Summary
openclaw skills install spclaudehome/skill-vetterfailed withInvalid skill slug: spclaudehome/skill-vettereven though owner-prefixed ClawHub references use that form.parseClawHubSkillSlug(raw, mode)accepts an optional<owner>/<name>prefix, validates both segments under the existing ASCII guard, and returns the canonical bare slug for the registry call, install dir, and lockfile key.normalizeTrackedSlug/validateRequestedSlugare thin wrappers; call sites unchanged.src/infra/clawhub.tsregistry I/O,src/cli/skills-cli.ts, the gatewayskills.install/skills.updateRPCs, the plugin install path, orsrc/infra/install-safe-path.ts. Path-traversal protection is unchanged (safeDirNamestill neutralizes/and\at the FS layer). Homograph rejection from fix: tighten skill slug validation to ASCII-only #53206 is preserved per-segment.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
safeDirNamefor slash-containing slugs at the archive layer; this PR completes that direction at the validator)Root Cause (if applicable)
normalizeTrackedSluginsrc/agents/skills-clawhub.ts:70rejected any input containing/as a path-traversal guard. Earlier issue analysis found owner-prefixed ClawHub install references such asopenclaw skills install spclaudehome/skill-vetter. The CLI did not understand that form, so copy-paste installs failed before any registry request.src/agents/skills-clawhub.test.tsexercised slugs with a/in them. PR fix: tighten skill slug validation to ASCII-only #53206's "backward compatible? Yes — slugs that were valid before remain valid" claim was technically true for single-segment slugs, but missed that the website was already shipping owner-prefixed install commands.slug.includes("/")rejection landed in the very first commit of this file (91b2800241) as a path-traversal guard. PR fix(clawhub): sanitize archive temp filenames #56779 later addedsafeDirNameat the archive layer (src/infra/install-safe-path.ts:28), making the validator's/-rejection redundant for safety. The validator never got the memo.Regression Test Plan (if applicable)
src/agents/skills-clawhub.test.tsinstallSkillFromClawHub({ slug: "spclaudehome/skill-vetter" })returns{ ok: true }and the downstreamfetchClawHubSkillDetail/downloadClawHubSkillArchivecalls receiveslug: "skill-vetter".updateSkillsFromClawHubagainst a workspace that tracks the bareskill-vetterresolves the owner-prefixed input to the same lockfile entry. All malformed forms (a/b/c,/x,x/,a\\b,a/../b, non-ASCII per segment, hyphen edges) still return{ ok: false, error: containing "Invalid skill slug" }without touching the network.skills.install/skills.update) converge throughinstallSkillFromClawHub/updateSkillsFromClawHuband the two normalizers. Asserting at that boundary covers every caller without integration tests.skills-clawhub.test.ts:224-329only exercised single-segment slugs.User-visible / Behavior Changes
openclaw skills install <owner>/<slug>andopenclaw skills update <owner>/<slug>are now accepted and behave identically to the bare-slug form — the owner prefix is informational and stripped before any further processing./are unchanged.Diagram (if applicable)
Security Impact (required)
NON_ASCII_PATTERNandVALID_SLUG_PATTERNto both segments inrequestedmode; path-traversal protection is preserved by rejecting\\,.., empty segments, and >1 separator before the registry/filesystem layers.Repro + Verification
Environment
Steps
pnpm installmain(pre-fix):pnpm test src/agents/skills-clawhub.test.ts -- --testNamePattern="owner-prefixed"→ fails (3 critical assertions).pnpm test src/agents/skills-clawhub.test.ts→ 27/27 pass (15 baseline + 12 new).pnpm check:changed→ conflict-markers ✓, typecheck core ✓, typecheck core tests ✓, lint core ✓, all guards ✓, 4542 tests ✓.Expected
Actual
Evidence
Regression proof — running the new tests against unfixed source produced exactly the failures predicted by the analysis:
After the fix on the same test file:
Tests 27 passed (27).Historical context from earlier issue analysis identified the owner-prefixed web snippet shape:
Registry confirmation:
GET /api/v1/skills/skill-vetter→ 200 withslug: "skill-vetter",owner.handle: "spclaudehome".GET /api/v1/skills/spclaudehome%2Fskill-vetter→ 404. Bare slug is canonical at the API; owner prefix is a UI affordance.Human Verification (required)
installSkillFromClawHub({ slug: "spclaudehome/skill-vetter" })callsfetchClawHubSkillDetail({ slug: "skill-vetter" })and returnsok: truewithslug: "skill-vetter".skill-vetter;updateSkillsFromClawHub({ slug: "spclaudehome/skill-vetter" })resolves to that lockfile entry and uses the saved registry URL.Invalid skill slug: <raw>error, no network call.skills-clawhub.test.ts:160) still passes —trackedmode keeps lenient ASCII handling...traversal, backslash,/skill,skill/,//,a/b/c, leading hyphen on either segment, trailing hyphen on either segment, Cyrillic homograph in either segment, mixed case, single-segment regression.slugunchanged toinstallSkillFromClawHub, so the boundary fix covers it). I did not exercise a real network round-trip againstclawhub.ai; the API contract was confirmed via directcurlto/api/v1/skills/skill-vetteronly.Review Conversations
Compatibility / Migration
Risks and Mitigations
react(bare) and runsupdate someowner/reactmight be confused if they thinksomeowner/reactis a different skill./api/v1/skills/<slug>). The owner prefix is purely informational, matching the website's own treatment.owner/nameandnamewould refer to different skills).