From 79f77e1efd062bce2186a7574514ee27c83b6925 Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 9 Sep 2026 05:51:18 +0530 Subject: [PATCH 1/6] feat(workspace): publish a locally-authored skill to the linked workspace (#1271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upload half of `skill-sync.ts`, which only ever pulls. A skill written locally had no route to the workspace, and nothing in the CLI said so. Shaped so agents and commands can ride the same path later: a workspace skill is a named bundle of files, and nothing here is skill-specific except the endpoint it posts to. `collectBundle` and the binary guard take a directory, not a skill. Three rules the module exists to enforce, each a bug if skipped: **Refuse non-UTF-8 files, naming the path.** The wire format is `{path, content}` with content as a STRING — the server does `content.encode("utf-8")` inbound and returns a decoded string outbound. A bundle carrying a PNG cannot round-trip: the declared byte size stops matching after the re-encode and `skill-sync` skips the whole skill, logging a warning nobody sees. Caught at publish it is one clear local error; uncaught, the upload succeeds and the skill silently vanishes from every OTHER machine, days later, with nothing tying symptom to cause. Decoding is strict (`fatal: true`) because the default substitutes U+FFFD and would hand back a "valid" string that reassembles into a different file. **Never publish from the managed snapshot.** `.altimate-code/skill/_workspace` holds skills the workspace sent us and sits under the same `{skill,skills}/**` glob as the user's own — deliberately, since that is how they load. A publish that walked "every skill in this project" would send the workspace's own skills back to it. The check compares against a separator-terminated prefix, so `_workspace-notes` is not mistaken for something inside `_workspace`. **Remember the server's id, so a second publish updates.** Names are unique per creator server-side, so a blind re-create answers 409 rather than duplicating — but that turns an ordinary second publish into an error the user has to interpret. The id lives in a local ledger, not `SKILL.md` frontmatter. Frontmatter is committed, so the id would travel with the skill: a colleague cloning the repo and publishing would UPDATE the original author's bundle rather than create their own. It is keyed on the resolved directory and scoped to the account it was published under, and rows are shape-checked on read rather than cast, so a corrupt entry costs its own skill a re-create instead of a PATCH against a garbage id. `privacy` is left unset — the server defaults to `private`. Publishing should attach a skill to a workspace, not disclose it org-wide as a side effect of a command whose name says nothing about visibility. A 404 on update falls through to create: the skill was deleted in the workspace since we published it, and failing would strand the user with a local id they can neither see nor clear. Tests: 11 new, 443 across `test/altimate/workspace`. Mutation-checked — 8 mutations, 8 killed: non-fatal decoding, dropping the managed-snapshot guard, prefix-matching without the separator, always creating, swallowing the 409, not re-creating after a 404, not recording the id, and defaulting privacy to public each fail a test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-publish.ts | 302 ++++++++++++++++++ .../altimate/workspace/skill-publish.test.ts | 212 ++++++++++++ 2 files changed, 514 insertions(+) create mode 100644 packages/opencode/src/altimate/workspace/skill-publish.ts create mode 100644 packages/opencode/test/altimate/workspace/skill-publish.test.ts diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts new file mode 100644 index 000000000..58357b4dd --- /dev/null +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -0,0 +1,302 @@ +// altimate_change - new file +// +// Publishing a locally-authored skill to the linked workspace — the upload half +// of `skill-sync.ts`, which only ever pulls. +// +// Shaped so agents and commands can ride the same path later. A workspace skill +// is a NAMED BUNDLE OF FILES, and nothing below is skill-specific except the +// endpoint it posts to and the `SKILL.md` it reads a name out of. `collectBundle` +// and the binary guard take a directory, not a skill. +// +// Three rules this module exists to enforce, each of which is a bug if skipped: +// +// 1. Refuse non-UTF-8 files, naming the path. The wire format is +// `{path, content}` with content as a STRING — the server does +// `content.encode("utf-8")` on the way in and hands back a decoded string on +// the way out. A bundle carrying a PNG therefore cannot round-trip: the +// declared byte size stops matching after the re-encode and `skill-sync` +// skips the whole skill, logging a warning nobody sees. Caught here it is +// one clear local error; caught there it is a skill that silently vanishes +// from every OTHER machine, days later, with nothing tying the symptom to +// the cause. +// +// 2. Never publish from the managed snapshot. `.altimate-code/skill/_workspace` +// holds skills the workspace sent us, and it sits under the same +// `{skill,skills}/**​/SKILL.md` glob as the user's own — deliberately, since +// that is how they load. A publish that walked "every skill in this project" +// would upload the workspace's own skills back to it. +// +// 3. Remember the server's id after a first publish, so publishing again +// UPDATES rather than creating a second bundle. Names are unique per creator +// server-side, so a blind re-create answers 409 rather than duplicating — +// but that turns an ordinary second publish into an error the user has to +// interpret. +import fs from "fs/promises" +import path from "path" +import { Log } from "@/altimate/util/log" +import { Global } from "@/global" +import { Filesystem } from "@/util/filesystem" +import { AltimateApi } from "@/altimate/api/client" +import { ConflictError, NotFoundError, altimateRequest } from "./api-client" + +const log = Log.create({ service: "altimate-workspace-skill-publish" }) + +const SKILLS_BASE = "/skills" +/** Must stay in step with `skill-sync.ts`. Duplicated rather than exported from + * there because importing it would pull the whole sync module — and its + * process-global store — into every caller that only wants to publish. */ +const MANAGED_DIR = path.join(".altimate-code", "skill", "_workspace") + +/** Mirrors the server's own ceilings so an oversized bundle fails locally, with a + * usable message, instead of after a long upload. */ +const MAX_BUNDLE_BYTES = 10 * 1024 * 1024 +const MAX_BUNDLE_FILES = 200 + +export interface BundleFile { + path: string + content: string +} + +export class BinaryFileError extends Error { + constructor(readonly filePath: string) { + super( + `"${filePath}" is not UTF-8 text. Workspace skill bundles are transported as text, ` + + `so binary files cannot be published — remove it, or keep it outside the skill.`, + ) + this.name = "BinaryFileError" + } +} + +export class ManagedSkillError extends Error { + constructor(readonly filePath: string) { + super( + `"${filePath}" is a skill this workspace sent to you, not one you authored. ` + + `Publishing it would send the workspace's own skill back to it.`, + ) + this.name = "ManagedSkillError" + } +} + +export class BundleTooLargeError extends Error {} + +/** The workspace already has a skill of this name owned by this user, and this + * machine has no id for it — so it was published from somewhere else. + * + * A distinct type rather than re-raising the API's `ConflictError`: that one + * carries a structured server detail, and constructing a fake one to hold a + * client-authored sentence would misrepresent the envelope. */ +export class SkillNameConflictError extends Error { + constructor(readonly skillName: string) { + super( + `You already have a skill named "${skillName}" in this workspace. It was published ` + + `from somewhere else, so this machine cannot update it — rename this one, or edit ` + + `it in the workspace.`, + ) + this.name = "SkillNameConflictError" + } +} + +export interface PublishReport { + action: "created" | "updated" + publicId: string + name: string + files: number + bytes: number +} + +/** Read one directory into a bundle, refusing anything that cannot survive the + * transport. + * + * Strict decoding is the whole point: `TextDecoder` with `fatal: true` throws on + * an invalid sequence, where the default silently substitutes U+FFFD and would + * hand us a "valid" string that reassembles into a different file. */ +export async function collectBundle(dir: string): Promise { + const root = path.resolve(dir) + const files: BundleFile[] = [] + let bytes = 0 + + const walk = async (current: string): Promise => { + const entries = await fs.readdir(current, { withFileTypes: true }) + for (const entry of entries) { + const full = path.join(current, entry.name) + if (entry.isDirectory()) { + await walk(full) + continue + } + if (!entry.isFile()) continue + const relative = path.relative(root, full).split(path.sep).join("/") + const raw = await fs.readFile(full) + let content: string + try { + content = new TextDecoder("utf-8", { fatal: true }).decode(raw) + } catch { + throw new BinaryFileError(relative) + } + bytes += raw.byteLength + files.push({ path: relative, content }) + if (files.length > MAX_BUNDLE_FILES) + throw new BundleTooLargeError(`This skill has more than ${MAX_BUNDLE_FILES} files.`) + if (bytes > MAX_BUNDLE_BYTES) + throw new BundleTooLargeError(`This skill is larger than ${MAX_BUNDLE_BYTES / (1024 * 1024)}MB.`) + } + } + + await walk(root) + files.sort((a, b) => a.path.localeCompare(b.path)) + return files +} + +/** True when this path lives inside the workspace-owned snapshot. */ +export function isManagedSkill(projectDirectory: string, skillDirectory: string): boolean { + const managed = path.resolve(projectDirectory, MANAGED_DIR) + const candidate = path.resolve(skillDirectory) + return candidate === managed || candidate.startsWith(managed + path.sep) +} + +// --------------------------------------------------------------------------- +// Published-id bookkeeping +// +// A local file rather than `SKILL.md` frontmatter, deliberately. Frontmatter is +// committed, so the id would travel with the skill: a colleague cloning the repo +// and publishing would UPDATE the original author's bundle rather than create +// their own. It would also put a server identifier into a file the user edits by +// hand, and show up in every diff. The id is a fact about "this machine published +// this skill to this workspace", which is exactly the scope of local state. +// --------------------------------------------------------------------------- + +interface PublishedRecord { + publicId: string + tenant: string + apiUrl: string +} + +function ledgerPath(): string { + return path.join(Global.Path.state, "altimate-published-skills.json") +} + +/** Shape check, not a cast. The file comes off disk and could be anything — an + * older layout, hand-edited, half-written. A malformed row must be dropped rather + * than trusted into a PATCH against a garbage id. */ +function isPublishedRecord(value: unknown): value is PublishedRecord { + if (!value || typeof value !== "object") return false + const r = value as Record + return typeof r.publicId === "string" && typeof r.tenant === "string" && typeof r.apiUrl === "string" +} + +async function readLedger(): Promise> { + try { + const raw = await Filesystem.readText(ledgerPath()) + const parsed = JSON.parse(raw) as unknown + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return {} + // Per-entry, so one corrupt row costs its own skill a re-create rather than + // discarding every other skill's id. + const out: Record = {} + for (const [key, value] of Object.entries(parsed as Record)) + if (isPublishedRecord(value)) out[key] = value + return out + } catch { + // Absent or unreadable both mean "nothing known", which costs a create that + // may 409 — recoverable — rather than an update against a guessed id. + return {} + } +} + +/** Keyed on the resolved skill directory so a rename of the skill's *name* does + * not orphan its id, and two skills in different projects cannot collide. */ +async function recordPublished(skillDir: string, record: PublishedRecord): Promise { + const ledger = await readLedger() + ledger[path.resolve(skillDir)] = record + try { + await Filesystem.writeJson(ledgerPath(), ledger) + } catch (err) { + // Best-effort. Losing the id costs a 409 on the next publish, not data. + log.warn("could not record the published skill id", { err: String(err) }) + } +} + +async function knownPublicId(skillDir: string): Promise { + const creds = await AltimateApi.getCredentials().catch(() => null) + if (!creds) return null + const record = (await readLedger())[path.resolve(skillDir)] + if (!record) return null + // Scoped to the account it was published under. The same checkout pointed at a + // different tenant must not update an id that does not exist there. + if (record.tenant !== creds.altimateInstanceName || record.apiUrl !== creds.altimateUrl) return null + return record.publicId +} + +/** Publish a skill directory to the workspace, creating it or updating the bundle + * already published from this machine. + * + * `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: { + projectDirectory: string + skillDirectory: string + name: string + description: string +}): Promise { + if (isManagedSkill(input.projectDirectory, input.skillDirectory)) + throw new ManagedSkillError(input.skillDirectory) + + const files = await collectBundle(input.skillDirectory) + if (files.length === 0) throw new BundleTooLargeError("This skill directory has no files to publish.") + const bytes = files.reduce((n, f) => n + Buffer.byteLength(f.content, "utf8"), 0) + + const existing = await knownPublicId(input.skillDirectory) + if (existing) { + try { + await altimateRequest("PATCH", `/${encodeURIComponent(existing)}`, { + base: SKILLS_BASE, + body: { name: input.name, description: input.description, files }, + allowEmptyBody: true, + }) + return { action: "updated", publicId: existing, name: input.name, files: files.length, bytes } + } catch (err) { + // The skill was deleted in the workspace since we published it. Falling + // through to create is the useful answer; failing would strand the user + // with a local id they cannot see or clear. + if (!(err instanceof NotFoundError)) throw err + log.info("published skill no longer exists in the workspace; creating it again", { + publicId: existing, + }) + } + } + + let created: unknown + try { + created = await altimateRequest("POST", "", { + base: SKILLS_BASE, + body: { name: input.name, description: input.description, files }, + }) + } catch (err) { + // Names are unique per creator server-side. Reached when the same skill was + // published from another machine, so this one holds no id for it. + if (err instanceof ConflictError) throw new SkillNameConflictError(input.name) + throw err + } + + const publicId = extractPublicId(created) + if (!publicId) throw new Error("The workspace accepted the skill but did not return an id for it.") + + const creds = await AltimateApi.getCredentials() + await recordPublished(input.skillDirectory, { + publicId, + tenant: creds.altimateInstanceName, + apiUrl: creds.altimateUrl, + }) + return { action: "created", publicId, name: input.name, files: files.length, bytes } +} + +/** Accepts the documented `{public_id}` and a `{skill: {public_id}}` envelope, so + * a compat wrapper on either side does not strand the id — the same tolerance + * `skill-sync` applies to the list and detail shapes. */ +function extractPublicId(payload: unknown): string | null { + if (!payload || typeof payload !== "object") return null + const direct = (payload as { public_id?: unknown }).public_id + if (typeof direct === "string" && direct) return direct + const nested = (payload as { skill?: { public_id?: unknown } }).skill?.public_id + if (typeof nested === "string" && nested) return nested + return null +} diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts new file mode 100644 index 000000000..a88e0748a --- /dev/null +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -0,0 +1,212 @@ +// altimate_change - new file +// +// Unit coverage for publishing a locally-authored skill (skill-publish.ts). +// +// House style, matching memory-sync.test.ts: no `mock.module()`. Real files in a +// real sandbox, network stubbed at `globalThis.fetch` so assertions are about the +// requests actually issued — method, path, body — rather than a mock's call log. +import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test" +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs" +import path from "node:path" +import os from "node:os" + +const ORIGINAL_XDG_STATE_HOME = process.env.XDG_STATE_HOME +const SANDBOX = path.join(os.tmpdir(), `altimate-publish-${process.pid}-${Date.now()}`) +mkdirSync(path.join(SANDBOX, "state"), { recursive: true }) +process.env.XDG_STATE_HOME = path.join(SANDBOX, "state") + +afterAll(() => { + if (ORIGINAL_XDG_STATE_HOME === undefined) delete process.env.XDG_STATE_HOME + else process.env.XDG_STATE_HOME = ORIGINAL_XDG_STATE_HOME + try { + rmSync(SANDBOX, { recursive: true, force: true }) + } catch { + /* best effort */ + } +}) + +const { AltimateApi } = await import("../../../src/altimate/api/client") +const { + BinaryFileError, + ManagedSkillError, + SkillNameConflictError, + collectBundle, + isManagedSkill, + publishSkill, +} = await import("../../../src/altimate/workspace/skill-publish") + +type Creds = Awaited> +// Saved and restored. Bun runs every test file in one process, so a stub left in +// place here leaks into sibling suites — which is exactly what happened: 47 +// unrelated workspace tests failed until this was put back. +const originalIsConfigured = AltimateApi.isConfigured +const originalGetCreds = AltimateApi.getCredentials +;(AltimateApi as unknown as { isConfigured: () => Promise }).isConfigured = async () => true +;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => + ({ altimateInstanceName: "acme", altimateUrl: "https://api.example.com", altimateApiKey: "k" }) as Creds + +afterAll(() => { + ;(AltimateApi as unknown as { isConfigured: typeof originalIsConfigured }).isConfigured = originalIsConfigured + ;(AltimateApi as unknown as { getCredentials: typeof originalGetCreds }).getCredentials = originalGetCreds +}) + +const originalFetch = globalThis.fetch +let requests: { method: string; url: string; body: any }[] = [] +/** Per-method status. `POST` 409 exercises the name conflict; `PATCH` 404 the + * published-then-deleted fallback. */ +let statuses: Record = {} + +let project = "" +let skillDir = "" + +beforeEach(() => { + requests = [] + statuses = {} + project = mkdtempSync(path.join(SANDBOX, "proj-")) + skillDir = path.join(project, "skills", "deploy") + mkdirSync(skillDir, { recursive: true }) + writeFileSync(path.join(skillDir, "SKILL.md"), "---\nname: deploy\n---\nrun it\n") + + globalThis.fetch = (async (input: any, init?: any) => { + const url = typeof input === "string" ? input : input.url + const method = (init?.method ?? "GET").toUpperCase() + let body: any = undefined + try { + body = init?.body ? JSON.parse(init.body) : undefined + } catch { + /* non-JSON bodies are not used here */ + } + requests.push({ method, url, body }) + const status = statuses[method] ?? (method === "POST" ? 201 : 200) + if (status >= 400) + return new Response(JSON.stringify({ detail: "nope" }), { + status, + headers: { "content-type": "application/json" }, + }) + return new Response(JSON.stringify({ public_id: "pub-1" }), { + status, + headers: { "content-type": "application/json" }, + }) + }) as typeof fetch +}) + +afterEach(() => { + globalThis.fetch = originalFetch +}) + +const publish = () => + publishSkill({ projectDirectory: project, skillDirectory: skillDir, name: "deploy", description: "d" }) + +describe("collectBundle", () => { + test("refuses a file that is not UTF-8, naming it", async () => { + // The wire format carries content as a string, so a binary file cannot + // round-trip. Caught here it is one local error; uncaught, the upload + // succeeds and the skill is skipped on every OTHER machine's pull. + writeFileSync(path.join(skillDir, "logo.png"), Buffer.from([0xff, 0xd8, 0xff, 0x00, 0x01])) + + const err = await collectBundle(skillDir).catch((e) => e) + + expect(err).toBeInstanceOf(BinaryFileError) + expect(String(err)).toContain("logo.png") + }) + + test("decodes strictly rather than substituting replacement characters", async () => { + // The default TextDecoder would turn an invalid sequence into U+FFFD and hand + // back a "valid" string, publishing a file that differs from the one on disk. + writeFileSync(path.join(skillDir, "notes.md"), Buffer.from([0x68, 0x69, 0xc3, 0x28])) + + await expect(collectBundle(skillDir)).rejects.toBeInstanceOf(BinaryFileError) + }) + + test("walks nested directories and reports posix-style relative paths", async () => { + mkdirSync(path.join(skillDir, "references"), { recursive: true }) + writeFileSync(path.join(skillDir, "references", "api.md"), "docs") + + const files = await collectBundle(skillDir) + + expect(files.map((f) => f.path).sort()).toEqual(["SKILL.md", "references/api.md"]) + }) +}) + +describe("isManagedSkill", () => { + test("recognises the workspace-owned snapshot", () => { + const managed = path.join(project, ".altimate-code", "skill", "_workspace", "theirs") + expect(isManagedSkill(project, managed)).toBe(true) + }) + + test("does not mistake a sibling path with the same prefix", () => { + // `_workspace-notes` starts with the managed path as a string but is not + // inside it — a plain `startsWith` without the separator would refuse it. + const sibling = path.join(project, ".altimate-code", "skill", "_workspace-notes") + expect(isManagedSkill(project, sibling)).toBe(false) + }) + + test("does not flag the user's own skills", () => { + expect(isManagedSkill(project, skillDir)).toBe(false) + }) +}) + +describe("publishSkill", () => { + test("refuses to publish a skill the workspace sent us", async () => { + const managed = path.join(project, ".altimate-code", "skill", "_workspace", "theirs") + mkdirSync(managed, { recursive: true }) + writeFileSync(path.join(managed, "SKILL.md"), "---\nname: theirs\n---\n") + + const err = await publishSkill({ + projectDirectory: project, + skillDirectory: managed, + name: "theirs", + description: "d", + }).catch((e) => e) + + expect(err).toBeInstanceOf(ManagedSkillError) + // And nothing was sent. A refusal that still uploaded would be worse than none. + expect(requests).toHaveLength(0) + }) + + test("creates on the first publish and carries the bundle", async () => { + const report = await publish() + + expect(report.action).toBe("created") + expect(report.publicId).toBe("pub-1") + const post = requests.find((r) => r.method === "POST")! + expect(post.body.name).toBe("deploy") + expect(post.body.files.map((f: any) => f.path)).toEqual(["SKILL.md"]) + // `privacy` is deliberately unset: the server defaults to private, and + // publishing should not disclose a skill org-wide as a side effect. + expect(post.body.privacy).toBeUndefined() + }) + + test("updates in place on the second publish rather than creating a duplicate", async () => { + await publish() + requests = [] + + const report = await publish() + + expect(report.action).toBe("updated") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + const patch = requests.find((r) => r.method === "PATCH")! + expect(patch.url).toContain("pub-1") + }) + + test("reports a name conflict as its own error, not a raw API conflict", async () => { + statuses.POST = 409 + + const err = await publish().catch((e) => e) + + expect(err).toBeInstanceOf(SkillNameConflictError) + expect(String(err)).toContain("published") + }) + + test("re-creates a skill that has been deleted in the workspace since we published it", async () => { + // Otherwise the user is stranded: a local id they cannot see, update or clear. + await publish() + requests = [] + statuses.PATCH = 404 + + const report = await publish() + + expect(report.action).toBe("created") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) + }) +}) From 77256d0e1f0235005b7d76213408503865768b16 Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 9 Sep 2026 20:43:31 +0530 Subject: [PATCH 2/6] fix(workspace): three review findings on skill publish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **A symlinked skill directory defeated the managed-snapshot check.** `path.resolve` is lexical: it normalises `..` and absolutises, but it does not follow links. So a skill directory that IS a link into `.altimate-code/skill/ _workspace` resolved to its own path, passed `isManagedSkill`, and the bundle walk then followed the link — publishing the workspace's own skills back to it under the user's name. Compared through `realpathSync` now, falling back to the lexical form for a path that does not exist, which cannot be a link into the snapshot anyway. **The bundle size guard could not stop the thing it exists to stop.** `collectBundle` read each file with `readFile` and only then checked the running total, so a single oversized file was pulled entirely into memory before being rejected. Size is checked before the read now; the cumulative check stays for many small files and as a backstop if the file grows in between. **A conflicting rename on the update path surfaced a raw API envelope.** The POST path maps 409 to `SkillNameConflictError`; the PATCH path only handled `NotFoundError`, so renaming a skill onto a name this creator already uses reached the caller as the server's own error shape — the exact outcome the typed errors in this module exist to prevent, and invisible from the create path. Tests: 14 in this file, 3 new, whole altimate suite green. Worth recording how the tests were arrived at, because the first versions were worthless: all three mutations SURVIVED. Asserting that an oversized bundle is rejected does not test this fix — the post-read check rejects it too — so the test now patches `readFile` and asserts the oversized file is never read at all. The other two had no coverage whatsoever. All three mutations fail now. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-publish.ts | 34 ++++++++- .../altimate/workspace/skill-publish.test.ts | 73 ++++++++++++++++++- 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index 58357b4dd..d9c7dd196 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -33,6 +33,7 @@ // interpret. import fs from "fs/promises" import path from "path" +import { realpathSync } from "fs" import { Log } from "@/altimate/util/log" import { Global } from "@/global" import { Filesystem } from "@/util/filesystem" @@ -125,6 +126,15 @@ export async function collectBundle(dir: string): Promise { } if (!entry.isFile()) continue const relative = path.relative(root, full).split(path.sep).join("/") + // Size BEFORE read. `readFile` pulls the whole file into memory, so + // checking the running total afterwards let a single oversized file + // through the very guard meant to stop it — the limit was enforced only + // once the damage was done. The cumulative check below stays as the + // answer for many small files, and as a backstop if the file grew + // between this stat and the read. + const stat = await fs.stat(full) + if (bytes + stat.size > MAX_BUNDLE_BYTES) + throw new BundleTooLargeError(`This skill is larger than ${MAX_BUNDLE_BYTES / (1024 * 1024)}MB.`) const raw = await fs.readFile(full) let content: string try { @@ -148,8 +158,23 @@ export async function collectBundle(dir: string): Promise { /** True when this path lives inside the workspace-owned snapshot. */ export function isManagedSkill(projectDirectory: string, skillDirectory: string): boolean { - const managed = path.resolve(projectDirectory, MANAGED_DIR) - const candidate = path.resolve(skillDirectory) + // `path.resolve` is lexical: it normalises `..` and makes the path absolute, + // but it does not follow links. A skill directory that IS a symlink into the + // workspace-owned snapshot therefore resolved to its own link path, missed + // this check, and `collectBundle` then walked through the link and published + // the workspace's own skills back to it. Compare real paths where they exist. + const real = (p: string): string => { + try { + return realpathSync(p) + } catch { + // Absent or unreadable: fall back to the lexical form. A path that does + // not exist cannot be a link into the snapshot, and `collectBundle` will + // fail on it in a moment anyway. + return path.resolve(p) + } + } + const managed = real(path.resolve(projectDirectory, MANAGED_DIR)) + const candidate = real(skillDirectory) return candidate === managed || candidate.startsWith(managed + path.sep) } @@ -257,6 +282,11 @@ export async function publishSkill(input: { // The skill was deleted in the workspace since we published it. Falling // through to create is the useful answer; failing would strand the user // with a local id they cannot see or clear. + // A PATCH that renames onto a name this creator already uses answers 409. + // Without this the raw server envelope reaches the caller — the exact + // thing the typed errors in this module exist to prevent — and only on + // the update path, so the create path looked correct in isolation. + if (err instanceof ConflictError) throw new SkillNameConflictError(input.name) if (!(err instanceof NotFoundError)) throw err log.info("published skill no longer exists in the workspace; creating it again", { publicId: existing, diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index a88e0748a..5f18934ff 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -6,8 +6,18 @@ // real sandbox, network stubbed at `globalThis.fetch` so assertions are about the // requests actually issued — method, path, body — rather than a mock's call log. import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test" -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs" +import { + closeSync, + ftruncateSync, + mkdirSync, + mkdtempSync, + openSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs" import path from "node:path" +import fsp from "node:fs/promises" import os from "node:os" const ORIGINAL_XDG_STATE_HOME = process.env.XDG_STATE_HOME @@ -210,3 +220,64 @@ describe("publishSkill", () => { expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) }) }) + +describe("the bundle size guard", () => { + test("refuses an oversized file WITHOUT reading it into memory", async () => { + // The guard checked the running total after `readFile`, so a single huge + // file was fully loaded before being rejected — the limit enforced only + // once the memory had already been spent. Asserting on the rejection alone + // does not test that: the post-read check rejects too, and the mutation + // survived. The property is that `readFile` is never called for the file. + const dir = path.join(SANDBOX, `oversize-${Math.random().toString(36).slice(2)}`) + mkdirSync(dir, { recursive: true }) + writeFileSync(path.join(dir, "SKILL.md"), "---\nname: big\n---\n") + const huge = path.join(dir, "huge.txt") + const fd = openSync(huge, "w") + try { + ftruncateSync(fd, 64 * 1024 * 1024) // sparse: past the limit, cheap on disk + } finally { + closeSync(fd) + } + + const read: string[] = [] + const originalReadFile = fsp.readFile + ;(fsp as unknown as { readFile: unknown }).readFile = ((...args: unknown[]) => { + read.push(String(args[0])) + return (originalReadFile as (...a: unknown[]) => unknown)(...args) + }) as unknown as typeof fsp.readFile + + try { + await expect(collectBundle(dir)).rejects.toThrow(/larger than/i) + } finally { + ;(fsp as unknown as { readFile: unknown }).readFile = originalReadFile + } + expect(read.some((r) => r.endsWith("huge.txt"))).toBe(false) + }) + + test("a symlinked skill directory into the managed snapshot is still managed", async () => { + // `path.resolve` is lexical, so a skill directory that IS a link into the + // workspace-owned snapshot resolved to its own path and passed the check — + // and the bundle walk then followed the link and would have published the + // workspace's own skills back to it. + const proj = mkdtempSync(path.join(SANDBOX, "symproj-")) + const managed = path.join(proj, ".altimate-code", "skill", "_workspace", "pub-a") + mkdirSync(managed, { recursive: true }) + const link = path.join(proj, "looks-local") + symlinkSync(managed, link) + + expect(isManagedSkill(proj, link)).toBe(true) + }) + + test("a rename that collides on the update path is a typed conflict", async () => { + // The create path mapped 409 to SkillNameConflictError; the update path did + // not, so a PATCH that renames onto an existing name surfaced the raw + // server envelope — the exact thing this module's typed errors exist to + // prevent. + await publish() // records the id, so the next call takes the PATCH branch + statuses.PATCH = 409 + + const err = await publish().catch((e) => e) + + expect(err).toBeInstanceOf(SkillNameConflictError) + }) +}) From 2be242d9b2e7e16e305d90189bfb87819ac1817e Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 9 Sep 2026 21:57:29 +0530 Subject: [PATCH 3/6] fix(workspace): scope the published-skill ledger by account and serialise its writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more from the cubic review on #1280. Both are the ledger describing something other than what is actually on the server. **One directory, two accounts, one id.** The ledger keyed on the resolved skill directory alone, so publishing the same skill under a second account overwrote the first account's record. Switching back found a row scoped to the other tenant, treated the skill as unpublished, created it again — and 409'd on the name that was already there, with the original id no longer reachable from this machine. The key now carries tenant and API URL alongside the directory, so each account keeps its own id. Reads still fall back to the old directory-only key, so ids written by an earlier version are not stranded into a needless re-create; the tenant check stays, because that fallback can return another account's row. **Concurrent publishes dropped each other's ids.** Each publish read the whole ledger, mutated its copy and wrote it back, so of two publishes in flight the later write carried the earlier one away, and that skill created again on its next run. Writes go through a promise chain now, and the re-read happens INSIDE the chain — reusing a copy read before the previous write landed would lose it just the same. Same shape `memory-index` already uses for the same reason. Tests: 16 in this file, 2 new, whole altimate suite green (5799 tests). Mutation-checked: keying by directory alone fails one, dropping the chain fails four. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-publish.ts | 54 ++++++++++++++----- .../altimate/workspace/skill-publish.test.ts | 52 ++++++++++++++++++ 2 files changed, 92 insertions(+), 14 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index d9c7dd196..1856ec279 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -226,27 +226,53 @@ async function readLedger(): Promise> { } } -/** Keyed on the resolved skill directory so a rename of the skill's *name* does - * not orphan its id, and two skills in different projects cannot collide. */ +/** Keyed on the resolved skill directory AND the account it was published + * under, so a rename of the skill's *name* does not orphan its id, two skills in + * different projects cannot collide, and — the reason the account is in the key + * — publishing one directory to two accounts keeps an id for each. + * + * A bare directory key held one record, so switching accounts overwrote the + * previous account's id: switching back created a second skill and then 409'd on + * the name that was already there, with no way to reach the original. */ +function ledgerKey(skillDir: string, scope: { tenant: string; apiUrl: string }): string { + return `${scope.tenant}|${scope.apiUrl}|${path.resolve(skillDir)}` +} + +/** Serialises ledger writes, the same way `memory-index` serialises its own. + * Two publishes running at once each read, mutate and write the whole file, so + * the later write dropped the earlier one's id — and that skill's next publish + * created again and 409'd on its own name. */ +let ledgerWriteChain: Promise = Promise.resolve() + async function recordPublished(skillDir: string, record: PublishedRecord): Promise { - const ledger = await readLedger() - ledger[path.resolve(skillDir)] = record - try { - await Filesystem.writeJson(ledgerPath(), ledger) - } catch (err) { - // Best-effort. Losing the id costs a 409 on the next publish, not data. - log.warn("could not record the published skill id", { err: String(err) }) - } + const task = ledgerWriteChain.then(async () => { + try { + // Re-read INSIDE the chain: a copy read before the previous write landed + // would carry that write away again when this one persists. + const ledger = await readLedger() + ledger[ledgerKey(skillDir, { tenant: record.tenant, apiUrl: record.apiUrl })] = record + await Filesystem.writeJson(ledgerPath(), ledger) + } catch (err) { + // Best-effort. Losing the id costs a 409 on the next publish, not data. + log.warn("could not record the published skill id", { err: String(err) }) + } + }) + ledgerWriteChain = task.catch(() => {}) + return task } async function knownPublicId(skillDir: string): Promise { const creds = await AltimateApi.getCredentials().catch(() => null) if (!creds) return null - const record = (await readLedger())[path.resolve(skillDir)] + const scope = { tenant: creds.altimateInstanceName, apiUrl: creds.altimateUrl } + const ledger = await readLedger() + // Composite key first; fall back to the old directory-only key so ids written + // by an earlier version are not stranded into a needless re-create. + const record = ledger[ledgerKey(skillDir, scope)] ?? ledger[path.resolve(skillDir)] if (!record) return null - // Scoped to the account it was published under. The same checkout pointed at a - // different tenant must not update an id that does not exist there. - if (record.tenant !== creds.altimateInstanceName || record.apiUrl !== creds.altimateUrl) return null + // Still checked, not implied by the key: the fallback lookup above can return + // a legacy row belonging to another account. + if (record.tenant !== scope.tenant || record.apiUrl !== scope.apiUrl) return null return record.publicId } diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index 5f18934ff..f5f730591 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -281,3 +281,55 @@ describe("the bundle size guard", () => { expect(err).toBeInstanceOf(SkillNameConflictError) }) }) + +describe("the published-id ledger", () => { + test("keeps a separate id per account for the same skill directory", async () => { + // A bare directory key held ONE record, so publishing to a second account + // overwrote the first account's id. Switching back created a second skill + // and 409'd on the name already there, with no way to reach the original. + await publish() // account "acme" -> pub-1 + expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) + + // Switch accounts, publish the same directory. + ;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => + ({ altimateInstanceName: "other", altimateUrl: "https://api.example.com", altimateApiKey: "k" }) as Creds + requests = [] + await publish() // must CREATE for "other", not update acme's id + expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) + + // Back to the first account: its id must still be there, so this UPDATES. + ;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => + ({ altimateInstanceName: "acme", altimateUrl: "https://api.example.com", altimateApiKey: "k" }) as Creds + requests = [] + const report = await publish() + + expect(report.action).toBe("updated") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + }) + + test("concurrent publishes do not drop each other's id", async () => { + // Each publish read, mutated and wrote the whole ledger, so the later write + // carried the earlier one away and that skill re-created on its next run. + const other = path.join(project, "skills", "second") + mkdirSync(other, { recursive: true }) + writeFileSync(path.join(other, "SKILL.md"), "---\nname: second\n---\n") + + await Promise.all([ + publish(), + publishSkill({ projectDirectory: project, skillDirectory: other, name: "second", description: "d" }), + ]) + + // Both ids survived: neither directory creates again. + requests = [] + const a = await publish() + const b = await publishSkill({ + projectDirectory: project, + skillDirectory: other, + name: "second", + description: "d", + }) + expect(a.action).toBe("updated") + expect(b.action).toBe("updated") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + }) +}) From d3cc5f73bec2470d01e8b8c1769ec33ccdce4427 Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 15 Sep 2026 03:04:03 +0530 Subject: [PATCH 4/6] fix(workspace): attach a published skill to the linked workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a skill and attaching it to a workspace are two calls on the server, and only the first was ever made. A skill that is created but attached to nothing appears in no workspace: the CLI lists workspace skills with `GET /skills?datamate_id=`, and so does the web UI. From the user's side, "publish" had done nothing visible — the exact report from workspaces UAT that this feature exists to close. The binding is resolved BEFORE anything is uploaded, and an unlinked project is refused with a typed `NotLinkedError`. Uploading first and failing to attach would create precisely the orphan being fixed. Attachment goes through `PUT /skills/{id}/datamates`, which REPLACES the whole set. A bare put of one id would silently detach the skill from every other workspace it is already on, so the current set is read from `GET /skills/{id}` (`attached_datamate_ids`) and merged. Already attached: no write. Attached on the update path as well: a skill published before this project was linked to its current workspace was otherwise refreshed but still absent from it. On the create path the attach runs AFTER the id is recorded, so a failed attach is retried by the next publish via the update path rather than creating a second copy and 409ing on the name. `AttachFailedError` carries the id so the caller can say exactly that. `PublishReport` gains `datamateId`, so a caller can name the workspace it went to. There is no caller yet — the command wiring is a follow-up — so no error-mapping changes here. Tests: 21 in the file, 5 new. Mutation-checked: never attaching on create, dropping the merge, uploading while unlinked, and skipping the attach on update each fail a test. Two existing tests needed the project linked under each account they switch to, which is what a real account switch resolves. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-publish.ts | 87 +++++++++++++++- .../altimate/workspace/skill-publish.test.ts | 99 ++++++++++++++++++- 2 files changed, 181 insertions(+), 5 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index 1856ec279..e3c041089 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -39,6 +39,7 @@ import { Global } from "@/global" import { Filesystem } from "@/util/filesystem" import { AltimateApi } from "@/altimate/api/client" import { ConflictError, NotFoundError, altimateRequest } from "./api-client" +import { resolveBinding } from "./state" const log = Log.create({ service: "altimate-workspace-skill-publish" }) @@ -86,6 +87,30 @@ export class BundleTooLargeError extends Error {} * A distinct type rather than re-raising the API's `ConflictError`: that one * carries a structured server detail, and constructing a fake one to hold a * client-authored sentence would misrepresent the envelope. */ +/** The project is not linked to a workspace, so there is nowhere to publish to. + * Raised BEFORE anything is uploaded: publishing first and failing to attach + * would leave a skill on the server attached to nothing — invisible in every + * workspace UI, which is exactly the report that motivated this module. */ +export class NotLinkedError extends Error { + constructor() { + super("This project is not linked to a workspace. Run `altimate-code link` first.") + this.name = "NotLinkedError" + } +} + +/** The skill exists on the server but could not be attached to the workspace. + * Carries the id so the caller can say so precisely: the next publish takes the + * update path and retries the attachment, so nothing is stranded. */ +export class AttachFailedError extends Error { + constructor( + readonly publicId: string, + cause: unknown, + ) { + super(`The skill was uploaded (id ${publicId}) but could not be attached to the workspace: ${String(cause)}`) + this.name = "AttachFailedError" + } +} + export class SkillNameConflictError extends Error { constructor(readonly skillName: string) { super( @@ -103,6 +128,8 @@ export interface PublishReport { name: string files: number bytes: number + /** The workspace the skill is now attached to. */ + datamateId: number } /** Read one directory into a bundle, refusing anything that cannot survive the @@ -276,6 +303,33 @@ async function knownPublicId(skillDir: string): Promise { return record.publicId } +/** Attach a published skill to the workspace this project is bound to. + * + * Creating a skill and attaching it are two calls on the server, and only the + * first was ever made. A skill that is created but attached to nothing does not + * appear in any workspace — the CLI lists workspace skills with + * ``GET /skills?datamate_id=``, and so does the web UI — so from the user's + * side "publish" had done nothing visible. + * + * ``PUT /skills/{id}/datamates`` REPLACES the whole set. A bare put with one id + * would silently detach the skill from every other workspace it was already on, + * so the current set is read first and merged. */ +async function attachToWorkspace(publicId: string, datamateId: number): Promise { + const detail = await altimateRequest<{ attached_datamate_ids?: unknown }>( + "GET", + `/${encodeURIComponent(publicId)}`, + { base: SKILLS_BASE }, + ) + const raw = detail?.attached_datamate_ids + const current = Array.isArray(raw) ? raw.filter((n): n is number => Number.isInteger(n)) : [] + if (current.includes(datamateId)) return + await altimateRequest("PUT", `/${encodeURIComponent(publicId)}/datamates`, { + base: SKILLS_BASE, + body: { datamate_ids: [...current, datamateId] }, + allowEmptyBody: true, + }) +} + /** Publish a skill directory to the workspace, creating it or updating the bundle * already published from this machine. * @@ -291,6 +345,12 @@ export async function publishSkill(input: { if (isManagedSkill(input.projectDirectory, input.skillDirectory)) throw new ManagedSkillError(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 + // prevent. + const binding = await resolveBinding(input.projectDirectory) + if (!binding) throw new NotLinkedError() + const files = await collectBundle(input.skillDirectory) if (files.length === 0) throw new BundleTooLargeError("This skill directory has no files to publish.") const bytes = files.reduce((n, f) => n + Buffer.byteLength(f.content, "utf8"), 0) @@ -303,7 +363,22 @@ export async function publishSkill(input: { body: { name: input.name, description: input.description, files }, allowEmptyBody: true, }) - return { action: "updated", publicId: existing, name: input.name, files: files.length, bytes } + // Attached on update too: a skill published before this project was + // linked to its current workspace is otherwise updated but still absent + // from it. + try { + await attachToWorkspace(existing, binding.datamateId) + } catch (err) { + throw new AttachFailedError(existing, err) + } + return { + action: "updated", + publicId: existing, + name: input.name, + files: files.length, + bytes, + datamateId: binding.datamateId, + } } catch (err) { // The skill was deleted in the workspace since we published it. Falling // through to create is the useful answer; failing would strand the user @@ -342,7 +417,15 @@ export async function publishSkill(input: { tenant: creds.altimateInstanceName, apiUrl: creds.altimateUrl, }) - return { action: "created", publicId, name: input.name, files: files.length, bytes } + // After the id is recorded, deliberately. If the attach fails, the next + // publish finds the id, takes the update path, and attaches again — rather + // than creating a second copy and 409ing on the name. + try { + await attachToWorkspace(publicId, binding.datamateId) + } catch (err) { + throw new AttachFailedError(publicId, err) + } + return { action: "created", publicId, name: input.name, files: files.length, bytes, datamateId: binding.datamateId } } /** Accepts the documented `{public_id}` and a `{skill: {public_id}}` envelope, so diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index f5f730591..594a53256 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -39,11 +39,13 @@ const { AltimateApi } = await import("../../../src/altimate/api/client") const { BinaryFileError, ManagedSkillError, + NotLinkedError, SkillNameConflictError, collectBundle, isManagedSkill, publishSkill, } = await import("../../../src/altimate/workspace/skill-publish") +const { recordApprovedBinding } = await import("../../../src/altimate/workspace/state") type Creds = Awaited> // Saved and restored. Bun runs every test file in one process, so a stub left in @@ -65,13 +67,17 @@ let requests: { method: string; url: string; body: any }[] = [] /** Per-method status. `POST` 409 exercises the name conflict; `PATCH` 404 the * published-then-deleted fallback. */ let statuses: Record = {} +/** What `GET /skills/{id}` reports as the skill's current workspaces. The attach + * endpoint REPLACES the set, so tests that care about merging seed this. */ +let attached: number[] = [] let project = "" let skillDir = "" -beforeEach(() => { +beforeEach(async () => { requests = [] statuses = {} + attached = [] project = mkdtempSync(path.join(SANDBOX, "proj-")) skillDir = path.join(project, "skills", "deploy") mkdirSync(skillDir, { recursive: true }) @@ -93,11 +99,24 @@ beforeEach(() => { status, headers: { "content-type": "application/json" }, }) - return new Response(JSON.stringify({ public_id: "pub-1" }), { + return new Response(JSON.stringify({ public_id: "pub-1", attached_datamate_ids: attached }), { status, headers: { "content-type": "application/json" }, }) }) as typeof fetch + + // Publishing requires a linked project — it fails closed otherwise, because an + // unattached skill is invisible in every workspace. Seeded after the stub is in + // place (the bind kicks off a best-effort skill sync that hits it), and the + // request log is cleared after so assertions see only what publish itself does. + await recordApprovedBinding(project, { + datamateId: 42, + datamateName: "Growth", + repoRemote: null, + projectPath: project, + linkedAt: Date.now(), + } as never) + requests = [] }) afterEach(() => { @@ -290,16 +309,34 @@ describe("the published-id ledger", () => { await publish() // account "acme" -> pub-1 expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) - // Switch accounts, publish the same directory. + // Switch accounts, publish the same directory. The binding cache is scoped + // by tenant, so the project must be linked under the new account too — + // publish now fails closed on an unlinked project, correctly. ;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => ({ altimateInstanceName: "other", altimateUrl: "https://api.example.com", altimateApiKey: "k" }) as Creds + await recordApprovedBinding(project, { + datamateId: 99, + datamateName: "Other", + repoRemote: null, + projectPath: project, + linkedAt: Date.now(), + } as never) requests = [] await publish() // must CREATE for "other", not update acme's id expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) // Back to the first account: its id must still be there, so this UPDATES. + // The binding cache is single-tenant, so the "other" link replaced acme's + // row — re-link, as a real account switch would resolve it again. ;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => ({ altimateInstanceName: "acme", altimateUrl: "https://api.example.com", altimateApiKey: "k" }) as Creds + await recordApprovedBinding(project, { + datamateId: 42, + datamateName: "Growth", + repoRemote: null, + projectPath: project, + linkedAt: Date.now(), + } as never) requests = [] const report = await publish() @@ -333,3 +370,59 @@ describe("the published-id ledger", () => { expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) }) }) + + +describe("attaching to the workspace", () => { + // Creating a skill and attaching it to a workspace are two calls on the + // server, and only the first was ever made. The result was a skill that + // existed but appeared in no workspace — the CLI and the web UI both list + // workspace skills by datamate id — which is the UAT report this closes. + const puts = () => requests.filter((r) => r.method === "PUT" && r.url.includes("/datamates")) + + test("attaches a newly created skill to the bound workspace", async () => { + const report = await publish() + expect(report.action).toBe("created") + expect(report.datamateId).toBe(42) + expect(puts()).toHaveLength(1) + expect(puts()[0].body).toEqual({ datamate_ids: [42] }) + }) + + test("merges with the workspaces the skill is already on, because the endpoint replaces", async () => { + // A bare put of [42] would silently detach the skill from workspace 7. + attached = [7] + await publish() + expect(puts()[0].body).toEqual({ datamate_ids: [7, 42] }) + }) + + test("does not re-attach a skill already on this workspace", async () => { + attached = [42] + await publish() + expect(puts()).toHaveLength(0) + }) + + test("attaches on the update path too", async () => { + // Published before this project was linked here: the update must attach, + // or the skill is refreshed but still absent from the current workspace. + await publish() + requests = [] + const report = await publish() + expect(report.action).toBe("updated") + expect(puts()).toHaveLength(1) + }) + + test("refuses to publish from an unlinked project, before uploading anything", async () => { + 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) + // The property that matters: nothing reached the server. Uploading first + // would create exactly the orphan the attach step exists to prevent. + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + }) +}) From 084c030fd8167cc6e9cb8bbc0d24c0311ffaaff5 Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 15 Sep 2026 03:54:46 +0530 Subject: [PATCH 5/6] fix(workspace): the attach envelope, bounded reads, and ledger identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eleven findings from the bot reviews on #1280. The first is the one that mattered, and the test fixture is how it got through. - `attachToWorkspace` read `attached_datamate_ids` at the top level of `GET /skills/{id}`. The server answers `{skill: {...}}`, so it found nothing, and the replace-set PUT then detached the skill from every workspace it was already on. The test stub served a flat body — a hand-written envelope, not the real one — and passed. The stub now serves the server's shape, and the merge test fails without the unwrap. - Symbolic links inside a skill are refused by name (`SymlinkError`) instead of being silently dropped from the bundle. Local discovery follows links, so the skill worked here and arrived everywhere else incomplete. - The file read is bounded: a stat for the cheap refusal, then a chunked handle read that stops the moment the budget is exceeded, so a file that grew after it was measured cannot be pulled into memory whole. - Uploads get a 120s budget (`timeoutMs` on `altimateRequest`); the shared 15s one covered the request body and could not carry a legal 10MB bundle on an ordinary uplink. - The ledger key uses the skill directory's real path, so one directory reached through a link is one skill rather than a create-then-409; and it carries a digest of the account key, so two users of one tenant do not share an id the server would 403 the second on. - Ledger reads go through the same chain as its writes, the write is atomic (write-then-rename), and a publish holds a per-directory lock, so two publishes of the same skill at once create it once. - An empty skill directory is `EmptyBundleError`, not a size error; `BundleTooLargeError` sets its name like its siblings. Tests: credentials are restored after every test, every bind awaits its detached work, and the update-path attach test re-links the project to a second workspace so it covers the case its comment described. Verified: `test/altimate/workspace` + `test/altimate/plugin` green (500), typecheck clean. Mutation-checked: reverting the envelope unwrap, the symlink refusal, the chunk bound, the stat refusal, the real-path identity, the key digest, the publish lock and the empty-bundle type each fail a test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/api-client.ts | 9 +- .../src/altimate/workspace/skill-publish.ts | 249 ++++++++++++++---- .../altimate/workspace/skill-publish.test.ts | 209 +++++++++++---- 3 files changed, 358 insertions(+), 109 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/api-client.ts b/packages/opencode/src/altimate/workspace/api-client.ts index 0ad47b147..f8a1a80cf 100644 --- a/packages/opencode/src/altimate/workspace/api-client.ts +++ b/packages/opencode/src/altimate/workspace/api-client.ts @@ -153,14 +153,19 @@ async function req( * instead of throwing. Only set for endpoints known to return 204 or a * bare 200 with no payload. */ allowEmptyBody?: boolean + /** Override the shared 15s budget. That budget was sized for small JSON + * exchanges and covers the request body too, so a call that uploads + * megabytes (a skill bundle) needs its own. */ + timeoutMs?: number } = {}, ): Promise { + const timeoutMs = opts.timeoutMs ?? REQUEST_TIMEOUT_MS const { url, instance, apiKey } = await creds() const qs = opts.query ? "?" + new URLSearchParams(opts.query).toString() : "" const basePath = opts.base ?? "/datamate-project-bindings" const target = `${url}${basePath}${subpath}${qs}` const controller = new AbortController() - const timeout = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS) + const timeout = setTimeout(() => controller.abort(), timeoutMs) let res: Response let text: string try { @@ -204,7 +209,7 @@ async function req( const name = (err as { name?: string } | undefined)?.name if (name === "AbortError") { throw new WorkspaceApiError( - `Request to ${target} timed out after ${Math.round(REQUEST_TIMEOUT_MS / 1000)}s`, + `Request to ${target} timed out after ${Math.round(timeoutMs / 1000)}s`, ) } const msg = err instanceof Error ? err.message : String(err) diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index e3c041089..631468852 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -33,6 +33,7 @@ // interpret. import fs from "fs/promises" import path from "path" +import { createHash } from "crypto" import { realpathSync } from "fs" import { Log } from "@/altimate/util/log" import { Global } from "@/global" @@ -53,6 +54,11 @@ const MANAGED_DIR = path.join(".altimate-code", "skill", "_workspace") * usable message, instead of after a long upload. */ const MAX_BUNDLE_BYTES = 10 * 1024 * 1024 const MAX_BUNDLE_FILES = 200 +/** The shared request budget is 15s and covers the upload itself; a legal 10MB + * bundle needs ~5.5 Mbps sustained just to fit inside it. Uploads get their + * own. */ +const UPLOAD_TIMEOUT_MS = 120_000 +const READ_CHUNK_BYTES = 256 * 1024 export interface BundleFile { path: string @@ -79,7 +85,36 @@ export class ManagedSkillError extends Error { } } -export class BundleTooLargeError extends Error {} +export class BundleTooLargeError extends Error { + constructor(message: string) { + super(message) + this.name = "BundleTooLargeError" + } +} + +/** Nothing to publish. Its own type: a caller switching on errors must not file + * "empty" under the size ceiling. */ +export class EmptyBundleError extends Error { + constructor() { + super("This skill directory has no files to publish.") + this.name = "EmptyBundleError" + } +} + +/** A bundle must be self-contained. Local discovery follows links, so a skill + * that reaches its files through one works here — and a publish that silently + * skipped the link would arrive everywhere else with those files missing, the + * same silent-vanish rule 1 exists to prevent. Following the link instead would + * publish whatever it points at, which may be outside the project entirely. */ +export class SymlinkError extends Error { + constructor(readonly filePath: string) { + super( + `"${filePath}" is a symbolic link. Workspace skill bundles must be self-contained — ` + + `copy the target into the skill, or keep it outside.`, + ) + this.name = "SymlinkError" + } +} /** The workspace already has a skill of this name owned by this user, and this * machine has no id for it — so it was published from somewhere else. @@ -143,26 +178,49 @@ export async function collectBundle(dir: string): Promise { const files: BundleFile[] = [] let bytes = 0 + const tooLarge = () => new BundleTooLargeError(`This skill is larger than ${MAX_BUNDLE_BYTES / (1024 * 1024)}MB.`) + const walk = async (current: string): Promise => { const entries = await fs.readdir(current, { withFileTypes: true }) for (const entry of entries) { const full = path.join(current, entry.name) + const relative = path.relative(root, full).split(path.sep).join("/") if (entry.isDirectory()) { await walk(full) continue } + // Named, not skipped. `readdir` reports a link as neither file nor + // directory, and a bare `continue` here dropped it from the bundle with + // nothing said. + if (entry.isSymbolicLink()) throw new SymlinkError(relative) if (!entry.isFile()) continue - const relative = path.relative(root, full).split(path.sep).join("/") - // Size BEFORE read. `readFile` pulls the whole file into memory, so - // checking the running total afterwards let a single oversized file - // through the very guard meant to stop it — the limit was enforced only - // once the damage was done. The cumulative check below stays as the - // answer for many small files, and as a backstop if the file grew - // between this stat and the read. - const stat = await fs.stat(full) - if (bytes + stat.size > MAX_BUNDLE_BYTES) - throw new BundleTooLargeError(`This skill is larger than ${MAX_BUNDLE_BYTES / (1024 * 1024)}MB.`) - const raw = await fs.readFile(full) + // Bounded read. `readFile` pulls the whole file into memory before any + // size check can run, so a single oversized file got through the very + // guard meant to stop it — and a stat beforehand only narrows the + // window, since the file can grow between the stat and the read. The + // stat is kept as the cheap refusal; the read itself goes through a + // handle in chunks and stops the moment the budget is exceeded, so + // what is held in memory never passes the limit by more than a chunk. + const allowed = MAX_BUNDLE_BYTES - bytes + const handle = await fs.open(full, "r") + let raw: Buffer + try { + const stat = await handle.stat() + if (stat.size > allowed) throw tooLarge() + const chunks: Buffer[] = [] + let total = 0 + for (;;) { + const chunk = Buffer.allocUnsafe(READ_CHUNK_BYTES) + const { bytesRead } = await handle.read(chunk, 0, chunk.length, total) + if (bytesRead === 0) break + total += bytesRead + if (total > allowed) throw tooLarge() + chunks.push(chunk.subarray(0, bytesRead)) + } + raw = Buffer.concat(chunks, total) + } finally { + await handle.close() + } let content: string try { content = new TextDecoder("utf-8", { fatal: true }).decode(raw) @@ -173,8 +231,6 @@ export async function collectBundle(dir: string): Promise { files.push({ path: relative, content }) if (files.length > MAX_BUNDLE_FILES) throw new BundleTooLargeError(`This skill has more than ${MAX_BUNDLE_FILES} files.`) - if (bytes > MAX_BUNDLE_BYTES) - throw new BundleTooLargeError(`This skill is larger than ${MAX_BUNDLE_BYTES / (1024 * 1024)}MB.`) } } @@ -253,54 +309,120 @@ async function readLedger(): Promise> { } } -/** Keyed on the resolved skill directory AND the account it was published - * under, so a rename of the skill's *name* does not orphan its id, two skills in - * different projects cannot collide, and — the reason the account is in the key - * — publishing one directory to two accounts keeps an id for each. +/** The account a publish runs under, as the ledger scopes it. The key + * fingerprint is in it because the server scopes skill names per CREATOR: + * two users of one tenant who publish the same directory are two creators, + * and a ledger keyed on tenant alone handed the second user the first user's + * id — a PATCH the server refuses with 403. A digest, never the key itself: + * the ledger is a plain file. */ +interface LedgerScope { + tenant: string + apiUrl: string + keyDigest: string +} + +function keyDigest(apiKey: string): string { + return createHash("sha256").update(apiKey).digest("hex").slice(0, 16) +} + +async function currentScope(): Promise { + const creds = await AltimateApi.getCredentials().catch(() => null) + if (!creds) return null + return { tenant: creds.altimateInstanceName, apiUrl: creds.altimateUrl, keyDigest: keyDigest(creds.altimateApiKey) } +} + +/** The skill directory as the ledger identifies it: its real path. `path.resolve` + * is lexical, so one directory reached through a link — `/tmp` and + * `/private/tmp`, a linked worktree — was two ledger keys, and the second + * publish created again and 409'd on its own name. Same fallback + * `isManagedSkill` uses: a path that does not exist cannot be a link. */ +function skillIdentity(skillDir: string): string { + try { + return realpathSync(skillDir) + } catch { + return path.resolve(skillDir) + } +} + +/** Keyed on the skill directory AND the account it was published under, so a + * rename of the skill's *name* does not orphan its id, two skills in different + * projects cannot collide, and — the reason the account is in the key — + * publishing one directory to two accounts keeps an id for each. * * A bare directory key held one record, so switching accounts overwrote the * previous account's id: switching back created a second skill and then 409'd on * the name that was already there, with no way to reach the original. */ -function ledgerKey(skillDir: string, scope: { tenant: string; apiUrl: string }): string { - return `${scope.tenant}|${scope.apiUrl}|${path.resolve(skillDir)}` +function ledgerKey(skillDir: string, scope: LedgerScope): string { + return `${scope.tenant}|${scope.apiUrl}|${scope.keyDigest}|${skillIdentity(skillDir)}` } -/** Serialises ledger writes, the same way `memory-index` serialises its own. +/** Serialises ledger access, the same way `memory-index` serialises its own. * Two publishes running at once each read, mutate and write the whole file, so * the later write dropped the earlier one's id — and that skill's next publish - * created again and 409'd on its own name. */ -let ledgerWriteChain: Promise = Promise.resolve() + * created again and 409'd on its own name. Reads go through it too: a read that + * overlapped a queued write saw a ledger without the id it was about to hold. + * + * In-process only. Two `altimate` processes publishing at once still race the + * file; the atomic write below keeps that from corrupting it, and the cost of + * losing is one id — a 409 on that skill's next publish, not data. */ +let ledgerChain: Promise = Promise.resolve() + +function withLedger(task: () => Promise): Promise { + const run = ledgerChain.then(task) + ledgerChain = run.catch(() => {}) + return run +} -async function recordPublished(skillDir: string, record: PublishedRecord): Promise { - const task = ledgerWriteChain.then(async () => { +async function recordPublished(skillDir: string, scope: LedgerScope, record: PublishedRecord): Promise { + return withLedger(async () => { try { // Re-read INSIDE the chain: a copy read before the previous write landed // would carry that write away again when this one persists. const ledger = await readLedger() - ledger[ledgerKey(skillDir, { tenant: record.tenant, apiUrl: record.apiUrl })] = record - await Filesystem.writeJson(ledgerPath(), ledger) + ledger[ledgerKey(skillDir, scope)] = record + // Atomic (write-then-rename), so a process killed mid-write leaves the + // previous ledger rather than a truncated one — which `readLedger` + // would read as empty, dropping EVERY skill's id at once. + Filesystem.writeJsonAtomic(ledgerPath(), ledger) } catch (err) { // Best-effort. Losing the id costs a 409 on the next publish, not data. log.warn("could not record the published skill id", { err: String(err) }) } }) - ledgerWriteChain = task.catch(() => {}) - return task } -async function knownPublicId(skillDir: string): Promise { - const creds = await AltimateApi.getCredentials().catch(() => null) - if (!creds) return null - const scope = { tenant: creds.altimateInstanceName, apiUrl: creds.altimateUrl } - const ledger = await readLedger() - // Composite key first; fall back to the old directory-only key so ids written - // by an earlier version are not stranded into a needless re-create. - const record = ledger[ledgerKey(skillDir, scope)] ?? ledger[path.resolve(skillDir)] - if (!record) return null - // Still checked, not implied by the key: the fallback lookup above can return - // a legacy row belonging to another account. - if (record.tenant !== scope.tenant || record.apiUrl !== scope.apiUrl) return null - return record.publicId +async function knownPublicId(skillDir: string, scope: LedgerScope): Promise { + return withLedger(async () => { + const ledger = await readLedger() + // Composite key first; fall back to the pre-fingerprint composite key and + // then to the directory-only key, so ids written by earlier versions are + // not stranded into a needless re-create. + const record = + ledger[ledgerKey(skillDir, scope)] ?? + ledger[`${scope.tenant}|${scope.apiUrl}|${path.resolve(skillDir)}`] ?? + ledger[path.resolve(skillDir)] + if (!record) return null + // Still checked, not implied by the key: the fallback lookups above can + // return a legacy row belonging to another account. + if (record.tenant !== scope.tenant || record.apiUrl !== scope.apiUrl) return null + return record.publicId + }) +} + +/** One publish per skill directory at a time. The ledger chain serialises the + * bookkeeping, but two publishes of the SAME directory overlapping their + * lookup-and-create both found no id, both POSTed, and the loser was told the + * skill "was published from somewhere else" — by this machine, seconds ago. */ +const publishChains = new Map>() + +function withPublishLock(skillDir: string, task: () => Promise): Promise { + const key = skillIdentity(skillDir) + const run = (publishChains.get(key) ?? Promise.resolve()).then(task) + const settled = run.catch(() => {}).then(() => { + if (publishChains.get(key) === settled) publishChains.delete(key) + }) + publishChains.set(key, settled) + return run } /** Attach a published skill to the workspace this project is bound to. @@ -315,12 +437,15 @@ async function knownPublicId(skillDir: string): Promise { * would silently detach the skill from every other workspace it was already on, * so the current set is read first and merged. */ async function attachToWorkspace(publicId: string, datamateId: number): Promise { - const detail = await altimateRequest<{ attached_datamate_ids?: unknown }>( - "GET", - `/${encodeURIComponent(publicId)}`, - { base: SKILLS_BASE }, - ) - const raw = detail?.attached_datamate_ids + type Attached = { attached_datamate_ids?: unknown } + const detail = await altimateRequest("GET", `/${encodeURIComponent(publicId)}`, { + base: SKILLS_BASE, + }) + // The server answers `{skill: {...}}` (`CustomSkillResponse`); a flat body + // is tolerated the way `extractPublicId` tolerates both. Reading only the + // top level found nothing, and the replace below then detached the skill + // from every workspace it was already on. + const raw = detail?.skill?.attached_datamate_ids ?? detail?.attached_datamate_ids const current = Array.isArray(raw) ? raw.filter((n): n is number => Number.isInteger(n)) : [] if (current.includes(datamateId)) return await altimateRequest("PUT", `/${encodeURIComponent(publicId)}/datamates`, { @@ -341,6 +466,15 @@ export async function publishSkill(input: { skillDirectory: string name: string description: string +}): Promise { + return withPublishLock(input.skillDirectory, () => publishSkillUnlocked(input)) +} + +async function publishSkillUnlocked(input: { + projectDirectory: string + skillDirectory: string + name: string + description: string }): Promise { if (isManagedSkill(input.projectDirectory, input.skillDirectory)) throw new ManagedSkillError(input.skillDirectory) @@ -352,16 +486,23 @@ export async function publishSkill(input: { if (!binding) throw new NotLinkedError() const files = await collectBundle(input.skillDirectory) - if (files.length === 0) throw new BundleTooLargeError("This skill directory has no files to publish.") + if (files.length === 0) throw new EmptyBundleError() const bytes = files.reduce((n, f) => n + Buffer.byteLength(f.content, "utf8"), 0) - const existing = await knownPublicId(input.skillDirectory) + // Resolved once and pinned. The ledger lookup and the record after the + // upload must describe the same account, or a credential change mid-publish + // files the id under one and looks for it under the other. + const scope = await currentScope() + if (!scope) throw new NotLinkedError() + + const existing = await knownPublicId(input.skillDirectory, scope) if (existing) { try { await altimateRequest("PATCH", `/${encodeURIComponent(existing)}`, { base: SKILLS_BASE, body: { name: input.name, description: input.description, files }, allowEmptyBody: true, + timeoutMs: UPLOAD_TIMEOUT_MS, }) // Attached on update too: a skill published before this project was // linked to its current workspace is otherwise updated but still absent @@ -400,6 +541,7 @@ export async function publishSkill(input: { created = await altimateRequest("POST", "", { base: SKILLS_BASE, body: { name: input.name, description: input.description, files }, + timeoutMs: UPLOAD_TIMEOUT_MS, }) } catch (err) { // Names are unique per creator server-side. Reached when the same skill was @@ -411,12 +553,7 @@ export async function publishSkill(input: { const publicId = extractPublicId(created) if (!publicId) throw new Error("The workspace accepted the skill but did not return an id for it.") - const creds = await AltimateApi.getCredentials() - await recordPublished(input.skillDirectory, { - publicId, - tenant: creds.altimateInstanceName, - apiUrl: creds.altimateUrl, - }) + await recordPublished(input.skillDirectory, scope, { publicId, tenant: scope.tenant, apiUrl: scope.apiUrl }) // After the id is recorded, deliberately. If the attach fails, the next // publish finds the id, takes the update path, and attaches again — rather // than creating a second copy and 409ing on the name. diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index 594a53256..9769f5661 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -38,9 +38,11 @@ afterAll(() => { const { AltimateApi } = await import("../../../src/altimate/api/client") const { BinaryFileError, + EmptyBundleError, ManagedSkillError, NotLinkedError, SkillNameConflictError, + SymlinkError, collectBundle, isManagedSkill, publishSkill, @@ -53,9 +55,12 @@ type Creds = Awaited> // unrelated workspace tests failed until this was put back. const originalIsConfigured = AltimateApi.isConfigured const originalGetCreds = AltimateApi.getCredentials +const stubCreds = (over: Partial = {}) => { + ;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => + ({ altimateInstanceName: "acme", altimateUrl: "https://api.example.com", altimateApiKey: "k", ...over }) as Creds +} ;(AltimateApi as unknown as { isConfigured: () => Promise }).isConfigured = async () => true -;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => - ({ altimateInstanceName: "acme", altimateUrl: "https://api.example.com", altimateApiKey: "k" }) as Creds +stubCreds() afterAll(() => { ;(AltimateApi as unknown as { isConfigured: typeof originalIsConfigured }).isConfigured = originalIsConfigured @@ -99,30 +104,43 @@ beforeEach(async () => { status, headers: { "content-type": "application/json" }, }) - return new Response(JSON.stringify({ public_id: "pub-1", attached_datamate_ids: attached }), { - status, - headers: { "content-type": "application/json" }, - }) + // The server's real envelope: skill reads and writes answer + // `{skill: {...}}` (`CustomSkillResponse`); the set-workspaces endpoint + // answers flat. A flat stub for the detail read hid a real defect — the + // attachment list was read at the top level, found nothing, and the + // replace detached the skill from every other workspace. + const body_ = + method === "PUT" + ? { public_id: "pub-1", attached_datamate_ids: attached } + : { skill: { public_id: "pub-1", attached_datamate_ids: attached } } + return new Response(JSON.stringify(body_), { status, headers: { "content-type": "application/json" } }) }) as typeof fetch // Publishing requires a linked project — it fails closed otherwise, because an // unattached skill is invisible in every workspace. Seeded after the stub is in // place (the bind kicks off a best-effort skill sync that hits it), and the // request log is cleared after so assertions see only what publish itself does. - await recordApprovedBinding(project, { - datamateId: 42, - datamateName: "Growth", - repoRemote: null, - projectPath: project, - linkedAt: Date.now(), - } as never) + await link(42, "Growth") requests = [] }) afterEach(() => { globalThis.fetch = originalFetch + // A test that switched accounts must not leave the next one there. + stubCreds() }) +/** Link the sandbox project. Awaited through the bind's detached work, so its + * skill sync and backfill land inside this test's stubbed `fetch` and request + * log rather than straddling into the next test's. */ +async function link(datamateId: number, datamateName: string, dir = project) { + await recordApprovedBinding( + dir, + { datamateId, datamateName, repoRemote: null, projectPath: dir, linkedAt: Date.now() } as never, + { awaitBackfill: true }, + ) +} + const publish = () => publishSkill({ projectDirectory: project, skillDirectory: skillDir, name: "deploy", description: "d" }) @@ -155,6 +173,21 @@ describe("collectBundle", () => { expect(files.map((f) => f.path).sort()).toEqual(["SKILL.md", "references/api.md"]) }) + + test("names a symbolic link rather than silently leaving it out", async () => { + // `readdir` reports a link as neither file nor directory, and the walk + // skipped it with nothing said. Local discovery follows links, so the + // skill worked here and arrived everywhere else missing the linked files. + const shared = path.join(project, "shared") + mkdirSync(shared, { recursive: true }) + writeFileSync(path.join(shared, "api.md"), "docs") + symlinkSync(shared, path.join(skillDir, "references")) + + const err = await collectBundle(skillDir).catch((e) => e) + + expect(err).toBeInstanceOf(SymlinkError) + expect(String(err)).toContain("references") + }) }) describe("isManagedSkill", () => { @@ -227,6 +260,13 @@ describe("publishSkill", () => { expect(String(err)).toContain("published") }) + test("an empty skill directory is its own error, not a size problem", async () => { + rmSync(path.join(skillDir, "SKILL.md")) + const err = await publish().catch((e) => e) + expect(err).toBeInstanceOf(EmptyBundleError) + expect(requests).toHaveLength(0) + }) + test("re-creates a skill that has been deleted in the workspace since we published it", async () => { // Otherwise the user is stranded: a local id they cannot see, update or clear. await publish() @@ -241,36 +281,68 @@ describe("publishSkill", () => { }) describe("the bundle size guard", () => { + const sparse = (file: string, size: number) => { + const fd = openSync(file, "w") + try { + ftruncateSync(fd, size) // sparse: cheap on disk, and zeros decode as UTF-8 + } finally { + closeSync(fd) + } + } + test("refuses an oversized file WITHOUT reading it into memory", async () => { - // The guard checked the running total after `readFile`, so a single huge + // The guard checked the running total after the read, so a single huge // file was fully loaded before being rejected — the limit enforced only - // once the memory had already been spent. Asserting on the rejection alone - // does not test that: the post-read check rejects too, and the mutation - // survived. The property is that `readFile` is never called for the file. + // once the memory had already been spent. The property is that the file is + // never read at all. const dir = path.join(SANDBOX, `oversize-${Math.random().toString(36).slice(2)}`) mkdirSync(dir, { recursive: true }) writeFileSync(path.join(dir, "SKILL.md"), "---\nname: big\n---\n") - const huge = path.join(dir, "huge.txt") - const fd = openSync(huge, "w") + sparse(path.join(dir, "huge.txt"), 64 * 1024 * 1024) + + const reads: number[] = [] + const originalOpen = fsp.open + ;(fsp as unknown as { open: unknown }).open = (async (...args: unknown[]) => { + const handle = await (originalOpen as (...a: unknown[]) => Promise)(...args) + const read = handle.read.bind(handle) + ;(handle as unknown as { read: unknown }).read = (buffer: Buffer, ...rest: unknown[]) => { + reads.push(buffer.length) + return (read as (...a: unknown[]) => unknown)(buffer, ...rest) + } + return handle + }) as unknown as typeof fsp.open + try { - ftruncateSync(fd, 64 * 1024 * 1024) // sparse: past the limit, cheap on disk + await expect(collectBundle(dir)).rejects.toThrow(/larger than/i) } finally { - closeSync(fd) + ;(fsp as unknown as { open: unknown }).open = originalOpen } + // Refused on the measurement, before a single read. + expect(reads).toHaveLength(0) + }) + + test("a file that grew after it was measured is still refused", async () => { + // A stat before the read only narrows the window: a file can grow between + // the two, and a read sized by the stat then pulled the whole new file in + // before any check ran. The read is chunked and stops the moment the + // budget is exceeded, whatever the file measured. + const dir = path.join(SANDBOX, `grew-${Math.random().toString(36).slice(2)}`) + mkdirSync(dir, { recursive: true }) + sparse(path.join(dir, "grew.txt"), 10 * 1024 * 1024 + 1) - const read: string[] = [] - const originalReadFile = fsp.readFile - ;(fsp as unknown as { readFile: unknown }).readFile = ((...args: unknown[]) => { - read.push(String(args[0])) - return (originalReadFile as (...a: unknown[]) => unknown)(...args) - }) as unknown as typeof fsp.readFile + const originalOpen = fsp.open + ;(fsp as unknown as { open: unknown }).open = (async (...args: unknown[]) => { + const handle = await (originalOpen as (...a: unknown[]) => Promise)(...args) + // The measurement lies: the file "was" tiny when stat'd. + ;(handle as unknown as { stat: unknown }).stat = async () => ({ size: 10 }) + return handle + }) as unknown as typeof fsp.open try { await expect(collectBundle(dir)).rejects.toThrow(/larger than/i) } finally { - ;(fsp as unknown as { readFile: unknown }).readFile = originalReadFile + ;(fsp as unknown as { open: unknown }).open = originalOpen } - expect(read.some((r) => r.endsWith("huge.txt"))).toBe(false) }) test("a symlinked skill directory into the managed snapshot is still managed", async () => { @@ -312,15 +384,8 @@ describe("the published-id ledger", () => { // Switch accounts, publish the same directory. The binding cache is scoped // by tenant, so the project must be linked under the new account too — // publish now fails closed on an unlinked project, correctly. - ;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => - ({ altimateInstanceName: "other", altimateUrl: "https://api.example.com", altimateApiKey: "k" }) as Creds - await recordApprovedBinding(project, { - datamateId: 99, - datamateName: "Other", - repoRemote: null, - projectPath: project, - linkedAt: Date.now(), - } as never) + stubCreds({ altimateInstanceName: "other" }) + await link(99, "Other") requests = [] await publish() // must CREATE for "other", not update acme's id expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) @@ -328,15 +393,8 @@ describe("the published-id ledger", () => { // Back to the first account: its id must still be there, so this UPDATES. // The binding cache is single-tenant, so the "other" link replaced acme's // row — re-link, as a real account switch would resolve it again. - ;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => - ({ altimateInstanceName: "acme", altimateUrl: "https://api.example.com", altimateApiKey: "k" }) as Creds - await recordApprovedBinding(project, { - datamateId: 42, - datamateName: "Growth", - repoRemote: null, - projectPath: project, - linkedAt: Date.now(), - } as never) + stubCreds() + await link(42, "Growth") requests = [] const report = await publish() @@ -344,6 +402,47 @@ describe("the published-id ledger", () => { expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) }) + test("keeps a separate id per user of the same tenant", async () => { + // Skill names are unique per CREATOR server-side. Two users of one tenant + // publishing the same directory are two creators; a ledger keyed on the + // tenant handed the second user the first user's id, and the PATCH came + // back 403. The account's key is in the scope as a digest. + await publish() // user "k" -> pub-1 + stubCreds({ altimateApiKey: "someone-else" }) + requests = [] + + const report = await publish() + + expect(report.action).toBe("created") + expect(requests.filter((r) => r.method === "PATCH")).toHaveLength(0) + }) + + test("one directory reached by two paths is one skill", async () => { + // `path.resolve` is lexical: the same checkout through a link (`/tmp` and + // `/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" }) + requests = [] + + const report = await publish() + + expect(report.action).toBe("updated") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + }) + + test("two publishes of the same directory at once create it once", async () => { + // Serialising the ledger was not enough: both looked up before either + // recorded, both POSTed, and the loser got a name conflict for a skill + // this machine had just created. + const [a, b] = await Promise.all([publish(), publish()]) + + expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) + expect([a.action, b.action].sort()).toEqual(["created", "updated"]) + }) + test("concurrent publishes do not drop each other's id", async () => { // Each publish read, mutated and wrote the whole ledger, so the later write // carried the earlier one away and that skill re-created on its next run. @@ -400,14 +499,22 @@ describe("attaching to the workspace", () => { expect(puts()).toHaveLength(0) }) - test("attaches on the update path too", async () => { - // Published before this project was linked here: the update must attach, - // or the skill is refreshed but still absent from the current workspace. - await publish() + test("attaches on the update path too, keeping the workspace it was on", async () => { + // Published while the project was linked to one workspace, then the + // project is re-linked to another: the update must attach to the new one, + // or the skill is refreshed but still absent from it — and must not drop + // the first, which the replace semantics would do with a bare put. + await publish() // attached to 42 + attached = [42] + await link(77, "Platform") requests = [] + const report = await publish() + expect(report.action).toBe("updated") + expect(report.datamateId).toBe(77) expect(puts()).toHaveLength(1) + expect(puts()[0].body).toEqual({ datamate_ids: [42, 77] }) }) test("refuses to publish from an unlinked project, before uploading anything", async () => { From d4120918dae756750cede21ef9283571f2d24a09 Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 15 Sep 2026 04:19:04 +0530 Subject: [PATCH 6/6] fix(workspace): a recorded id the server says is not ours falls through to create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The legacy ledger keys predate creator scoping, so on a shared machine a row another user of the same tenant wrote can be found; the server answers the PATCH with 403. That skill is theirs — publish creates our own under the scoped key, the same way a 404 does. The size-guard test now asserts that the oversized file specifically was never read, rather than that nothing was, since directory order is the filesystem's. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-publish.ts | 19 +++++++++--- .../altimate/workspace/skill-publish.test.ts | 31 ++++++++++++++----- 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-publish.ts b/packages/opencode/src/altimate/workspace/skill-publish.ts index 631468852..7b7950425 100644 --- a/packages/opencode/src/altimate/workspace/skill-publish.ts +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -39,7 +39,7 @@ import { Log } from "@/altimate/util/log" import { Global } from "@/global" import { Filesystem } from "@/util/filesystem" import { AltimateApi } from "@/altimate/api/client" -import { ConflictError, NotFoundError, altimateRequest } from "./api-client" +import { ConflictError, ForbiddenError, NotFoundError, altimateRequest } from "./api-client" import { resolveBinding } from "./state" const log = Log.create({ service: "altimate-workspace-skill-publish" }) @@ -529,10 +529,19 @@ async function publishSkillUnlocked(input: { // thing the typed errors in this module exist to prevent — and only on // the update path, so the create path looked correct in isolation. if (err instanceof ConflictError) throw new SkillNameConflictError(input.name) - if (!(err instanceof NotFoundError)) throw err - log.info("published skill no longer exists in the workspace; creating it again", { - publicId: existing, - }) + // 403: the id is someone else's. Reachable through the legacy ledger + // keys, which predate creator scoping — on a shared machine a row + // written by another user of the same tenant is found and the server + // refuses the update. Their skill is not ours to touch; create our own, + // which records under the scoped key and never consults the legacy one + // again. + if (err instanceof ForbiddenError) { + log.info("published skill belongs to another user; creating our own", { publicId: existing }) + } else if (err instanceof NotFoundError) { + log.info("published skill no longer exists in the workspace; creating it again", { + publicId: existing, + }) + } else throw err } } diff --git a/packages/opencode/test/altimate/workspace/skill-publish.test.ts b/packages/opencode/test/altimate/workspace/skill-publish.test.ts index 9769f5661..46857a155 100644 --- a/packages/opencode/test/altimate/workspace/skill-publish.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -267,6 +267,21 @@ describe("publishSkill", () => { expect(requests).toHaveLength(0) }) + test("creates its own skill when the recorded id belongs to someone else", async () => { + // The legacy ledger keys predate creator scoping, so on a shared machine + // a row another user of the same tenant wrote can be found. The server + // answers the PATCH with 403; that skill is theirs, and publishing must + // not fail on it. + await publish() + requests = [] + statuses.PATCH = 403 + + const report = await publish() + + expect(report.action).toBe("created") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) + }) + test("re-creates a skill that has been deleted in the workspace since we published it", async () => { // Otherwise the user is stranded: a local id they cannot see, update or clear. await publish() @@ -300,14 +315,14 @@ describe("the bundle size guard", () => { writeFileSync(path.join(dir, "SKILL.md"), "---\nname: big\n---\n") sparse(path.join(dir, "huge.txt"), 64 * 1024 * 1024) - const reads: number[] = [] + const read: string[] = [] const originalOpen = fsp.open ;(fsp as unknown as { open: unknown }).open = (async (...args: unknown[]) => { const handle = await (originalOpen as (...a: unknown[]) => Promise)(...args) - const read = handle.read.bind(handle) - ;(handle as unknown as { read: unknown }).read = (buffer: Buffer, ...rest: unknown[]) => { - reads.push(buffer.length) - return (read as (...a: unknown[]) => unknown)(buffer, ...rest) + const inner = handle.read.bind(handle) + ;(handle as unknown as { read: unknown }).read = (...rest: unknown[]) => { + read.push(String(args[0])) + return (inner as (...a: unknown[]) => unknown)(...rest) } return handle }) as unknown as typeof fsp.open @@ -317,8 +332,10 @@ describe("the bundle size guard", () => { } finally { ;(fsp as unknown as { open: unknown }).open = originalOpen } - // Refused on the measurement, before a single read. - expect(reads).toHaveLength(0) + // The oversized file specifically: refused on its measurement, before a + // single read. `SKILL.md` may well have been read first — directory order + // is the filesystem's. + expect(read.some((p) => p.endsWith("huge.txt"))).toBe(false) }) test("a file that grew after it was measured is still refused", async () => {