From d77ef3e85ccc133ec68be5066dd309b8cd0e61c2 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 28 Sep 2026 01:43:03 +0530 Subject: [PATCH 1/4] feat(workspace): tell the user which workspace skills a sync skipped, and why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A skill that failed to sync was a log line only, so "nothing arrived" looked exactly like "this workspace has no skills". Surface it instead: - `syncSkills` now returns `skipped` (each skill with a short reason) and `error` (set when the whole sync could not proceed: an unusable or foreign folder, unreadable credentials, an unconfirmed workspace, an unreachable skill list, or a failed publish). - The per-turn sync shows a warning toast naming the skipped skills. The same problem is announced once, not on every retry; a clean run clears it. - `/workspace` Refresh folds skipped skills into its "problems" line, and the `serve` refresh report carries `skillsSkipped` through to the IDE. - Reasons are fixed, user-facing strings. The raw error can carry request URLs, server text or local paths; it stays in the log. Offline is reported at the binding lookup, not the list fetch: the binding is re-resolved on the network, so an outage fails that first. Found in e2e — a unit test that bound the project locally reached the list and missed it. Co-Authored-By: Claude Opus 5.5 --- .../opencode/src/altimate/workspace/manage.ts | 11 +- .../src/altimate/workspace/skill-sync.ts | 94 ++++++++- .../src/plugin/tui/altimate/workspace.tsx | 10 +- packages/opencode/src/session/prompt.ts | 14 +- .../altimate/workspace/skill-sync.test.ts | 178 ++++++++++++++++++ .../server/altimate-workspace-routes.test.ts | 2 + 6 files changed, 297 insertions(+), 12 deletions(-) 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. */ +function skipReason(err: unknown): string { + const msg = err instanceof Error ? err.message : "" + if (msg.startsWith("size mismatch")) return "its download was incomplete" + 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 announced = new Map() + +/** 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. A clean result clears it, 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) { + announced.delete(key) + return false + } + const text = `${problem.title}\n${problem.message}` + if (announced.get(key) === text) return false + announced.set(key, text) + return true +} + interface SyncStore { - inFlight: Map> + inFlight: Map> lastSyncedAt: Map registryAppliedAt: Map syncedFor: Map @@ -710,7 +769,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 +780,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 +791,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 +801,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 +834,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 +869,12 @@ 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 } + // Offline lands here, before the list is ever fetched: the binding is + // re-resolved on the network, so an outage fails this lookup first. So + // does a workspace this account cannot see, hence the wording. + // Unbound is a state, not a failure, and says nothing. + if (outcome.status === "unknown") + syncError = "could not confirm this project's workspace (offline, or no access to it)" return } const binding = outcome.binding @@ -818,6 +887,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 +899,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 +925,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 +995,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: summary.publicId, reason: "its id is not usable as a folder name" }) continue } try { @@ -1007,6 +1083,8 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean carriedPrevious: carried, err: String(err), }) + const why = skipReason(err) + skippedSkills.push({ skill: summary.publicId, reason: carried ? `${why} (kept the previous copy)` : why }) } } if (remote.length > 0 && Object.keys(next.skills).length === 0) { @@ -1068,6 +1146,9 @@ 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. + syncError ??= "could not update the workspace skills; kept the previous ones" } })() // Published to `inFlight` so a joining caller awaits the SAME settled result @@ -1079,6 +1160,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 +1180,7 @@ 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 } + 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..0884f648b 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace.tsx @@ -1856,18 +1856,22 @@ 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. + const skipped = result.skillsSkipped.map((s) => `${s.skill}: ${s.reason}`) + const problems = skipped.length > 0 ? [...result.errors, `skipped ${skipped.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..b180ee915 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -397,7 +397,19 @@ 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) => { + await refreshRegistry() + // 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) + // Called on a clean result too: that is what clears the memo. + const announce = skillSync.shouldAnnounce(dir, problem) + if (problem && announce) { + const { notify } = await import("../altimate/workspace/engine-probes") + await notify({ ...problem, variant: "warning" }) + } + }) 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..82d933909 100644 --- a/packages/opencode/test/altimate/workspace/skill-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-sync.test.ts @@ -48,6 +48,8 @@ writeFileSync( const { syncSkills, + describeSyncProblems, + shouldAnnounce, recentlySynced, lastSuccessfulSyncAt, registryStale, @@ -297,6 +299,99 @@ 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 download was incomplete") + // 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("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") + }) + + 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 +546,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 +1746,68 @@ 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("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("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..2f9e0c1ec 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: [], memory: { ok: true, status: "loaded", count: 4 }, errors: [], }) @@ -98,6 +99,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: [], }) From 44c77ac27219c228e2ec1485886a993aaac56db1 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 28 Sep 2026 02:52:51 +0530 Subject: [PATCH 2/4] fix(workspace): address review on the skipped-skill warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Headless `run` has no toast renderer, so print the warning to stderr there, the same way the workspace engine reports. - Clear the "already announced" latch inside `syncSkills` on any clean run, so a clean Refresh or IDE refresh resets it too. - Keep that latch in the process-global store, like the rest of this file's state, so a second module record cannot fork it. - Release the latch when the warning could not be shown (`notify` now reports whether it published), but only if it still holds that problem. - After a rebind or on a first sync, a failed pull no longer claims the previous skills were kept — there were none left to keep. - Cap the Refresh toast's skipped list the same way as the per-turn warning, by reusing `describeSyncProblems`. The IDE route still gets every entry. - Word a size mismatch as "its file size could not be verified": a valid binary bundle mismatches too, so "incomplete download" misdiagnoses it. - Test every skip reason, the kept-previous-copy suffix, the rebind wording, the latch lifecycle, and that the route passes `skillsSkipped` through. Co-Authored-By: Claude Opus 5.5 --- .../src/altimate/workspace/engine-probes.ts | 11 +- .../src/altimate/workspace/skill-sync.ts | 45 ++++++-- .../src/plugin/tui/altimate/workspace.tsx | 11 +- packages/opencode/src/session/prompt.ts | 24 +++- .../altimate/workspace/skill-sync.test.ts | 104 +++++++++++++++++- .../server/altimate-workspace-routes.test.ts | 5 +- 6 files changed, 178 insertions(+), 22 deletions(-) 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/skill-sync.ts b/packages/opencode/src/altimate/workspace/skill-sync.ts index 88596e866..3f826a013 100644 --- a/packages/opencode/src/altimate/workspace/skill-sync.ts +++ b/packages/opencode/src/altimate/workspace/skill-sync.ts @@ -173,36 +173,52 @@ export function describeSyncProblems(result: SyncResult): { title: string; messa * log, not for a toast. */ function skipReason(err: unknown): string { const msg = err instanceof Error ? err.message : "" - if (msg.startsWith("size mismatch")) return "its download was incomplete" + // 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 announced = new Map() +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. A clean result clears it, so the - * problem is announced again if it comes back. */ + * 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) { - announced.delete(key) + store.announced.delete(key) return false } - const text = `${problem.title}\n${problem.message}` - if (announced.get(key) === text) return false - announced.set(key, text) + 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> 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 @@ -211,6 +227,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 @@ -1147,8 +1164,12 @@ export async function syncSkills(directory: string): Promise { 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. - syncError ??= "could not update the workspace skills; kept the previous ones" + // 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 @@ -1180,6 +1201,10 @@ export async function syncSkills(directory: string): Promise { if (!changed && validated) await fs.writeFile(path.join(managedRoot(canon), SYNCED_MARKER), markerFor(validated, now)).catch(() => {}) } + // 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) diff --git a/packages/opencode/src/plugin/tui/altimate/workspace.tsx b/packages/opencode/src/plugin/tui/altimate/workspace.tsx index 0884f648b..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" @@ -1857,9 +1858,13 @@ async function runWorkspaceManage(api: TuiPluginApi, directory: string): Promise Manage.refresh(directory) .then((result) => { // Named, with reasons: a partial pull otherwise reads as success - // with fewer skills than the workspace has. - const skipped = result.skillsSkipped.map((s) => `${s.skill}: ${s.reason}`) - const problems = skipped.length > 0 ? [...result.errors, `skipped ${skipped.join("; ")}`] : result.errors + // 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, diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index b180ee915..6d8e6a8e1 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -403,11 +403,25 @@ export namespace SessionPrompt { // 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) - // Called on a clean result too: that is what clears the memo. - const announce = skillSync.shouldAnnounce(dir, problem) - if (problem && announce) { - const { notify } = await import("../altimate/workspace/engine-probes") - await notify({ ...problem, variant: "warning" }) + 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) })) diff --git a/packages/opencode/test/altimate/workspace/skill-sync.test.ts b/packages/opencode/test/altimate/workspace/skill-sync.test.ts index 82d933909..43769a18b 100644 --- a/packages/opencode/test/altimate/workspace/skill-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-sync.test.ts @@ -50,6 +50,7 @@ const { syncSkills, describeSyncProblems, shouldAnnounce, + forgetAnnouncement, recentlySynced, lastSuccessfulSyncAt, registryStale, @@ -329,7 +330,7 @@ describe("workspace skill sync", () => { 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 download was incomplete") + 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() }) @@ -384,6 +385,91 @@ describe("workspace skill sync", () => { 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 } + function serveSkills(spec: Record) { + globalThis.fetch = (async (input: string | URL) => { + const url = String(input) + 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 } }) + const result = await syncSkills(project) + + expect(result.error).toBe("could not install the workspace skills") + 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) @@ -1804,6 +1890,22 @@ describe("shouldAnnounce", () => { 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) diff --git a/packages/opencode/test/server/altimate-workspace-routes.test.ts b/packages/opencode/test/server/altimate-workspace-routes.test.ts index 2f9e0c1ec..4b9a22d31 100644 --- a/packages/opencode/test/server/altimate-workspace-routes.test.ts +++ b/packages/opencode/test/server/altimate-workspace-routes.test.ts @@ -40,7 +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: [], + skillsSkipped: [{ skill: "billing", reason: "it could not be downloaded" }], memory: { ok: true, status: "loaded", count: 4 }, errors: [], }) @@ -51,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") From 78fc7b81caa6f38387c084fa74057a28eb38044a Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 28 Sep 2026 03:31:14 +0530 Subject: [PATCH 3/4] test(workspace): answer the binding lookup in `serveSkills`, and pin the rebind reason `syncSkills` resolves the binding on the network every run. `serveSkills` did not answer that lookup, so the mock threw, the lookup was swallowed as "offline", and the new tests passed while running the stale-binding path instead of the bound one. It now confirms the workspace each test binds to. The rebind test also asserts the per-skill reason, so a regression that promised "kept the previous copy" after a rebind would fail. Co-Authored-By: Claude Opus 5.5 --- .../altimate/workspace/skill-sync.test.ts | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/opencode/test/altimate/workspace/skill-sync.test.ts b/packages/opencode/test/altimate/workspace/skill-sync.test.ts index 43769a18b..1ac3866de 100644 --- a/packages/opencode/test/altimate/workspace/skill-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-sync.test.ts @@ -388,9 +388,24 @@ describe("workspace skill sync", () => { // 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 } - function serveSkills(spec: Record) { + /** `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]) => ({ @@ -450,10 +465,12 @@ describe("workspace skill sync", () => { // 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 } }) + 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) }) From 47180fa5b6fb325758648a803e47cfeab37f86d3 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 28 Sep 2026 03:48:21 +0530 Subject: [PATCH 4/4] fix(workspace): address consensus review on the skipped-skill warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Show a skill id sanitised and capped. An id that failed `safePathComponent` is arbitrary text from the workspace service; control characters and line separators reached the toast and the serve JSON as-is. The raw id stays in the log. - Stay quiet when a binding lookup fails for a project with no sign of a workspace (no snapshot, no local binding). The server is always asked when there is no local row, so offline warned in every opted-in repo, including ones never linked. - Say "it could not be saved on this device" for local write failures (ENOSPC, EACCES, …) instead of blaming the download. - Keep the per-turn warning when the skill registry refresh fails; it had been skipped, for up to a poll interval after an account switch. Co-Authored-By: Claude Opus 5.5 --- .../src/altimate/workspace/skill-sync.ts | 47 ++++++++++++--- packages/opencode/src/session/prompt.ts | 7 ++- .../altimate/workspace/skill-sync.test.ts | 57 +++++++++++++++++++ 3 files changed, 101 insertions(+), 10 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-sync.ts b/packages/opencode/src/altimate/workspace/skill-sync.ts index 3f826a013..f2da90f05 100644 --- a/packages/opencode/src/altimate/workspace/skill-sync.ts +++ b/packages/opencode/src/altimate/workspace/skill-sync.ts @@ -43,7 +43,7 @@ import path from "path" import { Flag as CoreFlag } from "@opencode-ai/core/flag/flag" import { Log } from "@/altimate/util/log" import { AltimateApi } from "@/altimate/api/client" -import { resolveBindingOutcome, type CachedBinding } from "./state" +import { readLocalBinding, resolveBindingOutcome, type CachedBinding } from "./state" import { altimateRequest, WorkspaceApiError } from "./api-client" const log = Log.create({ service: "altimate-workspace-skill-sync" }) @@ -171,7 +171,12 @@ export function describeSyncProblems(result: SyncResult): { title: string; messa /** 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. */ -function skipReason(err: unknown): string { +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. @@ -181,6 +186,19 @@ function skipReason(err: unknown): string { 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}` } @@ -753,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. * @@ -886,11 +912,14 @@ export async function syncSkills(directory: string): Promise { if (outcome.status === "unbound") { if (await deactivate(canon, "this project is no longer bound to a workspace")) changed = true } - // Offline lands here, before the list is ever fetched: the binding is - // re-resolved on the network, so an outage fails this lookup first. So - // does a workspace this account cannot see, hence the wording. - // Unbound is a state, not a failure, and says nothing. - if (outcome.status === "unknown") + // 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 } @@ -1012,7 +1041,7 @@ export async function syncSkills(directory: string): Promise { skipped += 1 failed = true log.warn("skipping a workspace skill with an unusable id", { skill: summary.publicId }) - skippedSkills.push({ skill: summary.publicId, reason: "its id is not usable as a folder name" }) + skippedSkills.push({ skill: displayId(summary.publicId), reason: "its id is not usable as a folder name" }) continue } try { @@ -1101,7 +1130,7 @@ export async function syncSkills(directory: string): Promise { err: String(err), }) const why = skipReason(err) - skippedSkills.push({ skill: summary.publicId, reason: carried ? `${why} (kept the previous copy)` : why }) + skippedSkills.push({ skill: displayId(summary.publicId), reason: carried ? `${why} (kept the previous copy)` : why }) } } if (remote.length > 0 && Object.keys(next.skills).length === 0) { diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 6d8e6a8e1..8d0fe2b61 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -398,7 +398,12 @@ export namespace SessionPrompt { if (!(await skillSync.recentlySynced(dir))) { const applied = skillSync.syncSkills(dir).then(async (result) => { - await refreshRegistry() + // 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. diff --git a/packages/opencode/test/altimate/workspace/skill-sync.test.ts b/packages/opencode/test/altimate/workspace/skill-sync.test.ts index 1ac3866de..559b11889 100644 --- a/packages/opencode/test/altimate/workspace/skill-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-sync.test.ts @@ -51,6 +51,8 @@ const { describeSyncProblems, shouldAnnounce, forgetAnnouncement, + displayId, + skipReason, recentlySynced, lastSuccessfulSyncAt, registryStale, @@ -374,6 +376,31 @@ describe("workspace skill sync", () => { 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") @@ -1883,6 +1910,36 @@ describe("describeSyncProblems", () => { }) }) +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" }