From 88af60f151e01ee0c78b8996e3499cbad9e59c45 Mon Sep 17 00:00:00 2001 From: Haider Date: Fri, 18 Sep 2026 11:44:55 +0530 Subject: [PATCH 1/6] feat(workspace): `skill publish ` and a "Publish to workspace" action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retargeted onto main after #1280 merged; rebuilt as one commit carrying only this PR's change (the stacked history interleaved #1280's commits). The publish path from #1280 had no surface: nothing invoked it, so a locally authored skill still had no route to the workspace, and the CLI still did not say whether one existed. - `altimate-code skill publish ` resolves the skill the way `skill test` does, refuses a built-in (`skillSource`, which also knows the `~/.altimate/builtin` install), and prints one line on success. Every deliberate refusal — not linked, not the workspace's owner, workspace- owned, binary or linked file, empty, too large, name taken elsewhere, edited elsewhere mid-upload, uploaded but not attached — is printed as-is, since each already says what to do. - The Skills dialog gains "Publish to workspace" in the per-skill action picker, next to Show / Edit / Test / Remove — where a user who wonders whether publishing is possible will see it. Disabled for built-ins and for skills the workspace sent us; judged against `api.state.path .directory`, where the binding and the snapshot live. - `describePublish` and `explainPublishError` give both surfaces the same words; a `skill_published` telemetry event records the outcome with its source. Verified: 625 pass across the workspace, plugin and fork-guard suites on main; typecheck clean. `skill publish` exercised end to end against prod on a throwaway workspace (see #1280) — this command is what ran it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../opencode/src/altimate/telemetry/index.ts | 9 +++ .../src/altimate/workspace/skill-publish.ts | 28 +++++++++ packages/opencode/src/cli/cmd/skill.ts | 57 +++++++++++++++++++ .../src/plugin/tui/altimate/skill-ops.tsx | 47 +++++++++++++++ .../altimate/workspace/skill-publish.test.ts | 36 ++++++++++++ 5 files changed, 177 insertions(+) diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index eccd2a3c49..8754661a9c 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -716,6 +716,15 @@ export namespace Telemetry { skill_name: string source: "cli" | "tui" } + | { + type: "skill_published" + timestamp: number + session_id: string + skill_name: string + action: "created" | "updated" + file_count: number + source: "cli" | "tui" + } // altimate_change end // altimate_change start — plan refinement telemetry event | { diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index b31b420b6a..5fbee97644 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -721,6 +721,34 @@ async function publishSkillUnlocked(input: { return { action: "created", publicId, name: input.name, files: files.length, bytes, datamateId: binding.datamateId } } +/** One line for a surface to show after a publish. Both the CLI and the TUI + * say the same thing, so a user moving between them recognises the outcome. */ +export function describePublish(report: PublishReport): string { + const verb = report.action === "created" ? "Published" : "Updated" + const size = report.bytes >= 1024 ? `${Math.round(report.bytes / 1024)}KB` : `${report.bytes}B` + return `${verb} "${report.name}" in the workspace (${report.files} file${report.files === 1 ? "" : "s"}, ${size}).` +} + +/** The message for an error this module raised on purpose, or null for one it + * did not — a surface shows the former as-is (each already says what to do) + * and wraps the latter as a failure. */ +export function explainPublishError(err: unknown): string | null { + if ( + err instanceof NotLinkedError || + err instanceof ManagedSkillError || + err instanceof BinaryFileError || + err instanceof SymlinkError || + err instanceof EmptyBundleError || + err instanceof BundleTooLargeError || + err instanceof SkillNameConflictError || + err instanceof SkillChangedElsewhereError || + err instanceof NotWorkspaceOwnerError || + err instanceof AttachFailedError + ) + return err.message + return null +} + /** Which of the update path's conflicts this is. The server distinguishes them * in its detail; this module's typed errors then carry advice that fits. * Anything unrecognised keeps the server's own words rather than being diff --git a/packages/opencode/src/cli/cmd/skill.ts b/packages/opencode/src/cli/cmd/skill.ts index a658e6955c..d75e7ca600 100644 --- a/packages/opencode/src/cli/cmd/skill.ts +++ b/packages/opencode/src/cli/cmd/skill.ts @@ -11,6 +11,7 @@ import { Global } from "@/global" import { detectToolReferences, skillSource, isToolOnPath } from "./skill-helpers" // altimate_change start — telemetry for skill operations import { Telemetry } from "@/altimate/telemetry" +import { describePublish, explainPublishError, publishSkill } from "@/altimate/workspace/skill-publish" // altimate_change end // --------------------------------------------------------------------------- @@ -457,6 +458,61 @@ const SkillTestCommand = cmd({ }, }) +const SkillPublishCommand = cmd({ + command: "publish ", + describe: "publish a skill to the workspace this project is linked to", + builder: (yargs) => + yargs.positional("name", { + type: "string", + describe: "name of the skill to publish", + demandOption: true, + }), + async handler(args) { + const name = args.name as string + const cwd = process.cwd() + await bootstrap(cwd, async () => { + const skill = await Skill.get(name) + if (!skill) { + process.stderr.write(`Skill "${name}" not found. Check .opencode/skills/${name}/SKILL.md exists.` + EOL) + process.exitCode = 1 + return + } + // Built-in skills ship with altimate-code — embedded, or installed under + // `~/.altimate/builtin` — and are not the user's to publish; a skill the + // workspace sent us is refused by `publishSkill` itself. + if (skillSource(skill.location) === "builtin" || !path.isAbsolute(skill.location)) { + process.stderr.write(`"${name}" is a built-in skill and cannot be published.` + EOL) + process.exitCode = 1 + return + } + try { + const report = await publishSkill({ + projectDirectory: Instance.directory, + skillDirectory: path.dirname(skill.location), + name: skill.name, + description: skill.description ?? "", + }) + process.stdout.write(describePublish(report) + EOL) + try { + Telemetry.track({ + type: "skill_published", + timestamp: Date.now(), + session_id: Telemetry.getContext().sessionId || "", + skill_name: skill.name, + action: report.action, + file_count: report.files, + source: "cli", + }) + } catch {} + } catch (err) { + const known = explainPublishError(err) + process.stderr.write((known ?? `Publish failed: ${err instanceof Error ? err.message : String(err)}`) + EOL) + process.exitCode = 1 + } + }) + }, +}) + const SkillShowCommand = cmd({ command: "show ", describe: "display the full content of a skill", @@ -738,6 +794,7 @@ export const SkillCommand = cmd({ .command(SkillListCommand) .command(SkillCreateCommand) .command(SkillTestCommand) + .command(SkillPublishCommand) .command(SkillShowCommand) .command(SkillInstallCommand) .command(SkillRemoveCommand) diff --git a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx index e6da8c85d8..cd1c645363 100644 --- a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx +++ b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx @@ -27,6 +27,8 @@ import type { TuiPlugin, TuiPluginApi, TuiDialogSelectOption } from "@opencode-a import type { BuiltinTuiPlugin } from "@opencode-ai/tui/builtins" import { createMemo, createResource, createSignal, Show } from "solid-js" import { detectToolReferences } from "@/cli/cmd/skill-helpers" +import { describePublish, explainPublishError, isManagedSkill, publishSkill } from "@/altimate/workspace/skill-publish" +import { Telemetry } from "@/altimate/telemetry" import { spawn } from "child_process" import os from "os" import path from "path" @@ -503,12 +505,24 @@ function isRemovable(info: SkillInfo): boolean { function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillName: string, reopen: () => void) { const isBuiltin = !info || info.location.startsWith("builtin:") || !path.isAbsolute(info.location) const removable = !!info && isRemovable(info) + // A skill the workspace sent us is not ours to publish back to it. Judged + // against the project directory workspace sync uses — the binding and the + // managed snapshot live under `api.state.path.directory`, not the git root + // `workdir` resolves to, and the two differ in a worktree subdirectory. + const projectDirectory = api.state.path.directory || workdir(api) + const managed = !isBuiltin && isManagedSkill(projectDirectory, path.dirname(info!.location)) const actions: TuiDialogSelectOption[] = ( [ { title: "Show details", value: "show", description: "View skill info, tools, and location" }, { title: "Edit", value: "edit", description: "Open SKILL.md in your default editor", disabled: isBuiltin }, { title: "Test", value: "test", description: "Validate the paired CLI tool works" }, + { + title: "Publish to workspace", + value: "publish", + description: "Upload this skill to the linked workspace so your team gets it", + disabled: isBuiltin || managed, + }, { title: "Remove", value: "remove", description: "Delete this skill and its paired tool", disabled: !removable }, ] as TuiDialogSelectOption[] ).filter((a) => !a.disabled) @@ -559,6 +573,39 @@ function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillN reopen() break } + case "publish": { + if (!info) return + api.ui.toast({ message: `Publishing ${skillName}...`, variant: "info", duration: 120_000 }) + try { + const report = await publishSkill({ + projectDirectory, + skillDirectory: path.dirname(info.location), + name: skillName, + description: info.description ?? "", + }) + api.ui.toast({ message: describePublish(report), variant: "success", duration: 6000 }) + try { + Telemetry.track({ + type: "skill_published", + timestamp: Date.now(), + session_id: Telemetry.getContext().sessionId || "", + skill_name: skillName, + action: report.action, + file_count: report.files, + source: "tui", + }) + } catch {} + } catch (err) { + const known = explainPublishError(err) + api.ui.toast({ + message: known ?? `Publish failed: ${(err instanceof Error ? err.message : String(err)).slice(0, 150)}`, + variant: known ? "warning" : "error", + duration: 8000, + }) + } + reopen() + break + } case "remove": { if (!info) return try { diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index 853ec0c318..ee84eeeb8a 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -49,6 +49,8 @@ const { SkillNameConflictError, SymlinkError, collectBundle, + describePublish, + explainPublishError, isManagedSkill, ledgerPathForTests, publishSkill, @@ -803,3 +805,37 @@ describe("attaching to the workspace", () => { expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) }) }) + +describe("what a surface says", () => { + // The CLI and the TUI share these lines so a user moving between them + // recognises the outcome. + test("names the outcome, the skill, and the size", () => { + const line = describePublish({ action: "created", publicId: "p", name: "deploy", files: 3, bytes: 2048, datamateId: 1 }) + expect(line).toContain("Published") + expect(line).toContain('"deploy"') + expect(line).toContain("3 files") + expect(line).toContain("2KB") + expect(describePublish({ action: "updated", publicId: "p", name: "d", files: 1, bytes: 12, datamateId: 1 })).toContain( + "Updated", + ) + expect(describePublish({ action: "updated", publicId: "p", name: "d", files: 1, bytes: 12, datamateId: 1 })).toContain( + "1 file,", + ) + }) + + test("passes a deliberate error through and wraps nothing else", async () => { + // Each typed error already says what to do; an unexpected one must not be + // shown as if it were advice. + const unlinked = mkdtempSync(path.join(SANDBOX, "unlinked-")) + const dir = path.join(unlinked, "skills", "x") + mkdirSync(dir, { recursive: true }) + writeFileSync(path.join(dir, "SKILL.md"), "---\nname: x\n---\n") + const err = await publishSkill({ projectDirectory: unlinked, skillDirectory: dir, name: "x", description: "d" }).catch( + (e) => e, + ) + expect(err).toBeInstanceOf(NotLinkedError) + expect(explainPublishError(err)).toContain("altimate-code link") + expect(explainPublishError(new SymlinkError("references"))).toContain("references") + expect(explainPublishError(new Error("ECONNRESET"))).toBeNull() + }) +}) From 5cb64ce91bb283b23efbfb6bc3eecca2269be9dd Mon Sep 17 00:00:00 2001 From: Haider Date: Fri, 18 Sep 2026 11:53:07 +0530 Subject: [PATCH 2/6] fix(workspace): the TUI decides "built-in" the way the CLI does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ralph's F1 on #1313, which Kilo and Codex traced independently. The picker's predicate knew `builtin:` and non-absolute paths only; on any postinstall'd machine the loader prefers the filesystem copy under `~/.altimate/builtin` and registers it by ABSOLUTE path, so every shipped built-in was publishable from the TUI — and one published to a workspace syncs back as a managed skill that overrides the shipped one for every linked member, frozen at that version. `isBuiltinLocation` is the CLI's line (`skillSource`), and a test pins the three-predicate trace for an installed built-in: CLI true, TUI true, managed false. Also: the picker's publish case is one call to `publishFromPicker`, which returns the toast to show — the switch was at cognitive 37 with the try-inside-try inline; and `explainPublishError`'s test asserts the `NotWorkspaceOwnerError` wording it renders as advice. Verified: 628 pass across the workspace, plugin and fork-guard suites, typecheck clean; the prefix-only predicate fails the new test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/plugin/tui/altimate/skill-ops.tsx | 78 ++++++++++++------- .../altimate/plugin/skill-ops-builtin.test.ts | 36 +++++++++ .../altimate/workspace/skill-publish.test.ts | 2 + 3 files changed, 87 insertions(+), 29 deletions(-) create mode 100644 packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts diff --git a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx index cd1c645363..39ff7bf44f 100644 --- a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx +++ b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx @@ -26,7 +26,7 @@ import type { TuiPlugin, TuiPluginApi, TuiDialogSelectOption } from "@opencode-ai/plugin/tui" import type { BuiltinTuiPlugin } from "@opencode-ai/tui/builtins" import { createMemo, createResource, createSignal, Show } from "solid-js" -import { detectToolReferences } from "@/cli/cmd/skill-helpers" +import { detectToolReferences, skillSource } from "@/cli/cmd/skill-helpers" import { describePublish, explainPublishError, isManagedSkill, publishSkill } from "@/altimate/workspace/skill-publish" import { Telemetry } from "@/altimate/telemetry" import { spawn } from "child_process" @@ -502,8 +502,54 @@ function isRemovable(info: SkillInfo): boolean { return gitCheck.exitCode !== 0 // only removable if NOT git-tracked } +/** Built-in the way the CLI decides it: embedded (`builtin:`), or installed + * under `~/.altimate/builtin`, which the loader prefers when present and + * registers by ABSOLUTE path. A prefix check alone let every shipped + * built-in through as publishable on a normal install — and a built-in + * published to a workspace syncs back as a managed skill that overrides the + * shipped one for every linked member, frozen at that version. */ +export function isBuiltinLocation(location: string | undefined): boolean { + return !location || skillSource(location) === "builtin" || !path.isAbsolute(location) +} + +/** The publish half of the action picker, as one call returning the toast to + * show. Kept out of the picker's `onSelect` so that switch stays readable. */ +export async function publishFromPicker( + info: SkillInfo, + skillName: string, + projectDirectory: string, +): Promise<{ message: string; variant: "success" | "warning" | "error"; duration: number }> { + try { + const report = await publishSkill({ + projectDirectory, + skillDirectory: path.dirname(info.location), + name: skillName, + description: info.description ?? "", + }) + try { + Telemetry.track({ + type: "skill_published", + timestamp: Date.now(), + session_id: Telemetry.getContext().sessionId || "", + skill_name: skillName, + action: report.action, + file_count: report.files, + source: "tui", + }) + } catch {} + return { message: describePublish(report), variant: "success", duration: 6000 } + } catch (err) { + const known = explainPublishError(err) + return { + message: known ?? `Publish failed: ${(err instanceof Error ? err.message : String(err)).slice(0, 150)}`, + variant: known ? "warning" : "error", + duration: 8000, + } + } +} + function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillName: string, reopen: () => void) { - const isBuiltin = !info || info.location.startsWith("builtin:") || !path.isAbsolute(info.location) + const isBuiltin = isBuiltinLocation(info?.location) const removable = !!info && isRemovable(info) // A skill the workspace sent us is not ours to publish back to it. Judged // against the project directory workspace sync uses — the binding and the @@ -576,33 +622,7 @@ function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillN case "publish": { if (!info) return api.ui.toast({ message: `Publishing ${skillName}...`, variant: "info", duration: 120_000 }) - try { - const report = await publishSkill({ - projectDirectory, - skillDirectory: path.dirname(info.location), - name: skillName, - description: info.description ?? "", - }) - api.ui.toast({ message: describePublish(report), variant: "success", duration: 6000 }) - try { - Telemetry.track({ - type: "skill_published", - timestamp: Date.now(), - session_id: Telemetry.getContext().sessionId || "", - skill_name: skillName, - action: report.action, - file_count: report.files, - source: "tui", - }) - } catch {} - } catch (err) { - const known = explainPublishError(err) - api.ui.toast({ - message: known ?? `Publish failed: ${(err instanceof Error ? err.message : String(err)).slice(0, 150)}`, - variant: known ? "warning" : "error", - duration: 8000, - }) - } + api.ui.toast(await publishFromPicker(info, skillName, projectDirectory)) reopen() break } diff --git a/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts b/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts new file mode 100644 index 0000000000..a701c85527 --- /dev/null +++ b/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts @@ -0,0 +1,36 @@ +// altimate_change - new file +// +// The Skills dialog's action picker must decide "built-in" the way the CLI +// does. It decided by prefix alone — `builtin:` or a non-absolute path — and +// on any postinstall'd machine the loader prefers the filesystem copy under +// `~/.altimate/builtin`, registered by ABSOLUTE path. So every shipped +// built-in was publishable from the TUI, and one published to a workspace +// syncs back as a managed skill that overrides the shipped one for every +// linked member, frozen at that version. (Ralph, review of #1313; Kilo and +// Codex found the same trace independently.) +import { describe, expect, test } from "bun:test" +import path from "node:path" +import { Global } from "../../../src/global" +import { skillSource } from "../../../src/cli/cmd/skill-helpers" +import { isBuiltinLocation } from "../../../src/plugin/tui/altimate/skill-ops" +import { isManagedSkill } from "../../../src/altimate/workspace/skill-publish" + +describe("the action picker's notion of built-in", () => { + test("agrees with the CLI's for a filesystem-installed built-in", () => { + const installed = path.join(Global.Path.home, ".altimate", "builtin", "x", "SKILL.md") + // The three predicates Ralph traced: CLI true, TUI true, managed false. + expect(skillSource(installed)).toBe("builtin") + expect(isBuiltinLocation(installed)).toBe(true) + expect(isManagedSkill("/some/project", path.dirname(installed))).toBe(false) + }) + + test("still treats the embedded and the non-absolute forms as built-in", () => { + expect(isBuiltinLocation("builtin:x/SKILL.md")).toBe(true) + expect(isBuiltinLocation("relative/x/SKILL.md")).toBe(true) + expect(isBuiltinLocation(undefined)).toBe(true) + }) + + test("does not call a project skill built-in", () => { + expect(isBuiltinLocation("/some/project/.opencode/skills/deploy/SKILL.md")).toBe(false) + }) +}) diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index ee84eeeb8a..edd0c02344 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -836,6 +836,8 @@ describe("what a surface says", () => { expect(err).toBeInstanceOf(NotLinkedError) expect(explainPublishError(err)).toContain("altimate-code link") expect(explainPublishError(new SymlinkError("references"))).toContain("references") + // Advice, not a failure: the surfaces show this one as-is. + expect(explainPublishError(new NotWorkspaceOwnerError("ws"))).toContain("Skills can only be published") expect(explainPublishError(new Error("ECONNRESET"))).toBeNull() }) }) From 2346b7af1fa4e77ca20fd2171665504168e4f89c Mon Sep 17 00:00:00 2001 From: Haider Date: Fri, 18 Sep 2026 12:09:47 +0530 Subject: [PATCH 3/6] fix(workspace): only a project's own skills publish; one publish at a time from the picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round after the built-in fix found its siblings. - The shared publish path refuses a skill whose root is a symbolic link (`collectBundle` refused links inside a skill, but followed a linked root and published whatever it pointed at — `isManagedSkill` cannot see a target that is not the managed snapshot), and a skill whose real path is outside the project. `NotProjectSkillError` names both. Judged on the last path component, since `/var` and `/tmp` are links on macOS. - Personal skills (`~/.claude/skills` and the like, `skillSource` "global") are refused on both surfaces: the user's, but not this project's, and publishing would share them with the whole workspace. The CLI says where the skill lives and what to do; the TUI's row is disabled. - The picker publishes one skill at a time. `DialogSelect` calls the handler for every Enter without awaiting it, so a second press entered `publishSkill` again — serialised by the per-directory lock but not coalesced: a create, a redundant update, and two success toasts. - The CLI's not-found message no longer names `.opencode/skills` as the only place a skill can live. The "one directory reached by two paths" ledger test now uses the sandbox's lexical and real paths rather than a symlinked alias, which is refused. Verified: 631 pass across the workspace, plugin and fork-guard suites, typecheck clean. Mutation-checked: following a linked root (target inside the project, so only that rule catches it) and allowing an outside-project skill each fail a test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-publish.ts | 52 +++++++++++++++++++ packages/opencode/src/cli/cmd/skill.ts | 20 +++++-- .../src/plugin/tui/altimate/skill-ops.tsx | 29 +++++++++-- .../altimate/plugin/skill-ops-builtin.test.ts | 9 +++- .../altimate/workspace/skill-publish.test.ts | 51 ++++++++++++++++-- 5 files changed, 149 insertions(+), 12 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index 5fbee97644..184e130b30 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -168,6 +168,22 @@ export class NotLinkedError extends Error { } } +/** The skill directory is not a project skill: it lives outside the project + * (a personal skill under `~/.claude/skills` or the like), or it reaches + * the project only through a symbolic link. Publishing shares a bundle with + * the whole workspace — a personal skill is not the user's to share by + * accident, and a linked root would publish whatever it points at, which + * `isManagedSkill` cannot see if the target is not the managed snapshot. */ +export class NotProjectSkillError extends Error { + constructor(readonly skillDirectory: string) { + super( + `"${skillDirectory}" is not a skill of this project — it lives outside the project, or is reached ` + + `through a symbolic link. Only a project's own skills can be published to its workspace.`, + ) + this.name = "NotProjectSkillError" + } +} + /** The project is linked to a workspace the caller does not own. Linking * needs only visibility — a colleague's shared workspace can be linked to — * but attaching a skill is a write against the workspace and needs @@ -594,6 +610,12 @@ async function publishSkillUnlocked(input: { }): Promise { if (isManagedSkill(input.projectDirectory, input.skillDirectory)) throw new ManagedSkillError(input.skillDirectory) + // The root itself, resolved: `collectBundle` refuses links INSIDE the + // skill, but a root that is a link is followed, and would publish whatever + // it points at. And the resolved root must be inside the project: the + // loader also serves personal skills from under the home directory, which + // are not this workspace's to receive. + assertProjectSkill(input.projectDirectory, input.skillDirectory) // Before the bundle is even read. An unlinked project has nowhere to attach // to, and uploading first would create the orphan this module exists to @@ -736,6 +758,7 @@ export function explainPublishError(err: unknown): string | null { if ( err instanceof NotLinkedError || err instanceof ManagedSkillError || + err instanceof NotProjectSkillError || err instanceof BinaryFileError || err instanceof SymlinkError || err instanceof EmptyBundleError || @@ -760,6 +783,35 @@ function updateConflict(err: ConflictError, skillName: string): Error { return err } +function assertProjectSkill(projectDirectory: string, skillDirectory: string): void { + const lexical = path.resolve(skillDirectory) + let real: string + try { + real = realpathSync(lexical) + } catch { + // Absent: `collectBundle` fails on it in a moment with a better message. + return + } + // "The root is a link" is judged on the LAST component only: the parent's + // real path plus the skill's own name must equal the skill's real path. + // Comparing the whole path to its lexical form would call every skill on + // macOS a link, since `/var` and `/tmp` are links to `/private/...`. + let parentReal: string + try { + parentReal = realpathSync(path.dirname(lexical)) + } catch { + return + } + if (real !== path.join(parentReal, path.basename(lexical))) throw new NotProjectSkillError(skillDirectory) + let project: string + try { + project = realpathSync(projectDirectory) + } catch { + project = path.resolve(projectDirectory) + } + if (real !== project && !real.startsWith(project + path.sep)) throw new NotProjectSkillError(skillDirectory) +} + /** Refuse before upload when the bound workspace is not the caller's. Read * from the same list the picker uses, which carries each workspace's owner. A * list that omits the owner (an older server) cannot answer, and the attach diff --git a/packages/opencode/src/cli/cmd/skill.ts b/packages/opencode/src/cli/cmd/skill.ts index d75e7ca600..c16ec09665 100644 --- a/packages/opencode/src/cli/cmd/skill.ts +++ b/packages/opencode/src/cli/cmd/skill.ts @@ -473,18 +473,30 @@ const SkillPublishCommand = cmd({ await bootstrap(cwd, async () => { const skill = await Skill.get(name) if (!skill) { - process.stderr.write(`Skill "${name}" not found. Check .opencode/skills/${name}/SKILL.md exists.` + EOL) + process.stderr.write(`Skill "${name}" not found. Run \`altimate-code skill list\` to see the skills this project can reach.` + EOL) process.exitCode = 1 return } // Built-in skills ship with altimate-code — embedded, or installed under - // `~/.altimate/builtin` — and are not the user's to publish; a skill the - // workspace sent us is refused by `publishSkill` itself. - if (skillSource(skill.location) === "builtin" || !path.isAbsolute(skill.location)) { + // `~/.altimate/builtin` — and are not the user's to publish. A personal + // skill under the home directory is the user's, but not this project's: + // publishing shares it with the whole workspace, which is not what + // keeping it in `~/.claude/skills` says. A skill the workspace sent us + // is refused by `publishSkill` itself, as is a symlinked root. + const source = skillSource(skill.location) + if (source === "builtin" || !path.isAbsolute(skill.location)) { process.stderr.write(`"${name}" is a built-in skill and cannot be published.` + EOL) process.exitCode = 1 return } + if (source === "global") { + process.stderr.write( + `"${name}" is a personal skill (${path.dirname(skill.location)}), not one of this project's. ` + + `Copy it into the project's skills directory to publish it.` + EOL, + ) + process.exitCode = 1 + return + } try { const report = await publishSkill({ projectDirectory: Instance.directory, diff --git a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx index 39ff7bf44f..cf386d66a1 100644 --- a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx +++ b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx @@ -512,6 +512,19 @@ export function isBuiltinLocation(location: string | undefined): boolean { return !location || skillSource(location) === "builtin" || !path.isAbsolute(location) } +/** A personal skill under the home directory: the user's, but not this + * project's, so not the workspace's to receive. Same rule as the CLI. */ +export function isGlobalLocation(location: string | undefined): boolean { + return !!location && skillSource(location) === "global" +} + +/** One publish at a time from the picker. `DialogSelect` calls the handler + * for every Enter without awaiting it, so a second press before the first + * settled entered `publishSkill` again — the per-directory lock serialised + * the two but did not coalesce them, and the user got a create, a redundant + * update, and two success toasts. */ +let publishInFlight: string | null = null + /** The publish half of the action picker, as one call returning the toast to * show. Kept out of the picker's `onSelect` so that switch stays readable. */ export async function publishFromPicker( @@ -550,6 +563,7 @@ export async function publishFromPicker( function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillName: string, reopen: () => void) { const isBuiltin = isBuiltinLocation(info?.location) + const isGlobal = isGlobalLocation(info?.location) const removable = !!info && isRemovable(info) // A skill the workspace sent us is not ours to publish back to it. Judged // against the project directory workspace sync uses — the binding and the @@ -567,7 +581,7 @@ function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillN title: "Publish to workspace", value: "publish", description: "Upload this skill to the linked workspace so your team gets it", - disabled: isBuiltin || managed, + disabled: isBuiltin || isGlobal || managed, }, { title: "Remove", value: "remove", description: "Delete this skill and its paired tool", disabled: !removable }, ] as TuiDialogSelectOption[] @@ -621,8 +635,17 @@ function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillN } case "publish": { if (!info) return - api.ui.toast({ message: `Publishing ${skillName}...`, variant: "info", duration: 120_000 }) - api.ui.toast(await publishFromPicker(info, skillName, projectDirectory)) + if (publishInFlight) { + api.ui.toast({ message: `Still publishing ${publishInFlight}…`, variant: "info", duration: 2000 }) + return + } + publishInFlight = skillName + try { + api.ui.toast({ message: `Publishing ${skillName}...`, variant: "info", duration: 120_000 }) + api.ui.toast(await publishFromPicker(info, skillName, projectDirectory)) + } finally { + publishInFlight = null + } reopen() break } diff --git a/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts b/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts index a701c85527..10939ce766 100644 --- a/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts +++ b/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts @@ -12,7 +12,7 @@ import { describe, expect, test } from "bun:test" import path from "node:path" import { Global } from "../../../src/global" import { skillSource } from "../../../src/cli/cmd/skill-helpers" -import { isBuiltinLocation } from "../../../src/plugin/tui/altimate/skill-ops" +import { isBuiltinLocation, isGlobalLocation } from "../../../src/plugin/tui/altimate/skill-ops" import { isManagedSkill } from "../../../src/altimate/workspace/skill-publish" describe("the action picker's notion of built-in", () => { @@ -33,4 +33,11 @@ describe("the action picker's notion of built-in", () => { test("does not call a project skill built-in", () => { expect(isBuiltinLocation("/some/project/.opencode/skills/deploy/SKILL.md")).toBe(false) }) + + test("a personal skill under the home directory is global, and not publishable", () => { + for (const dir of [".claude", ".agents", ".altimate-code"]) { + expect(isGlobalLocation(path.join(Global.Path.home, dir, "skills", "x", "SKILL.md"))).toBe(true) + } + expect(isGlobalLocation("/some/project/.opencode/skills/deploy/SKILL.md")).toBe(false) + }) }) diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index edd0c02344..961a498141 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -42,6 +42,7 @@ const { AltimateApi } = await import("../../../src/altimate/api/client") const { BinaryFileError, EmptyBundleError, + NotProjectSkillError, NotWorkspaceOwnerError, SkillChangedElsewhereError, ManagedSkillError, @@ -580,12 +581,17 @@ describe("the published-id ledger", () => { // `/private/tmp`, a linked worktree) was two ledger keys, so the second // publish created again and 409'd on its own name — "published from // somewhere else", by this machine, a moment ago. - const alias = path.join(project, "skills", "deploy-alias") - symlinkSync(skillDir, alias) - await publishSkill({ projectDirectory: project, skillDirectory: alias, name: "deploy", description: "d" }) + // The two spellings: the sandbox's lexical path and its real path. On + // macOS `os.tmpdir()` is under `/var`, a link to `/private/var`, so these + // differ; elsewhere they are equal and the test still holds trivially. + // (Not a symlinked skill root — that is refused on purpose, see "what + // counts as a project skill".) + const lexical = skillDir + const real = realpathSync(skillDir) + await publishSkill({ projectDirectory: project, skillDirectory: lexical, name: "deploy", description: "d" }) requests = [] - const report = await publish() + const report = await publishSkill({ projectDirectory: project, skillDirectory: real, name: "deploy", description: "d" }) expect(report.action).toBe("updated") expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) @@ -629,6 +635,43 @@ describe("the published-id ledger", () => { }) +describe("what counts as a project skill", () => { + test("a skill root that is a symbolic link is refused before anything is read", async () => { + // The walk refuses links INSIDE a skill; a root that is itself a link was + // followed, and published whatever it pointed at — a built-in, say — + // which `isManagedSkill` cannot see because the target is not the + // managed snapshot. + // The target is INSIDE the project, so the outside-project rule does not + // catch it: only the root-is-a-link rule does. + const target = path.join(project, "vendor", "elsewhere") + mkdirSync(target, { recursive: true }) + writeFileSync(path.join(target, "SKILL.md"), "---\nname: elsewhere\n---\n") + const link = path.join(project, "skills", "looks-local") + symlinkSync(target, link) + + const err = await publishSkill({ projectDirectory: project, skillDirectory: link, name: "elsewhere", description: "d" }).catch( + (e) => e, + ) + + expect(err).toBeInstanceOf(NotProjectSkillError) + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + }) + + test("a skill outside the project is refused", async () => { + // A personal skill under the home directory is the user's, not this + // project's, and publishing would share it with the whole workspace. + const personal = mkdtempSync(path.join(SANDBOX, "personal-")) + writeFileSync(path.join(personal, "SKILL.md"), "---\nname: personal\n---\n") + + const err = await publishSkill({ projectDirectory: project, skillDirectory: personal, name: "personal", description: "d" }).catch( + (e) => e, + ) + + expect(err).toBeInstanceOf(NotProjectSkillError) + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + }) +}) + describe("a workspace the caller does not own", () => { // Linking needs only visibility, so a project can be bound to a colleague's // shared workspace. Attaching a skill needs ownership, and answers 404 From 266e123ebaa790f75c604ee2639e05ce91f20b1e Mon Sep 17 00:00:00 2001 From: Haider Date: Fri, 18 Sep 2026 13:53:04 +0530 Subject: [PATCH 4/6] fix(workspace): the project boundary is the worktree root, and containment is by path segment The previous commit's containment check regressed a valid case: it compared against the session's directory, but discovery walks up to the worktree root, so `skill publish x` run from `repo/models` refused `repo/.opencode/skills/x`. `publishSkill` takes a separate `projectRoot` boundary (the worktree on both surfaces; the session directory for a project with none) while the binding stays keyed on `projectDirectory`. `skillSource` contains by path segment (`path.relative`), not by string prefix: `~/.claude/skills-archive/x` is not inside `~/.claude/skills`, and the prefix check refused it as personal. The real path that passed the check is what `collectBundle` walks, so a root swapped after the check is not what uploads. The ledger-identity test now reaches the skill through a symlinked PARENT so it exercises canonicalisation on Linux too. Verified: 649 pass across the workspace, plugin, fork-guard and skill suites, typecheck clean. Mutation-checked: comparing against the session directory, and containing by prefix, each fail a test; reading the lexical root instead of the validated one has no observable difference without a concurrent writer, and is closed by construction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-publish.ts | 45 +++++++++++-------- .../opencode/src/cli/cmd/skill-helpers.ts | 12 ++++- packages/opencode/src/cli/cmd/skill.ts | 3 ++ .../src/plugin/tui/altimate/skill-ops.tsx | 7 ++- .../altimate/plugin/skill-ops-builtin.test.ts | 12 +++++ .../altimate/workspace/skill-publish.test.ts | 38 ++++++++++++---- 6 files changed, 87 insertions(+), 30 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index 184e130b30..edbede02ae 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -593,29 +593,35 @@ async function attachToWorkspace(publicId: string, datamateId: number): Promise< * `privacy` is left unset: the server defaults to `private`. Publishing should * attach a skill to a workspace, not disclose it to the whole organisation as a * side effect of a command whose name says nothing about visibility. */ -export async function publishSkill(input: { +export interface PublishInput { + /** Where the workspace binding lives — the directory the session was + * started in. `resolveBinding` is keyed on it. */ projectDirectory: string + /** The boundary a skill must lie within to count as this project's. + * Discovery walks up to the git worktree root, so a skill under + * `repo/.opencode/skills` is the project's even when the session started + * in `repo/models` — and `projectDirectory` alone would refuse it. + * Defaults to `projectDirectory` for a project with no worktree. */ + projectRoot?: string skillDirectory: string name: string description: string -}): Promise { +} + +export async function publishSkill(input: PublishInput): Promise { return withPublishLock(input.skillDirectory, () => publishSkillUnlocked(input)) } -async function publishSkillUnlocked(input: { - projectDirectory: string - skillDirectory: string - name: string - description: string -}): Promise { +async function publishSkillUnlocked(input: PublishInput): Promise { if (isManagedSkill(input.projectDirectory, input.skillDirectory)) throw new ManagedSkillError(input.skillDirectory) // The root itself, resolved: `collectBundle` refuses links INSIDE the // skill, but a root that is a link is followed, and would publish whatever // it points at. And the resolved root must be inside the project: the // loader also serves personal skills from under the home directory, which - // are not this workspace's to receive. - assertProjectSkill(input.projectDirectory, input.skillDirectory) + // are not this workspace's to receive. The REAL path that passed is what + // the walk reads, so a root swapped after the check is not what uploads. + const skillRoot = assertProjectSkill(input.projectRoot ?? input.projectDirectory, input.skillDirectory) // Before the bundle is even read. An unlinked project has nowhere to attach // to, and uploading first would create the orphan this module exists to @@ -636,7 +642,7 @@ async function publishSkillUnlocked(input: { // uploaded until the attach is known to be possible. await assertOwnsWorkspace(binding.datamateId, binding.datamateName, scope.userId) - const files = await collectBundle(input.skillDirectory) + const files = await collectBundle(skillRoot) if (files.length === 0) throw new EmptyBundleError() const bytes = files.reduce((n, f) => n + Buffer.byteLength(f.content, "utf8"), 0) @@ -783,14 +789,15 @@ function updateConflict(err: ConflictError, skillName: string): Error { return err } -function assertProjectSkill(projectDirectory: string, skillDirectory: string): void { +/** The skill's real directory, once it has passed. */ +function assertProjectSkill(projectRoot: string, skillDirectory: string): string { const lexical = path.resolve(skillDirectory) let real: string try { real = realpathSync(lexical) } catch { // Absent: `collectBundle` fails on it in a moment with a better message. - return + return lexical } // "The root is a link" is judged on the LAST component only: the parent's // real path plus the skill's own name must equal the skill's real path. @@ -800,16 +807,18 @@ function assertProjectSkill(projectDirectory: string, skillDirectory: string): v try { parentReal = realpathSync(path.dirname(lexical)) } catch { - return + return lexical } if (real !== path.join(parentReal, path.basename(lexical))) throw new NotProjectSkillError(skillDirectory) - let project: string + let root: string try { - project = realpathSync(projectDirectory) + root = realpathSync(projectRoot) } catch { - project = path.resolve(projectDirectory) + root = path.resolve(projectRoot) } - if (real !== project && !real.startsWith(project + path.sep)) throw new NotProjectSkillError(skillDirectory) + const rel = path.relative(root, real) + if (rel.startsWith("..") || path.isAbsolute(rel)) throw new NotProjectSkillError(skillDirectory) + return real } /** Refuse before upload when the bound workspace is not the caller's. Read diff --git a/packages/opencode/src/cli/cmd/skill-helpers.ts b/packages/opencode/src/cli/cmd/skill-helpers.ts index d32620c31b..72c94e3e17 100644 --- a/packages/opencode/src/cli/cmd/skill-helpers.ts +++ b/packages/opencode/src/cli/cmd/skill-helpers.ts @@ -61,7 +61,7 @@ export function skillSource(location: string): string { if (location.startsWith("builtin:")) return "builtin" const home = Global.Path.home // Builtin skills shipped with altimate-code - if (location.startsWith(path.join(home, ".altimate", "builtin"))) return "builtin" + if (isInside(location, path.join(home, ".altimate", "builtin"))) return "builtin" // Global user skills (~/.claude/skills/, ~/.agents/skills/, ~/.config/altimate-code/skills/) const globalDirs = [ path.join(home, ".claude", "skills"), @@ -69,11 +69,19 @@ export function skillSource(location: string): string { path.join(home, ".altimate-code", "skills"), path.join(Global.Path.config, "skills"), ] - if (globalDirs.some((dir) => location.startsWith(dir))) return "global" + if (globalDirs.some((dir) => isInside(location, dir))) return "global" // Everything else is project-level return "project" } +/** Path containment by segments, not by string prefix: `~/.claude/skills-archive/x` + * is not inside `~/.claude/skills`, and a raw `startsWith` said it was — which + * refused a project skill as personal. */ +function isInside(location: string, dir: string): boolean { + const rel = path.relative(dir, location) + return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)) +} + /** Check if a tool is available on the current PATH (including .altimate-code/tools/ and .opencode/tools/). */ export async function isToolOnPath(toolName: string, cwd: string): Promise { // Check project tools/ in both cwd and worktree (they may differ in monorepos) diff --git a/packages/opencode/src/cli/cmd/skill.ts b/packages/opencode/src/cli/cmd/skill.ts index c16ec09665..30daacc8de 100644 --- a/packages/opencode/src/cli/cmd/skill.ts +++ b/packages/opencode/src/cli/cmd/skill.ts @@ -500,6 +500,9 @@ const SkillPublishCommand = cmd({ try { const report = await publishSkill({ projectDirectory: Instance.directory, + // Discovery walks up to the worktree; so must the boundary, or a + // skill under the repository root is refused from a subdirectory. + projectRoot: Instance.worktree !== "/" ? Instance.worktree : Instance.directory, skillDirectory: path.dirname(skill.location), name: skill.name, description: skill.description ?? "", diff --git a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx index cf386d66a1..220e0a0747 100644 --- a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx +++ b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx @@ -531,10 +531,12 @@ export async function publishFromPicker( info: SkillInfo, skillName: string, projectDirectory: string, + projectRoot: string, ): Promise<{ message: string; variant: "success" | "warning" | "error"; duration: number }> { try { const report = await publishSkill({ projectDirectory, + projectRoot, skillDirectory: path.dirname(info.location), name: skillName, description: info.description ?? "", @@ -570,6 +572,9 @@ function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillN // managed snapshot live under `api.state.path.directory`, not the git root // `workdir` resolves to, and the two differ in a worktree subdirectory. const projectDirectory = api.state.path.directory || workdir(api) + // The boundary a skill must lie within: the worktree, since discovery + // walks up to it. `workdir` already resolves that. + const projectRoot = workdir(api) const managed = !isBuiltin && isManagedSkill(projectDirectory, path.dirname(info!.location)) const actions: TuiDialogSelectOption[] = ( @@ -642,7 +647,7 @@ function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillN publishInFlight = skillName try { api.ui.toast({ message: `Publishing ${skillName}...`, variant: "info", duration: 120_000 }) - api.ui.toast(await publishFromPicker(info, skillName, projectDirectory)) + api.ui.toast(await publishFromPicker(info, skillName, projectDirectory, projectRoot)) } finally { publishInFlight = null } diff --git a/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts b/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts index 10939ce766..a18c7d53d1 100644 --- a/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts +++ b/packages/opencode/test/altimate/plugin/skill-ops-builtin.test.ts @@ -41,3 +41,15 @@ describe("the action picker's notion of built-in", () => { expect(isGlobalLocation("/some/project/.opencode/skills/deploy/SKILL.md")).toBe(false) }) }) + +describe("skillSource contains by path segment, not by string prefix", () => { + test("a sibling directory sharing a global dir's prefix is a project skill", () => { + // `~/.claude/skills-archive/x` starts with the string `~/.claude/skills` + // but is not inside it; a raw prefix check refused it as personal. + const sibling = path.join(Global.Path.home, ".claude", "skills-archive", "x", "SKILL.md") + expect(skillSource(sibling)).toBe("project") + expect(isGlobalLocation(sibling)).toBe(false) + const builtinSibling = path.join(Global.Path.home, ".altimate", "builtin-old", "x", "SKILL.md") + expect(skillSource(builtinSibling)).toBe("project") + }) +}) diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index 961a498141..9068abf18c 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -581,17 +581,17 @@ describe("the published-id ledger", () => { // `/private/tmp`, a linked worktree) was two ledger keys, so the second // publish created again and 409'd on its own name — "published from // somewhere else", by this machine, a moment ago. - // The two spellings: the sandbox's lexical path and its real path. On - // macOS `os.tmpdir()` is under `/var`, a link to `/private/var`, so these - // differ; elsewhere they are equal and the test still holds trivially. - // (Not a symlinked skill root — that is refused on purpose, see "what - // counts as a project skill".) - const lexical = skillDir - const real = realpathSync(skillDir) - await publishSkill({ projectDirectory: project, skillDirectory: lexical, name: "deploy", description: "d" }) + // Two spellings of one directory, on every platform: the project's real + // path, and the project reached through a symlinked PARENT. Only the + // skill's own last component may not be a link, so a linked ancestor is + // allowed — it is the `/var` → `/private/var` case, made explicit. + const linkedParent = path.join(SANDBOX, `via-link-${Math.random().toString(36).slice(2)}`) + symlinkSync(project, linkedParent) + const viaLink = path.join(linkedParent, "skills", "deploy") + await publishSkill({ projectDirectory: project, skillDirectory: viaLink, name: "deploy", description: "d" }) requests = [] - const report = await publishSkill({ projectDirectory: project, skillDirectory: real, name: "deploy", description: "d" }) + const report = await publishSkill({ projectDirectory: project, skillDirectory: skillDir, name: "deploy", description: "d" }) expect(report.action).toBe("updated") expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) @@ -657,6 +657,26 @@ describe("what counts as a project skill", () => { expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) }) + test("a skill under the repository root publishes from a subdirectory", async () => { + // Discovery walks up to the worktree root, so a session started in + // `repo/models` can reach `repo/.opencode/skills/x`. The binding stays + // keyed on the session's directory; the containment boundary is the root. + const sub = path.join(project, "models") + mkdirSync(sub, { recursive: true }) + await link(42, "Growth", sub) + requests = [] + + const report = await publishSkill({ + projectDirectory: sub, + projectRoot: project, + skillDirectory: skillDir, // repo/skills/deploy — above `sub` + name: "deploy", + description: "d", + }) + + expect(report.action).toBe("created") + }) + test("a skill outside the project is refused", async () => { // A personal skill under the home directory is the user's, not this // project's, and publishing would share it with the whole workspace. From eec91fd690b3d3f74e7bc59779580e84c70e153c Mon Sep 17 00:00:00 2001 From: Haider Date: Fri, 18 Sep 2026 16:43:37 +0530 Subject: [PATCH 5/6] fix(workspace): a project with no git does not publish from `/` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ralph's one open item on the re-review, traced independently by CodeRabbit and Codex. `Project.fromDirectory` sets the worktree to the sentinel `/` for a project with no git; `workdir(api)` returned it unchanged, so the TUI's containment boundary was `/` and any discovered skill on the machine passed. The TUI now falls back to the session directory, as the CLI already did — and `assertProjectSkill` refuses a filesystem root as a boundary outright, so the next caller that forgets cannot reopen this. Also, cubic's optional one: parent traversal is tested exactly (`..` or `../…`), so a directory literally named `..foo` under the root is inside. Verified: 668 pass across the workspace, plugin, fork-guard and skill suites, typecheck clean. Mutation-checked: accepting a root of `/`, and refusing `..foo`, each fail a test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-publish.ts | 16 ++++++++++--- .../src/plugin/tui/altimate/skill-ops.tsx | 6 +++-- .../altimate/workspace/skill-publish.test.ts | 24 +++++++++++++++++++ 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index edbede02ae..d9adddb58d 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -789,8 +789,15 @@ function updateConflict(err: ConflictError, skillName: string): Error { return err } -/** The skill's real directory, once it has passed. */ -function assertProjectSkill(projectRoot: string, skillDirectory: string): string { +/** The skill's real directory, once it has passed. Exported for its test: + * the boundary rule has to hold for every caller, and a root of `/` — the + * sentinel a project with no git carries — is not a boundary at all. */ +export function assertProjectSkill(projectRoot: string, skillDirectory: string): string { + // A filesystem root would contain everything. Both callers substitute the + // session directory for the `/` sentinel; this refuses it in case one + // forgets, since the failure mode is publishing anything on the machine. + if (path.resolve(projectRoot) === path.parse(path.resolve(projectRoot)).root) + throw new NotProjectSkillError(skillDirectory) const lexical = path.resolve(skillDirectory) let real: string try { @@ -817,7 +824,10 @@ function assertProjectSkill(projectRoot: string, skillDirectory: string): string root = path.resolve(projectRoot) } const rel = path.relative(root, real) - if (rel.startsWith("..") || path.isAbsolute(rel)) throw new NotProjectSkillError(skillDirectory) + // Parent traversal exactly, not any name that begins with two dots: a + // skill directory literally named `..foo` is inside the project. + if (rel === ".." || rel.startsWith(".." + path.sep) || path.isAbsolute(rel)) + throw new NotProjectSkillError(skillDirectory) return real } diff --git a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx index 220e0a0747..d59894f5c7 100644 --- a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx +++ b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx @@ -573,8 +573,10 @@ function openActionPicker(api: TuiPluginApi, info: SkillInfo | undefined, skillN // `workdir` resolves to, and the two differ in a worktree subdirectory. const projectDirectory = api.state.path.directory || workdir(api) // The boundary a skill must lie within: the worktree, since discovery - // walks up to it. `workdir` already resolves that. - const projectRoot = workdir(api) + // walks up to it — EXCEPT for a project with no git, where the worktree + // is the sentinel `/` and `workdir` returns it unchanged. A root of `/` + // would accept any skill on the machine. Same fallback as the CLI. + const projectRoot = api.state.path.worktree === "/" ? projectDirectory : workdir(api) const managed = !isBuiltin && isManagedSkill(projectDirectory, path.dirname(info!.location)) const actions: TuiDialogSelectOption[] = ( diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index 9068abf18c..9a136ec7d9 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -49,6 +49,7 @@ const { NotLinkedError, SkillNameConflictError, SymlinkError, + assertProjectSkill, collectBundle, describePublish, explainPublishError, @@ -677,6 +678,29 @@ describe("what counts as a project skill", () => { expect(report.action).toBe("created") }) + test("a root of `/` is no boundary: the session directory must be the fallback", () => { + // A project with no git carries the sentinel worktree `/`. Passed through + // as the boundary, it contains every skill on the machine — a parent + // directory's `.opencode/skills/x`, an absolute `skills.paths` entry. + // Ralph traced it on the TUI, where `workdir` returned `/` unchanged; + // the shared path refuses it too, for the next caller that forgets. + const elsewhere = mkdtempSync(path.join(SANDBOX, "elsewhere-")) + writeFileSync(path.join(elsewhere, "SKILL.md"), "---\nname: x\n---\n") + expect(() => assertProjectSkill("/", elsewhere)).toThrow(NotProjectSkillError) + // The same skill against the session directory: outside it, refused; + // inside it, allowed. + expect(() => assertProjectSkill(project, elsewhere)).toThrow(NotProjectSkillError) + expect(assertProjectSkill(project, skillDir)).toBe(realpathSync(skillDir)) + }) + + test("a directory named with two leading dots is still inside the project", () => { + // Directly under the root, so `path.relative` is `..foo` itself — the + // one spelling a `startsWith("..")` check mistakes for traversal. + const odd = path.join(project, "..foo") + mkdirSync(odd, { recursive: true }) + expect(assertProjectSkill(project, odd)).toBe(realpathSync(odd)) + }) + test("a skill outside the project is refused", async () => { // A personal skill under the home directory is the user's, not this // project's, and publishing would share it with the whole workspace. From 83fba3511ed8bb77f0612ab6381e5803b921ca4a Mon Sep 17 00:00:00 2001 From: Haider Date: Fri, 18 Sep 2026 16:52:52 +0530 Subject: [PATCH 6/6] fix(workspace): refuse a filesystem root as the boundary on its resolved path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refusal was judged on the lexical root while the containment comparison below it used the real path — so a root that is a symbolic link to `/` passed the first and became `/` for the second. The root is resolved once, refused on that value, and the same value bounds the skill. Verified: 668 pass, typecheck clean; judging the root lexically fails the new link-to-`/` case. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-publish.ts | 25 +++++++++++-------- .../altimate/workspace/skill-publish.test.ts | 6 +++++ 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index d9adddb58d..d1ea64ab74 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -793,11 +793,20 @@ function updateConflict(err: ConflictError, skillName: string): Error { * the boundary rule has to hold for every caller, and a root of `/` — the * sentinel a project with no git carries — is not a boundary at all. */ export function assertProjectSkill(projectRoot: string, skillDirectory: string): string { - // A filesystem root would contain everything. Both callers substitute the - // session directory for the `/` sentinel; this refuses it in case one - // forgets, since the failure mode is publishing anything on the machine. - if (path.resolve(projectRoot) === path.parse(path.resolve(projectRoot)).root) - throw new NotProjectSkillError(skillDirectory) + // The boundary is the root's REAL path, resolved once and used for both + // checks below. A filesystem root would contain everything: both callers + // substitute the session directory for the `/` sentinel, and this refuses + // it in case one forgets, since the failure mode is publishing anything + // on the machine. Judged after resolving — a root that is a symbolic link + // to `/` is `/` for the containment comparison, so it must be refused on + // the same value that comparison uses. + let root: string + try { + root = realpathSync(projectRoot) + } catch { + root = path.resolve(projectRoot) + } + if (root === path.parse(root).root) throw new NotProjectSkillError(skillDirectory) const lexical = path.resolve(skillDirectory) let real: string try { @@ -817,12 +826,6 @@ export function assertProjectSkill(projectRoot: string, skillDirectory: string): return lexical } if (real !== path.join(parentReal, path.basename(lexical))) throw new NotProjectSkillError(skillDirectory) - let root: string - try { - root = realpathSync(projectRoot) - } catch { - root = path.resolve(projectRoot) - } const rel = path.relative(root, real) // Parent traversal exactly, not any name that begins with two dots: a // skill directory literally named `..foo` is inside the project. diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index 9a136ec7d9..080d000a63 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -687,6 +687,12 @@ describe("what counts as a project skill", () => { const elsewhere = mkdtempSync(path.join(SANDBOX, "elsewhere-")) writeFileSync(path.join(elsewhere, "SKILL.md"), "---\nname: x\n---\n") expect(() => assertProjectSkill("/", elsewhere)).toThrow(NotProjectSkillError) + // And a root that is merely a LINK to `/`: lexically it is a directory + // inside the sandbox, but the containment comparison resolves it, and + // the refusal must be judged on that same resolved value. + const rootLink = path.join(SANDBOX, `root-link-${Math.random().toString(36).slice(2)}`) + symlinkSync("/", rootLink) + expect(() => assertProjectSkill(rootLink, elsewhere)).toThrow(NotProjectSkillError) // The same skill against the session directory: outside it, refused; // inside it, allowed. expect(() => assertProjectSkill(project, elsewhere)).toThrow(NotProjectSkillError)