diff --git a/packages/opencode/src/altimate/workspace/api-client.ts b/packages/opencode/src/altimate/workspace/api-client.ts index 0ad47b147c..61790a58d5 100644 --- a/packages/opencode/src/altimate/workspace/api-client.ts +++ b/packages/opencode/src/altimate/workspace/api-client.ts @@ -21,6 +21,11 @@ export interface DatamateRef { * toggle in the workspace app, so callers that write memory must respect it. * Undefined when the backend omitted the field. */ memoryEnabled?: boolean + /** The user who owns the workspace. Linking needs only visibility, but + * attaching a skill is a write against the workspace and needs ownership — + * a caller that can see a colleague's shared workspace may link to it and + * still not publish into it. Undefined when the backend omitted the field. */ + ownerId?: number } export interface Binding { @@ -153,14 +158,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 +214,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) @@ -426,7 +436,7 @@ export namespace WorkspaceApi { // bare ``[...]``, and a generic ``{data: [...]}`` — so a backend // contract change (or compat layer) doesn't silently empty the picker. // (cubic-dev-ai round 3.) - type Row = { id: number | string; name: string; memory_enabled?: boolean } + type Row = { id: number | string; name: string; memory_enabled?: boolean; user_id?: number } const body = await req("GET", "/", { base: "/datamates", }) @@ -454,7 +464,22 @@ export namespace WorkspaceApi { // per-element rather than per-envelope malformed value. return rows .filter((d): d is Row => d !== null && typeof d === "object") - .map((d) => ({ id: Number(d.id), name: d.name, memoryEnabled: d.memory_enabled })) + .map((d) => ({ + id: Number(d.id), + name: d.name, + memoryEnabled: d.memory_enabled, + ownerId: Number.isInteger(d.user_id) ? d.user_id : undefined, + })) .filter((d) => Number.isInteger(d.id) && d.id > 0 && typeof d.name === "string") } + + /** The caller's own user id, from ``GET /users/me``. Needed wherever the + * client must compare ownership — skill attachment requires the caller to + * OWN the workspace, and the credentials carry no user id of their own. */ + export async function whoami(): Promise { + const me = await req<{ id?: unknown }>("GET", "/me", { base: "/users" }) + const id = Number(me?.id) + if (!Number.isInteger(id) || id <= 0) throw new WorkspaceApiError("The server did not say who this account is.") + return id + } } 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 0000000000..b31b420b6a --- /dev/null +++ b/packages/opencode/src/altimate/workspace/skill-publish.ts @@ -0,0 +1,775 @@ +// 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 { realpathSync } from "fs" +import { Log } from "@/altimate/util/log" +import { Global } from "@/global" +import { Filesystem } from "@/util/filesystem" +import { AltimateApi } from "@/altimate/api/client" +import { ConflictError, ForbiddenError, NotFoundError, WorkspaceApi, altimateRequest } from "./api-client" +import { resolveBinding } from "./state" + +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. `MAX_BUNDLE_FILES` and + * `MAX_BUNDLE_BYTES` in `app/service/custom_skills/bundle.py`; a mismatch + * here means a bundle that passes locally, uploads in full, and is refused + * with a 400 — which is the case these exist to prevent. */ +const MAX_BUNDLE_BYTES = 10 * 1024 * 1024 +const MAX_BUNDLE_FILES = 100 + +/** Never published, whatever is in the directory. + * + * A skill directory is a folder the user works in, so it accumulates things + * that are not the skill: an editor's swap file, macOS's `.DS_Store`, a `.git` + * from a skill installed by clone — and `.env` / `.envrc`, which are the ones + * that matter. Publishing is a share: a public skill's bundle is readable + * tenant-wide, and a secret that reaches it cannot be recalled by deleting the + * local file. Skipped silently, where a named refusal would be noise about + * files the user did not mean to publish either. + * + * A blocklist, so incomplete by construction: it catches the common shapes, + * not every file that could hold a secret. A `credentials.json` ships. */ +const NEVER_PUBLISH_DIRS = new Set([".git", "node_modules", "__pycache__"]) +function isJunkFile(name: string): boolean { + // Case-folded: Windows and macOS file systems are case-insensitive by + // default, so `.ENV` is the same file as `.env` there and must not slip + // past a case-sensitive match. + const lower = name.toLowerCase() + return ( + // A worktree's `.git` is a regular FILE pointing at the main repository, + // not a directory — so the directory skip alone did not cover it. + lower === ".git" || + lower === ".ds_store" || + lower === "thumbs.db" || + lower === ".env" || + lower === ".envrc" || + lower.startsWith(".env.") || + lower.endsWith("~") || + lower.endsWith(".swp") || + lower.endsWith(".swo") + ) +} +/** 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 + 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 { + 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. + * + * 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 project is linked to a workspace the caller does not own. Linking + * needs only visibility — a colleague's shared workspace can be linked to — + * but attaching a skill is a write against the workspace and needs + * ownership. Raised BEFORE anything is uploaded, for the same reason as + * `NotLinkedError`: a skill created and then refused attachment is the + * orphan this module exists to prevent, and nothing from the CLI would ever + * attach it. */ +export class NotWorkspaceOwnerError extends Error { + constructor(readonly workspaceName: string) { + super( + `This project is linked to "${workspaceName}", which belongs to someone else. ` + + `Skills can only be published to a workspace you own — link this project to one of yours, ` + + `or ask the owner to publish it.`, + ) + this.name = "NotWorkspaceOwnerError" + } +} + +/** 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" + } +} + +/** The skill changed in the workspace between this publish reading it and + * writing it — someone edited it in the web UI mid-upload. The server's + * compare-and-swap refused, nothing was written, and publishing again picks up + * their version. A distinct type because the advice is "try again", where a + * name conflict's is "rename". */ +export class SkillChangedElsewhereError extends Error { + constructor(readonly skillName: string) { + super( + `"${skillName}" was edited in the workspace while this publish was uploading, ` + + `so nothing was changed. Publish again to apply your version on top of theirs.`, + ) + this.name = "SkillChangedElsewhereError" + } +} + +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 + /** The workspace the skill is now attached to. */ + datamateId: 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 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()) { + if (NEVER_PUBLISH_DIRS.has(entry.name)) continue + await walk(full) + continue + } + if (isJunkFile(entry.name)) 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 + // 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. + // Before the read, like the byte ceiling: file 201 was read and decoded + // in full before being rejected. + if (files.length >= MAX_BUNDLE_FILES) + throw new BundleTooLargeError(`This skill has more than ${MAX_BUNDLE_FILES} files.`) + 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) + } catch { + throw new BinaryFileError(relative) + } + bytes += raw.byteLength + files.push({ path: relative, content }) + } + } + + 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 { + // `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) +} + +// --------------------------------------------------------------------------- +// 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 + /** The server's `created_by` for the skill. Present on rows written by + * this version; absent on legacy rows, which are re-homed on first read. */ + createdBy?: number +} + +function ledgerPath(): string { + return path.join(Global.Path.state, "altimate-published-skills.json") +} + +/** Test seam: where the ledger lives, so a test can seed a legacy row without + * guessing the state directory. */ +export const ledgerPathForTests = ledgerPath + +/** 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 + if (typeof r.publicId !== "string" || typeof r.tenant !== "string" || typeof r.apiUrl !== "string") return false + return r.createdBy === undefined || typeof r.createdBy === "number" +} + +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 {} + } +} + +/** The account a publish runs under, as the ledger scopes it. The USER 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. + * + * The user id, not a digest of the API key, which is what an earlier version + * used. A key is rotated; the user is not. Under the digest a rotation made + * every published id on this machine unreachable — the next publish created + * again, the server answered 409 on the name, and the user was told the + * skill "was published from somewhere else". */ +interface LedgerScope { + tenant: string + apiUrl: string + userId: number +} + +async function currentScope(): Promise { + const creds = await AltimateApi.getCredentials().catch(() => null) + if (!creds) return null + const userId = await WorkspaceApi.whoami() + return { tenant: creds.altimateInstanceName, apiUrl: creds.altimateUrl, userId } +} + +/** 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: LedgerScope): string { + return `${scope.tenant}|${scope.apiUrl}|u${scope.userId}|${skillIdentity(skillDir)}` +} + +/** 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. 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, 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, 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) }) + } + }) +} + +async function knownPublicId(skillDir: string, scope: LedgerScope): Promise { + const record = await withLedger(async () => { + const ledger = await readLedger() + // Current key first. Then ANY row for this directory under this account, + // whatever key shape an earlier version wrote it with. The digest shape + // in particular cannot be looked up by recomputing it: after a rotation + // the digest on disk is of a key nobody has any more — which is the + // whole case. So the fallback scans by directory and lets the server + // decide whose skill it is, below. + const exact = ledger[ledgerKey(skillDir, scope)] + if (exact) return exact + const real = skillIdentity(skillDir) + const lexical = path.resolve(skillDir) + const prefix = `${scope.tenant}|${scope.apiUrl}|` + for (const [key, row] of Object.entries(ledger)) { + const dir = key.startsWith(prefix) ? key.slice(key.lastIndexOf("|") + 1) : key + if (dir === real || dir === lexical) return row + } + return null + }) + if (!record) return null + // Still checked, not implied by the key: a legacy row can belong to + // another account. + if (record.tenant !== scope.tenant || record.apiUrl !== scope.apiUrl) return null + // A legacy row carries no creator. It is trusted only once the server says + // the skill is this user's — one GET, and the row is re-homed under the + // current key so the question is not asked again. Someone else's, or + // gone: not ours, and the create path takes over. + if (record.createdBy === undefined) { + const owner = await skillOwner(record.publicId) + if (owner !== scope.userId) return null + await recordPublished(skillDir, scope, { ...record, createdBy: owner }) + } else if (record.createdBy !== scope.userId) return null + return record.publicId +} + +/** `created_by` from the skill's detail, or null when the skill is gone. */ +async function skillOwner(publicId: string): Promise { + try { + const detail = await altimateRequest<{ created_by?: unknown; skill?: { created_by?: unknown } }>( + "GET", + `/${encodeURIComponent(publicId)}`, + { base: SKILLS_BASE }, + ) + const raw = detail?.skill?.created_by ?? detail?.created_by + return typeof raw === "number" ? raw : null + } catch (err) { + if (err instanceof NotFoundError) return null + throw err + } +} + +/** 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. + * + * 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 { + 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`, { + 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. + * + * `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 { + 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) + + // 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() + + // 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() + + // And the workspace must be the caller's. Linking needs only visibility, so + // a project can be bound to a colleague's shared workspace — where the + // attach would answer 404, on every publish, leaving a skill nothing from + // the CLI could ever attach. Same rule as the link check: nothing is + // uploaded until the attach is known to be possible. + await assertOwnsWorkspace(binding.datamateId, binding.datamateName, scope.userId) + + const files = await collectBundle(input.skillDirectory) + 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, scope) + if (existing) { + try { + await altimateRequest("PATCH", `/${encodeURIComponent(existing)}`, { + base: SKILLS_BASE, + // `replace_bundle` because `files` is the WHOLE bundle every time — + // `collectBundle` walks the directory, so a path missing from it is a + // file the user deleted. Without the flag the server refuses any + // publish that drops a path (409, by design: a partial `files` array + // from a REST-conventional client used to delete the rest silently). + // For this client the deletion IS the intent, and saying so is what + // makes "delete a file, publish again" work at all. + body: { name: input.name, description: input.description, files, replace_bundle: true }, + 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 + // 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 + // with a local id they cannot see or clear. + // The update path answers 409 for three different things, and calling + // all of them a name conflict told the user to rename a skill whose name + // was never the problem — with no way forward, since renaming does not + // help. Told apart by the server's own message. + if (err instanceof ConflictError) throw updateConflict(err, input.name) + // 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 + } + } + + let created: unknown + try { + 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 + // 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.") + + await recordPublished(input.skillDirectory, scope, { + publicId, + tenant: scope.tenant, + apiUrl: scope.apiUrl, + createdBy: extractCreatedBy(created) ?? scope.userId, + }) + // 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) { + // A 404 straight after a create means the workspace refused the + // attachment outright — not owned, or gone — and retrying will not change + // that. The skill just created would be an orphan; take it back so + // nothing is left behind, and say why. + if (err instanceof NotFoundError) { + await altimateRequest("DELETE", `/${encodeURIComponent(publicId)}`, { + base: SKILLS_BASE, + allowEmptyBody: true, + }).catch((cleanup) => log.warn("could not remove an unattachable skill", { publicId, err: String(cleanup) })) + await forgetPublished(input.skillDirectory, scope) + throw new NotWorkspaceOwnerError(binding.datamateName) + } + throw new AttachFailedError(publicId, err) + } + return { action: "created", publicId, name: input.name, files: files.length, bytes, datamateId: binding.datamateId } +} + +/** Which of the update path's conflicts this is. The server distinguishes them + * in its detail; this module's typed errors then carry advice that fits. + * Anything unrecognised keeps the server's own words rather than being + * relabelled — a wrong explanation is worse than a bare one. */ +function updateConflict(err: ConflictError, skillName: string): Error { + const detail = err.detail.message ?? "" + if (/already have a skill named/i.test(detail)) return new SkillNameConflictError(skillName) + if (/changed while you were editing/i.test(detail)) return new SkillChangedElsewhereError(skillName) + return err +} + +/** Refuse before upload when the bound workspace is not the caller's. Read + * from the same list the picker uses, which carries each workspace's owner. A + * list that omits the owner (an older server) cannot answer, and the attach + * itself then decides — with the compensation on the create path. */ +async function assertOwnsWorkspace(datamateId: number, datamateName: string, userId: number): Promise { + const workspaces = await WorkspaceApi.listDatamates() + const ws = workspaces.find((w) => w.id === datamateId) + if (ws?.ownerId !== undefined && ws.ownerId !== userId) throw new NotWorkspaceOwnerError(datamateName) +} + +function extractCreatedBy(payload: unknown): number | null { + if (!payload || typeof payload !== "object") return null + const direct = (payload as { created_by?: unknown }).created_by + if (typeof direct === "number") return direct + const nested = (payload as { skill?: { created_by?: unknown } }).skill?.created_by + return typeof nested === "number" ? nested : null +} + +async function forgetPublished(skillDir: string, scope: LedgerScope): Promise { + return withLedger(async () => { + try { + const ledger = await readLedger() + delete ledger[ledgerKey(skillDir, scope)] + Filesystem.writeJsonAtomic(ledgerPath(), ledger) + } catch (err) { + log.warn("could not forget an unattachable skill's id", { err: String(err) }) + } + }) +} + +/** 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 0000000000..853ec0c318 --- /dev/null +++ b/packages/opencode/test/altimate/workspace/skill-publish.test.ts @@ -0,0 +1,805 @@ +// 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 { + closeSync, + ftruncateSync, + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + openSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs" +import path from "node:path" +import { createHash } from "node:crypto" +import fsp from "node:fs/promises" +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, + EmptyBundleError, + NotWorkspaceOwnerError, + SkillChangedElsewhereError, + ManagedSkillError, + NotLinkedError, + SkillNameConflictError, + SymlinkError, + collectBundle, + isManagedSkill, + ledgerPathForTests, + 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 +// 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 +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 +stubCreds() + +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 = {} +/** 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[] = [] +/** The `detail` an error response carries. The update path answers 409 for a + * name collision, a refused bundle deletion and a lost compare-and-swap, and + * the client tells them apart by this string. */ +let conflictDetail = "You already have a skill named 'deploy'" +/** Who the server says the caller is (`GET /users/me`), and who owns each + * workspace in the list. Ownership, not visibility, is what attaching needs. */ +let me = 7 +let workspaceOwners: Record = { 42: 7, 77: 7, 99: 7 } +/** `created_by` on every skill the server answers with. */ +let skillCreator = 7 +/** Skill ids the server has deleted. */ +let deleted: string[] = [] + +let project = "" +let skillDir = "" + +beforeEach(async () => { + requests = [] + statuses = {} + attached = [] + conflictDetail = "You already have a skill named 'deploy'" + me = 7 + workspaceOwners = { 42: 7, 77: 7, 99: 7 } + skillCreator = 7 + deleted = [] + 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 }) + if (method === "GET" && url.endsWith("/users/me")) + return new Response(JSON.stringify({ id: me }), { status: 200, headers: { "content-type": "application/json" } }) + if (method === "GET" && url.endsWith("/datamates/")) + return new Response( + JSON.stringify({ + datamates: Object.entries(workspaceOwners).map(([id, owner]) => ({ + id: Number(id), + name: `ws-${id}`, + memory_enabled: false, + user_id: owner, + })), + }), + { status: 200, headers: { "content-type": "application/json" } }, + ) + if (method === "DELETE" && url.includes("/skills/")) { + deleted.push(decodeURIComponent(url.split("/skills/")[1])) + return new Response(null, { status: 204 }) + } + const status = statuses[method] ?? (method === "POST" ? 201 : 200) + if (status >= 400) + return new Response(JSON.stringify({ detail: conflictDetail }), { + 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, created_by: skillCreator } } + 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 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" }) + +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"]) + }) + + 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", () => { + 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 + conflictDetail = "You already have a skill named 'deploy'" + + const err = await publish().catch((e) => e) + + expect(err).toBeInstanceOf(SkillNameConflictError) + 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) + // The pre-flights read who we are and whose the workspace is; nothing + // was uploaded. + expect(requests.filter((r) => r.method !== "GET")).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() + requests = [] + statuses.PATCH = 404 + + const report = await publish() + + expect(report.action).toBe("created") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) + }) +}) + +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 the read, so a single huge + // file was fully loaded before being rejected — the limit enforced only + // 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") + sparse(path.join(dir, "huge.txt"), 64 * 1024 * 1024) + + 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 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 + + try { + await expect(collectBundle(dir)).rejects.toThrow(/larger than/i) + } finally { + ;(fsp as unknown as { open: unknown }).open = originalOpen + } + // 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 () => { + // 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 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 { open: unknown }).open = originalOpen + } + }) + + 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("republishing after deleting a file succeeds", async () => { + // `files` replaces the bundle server-side, and a path it omits is a + // deletion the server refuses unless the caller says so. Without + // `replace_bundle` this 409'd forever, and the user was told to rename a + // skill whose name was never the problem. + writeFileSync(path.join(skillDir, "extra.md"), "notes") + await publish() + rmSync(path.join(skillDir, "extra.md")) + requests = [] + + const report = await publish() + + expect(report.action).toBe("updated") + const patch = requests.find((r) => r.method === "PATCH")! + expect(patch.body.replace_bundle).toBe(true) + expect(patch.body.files.map((f: any) => f.path)).toEqual(["SKILL.md"]) + }) + + test("a skill edited in the workspace mid-upload says so, rather than blaming the name", async () => { + // The server's compare-and-swap refuses and nothing is written. Renaming + // does not help; publishing again does. + await publish() + statuses.PATCH = 409 + conflictDetail = "This skill changed while you were editing it, please reload" + + const err = await publish().catch((e) => e) + + expect(err).toBeInstanceOf(SkillChangedElsewhereError) + expect(String(err)).toContain("Publish again") + }) + + test("junk a skill directory accumulates never leaves the machine", async () => { + // A public skill's bundle is readable tenant-wide, and a secret that + // reaches it cannot be recalled by deleting the local file. + writeFileSync(path.join(skillDir, ".env"), "ALTIMATE_API_KEY=secret") + writeFileSync(path.join(skillDir, ".ENV.production"), "ALTIMATE_API_KEY=secret") // case-insensitive file systems + writeFileSync(path.join(skillDir, ".envrc"), "export ALTIMATE_API_KEY=secret") // direnv + writeFileSync(path.join(skillDir, ".DS_Store"), "junk") + writeFileSync(path.join(skillDir, "SKILL.md~"), "editor backup") + mkdirSync(path.join(skillDir, ".git"), { recursive: true }) + writeFileSync(path.join(skillDir, ".git", "config"), "[core]") + + const files = await collectBundle(skillDir) + + expect(files.map((f) => f.path)).toEqual(["SKILL.md"]) + }) + + test("a worktree's .git file is junk too, not only a .git directory", async () => { + // `git worktree add` leaves a regular file named `.git` holding + // `gitdir: /path/to/main/.git/worktrees/...`. The directory skip does not + // see it. + const wt = path.join(project, "skills", "wt") + mkdirSync(wt, { recursive: true }) + writeFileSync(path.join(wt, "SKILL.md"), "---\nname: wt\n---\n") + writeFileSync(path.join(wt, ".git"), "gitdir: /somewhere/.git/worktrees/wt\n") + + const files = await collectBundle(wt) + + expect(files.map((f) => f.path)).toEqual(["SKILL.md"]) + }) + + test("the file ceiling is the server's, and file 101 is refused before it is opened", async () => { + // The server's `MAX_BUNDLE_FILES` is 100. A local ceiling of 200 let a + // 101–200 file bundle read in full, upload in full, and be refused with a + // 400 — the case the constant exists to prevent. + const dir = path.join(SANDBOX, `many-${Math.random().toString(36).slice(2)}`) + mkdirSync(dir, { recursive: true }) + for (let i = 0; i < 101; i++) writeFileSync(path.join(dir, `f${String(i).padStart(3, "0")}.md`), "x") + + const opened: string[] = [] + const originalOpen = fsp.open + ;(fsp as unknown as { open: unknown }).open = (async (...args: unknown[]) => { + opened.push(path.basename(String(args[0]))) + return (originalOpen as (...a: unknown[]) => unknown)(...args) + }) as unknown as typeof fsp.open + try { + await expect(collectBundle(dir)).rejects.toThrow(/more than 100 files/) + } finally { + ;(fsp as unknown as { open: unknown }).open = originalOpen + } + expect(opened).toHaveLength(100) + }) + + 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. The second publish carries a NEW name, so it is a rename. + await publish() // records the id, so the next call takes the PATCH branch + statuses.PATCH = 409 + conflictDetail = "You already have a skill named 'release'" + requests = [] + + const err = await publishSkill({ + projectDirectory: project, + skillDirectory: skillDir, + name: "release", + description: "d", + }).catch((e) => e) + + expect(err).toBeInstanceOf(SkillNameConflictError) + expect((err as { skillName: string }).skillName).toBe("release") + // It was a rename on the PATCH, not a create under the new name. + expect(requests.find((r) => r.method === "PATCH")?.body.name).toBe("release") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + }) +}) + +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. 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. + 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) + + // 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. + stubCreds() + await link(42, "Growth") + requests = [] + const report = await publish() + + expect(report.action).toBe("updated") + 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 user's id is in the scope. + await publish() // user 7 -> pub-1 + me = 8 // another user of the same tenant + workspaceOwners = { 42: 8 } + 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. + 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) + }) +}) + + +describe("a workspace the caller does not own", () => { + // Linking needs only visibility, so a project can be bound to a colleague's + // shared workspace. Attaching a skill needs ownership, and answers 404 + // otherwise — on every publish. Without a pre-flight the create went + // through, the attach failed, and the skill sat on the server attached to + // nothing, forever: the UAT report this module exists to close. + test("is refused before anything is uploaded", async () => { + workspaceOwners = { 42: 99 } // someone else's + + const err = await publish().catch((e) => e) + + expect(err).toBeInstanceOf(NotWorkspaceOwnerError) + expect(String(err)).toContain('"Growth"') + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + }) + + test("takes back a skill the workspace then refuses, when the list could not say", async () => { + // An older server omits the owner from the list; only the attach can + // answer. A 404 there, straight after a create, is compensated: the skill + // just made is deleted and the id forgotten, so nothing is left behind + // and the next publish does not PATCH an orphan. + workspaceOwners = {} + const originalFetch2 = globalThis.fetch + globalThis.fetch = (async (input: any, init?: any) => { + const url = typeof input === "string" ? input : input.url + const method = (init?.method ?? "GET").toUpperCase() + if (method === "PUT" && url.includes("/datamates")) { + requests.push({ method, url, body: undefined }) + return new Response(JSON.stringify({ detail: "Workspace not found" }), { + status: 404, + headers: { "content-type": "application/json" }, + }) + } + return originalFetch2(input, init) + }) as typeof fetch + try { + const err = await publish().catch((e) => e) + expect(err).toBeInstanceOf(NotWorkspaceOwnerError) + expect(deleted).toEqual(["pub-1"]) + requests = [] + // The id was forgotten: the next publish creates, not updates. + await publish().catch(() => {}) + expect(requests.filter((r) => r.method === "PATCH")).toHaveLength(0) + expect(requests.filter((r) => r.method === "POST")).toHaveLength(1) + } finally { + globalThis.fetch = originalFetch2 + } + }) +}) + +describe("the published-id ledger survives a key rotation", () => { + test("a rotated API key for the same user still finds the id", async () => { + // The scope was a digest of the key, so a rotation made every id on this + // machine unreachable and the next publish created again — 409 on the + // name, and "published from somewhere else". The user is the identity. + await publish() + stubCreds({ altimateApiKey: "k-rotated" }) + requests = [] + + const report = await publish() + + expect(report.action).toBe("updated") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + }) + + test("a legacy row is re-homed once the server confirms the skill is this user's", async () => { + // Rows written under the digest scheme carry no creator. One GET decides, + // and the row is rewritten under the current key so it is not asked again. + await publish() + const file = ledgerPathForTests() + const ledger = JSON.parse(readFileSync(file, "utf8")) + const mine = Object.keys(ledger).find((k) => k.endsWith(realpathSync(skillDir)))! + const { createdBy, ...legacy } = ledger[mine] + delete ledger[mine] + // The digest of a key that is NOT the current one — which is what a real + // rotation leaves on disk, and why the lookup cannot be by digest. + ledger[`acme|https://api.example.com|${createHash("sha256").update("the-key-before-rotation").digest("hex").slice(0, 16)}|${realpathSync(skillDir)}`] = legacy + writeFileSync(file, JSON.stringify(ledger)) + requests = [] + + const report = await publish() + + expect(report.action).toBe("updated") + expect(requests.filter((r) => r.method === "POST")).toHaveLength(0) + const after = JSON.parse(readFileSync(file, "utf8")) + expect(Object.keys(after).some((k) => k.includes("|u7|"))).toBe(true) + }) + + test("a legacy row for someone else's skill is not trusted", async () => { + await publish() + const file = ledgerPathForTests() + const ledger = JSON.parse(readFileSync(file, "utf8")) + const mine = Object.keys(ledger).find((k) => k.endsWith(realpathSync(skillDir)))! + const { createdBy, ...legacy } = ledger[mine] + delete ledger[mine] + // The tenant-only shape keyed on `path.resolve`, not the real path. + ledger[`acme|https://api.example.com|${path.resolve(skillDir)}`] = legacy + writeFileSync(file, JSON.stringify(ledger)) + skillCreator = 99 // the server says the skill is another user's + requests = [] + + const report = await publish() + + expect(report.action).toBe("created") + // Decided by asking, not by assuming: the server was asked whose the + // skill is BEFORE anything was uploaded, and no PATCH went out. + const firstUpload = requests.findIndex((r) => r.method === "POST" || r.method === "PATCH") + const asked = requests.findIndex((r) => r.method === "GET" && r.url.endsWith("/skills/pub-1")) + expect(asked).toBeGreaterThanOrEqual(0) + expect(asked).toBeLessThan(firstUpload) + expect(requests.filter((r) => r.method === "PATCH")).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, 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 () => { + 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) + }) +})