feat(workspace): tell the user which workspace skills a sync skipped, and why (#1374)

* feat(workspace): tell the user which workspace skills a sync skipped, and why

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 <noreply@anthropic.com>

* fix(workspace): address review on the skipped-skill warnings

- 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* fix(workspace): address consensus review on the skipped-skill warnings

- 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Author: sahrizvi

packages/opencode/src/altimate/workspace/engine-probes.ts

@@ -274,14 +274,21 @@ function pidAlive(pid: number): boolean {
   }
 }
 
-export async function notify(toast: Toast): Promise<void> {
-  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<boolean> {
+  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
   }
 }
 

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<Re
   const errors: string[] = []
 
   let skillsChanged = false
+  let skillsSkipped: SkillSync.SkippedSkill[] = []
   try {
-    skillsChanged = (await SkillSync.syncSkills(directory)).changed
+    const result = await SkillSync.syncSkills(directory)
+    skillsChanged = result.changed
+    skillsSkipped = result.skipped
+    if (result.error) errors.push(`skills: ${result.error}`)
   } catch (err) {
     // `syncSkills` documents that it never throws. Caught anyway: this is the
     // user asking for a repair, and the one thing it must not do is fail the turn.
@@ -197,7 +204,7 @@ export async function refresh(directory: string, sessionID?: string): Promise<Re
     }
   }
 
-  return { skillsChanged, memory, memoryInvalidated, errors }
+  return { skillsChanged, skillsSkipped, memory, memoryInvalidated, errors }
 }
 
 /** Push: re-send local memory the workspace never received.

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" })
@@ -139,11 +139,104 @@ const STAGING_LEASE_MS = 30 * 60 * 1000
 
 const STORE_KEY = Symbol.for("altimate.workspace.skill-sync.store")
 
+export interface SkippedSkill {
+  skill: string
+  reason: string
+}
+
+export interface SyncResult {
+  changed: boolean
+  /** Skills this run dropped, each with a short, user-facing reason. Surfaced
+   * on the turn so a missing skill has an explanation instead of silence. */
+  skipped: SkippedSkill[]
+  /** Set when the whole sync could not proceed (unusable folder, unreadable
+   * credentials, unreachable list, or a failed publish). Per skill is
+   * `skipped`; this is the run. */
+  error?: string
+}
+
+/** One toast's worth of what a sync dropped, or null when nothing was.
+ * Pure — no UI imports — so the wording is testable; the caller shows it. */
+export function describeSyncProblems(result: SyncResult): { title: string; message: string } | null {
+  const n = result.skipped.length
+  if (n === 0) return result.error ? { title: "Workspace skills not synced", message: result.error } : null
+  const lines = result.skipped.slice(0, 3).map((s) => `${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<string, Promise<{ changed: boolean }>>
+  inFlight: Map<string, Promise<SyncResult>>
   lastSyncedAt: Map<string, number>
   registryAppliedAt: Map<string, number>
   syncedFor: Map<string, string>
+  /** 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<string, string>
 }
 
 const globals = globalThis as unknown as Record<symbol, SyncStore | undefined>
@@ -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<boolean> {
   }
 }
 
+/** 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<boolean> {
+  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<void> {
  * 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<SyncResult> {
   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 {

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(", ")}.`,