fix(skills): allow symlinked local skills in cloud tasks - #3810
Conversation
Resolve trusted local skill roots before bundling while retaining repository containment checks. Generated-By: PostHog Code Task-Id: 5cf8bddc-dae4-4dc9-84fb-195b06f48068
|
😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details. |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Keep repository and marketplace skill roots protected while allowing user-managed dotfile symlinks. Generated-By: PostHog Code Task-Id: 5cf8bddc-dae4-4dc9-84fb-195b06f48068
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
|
Reviews (1): Last reviewed commit: "Restrict symlinked skills to user roots" | Re-trigger Greptile |
Require explicit selection before dependency expansion follows a symlinked local skill. Generated-By: PostHog Code Task-Id: 5cf8bddc-dae4-4dc9-84fb-195b06f48068
There was a problem hiding this comment.
can we resolve realpath(path.dirname(skillDir)) before comparing against the user claude/codex roots, and select the owning folder in assertRepoSkillStaysInRepo the same way, so a skill that realpaths into an open folder always gets the repo check regardless of which alias addressed it? a ~/.claude/skills/hehe -> ~/work/some-repo silently uploads a whole working tree which I believe we might want to avoid
additionally, with ~/.claude/skills/{a,b,c} all symlinked into a dotfiles repo and a → b → c we have that each level costs another round trip, and the throw propagates out of loadCloudSkillBundles → uploadRunAttachments / uploadTaskStagedAttachments with no recovery, so the entire message send fails w/no attachments, no skills.
We have bundleLocalSkill that already permits these roots, the guard could use the same predicate so wecan refuse to follow an implicit symlinked dep when its parent is not a user claude/codex root, i.e. when bundling would fail anyway
Generated-By: PostHog Code Task-Id: 5cf8bddc-dae4-4dc9-84fb-195b06f48068
|
updated this to compare the canonical user roots and apply the open-workspace containment check from the real skill path, so a i kept explicit selection for implicit symlink dependencies. the resolver runs the dependency walk in one server call and bundles expanded refs concurrently, and attachment preparation stays fail-fast so we dont silently send a message without the selected context. |
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
Generated-By: PostHog Code Task-Id: 5cf8bddc-dae4-4dc9-84fb-195b06f48068
Problem
My local skills are managed through dotfiles and symlinked into the user skills directory. Cloud task setup rejected them before uploading the bundle. Path aliases also needed to retain their original repository ownership so they could not bundle a workspace root or files from another open repository.
Refs #1956
Changes
I allow explicitly selected symlinked skills only under the canonical user-owned Claude and Codex roots. Repository skills are validated against their lexical owner before user-root aliases into open workspaces are handled separately. Marketplace roots keep the symlink guard, and dependency expansion cannot follow a symlink without explicit selection.
How did you test this?
pnpm --filter @posthog/workspace-server exec vitest run src/services/skills/skills.test.ts(53 tests)pnpm exec biome check packages/workspace-server/src/services/skills/skills.ts packages/workspace-server/src/services/skills/skills.test.tspnpm --filter @posthog/workspace-server typecheckAutomatic notifications
Created with PostHog Code