diff --git a/packages/opencode/src/altimate/workspace/engine-probes.ts b/packages/opencode/src/altimate/workspace/engine-probes.ts index 5f1d7c9df..b63f9b4b3 100644 --- a/packages/opencode/src/altimate/workspace/engine-probes.ts +++ b/packages/opencode/src/altimate/workspace/engine-probes.ts @@ -274,14 +274,21 @@ function pidAlive(pid: number): boolean { } } -export async function notify(toast: Toast): Promise { - if (syncInternals.notify) return syncInternals.notify(toast) +/** Resolves `false` when the toast could not be published, so a caller that + * remembers what it announced can forget it and try again. Never throws. */ +export async function notify(toast: Toast): Promise { + if (syncInternals.notify) { + await syncInternals.notify(toast) + return true + } try { await AppRuntime.runPromise( EventV2Bridge.Service.use((events) => events.publish(TuiEvent.ToastShow, { ...toast, duration: 10000 })), ) + return true } catch (err) { log.warn("could not show the workspace engine toast", { err: String(err) }) + return false } } diff --git a/packages/opencode/src/altimate/workspace/manage.ts b/packages/opencode/src/altimate/workspace/manage.ts index 1da463233..c5687efe5 100644 --- a/packages/opencode/src/altimate/workspace/manage.ts +++ b/packages/opencode/src/altimate/workspace/manage.ts @@ -62,6 +62,9 @@ export interface RefreshReport { /** True when the skill snapshot on disk changed. The caller owns the registry * invalidation this implies; see the note at the top of the file. */ skillsChanged: boolean + /** Skills the re-sync dropped, with the reason for each. A partial sync + * otherwise reads as success with fewer skills than the workspace has. */ + skillsSkipped: SkillSync.SkippedSkill[] /** Absent when workspace memory is off, or when no session was supplied. */ memory?: MemorySync.RefreshResult /** Set when there was no session to reload in place, so the overlay was @@ -168,8 +171,12 @@ export async function refresh(directory: string, sessionID?: string): Promise `${s.skill}: ${s.reason}`) + if (n > 3) lines.push(`…and ${n - 3} more`) + // A skipped bundle and a failed publish are different news: the second means + // the snapshot as a whole did not change. + if (result.error) lines.push(result.error) + return { title: `${n} workspace skill${n === 1 ? "" : "s"} skipped`, message: lines.join("\n") } +} + +/** A fixed, user-facing reason for a skill that failed to sync. The raw error + * can carry request URLs, server text or local paths — diagnostics for the + * log, not for a toast. */ +export function skipReason(err: unknown): string { + // A local write failure is the user's disk, not the server — say so rather + // than pointing them at the download. Network errors have codes too, so only + // the filesystem ones are mapped. + const code = (err as NodeJS.ErrnoException | null)?.code + if (code && LOCAL_WRITE_ERRORS.has(code)) return "it could not be saved on this device" + const msg = err instanceof Error ? err.message : "" + // Not "incomplete": a binary file comes back decoded with replacement + // characters but its raw byte size, so a valid bundle mismatches too. + if (msg.startsWith("size mismatch")) return "its file size could not be verified" + if (msg.startsWith("would exceed the client snapshot limit")) return "it is too large for this client" + if (msg.startsWith("unrecognised")) return "the server sent an unexpected response for it" + return "it could not be downloaded" +} + +const LOCAL_WRITE_ERRORS = new Set(["ENOSPC", "EDQUOT", "EACCES", "EPERM", "EROFS"]) + +/** A skill id as it may be shown. Ids come from the workspace service and one + * that failed `safePathComponent` is arbitrary text; control characters and + * line separators would reach the toast and the serve JSON as-is. The raw id + * stays in the log. */ +export function displayId(id: string): string { + // eslint-disable-next-line no-control-regex + const clean = id.replace(/[\u0000-\u001F\u007F-\u009F\u2028\u2029]/g, "") + if (!clean) return "(unnamed skill)" + return clean.length > 64 ? `${clean.slice(0, 63)}…` : clean +} + +function problemText(problem: { title: string; message: string }): string { + return `${problem.title}\n${problem.message}` +} + +/** Whether a sync problem is new for this directory. The per-turn sync retries + * a failed run every message, so without this an offline user or a + * permanently bad bundle would be told the same thing on every turn — and + * twice when concurrent turns join one run. Any clean `syncSkills` run clears + * it (see `settled`), so the problem is announced again if it comes back. */ +export function shouldAnnounce(directory: string, problem: { title: string; message: string } | null): boolean { + const key = path.resolve(directory) + if (!problem) { + store.announced.delete(key) + return false + } + const text = problemText(problem) + if (store.announced.get(key) === text) return false + store.announced.set(key, text) + return true +} + +/** Undo `shouldAnnounce` when the warning could not be shown, so a later turn + * tries again. Only while it still holds THIS problem: a concurrent turn may + * have latched a newer one, and that must survive. */ +export function forgetAnnouncement(directory: string, problem: { title: string; message: string }): void { + const key = path.resolve(directory) + if (store.announced.get(key) === problemText(problem)) store.announced.delete(key) +} + interface SyncStore { - inFlight: Map> + inFlight: Map> lastSyncedAt: Map registryAppliedAt: Map syncedFor: Map + /** The sync problem last announced per directory. Here rather than a module + * Map for the same reason as the rest of the store: a second module record + * would otherwise keep its own copy, and the dedup would fork. */ + announced: Map } const globals = globalThis as unknown as Record @@ -152,6 +245,7 @@ const store: SyncStore = (globals[STORE_KEY] ??= { lastSyncedAt: new Map(), registryAppliedAt: new Map(), syncedFor: new Map(), + announced: new Map(), }) /** In-flight sync per canonical project directory, so a bind and a session @@ -677,6 +771,14 @@ async function hasManagedSnapshot(directory: string): Promise { } } +/** Whether local state says this project has a workspace: a snapshot a sync + * once published, or a cached binding. Decides if a failed binding lookup is + * worth telling the user about. */ +async function hasBindingEvidence(directory: string): Promise { + if (await hasManagedSnapshot(directory)) return true + return (await readLocalBinding(directory).catch(() => null)) !== null +} + /** Take the snapshot out of service when this client is no longer entitled to * serve it — the account was disconnected, or the feature was switched off. * @@ -710,7 +812,7 @@ async function removeManaged(directory: string): Promise { * Never throws: skills must not be able to block a bind or a turn. Every * failure path leaves whatever is already on disk in place, except the * deliberate purge described below. */ -export async function syncSkills(directory: string): Promise<{ changed: boolean }> { +export async function syncSkills(directory: string): Promise { const canon = path.resolve(directory) // Joined BEFORE the flag is read, so the opt-out purge is serialised against // a sync too. Both paths write the same tree; with the purge outside this @@ -721,7 +823,7 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean if (existing) { // Report the joined run's real outcome. Returning a hard-coded `false` is a // false answer waiting for the next caller to trust it. - return await existing.catch(() => ({ changed: false })) + return await existing.catch(() => ({ changed: false, skipped: [] })) } if (!isEnabled()) { // Opting out has to actually take effect: a snapshot left behind keeps @@ -732,7 +834,7 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean const dropped = (await pathsAreReal(canon).catch(() => false)) ? await deactivate(canon, "the workspace feature is off").catch(() => false) : false - return { changed: dropped } + return { changed: dropped, skipped: [] } })() inFlight.set(canon, purge) try { @@ -742,6 +844,9 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean } } let changed = false + // Named apart from the loop's `skipped` counter below, which it would shadow. + const skippedSkills: SkippedSkill[] = [] + let syncError: string | undefined /** The manifest an unchanged run validated, so its marker describes that * snapshot rather than whatever is live when the stamp is written. */ let validated: Manifest | null = null @@ -772,6 +877,7 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean path: managedRoot(canon), }) failed = true + syncError = "the workspace skill folder is not a real directory" return } @@ -806,6 +912,15 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean if (outcome.status === "unbound") { if (await deactivate(canon, "this project is no longer bound to a workspace")) changed = true } + // Unbound is a state, not a failure, and says nothing. Unknown is a failed + // lookup — but only worth a warning when this project is known to have a + // workspace. With a local binding row, offline resolves to a stale "bound" + // and fails later at the list; without one, the server is always asked, + // so offline lands here for EVERY opted-in project, including ones never + // linked. A snapshot on disk (a server-side binding that synced before) + // or a local row is that evidence; without either, stay quiet. + if (outcome.status === "unknown" && (await hasBindingEvidence(canon))) + syncError = "could not confirm this project's workspace (offline, or no access to it)" return } const binding = outcome.binding @@ -818,6 +933,7 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean "refusing to manage the workspace skill directory: it has contents this client did not write", { path: managedRoot(canon) }, ) + syncError = "the workspace skill folder has files this app did not create, so it was left alone" return } await sweepStaging(canon) @@ -829,6 +945,7 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean log.warn("could not read altimate credentials; keeping the existing snapshot", { err: String(err), }) + syncError = "could not read your Altimate credentials" return } @@ -854,7 +971,11 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean } const remote = await listAll(binding) - if (!remote) return // error, not empty — keep what is on disk + if (!remote) { + // error, not empty — keep what is on disk + syncError = "could not fetch the workspace's skill list" + return + } sawRemote = true syncedFor.set(canon, accountKeyOf(creds.altimateInstanceName, creds.altimateUrl)) @@ -920,6 +1041,7 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean skipped += 1 failed = true log.warn("skipping a workspace skill with an unusable id", { skill: summary.publicId }) + skippedSkills.push({ skill: displayId(summary.publicId), reason: "its id is not usable as a folder name" }) continue } try { @@ -1007,6 +1129,8 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean carriedPrevious: carried, err: String(err), }) + const why = skipReason(err) + skippedSkills.push({ skill: displayId(summary.publicId), reason: carried ? `${why} (kept the previous copy)` : why }) } } if (remote.length > 0 && Object.keys(next.skills).length === 0) { @@ -1068,6 +1192,13 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean failed = true await fs.rm(staging, { recursive: true, force: true }).catch(() => {}) log.warn("workspace skill sync failed; kept the existing snapshot", { err: String(err) }) + // Per-skill reasons are already in `skippedSkills`; this says the snapshot + // as a whole did not change. Only claim the old skills survived when they + // did: a rebind removed them above, and a first sync had none. + syncError ??= + foreign || !manifest + ? "could not install the workspace skills" + : "could not update the workspace skills; kept the previous ones" } })() // Published to `inFlight` so a joining caller awaits the SAME settled result @@ -1079,6 +1210,7 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean } catch (err) { ok = false log.warn("workspace skill sync errored", { err: String(err) }) + syncError ??= "workspace skill sync errored" } // Only a clean run earns the poll interval. `failed` is set by the inner // catch, which swallows so that skills can never block a turn. @@ -1098,7 +1230,11 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean if (!changed && validated) await fs.writeFile(path.join(managedRoot(canon), SYNCED_MARKER), markerFor(validated, now)).catch(() => {}) } - return { changed } + // Cleared here, not by callers, so a clean Refresh or IDE refresh resets it + // too — otherwise a problem it fixed would stay latched, and its return + // would never be announced. + if (skippedSkills.length === 0 && !syncError) store.announced.delete(canon) + return { changed, skipped: skippedSkills, error: syncError } })() inFlight.set(canon, settled) try { diff --git a/packages/opencode/src/plugin/tui/altimate/workspace.tsx b/packages/opencode/src/plugin/tui/altimate/workspace.tsx index 16ebc2942..7b55d8bb8 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace.tsx @@ -28,6 +28,7 @@ import { existsSync } from "node:fs" import open from "open" // altimate_change start - the /workspace action menu import * as Manage from "@/altimate/workspace/manage" +import { describeSyncProblems } from "@/altimate/workspace/skill-sync" import { inertWorkspaceName } from "@/altimate/workspace/workspace-name" // altimate_change end import { createSignal, onCleanup, onMount } from "solid-js" @@ -1856,18 +1857,26 @@ async function runWorkspaceManage(api: TuiPluginApi, directory: string): Promise if (option.value === "refresh") { Manage.refresh(directory) .then((result) => { + // Named, with reasons: a partial pull otherwise reads as success + // with fewer skills than the workspace has. Built by the same + // helper as the per-turn warning, so both cap the list alike; + // the IDE route still gets every entry in `skillsSkipped`. + const skipped = describeSyncProblems({ changed: result.skillsChanged, skipped: result.skillsSkipped }) + const problems = skipped + ? [...result.errors, `${skipped.title}: ${skipped.message.split("\n").join("; ")}`] + : result.errors const said = [ result.skillsChanged ? "skills updated" : "skills already current", result.memoryInvalidated ? "memory reloads on your next message" : null, ].filter(Boolean) api.ui.toast({ - variant: result.errors.length > 0 ? "warning" : "success", + variant: problems.length > 0 ? "warning" : "success", // The problems line still names what DID land: the halves are // independent, and a failed skill pull does not undo the memory // invalidation that happened beside it. message: - result.errors.length > 0 - ? `Refreshed with problems — ${result.errors.join("; ")}${ + problems.length > 0 + ? `Refreshed with problems — ${problems.join("; ")}${ result.memoryInvalidated ? "; memory reloads on your next message" : "" }` : `Refreshed: ${said.join(", ")}.`, diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index d2d2095fe..8d0fe2b61 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -397,7 +397,38 @@ export namespace SessionPrompt { await refreshRegistry() if (!(await skillSync.recentlySynced(dir))) { - const applied = skillSync.syncSkills(dir).then(refreshRegistry) + const applied = skillSync.syncSkills(dir).then(async (result) => { + // Its own catch: a failed refresh must not take the warning with it. + // After an account switch the next re-sync can be a poll interval + // away, so the problem would otherwise go unsaid for minutes. + await refreshRegistry().catch((err) => + log.warn("workspace skill registry refresh failed", { err: String(err) }), + ) + // A skill that silently fails to arrive looks exactly like a + // workspace with no skills. Say which, and why. Imported only when + // there is something to show, keeping the common path free of it. + const problem = skillSync.describeSyncProblems(result) + if (problem && skillSync.shouldAnnounce(dir, problem)) { + // Latched before delivery so concurrent turns joining this run do + // not both warn; released if nothing was shown, so a later turn + // tries again instead of the problem staying silent for good. + let shown = false + try { + const { isHeadless } = await import("../altimate/workspace/engine-seams") + const { notify, printLine } = await import("../altimate/workspace/engine-probes") + if (isHeadless()) { + // `altimate-code run` has no toast renderer; the engine reports + // the same way. One line: printLine strips newlines. + printLine(`${problem.title}: ${problem.message.split("\n").join("; ")}`) + shown = true + } else { + shown = await notify({ ...problem, variant: "warning" }) + } + } finally { + if (!shown) skillSync.forgetAnnouncement(dir, problem) + } + } + }) applied.catch((err) => log.warn("workspace skill sync failed", { err: String(err) })) // Timer cleared when the sync wins the race: an armed timer keeps the // event loop alive, so a short-lived `run` would linger for the rest diff --git a/packages/opencode/test/altimate/workspace/skill-sync.test.ts b/packages/opencode/test/altimate/workspace/skill-sync.test.ts index dff7aa783..559b11889 100644 --- a/packages/opencode/test/altimate/workspace/skill-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-sync.test.ts @@ -48,6 +48,11 @@ writeFileSync( const { syncSkills, + describeSyncProblems, + shouldAnnounce, + forgetAnnouncement, + displayId, + skipReason, recentlySynced, lastSuccessfulSyncAt, registryStale, @@ -297,6 +302,226 @@ describe("workspace skill sync", () => { expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(true) }) + // A skill that fails to arrive must say so. Before this, every path below + // was a log line only, and "nothing synced" was indistinguishable from "the + // workspace has no skills". + test("a partial sync names the dropped skill and why, and still publishes the rest", async () => { + globalThis.fetch = (async (input: string | URL) => { + const url = String(input) + if (url.includes("datamate_id")) + return json({ + items: [ + { public_id: "pub-1", name: "p1", file_count: 1, updated_at: "2026-03-01T00:00:00Z" }, + { public_id: "pub-2", name: "p2", file_count: 1, updated_at: "2026-03-01T00:00:00Z" }, + ], + total: 2, + page: 1, + size: 50, + pages: 1, + }) + if (url.includes("/pub-1/files/")) return json({ path: "SKILL.md", content: "good" }) + if (url.includes("/pub-2/files/")) return json({ path: "SKILL.md", content: "short" }) + if (url.includes("/pub-1")) return json({ skill: { public_id: "pub-1", files: [{ path: "SKILL.md", size: 4 }], content: "" } }) + return json({ skill: { public_id: "pub-2", files: [{ path: "SKILL.md", size: 9999 }], content: "" } }) + }) as unknown as typeof fetch + + const result = await syncSkills(project) + + expect(result.changed).toBe(true) + expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(true) + expect(result.skipped).toHaveLength(1) + expect(result.skipped[0].skill).toBe("pub-2") + // A fixed reason, not the raw error: that carries paths and byte counts. + expect(result.skipped[0].reason).toBe("its file size could not be verified") + // Per-skill trouble is not a whole-sync failure. + expect(result.error).toBeUndefined() + }) + + test("when every skill fails, each is named and the run reports it kept the previous snapshot", async () => { + serve({ "pub-1": { "SKILL.md": "good" } }) + await syncSkills(project) + + globalThis.fetch = (async (input: string | URL) => { + const url = String(input) + if (url.includes("/files/")) return json({ path: "SKILL.md", content: "short" }) + if (url.includes("datamate_id")) + return json({ + items: [{ public_id: "pub-2", name: "p2", file_count: 1, updated_at: "2026-02-02T00:00:00Z" }], + total: 1, + page: 1, + size: 50, + pages: 1, + }) + return json({ skill: { public_id: "pub-2", files: [{ path: "SKILL.md", size: 9999 }], content: "" } }) + }) as unknown as typeof fetch + + const result = await syncSkills(project) + + expect(result.skipped.map((s) => s.skill)).toEqual(["pub-2"]) + expect(result.error).toBe("could not update the workspace skills; kept the previous ones") + expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(true) + }) + + test("an unreachable skill list is reported as a whole-sync error, not as zero skills", async () => { + serve({ "pub-1": { "SKILL.md": "one" } }) + await syncSkills(project) + + globalThis.fetch = (async () => { + throw new Error("offline") + }) as unknown as typeof fetch + const result = await syncSkills(project) + + expect(result.error).toBe("could not fetch the workspace's skill list") + expect(result.skipped).toEqual([]) + expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(true) + }) + + test("offline in a project never linked says nothing", async () => { + // No local binding row and no snapshot: the server is always asked, so a + // warning here would fire for every opted-in repo the user is offline in. + unbind() + globalThis.fetch = (async () => { + throw new Error("offline") + }) as unknown as typeof fetch + const result = await syncSkills(project) + expect(result.error).toBeUndefined() + expect(result.skipped).toEqual([]) + }) + + test("an unusable remote id is shown sanitised and bounded, never raw", async () => { + const esc = String.fromCharCode(27) + const raw = `../evil${esc}[31m${String.fromCharCode(10)}${String.fromCharCode(0x2028)}${"x".repeat(100)}` + serveSkills({ good: {}, [raw]: {} }) + const result = await syncSkills(project) + const shown = result.skipped.map((s) => s.skill) + expect(shown).toHaveLength(1) + expect(shown[0]).not.toContain(esc) + expect(shown[0]).not.toContain(String.fromCharCode(10)) + expect(shown[0]).not.toContain(String.fromCharCode(0x2028)) + expect(shown[0].length).toBeLessThanOrEqual(64) + }) + + test("a folder this client did not create is left alone, and the user is told", async () => { + mkdirSync(path.join(project, MANAGED), { recursive: true }) + writeFileSync(path.join(project, MANAGED, "mine.md"), "the user's own file") + serve({ "pub-1": { "SKILL.md": "one" } }) + + const result = await syncSkills(project) + + expect(result.error).toBe("the workspace skill folder has files this app did not create, so it was left alone") + expect(readFileSync(path.join(project, MANAGED, "mine.md"), "utf8")).toBe("the user's own file") + }) + + // Serves a workspace whose skills can each misbehave in one way. `detail` + // replaces the detail envelope; `size` overrides the declared file size. + type Spec = { content?: string; size?: number; updated?: string; detail?: unknown; files?: number } + /** `datamateId` is the workspace the server confirms on the binding + * lookup `syncSkills` makes every run. Answered, not left to fall through: + * an unanswered lookup was swallowed as "offline", so a test passed for a + * degraded reason while claiming to exercise the bound path. */ + function serveSkills(spec: Record, datamateId = 1) { + globalThis.fetch = (async (input: string | URL) => { + const url = String(input) + if (url.includes("/datamate-project-bindings/by-")) + return json({ + binding: { + id: 7, + datamate_id: datamateId, + datamate_name: `ws-${datamateId}`, + repo_remote: null, + project_path: project, + }, + datamate: { id: datamateId, name: `ws-${datamateId}` }, + }) + if (url.includes("datamate_id")) + return json({ + items: Object.entries(spec).map(([id, s]) => ({ + public_id: id, + name: id, + file_count: 1, + updated_at: s.updated ?? "2026-03-01T00:00:00Z", + })), + total: Object.keys(spec).length, + page: 1, + size: 50, + pages: 1, + }) + const id = Object.keys(spec).find((k) => url.includes(`/${k}`))! + const s = spec[id] + const content = s.content ?? "fine" + if (url.includes("/files/")) return json({ path: "SKILL.md", content }) + if (s.detail !== undefined) return json(s.detail) + const files = s.files + ? Array.from({ length: s.files }, (_, i) => ({ path: `f${i}.md`, size: 1 })) + : [{ path: "SKILL.md", size: s.size ?? Buffer.byteLength(content) }] + return json({ skill: { public_id: id, files, content: "" } }) + }) as unknown as typeof fetch + } + + test("each failure kind gets its own fixed reason", async () => { + serveSkills({ + good: {}, + huge: { files: 2001 }, + odd: { detail: { unexpected: "envelope" } }, + }) + const result = await syncSkills(project) + expect(Object.fromEntries(result.skipped.map((s) => [s.skill, s.reason]))).toEqual({ + huge: "it is too large for this client", + odd: "the server sent an unexpected response for it", + }) + expect(existsSync(skillFile("good", "SKILL.md"))).toBe(true) + }) + + test("a skill that fails again says its previous copy was kept", async () => { + serveSkills({ pub1: { content: "v1" }, pub2: {} }) + await syncSkills(project) + + serveSkills({ pub1: { content: "short", size: 9999, updated: "2026-04-01T00:00:00Z" }, pub2: {} }) + const result = await syncSkills(project) + + expect(result.skipped).toEqual([ + { skill: "pub1", reason: "its file size could not be verified (kept the previous copy)" }, + ]) + expect(readFileSync(skillFile("pub1", "SKILL.md"), "utf8")).toBe("v1") + }) + + test("a failed pull after a rebind does not claim the old skills were kept", async () => { + serve({ "pub-1": { "SKILL.md": "from workspace 1" } }) + await syncSkills(project) + + // The rebind drops workspace 1's snapshot before the pull — so when every + // new skill fails, nothing of the old one is left to have been "kept". + bindTo(2) + serveSkills({ pub9: { content: "short", size: 9999 } }, 2) + const result = await syncSkills(project) + + expect(result.error).toBe("could not install the workspace skills") + // The per-skill line must not promise a surviving copy either. + expect(result.skipped).toEqual([{ skill: "pub9", reason: "its file size could not be verified" }]) + expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(false) + }) + + test("any clean sync clears a latched warning, so a problem that returns is announced again", async () => { + const problem = { title: "Workspace skills not synced", message: "offline" } + shouldAnnounce(project, null) + expect(shouldAnnounce(project, problem)).toBe(true) + expect(shouldAnnounce(project, problem)).toBe(false) + + // A clean run — e.g. a manual Refresh, which never calls shouldAnnounce. + serve({ "pub-1": { "SKILL.md": "one" } }) + await syncSkills(project) + + expect(shouldAnnounce(project, problem)).toBe(true) + }) + + test("a clean sync reports nothing to surface", async () => { + serve({ "pub-1": { "SKILL.md": "one" } }) + const result = await syncSkills(project) + expect(result.skipped).toEqual([]) + expect(result.error).toBeUndefined() + expect(describeSyncProblems(result)).toBeNull() + }) + test("an unchanged workspace issues no detail or file requests on the second run", async () => { serve({ "pub-1": { "SKILL.md": "one" } }) await syncSkills(project) @@ -451,6 +676,24 @@ describe("workspace skill sync", () => { expect(existsSync(skillFile("pub-2", "SKILL.md"))).toBe(true) }) + // Found by e2e: with the binding re-resolved on the network, an outage fails + // the binding lookup before the list is fetched, so reporting only a failed + // list left "offline" silent. + test("offline before the binding resolves is reported, and the snapshot is kept", async () => { + serve({ "pub-1": { "SKILL.md": "one" } }) + await syncSkills(project) + + unbind() + globalThis.fetch = (async () => { + throw new Error("offline") + }) as unknown as typeof fetch + const result = await syncSkills(project) + + expect(result.error).toBe("could not confirm this project's workspace (offline, or no access to it)") + expect(result.skipped).toEqual([]) + expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(true) + }) + test("recentlySynced rate-limits the per-message poll", async () => { // The caller on the per-message path skips the network while this is true. // If it never went true, every turn would pay an HTTP round trip; if it @@ -1633,3 +1876,114 @@ describe("workspace skill sync", () => { } }) }) + +describe("describeSyncProblems", () => { + const skip = (skill: string) => ({ skill, reason: `${skill} failed` }) + + test("nothing to say when nothing went wrong", () => { + expect(describeSyncProblems({ changed: true, skipped: [] })).toBeNull() + }) + + test("a whole-sync error with no per-skill detail says the sync did not happen", () => { + expect(describeSyncProblems({ changed: false, skipped: [], error: "offline" })).toEqual({ + title: "Workspace skills not synced", + message: "offline", + }) + }) + + test("names one skipped skill with its reason, singular", () => { + expect(describeSyncProblems({ changed: true, skipped: [skip("billing")] })).toEqual({ + title: "1 workspace skill skipped", + message: "billing: billing failed", + }) + }) + + test("a failed publish is still reported when skills were also skipped", () => { + const problem = describeSyncProblems({ changed: false, skipped: [skip("billing")], error: "snapshot kept" }) + expect(problem?.message.split("\n")).toEqual(["billing: billing failed", "snapshot kept"]) + }) + + test("caps the list at three and counts the rest", () => { + const problem = describeSyncProblems({ changed: true, skipped: ["a", "b", "c", "d", "e"].map(skip) }) + expect(problem?.title).toBe("5 workspace skills skipped") + expect(problem?.message.split("\n")).toEqual(["a: a failed", "b: b failed", "c: c failed", "…and 2 more"]) + }) +}) + +describe("skipReason", () => { + const errno = (code: string) => Object.assign(new Error(`${code}: boom`), { code }) + + test("a local write failure points at the device, not the server", () => { + for (const code of ["ENOSPC", "EACCES", "EPERM", "EROFS", "EDQUOT"]) + expect(skipReason(errno(code))).toBe("it could not be saved on this device") + }) + + test("a network error is still a download failure", () => { + expect(skipReason(errno("ECONNRESET"))).toBe("it could not be downloaded") + }) +}) + +describe("displayId", () => { + test("keeps an ordinary id as it is", () => { + expect(displayId("billing-report")).toBe("billing-report") + }) + + test("strips control characters and caps the length", () => { + const shown = displayId(`a${String.fromCharCode(27)}b${String.fromCharCode(0x2029)}${"c".repeat(80)}`) + expect(shown.startsWith("ab")).toBe(true) + expect(shown.length).toBe(64) + expect(shown.endsWith("…")).toBe(true) + }) + + test("an id with nothing printable still says something", () => { + expect(displayId(String.fromCharCode(1, 2))).toBe("(unnamed skill)") + }) +}) + +describe("shouldAnnounce", () => { + const dir = "/tmp/announce-test" + const problem = { title: "1 workspace skill skipped", message: "billing: it could not be downloaded" } + + test("announces a problem once, not on every retry", () => { + shouldAnnounce(dir, null) + expect(shouldAnnounce(dir, problem)).toBe(true) + expect(shouldAnnounce(dir, problem)).toBe(false) + expect(shouldAnnounce(dir, problem)).toBe(false) + }) + + test("a different problem is announced", () => { + shouldAnnounce(dir, null) + shouldAnnounce(dir, problem) + expect(shouldAnnounce(dir, { ...problem, message: "billing: it is too large for this client" })).toBe(true) + }) + + test("a clean run clears it, so a problem that returns is announced again", () => { + shouldAnnounce(dir, null) + shouldAnnounce(dir, problem) + expect(shouldAnnounce(dir, null)).toBe(false) + expect(shouldAnnounce(dir, problem)).toBe(true) + }) + + test("a warning that was never shown is forgotten, so the next turn retries it", () => { + shouldAnnounce(dir, null) + shouldAnnounce(dir, problem) + forgetAnnouncement(dir, problem) + expect(shouldAnnounce(dir, problem)).toBe(true) + }) + + test("forgetting an old problem does not clear a newer one a concurrent turn latched", () => { + const newer = { ...problem, message: "billing: it is too large for this client" } + shouldAnnounce(dir, null) + shouldAnnounce(dir, problem) + shouldAnnounce(dir, newer) + forgetAnnouncement(dir, problem) + expect(shouldAnnounce(dir, newer)).toBe(false) + }) + + test("is per directory", () => { + shouldAnnounce(dir, null) + shouldAnnounce("/tmp/other", null) + shouldAnnounce(dir, problem) + expect(shouldAnnounce("/tmp/other", problem)).toBe(true) + }) +}) diff --git a/packages/opencode/test/server/altimate-workspace-routes.test.ts b/packages/opencode/test/server/altimate-workspace-routes.test.ts index c3d1ebd9c..4b9a22d31 100644 --- a/packages/opencode/test/server/altimate-workspace-routes.test.ts +++ b/packages/opencode/test/server/altimate-workspace-routes.test.ts @@ -40,6 +40,7 @@ describe("POST /altimate/workspace/refresh", () => { spies.push(spyOn(Session, "get").mockResolvedValue({ directory: process.cwd() } as never)) const refresh = spyOn(Manage, "refresh").mockResolvedValue({ skillsChanged: true, + skillsSkipped: [{ skill: "billing", reason: "it could not be downloaded" }], memory: { ok: true, status: "loaded", count: 4 }, errors: [], }) @@ -50,6 +51,9 @@ describe("POST /altimate/workspace/refresh", () => { const body = (await response.json()) as Record expect(body.ok).toBe(true) expect(body.skillsChanged).toBe(true) + // Non-empty on purpose: the IDE needs every skipped skill, so a route that + // dropped the field would otherwise still pass. + expect(body.skillsSkipped).toEqual([{ skill: "billing", reason: "it could not be downloaded" }]) expect(body.errors).toEqual([]) expect(refresh).toHaveBeenCalledTimes(1) expect(refresh.mock.calls[0][1]).toBe("ses_123") @@ -98,6 +102,7 @@ describe("POST /altimate/workspace/refresh", () => { test("works without a body, leaving the memory overlay to reload on the next turn", async () => { const refresh = spyOn(Manage, "refresh").mockResolvedValue({ skillsChanged: false, + skillsSkipped: [], memoryInvalidated: true, errors: [], })