From c6f2d15159a9d879f8b5b94c35e411d307e66859 Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 15 Sep 2026 15:01:21 +0530 Subject: [PATCH 1/5] feat(workspace): show memory and skill-sync state in the sidebar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retargeted onto main after #1278 merged: the branch's history was rebuilt as one commit carrying only the sidebar change and its supporting fixes, which the stacked history had interleaved with #1278's own commits. The pane names the workspace but said nothing about whether local state has drifted from it, so there was no moment at which a user learned they have memory the workspace never received, or skills that have not synced. Two lines under the name and manage URL: 12 memories · 3 not synced skills synced 6m ago Status, not affordances. The tile refreshes every 30s and reacts at once to a link, unlink or rebind made in this process (`onBindingChanged`). Supporting changes, each with the review round that asked for it: - `Manage.status(dir, { poll })`: the poller resolves the memory setting on a rate-limited path (at most once per five minutes on a "no", never once it is "yes"); the `/workspace` menu stays cache-only and off the network. - `memory-sync`: the poller's scoped memo is its only cache, `pendingCount` trusts a settled enablement, `resetOverlay` clears every memo. - `skill-sync`: `lastSuccessfulSyncAt` reads a `.synced-at` marker written on clean runs only; a partial run and a clean up-to-date run both leave the manifest unusable for this. Missing or malformed marker is unknown. - `state`: binding-change listeners; `lastValidatedAt` stamped only when the server says bound. - Sidebar: scope-aware rebind (clears detail on an account switch even for a same-numbered workspace), floored age labels, coalesced refresh that stops after unmount. - Sync toast: `gatedBecause` names why a sweep never ran, and a gated sweep is never a green success. Tests: 630 pass across the workspace, plugin and telemetry suites on main; every guard above was mutation-checked in the original review rounds. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../opencode/src/altimate/workspace/manage.ts | 81 +++-- .../src/altimate/workspace/memory-sync.ts | 90 +++++- .../src/altimate/workspace/skill-sync.ts | 54 +++- .../opencode/src/altimate/workspace/state.ts | 76 ++++- .../plugin/tui/altimate/workspace-sidebar.tsx | 151 ++++++++- .../src/plugin/tui/altimate/workspace.tsx | 12 +- .../plugin/workspace-sync-message.test.ts | 35 +- .../test/altimate/workspace/manage.test.ts | 304 +++++++++++++++++- .../altimate/workspace/skill-sync.test.ts | 35 ++ 9 files changed, 793 insertions(+), 45 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/manage.ts b/packages/opencode/src/altimate/workspace/manage.ts index 1ad3792f3..b371ec394 100644 --- a/packages/opencode/src/altimate/workspace/manage.ts +++ b/packages/opencode/src/altimate/workspace/manage.ts @@ -44,11 +44,17 @@ export interface StatusReport { /** Blocks held locally for this project, and how many have not reached the * workspace. `null` when memory is off — "not synced" and "not applicable" are * different answers and a status line must not conflate them. */ - /** `unsynced: null` means the workspace's memory setting is not known from - * cache and status did not go to the network to find out. Rendering that as + /** `unsynced: null` means the workspace's memory setting is not known — from + * cache for the menu, which never asks; from the bounded poller path for the + * sidebar, when the service could not be reached. Rendering that as * 0 would tell the user their memory is current when nobody knows. */ memory: { local: number; unsynced: number | null } | null skillsEnabled: boolean + /** When workspace skills last synced successfully, or null if they have not in + * this process. Null is genuinely "unknown", not "never" — the store is + * per-process, so a fresh session has not synced yet even for a project whose + * snapshot is current on disk. Callers must not render it as "never synced". */ + skillsSyncedAt: number | null } export interface RefreshReport { @@ -92,22 +98,40 @@ export interface SyncReport { /** What the project is linked to and how far its local state has drifted. * * Cheap enough for a status line: one binding read from the local cache and, when - * memory is on, one index read. Network only when there is no cached row. */ -export async function status(directory: string): Promise { - // The cached row first, and the resolver only when there is none. A fresh - // clone, or a new machine, whose project is still bound server-side has no - // cached row, and reading only the cache answered "this project is not - // linked" with a lone Done — so that case asks. But the resolver revalidates - // a cached row too, and on the first call of a process nothing has been - // validated yet: the menu then sat on the API's full timeout when the - // service was unreachable. A cached row is taken as it is here; the poll and - // the operations behind the menu are what revalidate it. - const binding = - (await readLocalBinding(directory).catch(() => null)) ?? (await resolveBinding(directory).catch(() => null)) + * memory is on, one index read. Network only when there is no cached row, or + * on the poller path. */ +export async function status( + directory: string, + opts: { + /** Set for the sidebar poller. Resolves the workspace's memory setting + * through the poller path — the service is asked at most once every few + * minutes when the answer is "no" and not at all once it is "yes" — because + * on a cold cache nothing else would ever warm it, and the counts would + * simply never appear. The binding goes through the resolver too: the + * poll runs in the background, and it is what revalidates a cached row. + * + * Unset for the `/workspace` menu, which is awaited before the dialog can + * open and must NOT wait on the network: there the setting is read from + * cache alone and reported as unknown (`null`) if not held, and a cached + * row is taken as it is. `sync` does the live check. */ + poll?: boolean + } = {}, +): Promise { + // The cached row first, and the resolver only when there is none — for the + // menu. A fresh clone, or a new machine, whose project is still bound + // server-side has no cached row, and reading only the cache answered "this + // project is not linked" with a lone Done — so that case asks. But the + // resolver revalidates a cached row too, and on the first call of a process + // nothing has been validated yet: the menu then sat on the API's full + // timeout when the service was unreachable. + const binding = opts.poll + ? await resolveBinding(directory).catch(() => null) + : ((await readLocalBinding(directory).catch(() => null)) ?? (await resolveBinding(directory).catch(() => null))) return { binding, - memory: await memoryCounts(directory, binding), + memory: await memoryCounts(directory, binding, opts.poll === true), skillsEnabled: SkillSync.isEnabled(), + skillsSyncedAt: await SkillSync.lastSuccessfulSyncAt(directory), } } @@ -221,18 +245,35 @@ export async function sync(directory: string): Promise { * accurate claim — nothing is pending against a workspace that accepts nothing. * Best-effort: a status line must not fail because an index read did. * - * Cache-only. `status` is awaited before the `/workspace` dialog can appear, - * so it must not sit on the network: the enablement check behind `pendingCount` - * is a GET with a 15s budget, and on a slow or dead link the menu looked like it - * did nothing. When the setting is not known from cache the count is `null` — - * unknown — and `sync` does the live check. */ + * The enablement policy differs by caller, and both are deliberate. + * + * The `/workspace` menu (`poll` false) is awaited before the dialog can appear, + * so it reads the setting from cache alone: the check behind `pendingCount` is a + * GET with a 15s budget, and on a slow or dead link the menu looked like it did + * nothing. Not known from cache is `null`, and `sync` does the live check. + * + * The sidebar poller (`poll` true) may ask, on the rate-limited poller path. + * It runs in the background, so a wait costs nothing visible — and on a cold + * cache it is the only thing that will ever warm the answer the menu then + * reads. Either way, "unknown" is reported as unknown, never as zero. */ async function memoryCounts( directory: string, binding: CachedBinding | null, + poll: boolean, ): Promise<{ local: number; unsynced: number | null } | null> { if (!MemorySync.isEnabled()) return null try { const blocks = await MemoryStore.listAll({ directory }) + if (poll && binding) { + const status = await MemorySync.memoryEnabledForPoller(binding) + // "disabled" is a real answer: memory is off, so nothing is outstanding + // and 0 is the truth. "unknown" is not — the service could not be + // reached, and reporting 0 there claims the workspace is up to date on + // the strength of a failed request. + if (status !== "enabled") return { local: blocks.length, unsynced: status === "disabled" ? 0 : null } + // Enablement is settled; `pendingCount`'s own gate must not re-ask. + return { local: blocks.length, unsynced: await MemorySync.pendingCount(blocks, binding, { trustEnabled: true }) } + } return { local: blocks.length, unsynced: await MemorySync.pendingCount(blocks, binding, { network: false }) } } catch (err) { log.warn("could not count local memory for the workspace status", { err: String(err) }) diff --git a/packages/opencode/src/altimate/workspace/memory-sync.ts b/packages/opencode/src/altimate/workspace/memory-sync.ts index 7866aec84..aeedc0e6d 100644 --- a/packages/opencode/src/altimate/workspace/memory-sync.ts +++ b/packages/opencode/src/altimate/workspace/memory-sync.ts @@ -27,6 +27,7 @@ import { TRAINING_META_COMMENT } from "@/altimate/training/types" import { resolveBinding as resolveProjectBinding, type CachedBinding } from "./state" import { indexKey, readIndex, readIndexEntry, recordIndexEntry } from "./memory-index" import { WorkspaceApi } from "./api-client" +import { AltimateApi } from "@/altimate/api/client" import { LIST_LIMIT, MemoryApi, @@ -207,8 +208,17 @@ async function memoryEnabled(binding: CachedBinding): Promise { /** Three-way, because a read that cannot reach the service must not be reported * as "this workspace has no memory" — that reads as success while destroying * whatever the session already had. */ -async function memoryStatus(binding: CachedBinding): Promise<"enabled" | "disabled" | "error"> { - const cached = memoryEnabledCache.get(binding.datamateId) +async function memoryStatus( + binding: CachedBinding, + opts: { + /** Skip the positive cache and ask. The cache is keyed by bare workspace + * id, which is safe for the write path (its credentials are fixed) but not + * for a caller whose own memo is tenant-scoped: on a memo miss it must not + * inherit a positive written under a previous account. */ + fresh?: boolean + } = {}, +): Promise<"enabled" | "disabled" | "error"> { + const cached = opts.fresh ? undefined : memoryEnabledCache.get(binding.datamateId) if (cached && Date.now() - cached.checkedAt < MEMORY_ENABLED_TTL_MS) return "enabled" try { const workspaces = await WorkspaceApi.listDatamates() @@ -660,6 +670,75 @@ async function runQueue( return { ok, failed, declined, skipped, deferred } } +/** How long a poller trusts either answer. Deliberately separate from + * `MEMORY_ENABLED_TTL_MS` and from the main cache: `memoryEnabled` stays + * positive-only with a 60s TTL so the WRITE path picks up a newly enabled + * workspace almost at once, which is the property that matters for not losing + * memory. A poller can afford to be a few minutes behind; what it cannot afford + * is a request every tick. + * + * Both answers are memoized, not just the "no". Reusing the write path's 60s + * positive meant that a minute after the first check an ENABLED workspace went + * back to the network on every other tick — a steady drip of `/datamates` + * requests for the life of the session. (cubic P2 on #1279.) */ +const POLL_TTL_MS = 5 * 60 * 1000 + +/** Keyed by tenant and API URL as well as workspace id. Workspace ids are + * tenant-local, so a bare id let a same-numbered workspace in a NEWLY switched + * account inherit the previous tenant's answer and hide its unsynced count for + * the whole TTL. (cubic P2 on #1279.) */ +const pollMemo = new Map() + +async function pollMemoKey(binding: CachedBinding): Promise { + try { + const creds = await AltimateApi.getCredentials() + return `${creds.altimateInstanceName}|${creds.altimateUrl}|${binding.datamateId}` + } catch { + // No credentials resolved: fall back to an id-only key. The caller is about + // to fail its lookup anyway, and a wrong-tenant hit is impossible when + // there is no tenant. + return `?|?|${binding.datamateId}` + } +} + +/** Whether this workspace has memory on, for a caller that polls. + * + * Three-way on purpose. `memoryEnabled` folds "the service could not be + * reached" into `false`, which is right for the write path — it fails closed so + * an outage cannot leak a mirror — but wrong for a status line: rendering an + * unreachable service as "0 not synced" tells the user their memory is current + * when nobody knows. `memoryStatus` already draws that distinction; this used + * to throw it away and then memoize the result for five minutes. (cubic P2 on + * #1279.) + * + * An "unknown" is never memoized: the next tick should ask again rather than + * inherit a network blip. */ +export async function memoryEnabledForPoller( + binding: CachedBinding, +): Promise<"enabled" | "disabled" | "unknown"> { + // The scoped memo is the poller's only cache. `memoryEnabledCache` is keyed by + // bare workspace id — right for the write path, which is tenant-bound by its + // credentials — but consulting it here reopened a 60s cross-tenant window the + // scoped memo had closed: a positive written under the previous account was + // served to a same-numbered workspace in the next. One extra request per five + // minutes per tenant is the price, and `memoryStatus` still warms both. + const key = await pollMemoKey(binding) + const memo = pollMemo.get(key) + if (memo && Date.now() - memo.at < POLL_TTL_MS) return memo.status + const status = await memoryStatus(binding, { fresh: true }) + if (status === "error") return "unknown" + pollMemo.set(key, { at: Date.now(), status }) + return status +} + +/** Test seam: the poller memo is process-global and would otherwise leak between + * cases in the same file. */ +export function resetPollMemoForTests(): void { + pollMemo.clear() + memoryEnabledCache.clear() + memoryDisabledMemo.clear() +} + /** Split blocks into those the workspace still needs and those already there at * their current payload. * @@ -718,10 +797,17 @@ export async function pendingCount( * known"), never wait on the network. The default asks, and is what a * sweep wants. */ network?: boolean + /** The caller has already resolved the workspace's setting as enabled — + * the poller does, on its own rate-limited path — so the gate here must + * not ask again. Without this the poll dripped a `/datamates` request every + * 60s once the write path's positive expired, which is the exact drip the + * poller memo exists to stop. */ + trustEnabled?: boolean } = {}, ): Promise { if (blocks.length === 0) return 0 if (!isEnabled()) return 0 + if (opts.trustEnabled && binding) return partitionPending(blocks, binding, await readIndex()).pending.length if (opts.network === false && binding) { const cached = memoryEnabledCached(binding) if (cached === "unknown") return null diff --git a/packages/opencode/src/altimate/workspace/skill-sync.ts b/packages/opencode/src/altimate/workspace/skill-sync.ts index 2fc157a6e..3ab4fc734 100644 --- a/packages/opencode/src/altimate/workspace/skill-sync.ts +++ b/packages/opencode/src/altimate/workspace/skill-sync.ts @@ -68,6 +68,10 @@ const MANAGED_DIR = path.join(".altimate-code", "skill", "_workspace") * discovery scans. See the swap in `syncSkills`. */ const STAGING_DIR = path.join(".altimate-code", "skill-staging") const MANIFEST_NAME = ".manifest.json" +/** Written at the managed root after every CLEAN run, holding the epoch ms. + * The manifest cannot serve: a partial run publishes one too, and a clean run + * that finds the snapshot up to date publishes nothing. */ +const SYNCED_MARKER = ".synced-at" export interface ManifestSkill { /** Server's ``updated_at``, verbatim. The only change signal the API offers. */ @@ -251,6 +255,40 @@ export async function flushPendingSyncs(timeoutMs = 30_000): Promise { } } +/** When this project's workspace skills last synced successfully, or null if + * they never have in this process. + * + * Exposed for the sidebar. `recentlySynced` answers a boolean against the poll + * interval, which cannot say "6 minutes ago" — and a status line whose whole job + * is to make staleness visible needs the age, not a threshold. Reads the + * process-global store, so the TUI plugin realm sees the same map the sync + * writes (see `STORE_KEY` above). */ +export async function lastSuccessfulSyncAt(directory: string): Promise { + // From disk, not from the map. The map is on `globalThis`, which is shared + // across module realms but NOT across threads — and the per-message sync + // that does most of the stamping runs in the server worker, while the TUI + // and its sidebar render on the main thread. Read from the map alone, the + // "skills synced Xm ago" line never saw the syncs that actually happened. + // The marker is written on exactly the runs that stamp the map, so the two + // agree; the manifest's mtime did not — a partial run publishes one, and a + // clean up-to-date run publishes nothing. + try { + // Validated, not coerced: `Number("")` is 0, and a truncated marker would + // otherwise render as a sync from 1970. + const raw = (await fs.readFile(path.join(managedRoot(directory), SYNCED_MARKER), "utf8")).trim() + if (!/^\d+$/.test(raw)) return null + const at = Number(raw) + return Number.isSafeInteger(at) && at > 0 ? at : null + } catch (err) { + // No snapshot is no sync: after an unlink, a rebind, or an empty + // workspace, an in-memory stamp from before would report a sync that no + // longer describes what is on disk. + const code = (err as NodeJS.ErrnoException)?.code + if (code === "ENOENT" || code === "ENOTDIR") return null + return lastSyncedAt.get(path.resolve(directory)) ?? null + } +} + /** Has this project's snapshot been checked within the poll interval? Callers * on a per-message path use this to skip the network entirely. */ export async function recentlySynced(directory: string): Promise { @@ -323,10 +361,10 @@ function safePathComponent(p: unknown): p is string { if (typeof p !== "string" || !p) return false if (p === "." || p === "..") return false if (path.isAbsolute(p)) return false - // These two are written as FILES at the staged root. An id of either name - // becomes a directory there, the write fails EISDIR, and that workspace can - // never sync again. - if (p === MANIFEST_NAME || p === ".gitignore") return false + // These are written as FILES at the managed root. An id of any of these + // names becomes a directory there, the write fails EISDIR, and that + // workspace can never sync again. + if (p === MANIFEST_NAME || p === ".gitignore" || p === SYNCED_MARKER) return false return !/[\\/\0]/.test(p) } @@ -977,7 +1015,13 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean } // 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. - if (ok && !failed && sawRemote) lastSyncedAt.set(canon, Date.now()) + if (ok && !failed && sawRemote) { + const now = Date.now() + lastSyncedAt.set(canon, now) + // Only where a snapshot exists: a clean run against an empty workspace + // removed the root, and there is nothing for an age to describe. + await fs.writeFile(path.join(managedRoot(canon), SYNCED_MARKER), String(now)).catch(() => {}) + } return { changed } })() inFlight.set(canon, settled) diff --git a/packages/opencode/src/altimate/workspace/state.ts b/packages/opencode/src/altimate/workspace/state.ts index ed3e82541..8538a44de 100644 --- a/packages/opencode/src/altimate/workspace/state.ts +++ b/packages/opencode/src/altimate/workspace/state.ts @@ -389,11 +389,15 @@ export async function resolveBindingOutcome(directory: string): Promise void>() + +export function onBindingChanged(listener: () => void): () => void { + bindingChangeListeners.add(listener) + return () => { + bindingChangeListeners.delete(listener) + } +} + +/** Never throws: a listener is a UI refresh, and one bad subscriber must not + * fail the link or unlink that notified it. Iterates a copy so a listener that + * unsubscribes itself mid-notify cannot skip the next one. */ +function notifyBindingChanged(): void { + // Snapshot first: a listener may subscribe or unsubscribe while being + // notified, and iterating the live Set would then walk a collection that + // changed underneath us. + const listeners = Array.from(bindingChangeListeners) + for (const listener of listeners) { + try { + listener() + } catch (err) { + log.warn("a binding-change listener threw", { err: String(err) }) + } + } +} + /** Every key in the file that names this directory. Normally one — the * canonical path — but a file written before keys were canonicalised, or whose * migration could not be written back, can still hold a raw alias @@ -537,6 +580,22 @@ function forgetBinding( } catch (err) { log.warn("could not drop a binding the server no longer recognises", { err: String(err) }) } + // Outside the try on purpose. A listener is a UI refresh; its failure is not + // a failed cache drop, and notifying from inside would log a throwing + // subscriber as "could not drop a binding" — a misleading line about a write + // that had already succeeded. + // + // Notified even when the write FAILED, which is not obvious. The server-side + // unlink has already happened by the time we get here, and the resolve path + // does not depend on this file having been rewritten: `clearLocalBinding` + // drops the revalidation stamp and records a lookup miss, so the next resolve + // asks the server, hears "unbound", and the tile updates. Skipping the + // notification on a failed write left the pane naming a workspace this + // project is no longer bound to until the next poll — the exact lag the + // notifier exists to remove, in the case where something is already wrong. + // (An earlier version guarded this on whether the write succeeded, and + // called the difference unobservable. It was not.) + notifyBindingChanged() return true } @@ -704,6 +763,9 @@ export async function recordApprovedBinding( // synchronously on the `link` path, which awaits the seed. let bindingChanged = true let alreadySeeded = false + /** The name as it was on disk, so a rename can be detected even when the + * binding's identity is unchanged. `undefined` when there was no prior row. */ + let priorName: string | undefined try { const existing = readCache() const cache: CacheFile = @@ -711,6 +773,7 @@ export async function recordApprovedBinding( ? existing : { version: CACHE_VERSION, tenant: key.tenant, apiUrl: key.apiUrl, bindings: {} } const prior = cache.bindings[canonicalizeKey(directory)] + priorName = prior?.datamateName bindingChanged = !prior || !sameBinding(prior, binding) alreadySeeded = !bindingChanged && !!prior?.seededAt // Carry the seed marker across a warm so a completed seed is not repeated. @@ -727,6 +790,17 @@ export async function recordApprovedBinding( }) } + // Only when something a subscriber could render actually changed. A warm + // cache re-read must not wake the tile on every resolve. + // + // `bindingChanged` alone is not the right test: `sameBinding` compares + // identity (id, remote, path) because it also gates the memory seed, and + // widening it would re-seed a whole workspace every time someone renamed one. + // But the sidebar renders `datamateName`, so a rename is a visible change + // with an unchanged identity. Checked separately for that reason. (cubic P2 + // on #1279.) + if (bindingChanged || priorName !== binding.datamateName) notifyBindingChanged() + // altimate_change start - seed the workspace with the memory this machine // already holds. Deliberately OUTSIDE the try above: a failed cache write // must not skip the backfill, and a failed backfill must not read as a failed diff --git a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx index f00d8ab06..dd5ea915f 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx @@ -9,7 +9,10 @@ import type { TuiPlugin, TuiPluginApi } from "@opencode-ai/plugin/tui" import type { BuiltinTuiPlugin } from "@opencode-ai/tui/builtins" import { createSignal, onCleanup, onMount, Show } from "solid-js" -import { readLocalBinding, type CachedBinding } from "@/altimate/workspace/state" +import { onBindingChanged, resolveBindingOutcome, type CachedBinding } from "@/altimate/workspace/state" +// altimate_change start - status lines +import * as Manage from "@/altimate/workspace/manage" +// altimate_change end import { buildManageUrl, resolveWorkspaceWebUrl } from "@/altimate/workspace/browser-handoff" import { getResolvedWorkspaceId } from "@/altimate/workspace/session-context" import { AltimateApi } from "@/altimate/api/client" @@ -50,27 +53,123 @@ async function resolveManageBase(): Promise { } } +// altimate_change start - status lines +/** Coarse relative age. Deliberately not a timestamp: the point is "is this + * stale?", and a clock time makes the reader do the subtraction. */ +function describeAge(at: number): string { + // Floored from the raw elapsed time, so every label owns a full window: + // "1m ago" is 60–119s. Rounding — and rounding twice, seconds then minutes + // — had squeezed it into ~30s (89.5s → 90s → "2m ago"). + const ms = Math.max(0, Date.now() - at) + if (ms < 60_000) return "just now" + const minutes = Math.floor(ms / 60_000) + if (minutes < 60) return `${minutes}m ago` + return `${Math.floor(ms / 3_600_000)}h ago` +} +// altimate_change end + function View(props: { api: TuiPluginApi }) { const theme = () => props.api.theme.current - const [binding, setBinding] = createSignal(null) + // Three states, not two. `undefined` is "the first read has not come back + // yet"; `null` is "read, and this project is not linked". Starting at `null` + // made the pane assert "Not linked — run altimate-code link" for the first + // moments of every session, including projects that ARE linked — a false + // statement plus an instruction to run a command the user does not need. + const [binding, setBinding] = createSignal(undefined) const [manageUrl, setManageUrl] = createSignal(null) + // altimate_change start - status lines + const [detail, setDetail] = createSignal(null) + // altimate_change end let refreshInFlight = false + let refreshQueued = false + let disposed = false + /** `tenant|apiUrl` the current binding was resolved under. */ + let boundScope: string | null = null + const currentScope = async (): Promise => { + try { + const creds = await AltimateApi.getCredentials() + return `${creds.altimateInstanceName}|${creds.altimateUrl}` + } catch { + return null + } + } const refresh = async () => { - if (refreshInFlight) return + // Coalesce rather than drop. A binding-change notification can land while a + // poll is mid-flight, and that pass may already have read the old binding — + // returning early would leave the tile stale until the next tick, which is + // exactly the lag the listener exists to remove. One queued re-run is + // enough however many notifications arrive while we are busy. + if (refreshInFlight) { + refreshQueued = true + return + } refreshInFlight = true try { const dir = props.api.state.path.directory - const b = await readLocalBinding(dir).catch(() => null) - setBinding(b) - if (!b) { + // Resolve, don't just read the cache. `readLocalBinding` never touches the + // network, so on a cold cache it returns null and this tile asserted + // "Not linked — run altimate-code link" about a project that IS linked, + // until some unrelated code path happened to warm the cache. Same shape as + // the counts bug directly above. + // + // `resolveBindingOutcome` is already safe to poll: a confirmed "unbound" + // is memoized for MISS_TTL_MS and a known binding is trusted for + // REVALIDATE_MS, so the worst case is one request per five minutes. + // "unknown" (unreachable, 5xx) deliberately leaves the last answer + // standing — a network blip must not downgrade a working tile to + // "Not linked", which is the one state that tells the user to go and run + // a command. + const outcome = await resolveBindingOutcome(dir).catch(() => ({ status: "unknown" }) as const) + if (outcome.status === "bound") { + // Counts and the manage URL belong to a SPECIFIC workspace. On a rebind + // they would otherwise keep describing the old one until the new status + // resolved — the wrong numbers under the right name. Cleared only on a + // real change; an "unknown" outcome deliberately leaves everything + // standing rather than blanking a working tile over a blip. (cubic P2 + // on #1279.) + // Compared with the account scope, not the id alone. Workspace ids are + // tenant-local, so after an account switch a same-numbered workspace + // in the new tenant would otherwise be treated as unchanged and keep + // the old counts under the new name. + const scope = await currentScope() + if (binding()?.datamateId !== outcome.binding.datamateId || scope !== boundScope) { + setDetail(null) + setManageUrl(null) + } + boundScope = scope + setBinding(outcome.binding) + } else if (outcome.status === "unbound") { + setDetail(null) setManageUrl(null) - return + setBinding(null) } + const b = binding() + // No clear here: every path that reaches this with no binding has already + // cleared the manage URL, or never set one. + if (!b) return const base = await resolveManageBase() setManageUrl(base ? buildManageUrl(base, b.datamateId) : null) + // altimate_change start - status lines + // `poll: true` marks this as the POLLER path: `status` then resolves the + // memory setting through a rate-limited resolver that asks at most once + // every few minutes on a "no" and never once it is "yes", instead of once + // per POLL_MS. It is a bound, not a ban — reading it as "never ask" is + // what left these counts blank until something else happened to warm the + // cache. The `/workspace` menu, by contrast, is cache-only, because it is + // awaited before the dialog can open. See `Manage.status`. + setDetail(await Manage.status(dir, { poll: true }).catch(() => null)) + // altimate_change end } finally { refreshInFlight = false + // Not after disposal. A notification can land mid-refresh and unmount can + // follow before it settles, and the queued run would then do network and + // status work for a view nobody is looking at, writing to signals that no + // longer render. (cubic P3 on #1279.) + if (refreshQueued && !disposed) { + refreshQueued = false + void refresh() + } } } @@ -79,7 +178,18 @@ function View(props: { api: TuiPluginApi }) { const timer = setInterval(() => void refresh(), POLL_MS) // eslint-disable-next-line @typescript-eslint/no-explicit-any ;(timer as any)?.unref?.() - onCleanup(() => clearInterval(timer)) + // Link and unlink happen in THIS process, so the tile can hear about them + // directly instead of waiting out the poll. Without this, Unlink shows a + // success toast while the pane beside it keeps naming the workspace for up + // to POLL_MS — the UI contradicting itself, with the stale half looking + // authoritative. The interval stays: it is what catches a change made by + // another process, which no in-process listener can see. + const unsubscribe = onBindingChanged(() => void refresh()) + onCleanup(() => { + disposed = true + clearInterval(timer) + unsubscribe() + }) }) return ( @@ -90,9 +200,14 @@ function View(props: { api: TuiPluginApi }) { - Not linked — run altimate-code link - + // Only once a read has actually returned `null`. While the answer is + // still unknown the tile shows its heading and nothing under it, + // which reads as "loading" rather than as a claim. + + + Not linked — run altimate-code link + + } > {(b) => ( @@ -135,6 +250,20 @@ function View(props: { api: TuiPluginApi }) { (pinned via --workspace) + {/* altimate_change start - status lines: what has drifted, so the + * reason to run `/workspace` is visible before you need it. */} + + {(m) => ( + + {m().local} {m().local === 1 ? "memory" : "memories"} + {m().unsynced !== null && m().unsynced! > 0 ? ` · ${m().unsynced} not synced` : ""} + + )} + + + {(at) => {`skills synced ${describeAge(at())}`}} + + {/* altimate_change end */} {(u) => ( openManageUrl(props.api, u())}> diff --git a/packages/opencode/src/plugin/tui/altimate/workspace.tsx b/packages/opencode/src/plugin/tui/altimate/workspace.tsx index 8c81c11b7..228a279aa 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace.tsx @@ -1733,6 +1733,16 @@ export { syncMessage as syncMessageForTests } * sweep that sent nothing because everything was refused or deferred read as a * clean all-clear. `skipped` alone (present at its current payload) is the * healthy case, and is deliberately not surfaced as a number. */ +/** Paired with `syncMessage`. A gated sweep has every count at zero, which the + * count-based rule read as a green success — with "Could not read this + * project's local memory" as the text. A failed read is a warning; the other + * gates are states, not outcomes, and are told as information. */ +function syncVariant(result: Manage.SyncReport): "info" | "success" | "warning" { + if (result.gated) return result.gatedBecause === "read-failed" ? "warning" : "info" + return result.failed > 0 || result.declined > 0 || result.deferred > 0 ? "warning" : "success" +} +export { syncVariant as syncVariantForTests } + function syncMessage(result: Manage.SyncReport): string { if (result.gated) { switch (result.gatedBecause) { @@ -1830,7 +1840,7 @@ async function runWorkspaceManage(api: TuiPluginApi, directory: string): Promise Manage.sync(directory) .then((result) => { api.ui.toast({ - variant: result.failed > 0 || result.declined > 0 || result.deferred > 0 ? "warning" : "success", + variant: syncVariant(result), message: syncMessage(result), duration: 8_000, }) diff --git a/packages/opencode/test/altimate/plugin/workspace-sync-message.test.ts b/packages/opencode/test/altimate/plugin/workspace-sync-message.test.ts index ac1edd254..edf0d8643 100644 --- a/packages/opencode/test/altimate/plugin/workspace-sync-message.test.ts +++ b/packages/opencode/test/altimate/plugin/workspace-sync-message.test.ts @@ -6,7 +6,10 @@ // already in the workspace", and one folded deferrals into "already present" so // a sweep that sent nothing read as a clean all-clear. import { describe, expect, test } from "bun:test" -import { syncMessageForTests as message } from "../../../src/plugin/tui/altimate/workspace" +import { + syncMessageForTests as message, + syncVariantForTests as variant, +} from "../../../src/plugin/tui/altimate/workspace" const report = (over: Partial[0]> = {}) => ({ gated: false, @@ -48,6 +51,17 @@ describe("the sync toast", () => { expect(message(report({ gated: true }))).toContain("memory is off") }) + test("a gated sweep is never a green success", () => { + // Every count is zero when a sweep never ran, and the count-based rule + // rendered "Could not read this project's local memory" in the success + // colour. + expect(variant(report({ gated: true, gatedBecause: "read-failed" }))).toBe("warning") + expect(variant(report({ gated: true, gatedBecause: "memory-off" }))).toBe("info") + expect(variant(report({ gated: true, gatedBecause: "no-binding" }))).toBe("info") + expect(variant(report({ skipped: 3 }))).toBe("success") + expect(variant(report({ deferred: 1 }))).toBe("warning") + }) + test("names the actual reason a sweep never ran", () => { // Four things gate a sweep and only one is the workspace's memory toggle. // Told "memory is off" for a failed local read, the user went to a setting @@ -72,4 +86,23 @@ describe("the sync toast", () => { expect(out).toContain("3 deferred") expect(out).not.toContain("all") }) + + test("does not hide transport failures behind a refusal", () => { + // Second regression, found the same way as the first: six blocks, five + // refused and one failed, reported as "The workspace refused all 5 + // memories". The failure was dropped and "all" was false. A transport + // failure is the retryable outcome — it is the one that must survive. + const out = message(report({ sent: 0, failed: 1, declined: 5 })) + expect(out).toContain("1 failed") + expect(out).toContain("5 refused") + expect(out).not.toContain("all 5") + }) + + test("still claims 'all' only when the refusal really was all of it", () => { + expect(message(report({ declined: 4 }))).toContain("refused all 4") + }) + + test("leads with what happened, not a count of zero", () => { + expect(message(report({ sent: 0, failed: 2, declined: 1 }))).toContain("Nothing was sent") + }) }) diff --git a/packages/opencode/test/altimate/workspace/manage.test.ts b/packages/opencode/test/altimate/workspace/manage.test.ts index 425713bc0..e741a3f72 100644 --- a/packages/opencode/test/altimate/workspace/manage.test.ts +++ b/packages/opencode/test/altimate/workspace/manage.test.ts @@ -41,11 +41,17 @@ afterAll(() => { const { AltimateApi } = await import("../../../src/altimate/api/client") const { unlink, sync, status, refresh } = await import("../../../src/altimate/workspace/manage") -const { resetEnablementMemoForTests } = await import("../../../src/altimate/workspace/memory-sync") -const { readLocalBinding, recordApprovedBinding, resolveBindingOutcome, expireValidationForTests, cachePath } = - await import("../../../src/altimate/workspace/state") +const { + readLocalBinding, + recordApprovedBinding, + onBindingChanged, + resolveBindingOutcome, + expireValidationForTests, + cachePath, +} = await import("../../../src/altimate/workspace/state") const { resolveProjectIdentifier } = await import("../../../src/altimate/workspace/detect") -const { pendingCount } = await import("../../../src/altimate/workspace/memory-sync") +const { resetPollMemoForTests, resetEnablementMemoForTests, pendingCount, memoryEnabledCache, memoryEnabledForPoller } = + await import("../../../src/altimate/workspace/memory-sync") type Creds = Awaited> const originalIsConfigured = AltimateApi.isConfigured @@ -190,6 +196,181 @@ describe("status", () => { expect(report.binding).toBeNull() }) + + test("a poller resolves the workspace setting once, not on every tick", async () => { + // The sidebar calls this every 30 seconds. The shared enablement cache is + // positive-only — a workspace with memory switched OFF is never memoized — + // so asking it directly on each tick would put a request on the wire every + // 30 seconds, forever, for exactly the workspaces whose answer is "no". + // + // The fix is a bound, NOT a ban. An earlier version refused the network + // outright and the counts then never appeared at all on a session where + // nothing else warmed the cache — the very drift the line exists to surface. + await bind(projectDir) + resetPollMemoForTests() + // Counted against the workspace-list endpoint specifically, not every + // request: `recordApprovedBinding` starts a fire-and-forget backfill whose + // traffic lands at an unpredictable moment, so a total-request assertion + // passes alone and fails in a full run. + const listCalls = () => requests.filter((r) => r.url.includes("/datamates")).length + const before = listCalls() + + await status(projectDir, { poll: true }) + const afterFirst = listCalls() + await status(projectDir, { poll: true }) + await status(projectDir, { poll: true }) + + // The first poll asks. + expect(afterFirst).toBeGreaterThan(before) + // The next two do not. + expect(listCalls()).toBe(afterFirst) + }) + + test("a poller still reports the local block count when memory is off", async () => { + // "How many memories do I have" is answerable without the service; only + // "how many are outstanding" depends on the workspace setting. Reporting + // nothing at all would hide the first fact to protect the second. + await bind(projectDir) + resetPollMemoForTests() + + const report = await status(projectDir, { poll: true }) + + expect(report.memory).not.toBeNull() + // The stub workspace has memory off, so nothing is outstanding — a sweep + // would refuse to send any of it. + expect(report.memory?.unsynced).toBe(0) + expect(report.binding?.datamateName).toBe("Growth") + }) +}) + +describe("binding-change notifications", () => { + // The sidebar tile polls every 30s. Without these, Unlink shows a success + // toast while the pane beside it keeps naming the workspace until the next + // tick — the UI contradicting itself, with the stale half looking + // authoritative. Found by watching the real TUI, like the rest of this file. + test("unlink wakes subscribers so the tile does not wait out the poll", async () => { + await bind(projectDir) + let fired = 0 + const stop = onBindingChanged(() => { + fired++ + }) + try { + await unlink(projectDir) + expect(fired).toBeGreaterThan(0) + } finally { + stop() + } + }) + + test("a new bind wakes subscribers", async () => { + let fired = 0 + const stop = onBindingChanged(() => { + fired++ + }) + try { + await bind(projectDir) + expect(fired).toBeGreaterThan(0) + } finally { + stop() + } + }) + + test("re-recording the SAME binding does not", async () => { + // A warm cache re-read is not a change. Waking the tile on every resolve + // would undo the point of the poll interval. + await bind(projectDir) + let fired = 0 + const stop = onBindingChanged(() => { + fired++ + }) + try { + await bind(projectDir) + expect(fired).toBe(0) + } finally { + stop() + } + }) + + test("unsubscribing stops them", async () => { + let fired = 0 + const stop = onBindingChanged(() => { + fired++ + }) + stop() + await bind(projectDir) + expect(fired).toBe(0) + }) + + test("a listener that throws does not fail the unlink", async () => { + await bind(projectDir) + const stop = onBindingChanged(() => { + throw new Error("subscriber blew up") + }) + try { + const report = await unlink(projectDir) + expect(report.removedServerSide).toBe(true) + } finally { + stop() + } + }) +}) + +describe("what the status line is allowed to claim", () => { + test("does not report '0 not synced' when the workspace setting cannot be resolved", async () => { + // The write path folds "unreachable" into "disabled" on purpose — it fails + // closed so an outage cannot leak a mirror. A status line must not inherit + // that: rendering an unreachable service as "nothing outstanding" tells the + // user their memory is current on the strength of a failed request. + await bind(projectDir) + resetPollMemoForTests() + const failing = globalThis.fetch + globalThis.fetch = (async (input: any, init?: any) => { + const url = typeof input === "string" ? input : input.url + if (url.includes("/datamates")) throw new Error("network down") + return failing(input, init) + }) as typeof fetch + + const report = await status(projectDir, { poll: true }) + expect(report.memory).not.toBeNull() + // Local blocks are still countable without a service; how many are + // outstanding is genuinely unknown, and null is how that is said. + expect(report.memory?.unsynced).toBeNull() + }) + + test("still reports 0 outstanding when memory is genuinely off", async () => { + // The contrast that gives the test above its meaning: "disabled" IS an + // answer, and 0 is the truth for it. + await bind(projectDir) + resetPollMemoForTests() + const report = await status(projectDir, { poll: true }) + expect(report.memory?.unsynced).toBe(0) + }) +}) + +describe("renames", () => { + test("wake the sidebar even though the binding identity is unchanged", async () => { + // `sameBinding` compares id/remote/path because it also gates the memory + // seed — widening it would re-seed a workspace on every rename. But the + // tile renders the NAME, so a rename is a visible change that the identity + // check alone would swallow. + await bind(projectDir) + let fired = 0 + const stop = onBindingChanged(() => { + fired++ + }) + try { + await recordApprovedBinding(projectDir, { + datamateId: 42, + datamateName: "Growth Renamed", + repoRemote: "git@github.com:acme/app.git", + projectPath: projectDir, + linkedAt: Date.now(), + } as any) + expect(fired).toBeGreaterThan(0) + } finally { + stop() + } + }) }) describe("what unlink leaves on disk", () => { @@ -757,3 +938,118 @@ describe("what /workspace status may cost and claim (review round 2)", () => { expect(report.skillsLeftBehind).toBe(false) }) }) + +describe("the sidebar's skills-synced age", () => { + test("comes from the snapshot on disk, not a per-thread map", async () => { + // The per-message sync stamps its map in the server worker; the sidebar + // reads on the main thread, which has its own `globalThis` and so its own + // map. The marker the sync writes on every clean run is the answer both + // threads can see. + await bind(projectDir) + const managed = path.join(projectDir, ".altimate-code", "skill", "_workspace") + mkdirSync(managed, { recursive: true }) + const before = Date.now() - 5 * 60_000 + writeFileSync(path.join(managed, ".synced-at"), String(before)) + + const report = await status(projectDir) + + expect(report.skillsSyncedAt).toBe(before) + }) + + test("is nothing when the marker is empty or garbage", async () => { + // A truncated marker must read as unknown, not as a sync from 1970. + await bind(projectDir) + const managed = path.join(projectDir, ".altimate-code", "skill", "_workspace") + mkdirSync(managed, { recursive: true }) + for (const junk of ["", " \n", "soon", "12abc"]) { + writeFileSync(path.join(managed, ".synced-at"), junk) + expect((await status(projectDir)).skillsSyncedAt).toBeNull() + } + }) + + test("is nothing when there is no snapshot", async () => { + // After an unlink, a rebind or an empty workspace the root is gone, and an + // in-memory stamp from before would describe a sync that no longer is. + await bind(projectDir) + const report = await status(projectDir) + expect(report.skillsSyncedAt).toBeNull() + }) +}) + +describe("the poller does not drip", () => { + test("once enablement is memoized as enabled, computing the count asks nothing", async () => { + await bind(projectDir) + resetPollMemoForTests() + // A real block, or `pendingCount` returns before its gate and the test + // cannot see whether the gate asks. + const memDir = path.join(projectDir, ".altimate-code", "memory") + mkdirSync(memDir, { recursive: true }) + writeFileSync( + path.join(memDir, "one.md"), + "---\nid: one\nscope: project\ncreated: 2026-09-01T00:00:00Z\nupdated: 2026-09-01T00:00:00Z\n---\n\nA block.\n", + ) + // Warm the poller memo: the stub answers a workspace with memory ON. + const originalFetch3 = globalThis.fetch + globalThis.fetch = (async (input: any, init?: any) => { + const url = typeof input === "string" ? input : input.url + const method = (init?.method ?? "GET").toUpperCase() + requests.push({ method, url }) + if (method === "GET" && url.endsWith("/datamates/")) + return new Response(JSON.stringify({ datamates: [{ id: 42, name: "Growth", memory_enabled: true }] }), { + status: 200, + headers: { "content-type": "application/json" }, + }) + return originalFetch3(input, init) + }) as typeof fetch + try { + await status(projectDir, { poll: true }) // asks once, memoizes "enabled" + // The drip only starts once the write path's 60s positive has expired + // while the poller's 5-minute memo has not. Expire it explicitly — this + // is the state every poll after the first minute is in. + memoryEnabledCache.clear() + requests = [] + await status(projectDir, { poll: true }) + await status(projectDir, { poll: true }) + expect(requests.filter((r) => r.method === "GET" && r.url.endsWith("/datamates/"))).toHaveLength(0) + } finally { + globalThis.fetch = originalFetch3 + } + }) +}) + +describe("the poller after an account switch", () => { + test("does not serve the previous tenant's positive to a same-numbered workspace", async () => { + // Workspace ids are tenant-local. The write path's positive cache is keyed + // by bare id — fine there, its credentials are fixed — but the poller + // consulting it first reopened a 60s window in which tenant A's "enabled" + // was served to tenant B's workspace 42. + await bind(projectDir) + resetPollMemoForTests() + const binding = (await readLocalBinding(projectDir))! + const originalFetch4 = globalThis.fetch + let tenantMemory = true + globalThis.fetch = (async (input: any, init?: any) => { + const url = typeof input === "string" ? input : input.url + const method = (init?.method ?? "GET").toUpperCase() + requests.push({ method, url }) + if (method === "GET" && url.endsWith("/datamates/")) + return new Response(JSON.stringify({ datamates: [{ id: 42, name: "W", memory_enabled: tenantMemory }] }), { + status: 200, + headers: { "content-type": "application/json" }, + }) + return originalFetch4(input, init) + }) as typeof fetch + try { + expect(await memoryEnabledForPoller(binding)).toBe("enabled") // tenant A, warms the bare-id positive + // Switch accounts: same workspace id, memory OFF there. + ;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => + ({ altimateInstanceName: "other", altimateUrl: "https://api.example.com", altimateApiKey: "k" }) as Creds + tenantMemory = false + expect(await memoryEnabledForPoller(binding)).toBe("disabled") + } finally { + globalThis.fetch = originalFetch4 + ;(AltimateApi as unknown as { getCredentials: () => Promise }).getCredentials = async () => + ({ altimateInstanceName: "acme", altimateUrl: "https://api.example.com", altimateApiKey: "key-a" }) as Creds + } + }) +}) diff --git a/packages/opencode/test/altimate/workspace/skill-sync.test.ts b/packages/opencode/test/altimate/workspace/skill-sync.test.ts index 9fa866967..93d8fce13 100644 --- a/packages/opencode/test/altimate/workspace/skill-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-sync.test.ts @@ -49,6 +49,7 @@ writeFileSync( const { syncSkills, recentlySynced, + lastSuccessfulSyncAt, registryStale, markRegistryApplied, flushPendingSyncs, @@ -560,6 +561,40 @@ describe("workspace skill sync", () => { expect(existsSync(skillFile("pub-1", "references/g.md"))).toBe(true) }) + test("a partial sync is not reported as the last successful one", async () => { + // A run that could not fetch one skill still publishes a snapshot of the + // rest — so the manifest's mtime moved on a run the sync itself marked + // failed, and the sidebar showed a fresh "synced" for a snapshot with a + // hole in it. + serve({ "pub-1": { "SKILL.md": "one" }, "pub-2": { "SKILL.md": "two" } }) + const inner = globalThis.fetch + globalThis.fetch = (async (input: string | URL, init?: RequestInit) => { + if (String(input).includes("/pub-2/files/")) throw new Error("offline") + return inner(input as never, init as never) + }) as unknown as typeof fetch + await syncSkills(project) + expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(true) + expect(await lastSuccessfulSyncAt(project)).toBeNull() + + // The next clean run is one. + serve({ "pub-1": { "SKILL.md": "one" }, "pub-2": { "SKILL.md": "two" } }, "2026-01-02T00:00:00Z") + await syncSkills(project) + expect(await lastSuccessfulSyncAt(project)).not.toBeNull() + }) + + test("a removed snapshot has no last sync, whatever the process remembers", async () => { + // The in-memory stamp survives the purge; the answer must not. After an + // unlink or a rebind the root is gone, and "synced 2m ago" would describe + // a snapshot that no longer exists. + serve({ "pub-1": { "SKILL.md": "one" } }) + await syncSkills(project) + expect(await lastSuccessfulSyncAt(project)).not.toBeNull() + + rmSync(path.join(project, MANAGED), { recursive: true, force: true }) + + expect(await lastSuccessfulSyncAt(project)).toBeNull() + }) + test("a failed sync does not consume the poll window", async () => { globalThis.fetch = (async () => { throw new Error("offline") From 64cb237b06b62ebc24e6c4d0b28cf15cd2345fa1 Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 15 Sep 2026 15:26:00 +0530 Subject: [PATCH 2/5] fix(workspace): coalesce poller misses, scope the sync marker to the binding, clear the tile on an account switch Three from the codex claims review of the retargeted PR. - The poller memoizes the in-flight ask per scoped key, not only the settled answer: two refreshes overlapping on a cold memo (a remount while a slow one is out) both put a request on the wire. - `lastSuccessfulSyncAt` takes the binding it reports under and answers null when the manifest beside the marker is another workspace's or account's. After a rebind the sidebar can refresh before the detached sync replaced the previous snapshot, and rendered A's age under B's name. - The sidebar checks the account scope before the outcome: a scope change clears the rendered workspace, counts and manage URL and leaves the tile undecided, so an "unknown" first lookup under the new account no longer keeps the previous tenant's state on screen. Verified: 543 pass across the workspace + plugin suites, typecheck clean. Mutation-checked: dropping the in-flight memo and skipping the manifest check each fail a test; the sidebar change has no harness and was reviewed by reading. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../opencode/src/altimate/workspace/manage.ts | 10 +++- .../src/altimate/workspace/memory-sync.ts | 23 +++++++-- .../src/altimate/workspace/skill-sync.ts | 20 +++++++- .../plugin/tui/altimate/workspace-sidebar.tsx | 15 +++++- .../test/altimate/workspace/manage.test.ts | 51 +++++++++++++++++++ 5 files changed, 112 insertions(+), 7 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/manage.ts b/packages/opencode/src/altimate/workspace/manage.ts index b371ec394..106795d7a 100644 --- a/packages/opencode/src/altimate/workspace/manage.ts +++ b/packages/opencode/src/altimate/workspace/manage.ts @@ -131,10 +131,18 @@ export async function status( binding, memory: await memoryCounts(directory, binding, opts.poll === true), skillsEnabled: SkillSync.isEnabled(), - skillsSyncedAt: await SkillSync.lastSuccessfulSyncAt(directory), + skillsSyncedAt: await skillsSyncedAt(directory, binding), } } +/** The age of the last clean skill sync FOR THIS BINDING, or null. */ +async function skillsSyncedAt(directory: string, binding: CachedBinding | null): Promise { + if (!binding) return null + const scope = await currentScope() + if (!scope) return null + return SkillSync.lastSuccessfulSyncAt(directory, { datamateId: binding.datamateId, ...scope }) +} + /** Pull: bring local state in line with the workspace. * * Both halves are attempted even if one fails — they are independent, and a diff --git a/packages/opencode/src/altimate/workspace/memory-sync.ts b/packages/opencode/src/altimate/workspace/memory-sync.ts index aeedc0e6d..b3c4af5d7 100644 --- a/packages/opencode/src/altimate/workspace/memory-sync.ts +++ b/packages/opencode/src/altimate/workspace/memory-sync.ts @@ -688,6 +688,7 @@ const POLL_TTL_MS = 5 * 60 * 1000 * account inherit the previous tenant's answer and hide its unsynced count for * the whole TTL. (cubic P2 on #1279.) */ const pollMemo = new Map() +const pollInFlight = new Map>() async function pollMemoKey(binding: CachedBinding): Promise { try { @@ -725,16 +726,30 @@ export async function memoryEnabledForPoller( const key = await pollMemoKey(binding) const memo = pollMemo.get(key) if (memo && Date.now() - memo.at < POLL_TTL_MS) return memo.status - const status = await memoryStatus(binding, { fresh: true }) - if (status === "error") return "unknown" - pollMemo.set(key, { at: Date.now(), status }) - return status + // The in-flight ask is memoized too, not only the settled answer. Two + // refreshes overlapping on a cold memo — a remount while a slow one is + // still out — both saw it empty and both put a request on the wire. + const pending = pollInFlight.get(key) + if (pending) return pending + const ask = (async () => { + try { + const status = await memoryStatus(binding, { fresh: true }) + if (status === "error") return "unknown" as const + pollMemo.set(key, { at: Date.now(), status }) + return status + } finally { + pollInFlight.delete(key) + } + })() + pollInFlight.set(key, ask) + return ask } /** Test seam: the poller memo is process-global and would otherwise leak between * cases in the same file. */ export function resetPollMemoForTests(): void { pollMemo.clear() + pollInFlight.clear() memoryEnabledCache.clear() memoryDisabledMemo.clear() } diff --git a/packages/opencode/src/altimate/workspace/skill-sync.ts b/packages/opencode/src/altimate/workspace/skill-sync.ts index 3ab4fc734..bc3676f13 100644 --- a/packages/opencode/src/altimate/workspace/skill-sync.ts +++ b/packages/opencode/src/altimate/workspace/skill-sync.ts @@ -263,7 +263,25 @@ export async function flushPendingSyncs(timeoutMs = 30_000): Promise { * is to make staleness visible needs the age, not a threshold. Reads the * process-global store, so the TUI plugin realm sees the same map the sync * writes (see `STORE_KEY` above). */ -export async function lastSuccessfulSyncAt(directory: string): Promise { +export async function lastSuccessfulSyncAt( + directory: string, + /** The binding the age is being reported under. A marker beside a manifest + * for another workspace or account is not this binding's sync: after a + * rebind the sidebar can refresh before the detached sync has replaced the + * previous workspace's snapshot, and would otherwise show A's age under + * B's name. */ + binding?: { datamateId: number; tenant: string; apiUrl: string }, +): Promise { + if (binding) { + const manifest = await readManifest(directory) + if ( + !manifest || + manifest.datamateId !== binding.datamateId || + manifest.tenant !== binding.tenant || + manifest.apiUrl !== binding.apiUrl + ) + return null + } // From disk, not from the map. The map is on `globalThis`, which is shared // across module realms but NOT across threads — and the per-message sync // that does most of the stamping runs in the server worker, while the TUI diff --git a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx index dd5ea915f..daf0f7ea6 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx @@ -120,6 +120,20 @@ function View(props: { api: TuiPluginApi }) { // standing — a network blip must not downgrade a working tile to // "Not linked", which is the one state that tells the user to go and run // a command. + // The account scope first, independently of the outcome. An "unknown" + // answer leaves a working tile standing over a blip — but only within + // the same account. After a credential switch the tile would otherwise + // keep showing the previous tenant's workspace, counts and manage URL + // while the process is using the next, for as long as the new lookup + // failed. A scope change clears what was rendered and leaves the tile + // undecided until the new account answers. + const scope = await currentScope() + if (boundScope !== null && scope !== boundScope) { + setDetail(null) + setManageUrl(null) + setBinding(undefined) + boundScope = null + } const outcome = await resolveBindingOutcome(dir).catch(() => ({ status: "unknown" }) as const) if (outcome.status === "bound") { // Counts and the manage URL belong to a SPECIFIC workspace. On a rebind @@ -132,7 +146,6 @@ function View(props: { api: TuiPluginApi }) { // tenant-local, so after an account switch a same-numbered workspace // in the new tenant would otherwise be treated as unchanged and keep // the old counts under the new name. - const scope = await currentScope() if (binding()?.datamateId !== outcome.binding.datamateId || scope !== boundScope) { setDetail(null) setManageUrl(null) diff --git a/packages/opencode/test/altimate/workspace/manage.test.ts b/packages/opencode/test/altimate/workspace/manage.test.ts index e741a3f72..345536297 100644 --- a/packages/opencode/test/altimate/workspace/manage.test.ts +++ b/packages/opencode/test/altimate/workspace/manage.test.ts @@ -939,6 +939,9 @@ describe("what /workspace status may cost and claim (review round 2)", () => { }) }) +const manifestFor = (datamateId: number) => + JSON.stringify({ version: 1, tenant: "acme", apiUrl: "https://api.example.com", datamateId, skills: {} }) + describe("the sidebar's skills-synced age", () => { test("comes from the snapshot on disk, not a per-thread map", async () => { // The per-message sync stamps its map in the server worker; the sidebar @@ -948,6 +951,7 @@ describe("the sidebar's skills-synced age", () => { await bind(projectDir) const managed = path.join(projectDir, ".altimate-code", "skill", "_workspace") mkdirSync(managed, { recursive: true }) + writeFileSync(path.join(managed, ".manifest.json"), manifestFor(42)) const before = Date.now() - 5 * 60_000 writeFileSync(path.join(managed, ".synced-at"), String(before)) @@ -956,11 +960,25 @@ describe("the sidebar's skills-synced age", () => { expect(report.skillsSyncedAt).toBe(before) }) + test("is nothing when the snapshot beside the marker belongs to another workspace", async () => { + // After a rebind the sidebar can refresh before the detached sync has + // replaced the previous workspace's snapshot, and would otherwise render + // A's age under B's name. + await bind(projectDir) + const managed = path.join(projectDir, ".altimate-code", "skill", "_workspace") + mkdirSync(managed, { recursive: true }) + writeFileSync(path.join(managed, ".manifest.json"), manifestFor(7)) + writeFileSync(path.join(managed, ".synced-at"), String(Date.now() - 60_000)) + + expect((await status(projectDir)).skillsSyncedAt).toBeNull() + }) + test("is nothing when the marker is empty or garbage", async () => { // A truncated marker must read as unknown, not as a sync from 1970. await bind(projectDir) const managed = path.join(projectDir, ".altimate-code", "skill", "_workspace") mkdirSync(managed, { recursive: true }) + writeFileSync(path.join(managed, ".manifest.json"), manifestFor(42)) for (const junk of ["", " \n", "soon", "12abc"]) { writeFileSync(path.join(managed, ".synced-at"), junk) expect((await status(projectDir)).skillsSyncedAt).toBeNull() @@ -1017,6 +1035,39 @@ describe("the poller does not drip", () => { }) }) +describe("the poller coalesces overlapping misses", () => { + test("two refreshes on a cold memo put one request on the wire", async () => { + // A remount while a slow refresh is still out started a second one; both + // saw the memo empty and both asked. The in-flight ask is memoized too. + await bind(projectDir) + resetPollMemoForTests() + const originalFetch3 = globalThis.fetch + globalThis.fetch = (async (input: any, init?: any) => { + const url = typeof input === "string" ? input : input.url + const method = (init?.method ?? "GET").toUpperCase() + requests.push({ method, url }) + if (method === "GET" && url.endsWith("/datamates/")) { + await new Promise((r) => setTimeout(r, 20)) + return new Response(JSON.stringify({ datamates: [{ id: 42, name: "Growth", memory_enabled: true }] }), { + status: 200, + headers: { "content-type": "application/json" }, + }) + } + return originalFetch3(input, init) + }) as typeof fetch + try { + requests = [] + const b = (await readLocalBinding(projectDir))! + const [a, c] = await Promise.all([memoryEnabledForPoller(b), memoryEnabledForPoller(b)]) + expect(a).toBe("enabled") + expect(c).toBe("enabled") + expect(requests.filter((r) => r.method === "GET" && r.url.endsWith("/datamates/"))).toHaveLength(1) + } finally { + globalThis.fetch = originalFetch3 + } + }) +}) + describe("the poller after an account switch", () => { test("does not serve the previous tenant's positive to a same-numbered workspace", async () => { // Workspace ids are tenant-local. The write path's positive cache is keyed From e526f9cc68fd3d60b47c1cafb1b1aba7a81712b7 Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 15 Sep 2026 15:31:43 +0530 Subject: [PATCH 3/5] fix(workspace): no notification on a failed cache write, no re-queued ticks, no double resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ralph's review of #1279, both blocking items and the notes. - `forgetBinding` notifies only when something on disk changed. Notifying on a failed write too made a hot loop when the state directory could not be written: the sidebar answers a notification with a resolve, the resolve hears the memoized miss and re-enters `forgetBinding`, the write fails again, it notifies again. A read-only state directory now costs one poll interval of staleness. Reproduced as a test: forty notifications before, zero after. - The sidebar queues a re-run only for a binding-change notification; a tick that lands mid-refresh is dropped. During an outage each refresh outlasted the tick, so the interval re-queued the next one back to back. - `Manage.status` takes the binding the sidebar already resolved instead of resolving it again — two unmemoized requests per pass during an outage. - Adoption in `lookupBinding` notifies, so a `/workspace` open that adopts wakes the tile; the row is stamped validated first, so the answering resolve does not come back. - `lastSuccessfulSyncAt`'s docstring describes the marker, not the map. Verified: 547 pass across the workspace + plugin suites, typecheck clean. Mutation-checked: notifying regardless, and ignoring the handed binding, each fail a test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../opencode/src/altimate/workspace/manage.ts | 14 ++++- .../src/altimate/workspace/skill-sync.ts | 10 ++-- .../opencode/src/altimate/workspace/state.ts | 29 +++++---- .../plugin/tui/altimate/workspace-sidebar.tsx | 24 +++++--- .../test/altimate/workspace/manage.test.ts | 59 ++++++++++++++++++- 5 files changed, 107 insertions(+), 29 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/manage.ts b/packages/opencode/src/altimate/workspace/manage.ts index 106795d7a..32b09358b 100644 --- a/packages/opencode/src/altimate/workspace/manage.ts +++ b/packages/opencode/src/altimate/workspace/manage.ts @@ -115,6 +115,10 @@ export async function status( * cache alone and reported as unknown (`null`) if not held, and a cached * row is taken as it is. `sync` does the live check. */ poll?: boolean + /** A binding the caller has already resolved this pass. The sidebar + * resolves before it asks for status; resolving again here doubled the + * requests during an outage, when neither answer is memoized. */ + binding?: CachedBinding | null } = {}, ): Promise { // The cached row first, and the resolver only when there is none — for the @@ -124,9 +128,13 @@ export async function status( // resolver revalidates a cached row too, and on the first call of a process // nothing has been validated yet: the menu then sat on the API's full // timeout when the service was unreachable. - const binding = opts.poll - ? await resolveBinding(directory).catch(() => null) - : ((await readLocalBinding(directory).catch(() => null)) ?? (await resolveBinding(directory).catch(() => null))) + const binding = + opts.binding !== undefined + ? opts.binding + : opts.poll + ? await resolveBinding(directory).catch(() => null) + : ((await readLocalBinding(directory).catch(() => null)) ?? + (await resolveBinding(directory).catch(() => null))) return { binding, memory: await memoryCounts(directory, binding, opts.poll === true), diff --git a/packages/opencode/src/altimate/workspace/skill-sync.ts b/packages/opencode/src/altimate/workspace/skill-sync.ts index bc3676f13..d9790fd12 100644 --- a/packages/opencode/src/altimate/workspace/skill-sync.ts +++ b/packages/opencode/src/altimate/workspace/skill-sync.ts @@ -255,14 +255,14 @@ export async function flushPendingSyncs(timeoutMs = 30_000): Promise { } } -/** When this project's workspace skills last synced successfully, or null if - * they never have in this process. +/** When this project's workspace skills were last brought up to date by a + * CLEAN sync, or null when there is no snapshot, no marker, or the snapshot is + * another binding's. * * Exposed for the sidebar. `recentlySynced` answers a boolean against the poll * interval, which cannot say "6 minutes ago" — and a status line whose whole job - * is to make staleness visible needs the age, not a threshold. Reads the - * process-global store, so the TUI plugin realm sees the same map the sync - * writes (see `STORE_KEY` above). */ + * is to make staleness visible needs the age, not a threshold. Read from the + * marker on disk, which every thread and module realm sees alike. */ export async function lastSuccessfulSyncAt( directory: string, /** The binding the age is being reported under. A marker beside a manifest diff --git a/packages/opencode/src/altimate/workspace/state.ts b/packages/opencode/src/altimate/workspace/state.ts index 8538a44de..9c718aa06 100644 --- a/packages/opencode/src/altimate/workspace/state.ts +++ b/packages/opencode/src/altimate/workspace/state.ts @@ -546,6 +546,7 @@ function forgetBinding( expect?: ExpectedRow, before?: UnscopedRow | null, ): boolean { + let dropped = false try { const cache = readCache() if (!cache) return true @@ -577,6 +578,7 @@ function forgetBinding( } for (const k of keys) delete cache.bindings[k] writeCache(cache) + dropped = true } catch (err) { log.warn("could not drop a binding the server no longer recognises", { err: String(err) }) } @@ -585,17 +587,15 @@ function forgetBinding( // subscriber as "could not drop a binding" — a misleading line about a write // that had already succeeded. // - // Notified even when the write FAILED, which is not obvious. The server-side - // unlink has already happened by the time we get here, and the resolve path - // does not depend on this file having been rewritten: `clearLocalBinding` - // drops the revalidation stamp and records a lookup miss, so the next resolve - // asks the server, hears "unbound", and the tile updates. Skipping the - // notification on a failed write left the pane naming a workspace this - // project is no longer bound to until the next poll — the exact lag the - // notifier exists to remove, in the case where something is already wrong. - // (An earlier version guarded this on whether the write succeeded, and - // called the difference unobservable. It was not.) - notifyBindingChanged() + // Only when something on disk changed. Notifying on a failed write too was + // tried, so the tile would not name an unlinked workspace until the next + // poll — and it made a hot loop: the sidebar answers a notification with a + // resolve, the resolve hears the memoized miss and re-enters here, the + // write fails again, and it notifies again, forty times in as many + // milliseconds for as long as the state directory stays unwritable. A + // read-only state directory now costs one poll interval of staleness + // instead, which is the right trade. (Ralph, review of #1279.) + if (dropped) notifyBindingChanged() return true } @@ -646,6 +646,7 @@ async function lookupBinding( projectPath: row.project_path ?? null, linkedAt: Date.now(), } + let adoptedNow = false try { const existing = readCache() const cache: CacheFile = @@ -661,6 +662,7 @@ async function lookupBinding( ? { ...adopted, adopted: prior.adopted, seededAt: prior.seededAt, linkedAt: prior.linkedAt } : adopted writeCache(cache) + adoptedNow = !prior || prior.datamateId !== adopted.datamateId } catch (err) { // The binding still stands for this call; only the cache write failed, so // the next process looks it up again. Same reasoning as recordApprovedBinding. @@ -670,6 +672,11 @@ async function lookupBinding( log.info("adopted the workspace binding this project already has on the server", { datamateId: adopted.datamateId, }) + // An adoption is a binding change this process made to its cache, and the + // sidebar is not always the caller — a `/workspace` open that adopts left + // the tile to the next poll. Stamped as validated above, so the sidebar's + // answering resolve trusts the row and does not come back here. + if (adoptedNow) notifyBindingChanged() return { status: "bound", binding: adopted } } diff --git a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx index daf0f7ea6..ca0e7beaa 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx @@ -94,14 +94,17 @@ function View(props: { api: TuiPluginApi }) { return null } } - const refresh = async () => { - // Coalesce rather than drop. A binding-change notification can land while a - // poll is mid-flight, and that pass may already have read the old binding — - // returning early would leave the tile stale until the next tick, which is - // exactly the lag the listener exists to remove. One queued re-run is - // enough however many notifications arrive while we are busy. + const refresh = async (why: "poll" | "notify" = "poll") => { + // A notification that lands mid-refresh is queued, not dropped: that pass + // may already have read the old binding, and returning early would leave + // the tile stale until the next tick — the lag the listener exists to + // remove. A TICK that lands mid-refresh is dropped: it carries no news, + // and queuing it meant that while the service was unreachable — three + // calls on a 15s budget each, longer than the 30s tick — the next refresh + // started the moment the last one ended, back to back for as long as the + // outage lasted. (Ralph, review of #1279.) if (refreshInFlight) { - refreshQueued = true + if (why === "notify") refreshQueued = true return } refreshInFlight = true @@ -171,7 +174,10 @@ function View(props: { api: TuiPluginApi }) { // what left these counts blank until something else happened to warm the // cache. The `/workspace` menu, by contrast, is cache-only, because it is // awaited before the dialog can open. See `Manage.status`. - setDetail(await Manage.status(dir, { poll: true }).catch(() => null)) + // Handed the binding this pass resolved, so `status` does not resolve it + // again — during an outage neither answer is memoized, and that was two + // requests where one was already too many. + setDetail(await Manage.status(dir, { poll: true, binding: b }).catch(() => null)) // altimate_change end } finally { refreshInFlight = false @@ -197,7 +203,7 @@ function View(props: { api: TuiPluginApi }) { // to POLL_MS — the UI contradicting itself, with the stale half looking // authoritative. The interval stays: it is what catches a change made by // another process, which no in-process listener can see. - const unsubscribe = onBindingChanged(() => void refresh()) + const unsubscribe = onBindingChanged(() => void refresh("notify")) onCleanup(() => { disposed = true clearInterval(timer) diff --git a/packages/opencode/test/altimate/workspace/manage.test.ts b/packages/opencode/test/altimate/workspace/manage.test.ts index 345536297..a29b3cfc3 100644 --- a/packages/opencode/test/altimate/workspace/manage.test.ts +++ b/packages/opencode/test/altimate/workspace/manage.test.ts @@ -14,7 +14,7 @@ // issued — method, path, query — and the binding cache is a real file in a real // sandbox directory. import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test" -import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs" +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs" import { execFileSync } from "node:child_process" import path from "node:path" import os from "node:os" @@ -1035,6 +1035,63 @@ describe("the poller does not drip", () => { }) }) +describe("a state directory that cannot be written", () => { + test("does not turn the notify → resolve chain into a hot loop", async () => { + // Ralph's reproduction on #1279: a row on disk, the server answering + // unbound, the state directory read-only, and a subscriber that resolves + // the way the sidebar's queued refresh does. Notifying on a failed write + // made every resolve re-enter `forgetBinding`, which notified, which the + // subscriber answered with another resolve — forty times in 24ms. + await bind(projectDir) + expireValidationForTests(projectDir) + const originalFetch3 = globalThis.fetch + globalThis.fetch = (async (input: any, init?: any) => { + const url = typeof input === "string" ? input : input.url + const method = (init?.method ?? "GET").toUpperCase() + requests.push({ method, url }) + if (method === "GET" && url.includes("/datamate-project-bindings/by-")) + return new Response(JSON.stringify({ detail: "gone" }), { + status: 404, + headers: { "content-type": "application/json" }, + }) + return originalFetch3(input, init) + }) as typeof fetch + let notifications = 0 + let resolves = 0 + const unsubscribe = onBindingChanged(() => { + notifications++ + if (notifications < 50) void resolveBindingOutcome(projectDir).then(() => resolves++) + }) + const stateDir = path.dirname(cachePath()) + chmodSync(stateDir, 0o555) + try { + const outcome = await resolveBindingOutcome(projectDir) + expect(outcome.status).toBe("unbound") + await new Promise((r) => setTimeout(r, 100)) + } finally { + chmodSync(stateDir, 0o755) + unsubscribe() + globalThis.fetch = originalFetch3 + } + // Nothing on disk changed, so nobody was told; the row stays until the + // next poll, which is one interval of staleness rather than a loop. + expect(notifications).toBe(0) + expect(resolves).toBe(0) + }) +}) + +describe("status reuses a binding the caller resolved", () => { + test("makes no binding request of its own when handed one", async () => { + await bind(projectDir) + expireValidationForTests(projectDir) + const b = (await readLocalBinding(projectDir))! + requests = [] + const report = await status(projectDir, { poll: true, binding: b }) + expect(report.binding?.datamateId).toBe(42) + expect(requests.filter((r) => r.url.includes("/datamate-project-bindings/"))).toHaveLength(0) + }) +}) + describe("the poller coalesces overlapping misses", () => { test("two refreshes on a cold memo put one request on the wire", async () => { // A remount while a slow refresh is still out started a second one; both From e925a554435160652514b86f276f4e2d439a0077 Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 15 Sep 2026 15:36:37 +0530 Subject: [PATCH 4/5] fix(workspace): the sync marker rides the snapshot swap; the sidebar's scope check tolerates a blip and a race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The clean-sync marker is written into the staged tree beside the manifest and lands in the same rename, so the two can never describe different workspaces. Written afterwards at the root, there was a window in which B's manifest sat beside A's marker. The clean up-to-date run, which publishes nothing, still stamps at the root — its manifest is unchanged. - The sidebar treats a null scope (credentials unreadable this instant) as no information rather than as an account change, and re-reads the scope after the resolve: a scope that moved underneath the resolver drops that outcome for the next tick instead of comparing it against the old scope. Verified: 548 pass across the workspace + plugin suites, typecheck clean. Mutation-checked: dropping either marker write fails a test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/skill-sync.ts | 16 +++++++++++++--- .../plugin/tui/altimate/workspace-sidebar.tsx | 15 +++++++++++++-- .../test/altimate/workspace/skill-sync.test.ts | 12 ++++++++++++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/skill-sync.ts b/packages/opencode/src/altimate/workspace/skill-sync.ts index d9790fd12..72aacd530 100644 --- a/packages/opencode/src/altimate/workspace/skill-sync.ts +++ b/packages/opencode/src/altimate/workspace/skill-sync.ts @@ -990,6 +990,13 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean // Written into staging so it lands atomically with the snapshot. await fs.writeFile(path.join(staging, ".gitignore"), "*\n") await fs.writeFile(path.join(staging, MANIFEST_NAME), JSON.stringify(next, null, 2)) + // The clean-sync marker lands in the same rename as the manifest, so the + // two can never describe different workspaces: written afterwards, at + // the root, there was a window in which B's manifest sat beside A's + // marker and B was reported with A's sync age. `failed` is settled by + // now — every skill has been fetched or skipped — so a partial publish + // carries no marker, and the previous one went with the retired tree. + if (!failed) await fs.writeFile(path.join(staging, SYNCED_MARKER), String(Date.now())) // Move the live tree aside rather than deleting it first. `rm` then // `rename` leaves a window with no snapshot at all — a crash or a reader // inside it sees the skills vanish. The retired tree is removed only @@ -1036,9 +1043,12 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean if (ok && !failed && sawRemote) { const now = Date.now() lastSyncedAt.set(canon, now) - // Only where a snapshot exists: a clean run against an empty workspace - // removed the root, and there is nothing for an age to describe. - await fs.writeFile(path.join(managedRoot(canon), SYNCED_MARKER), String(now)).catch(() => {}) + // A clean run that published carried its marker in the swap. This is + // the clean run that found the snapshot up to date and published + // nothing: the manifest on disk is unchanged, so stamping beside it + // cannot pair it with another workspace. Only where a snapshot exists — + // a clean run against an empty workspace removed the root. + if (!changed) await fs.writeFile(path.join(managedRoot(canon), SYNCED_MARKER), String(now)).catch(() => {}) } return { changed } })() diff --git a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx index ca0e7beaa..61cc9067c 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx @@ -130,14 +130,25 @@ function View(props: { api: TuiPluginApi }) { // while the process is using the next, for as long as the new lookup // failed. A scope change clears what was rendered and leaves the tile // undecided until the new account answers. - const scope = await currentScope() - if (boundScope !== null && scope !== boundScope) { + // `null` is "could not read the credentials this instant", not "a + // different account": a transient read failure must not blank a tile + // the resolver would have preserved. Only a scope that READS as another + // one clears. + const scopeBefore = await currentScope() + if (boundScope !== null && scopeBefore !== null && scopeBefore !== boundScope) { setDetail(null) setManageUrl(null) setBinding(undefined) boundScope = null } const outcome = await resolveBindingOutcome(dir).catch(() => ({ status: "unknown" }) as const) + // Read again after the resolve. The credentials can change between the + // two reads, and the resolver runs under whatever they were when it + // ran; a scope that moved underneath it means this outcome cannot be + // trusted against the scope read first. Drop it: the next tick reads a + // settled pair. + const scope = await currentScope() + if (scope !== scopeBefore) return if (outcome.status === "bound") { // Counts and the manage URL belong to a SPECIFIC workspace. On a rebind // they would otherwise keep describing the old one until the new status diff --git a/packages/opencode/test/altimate/workspace/skill-sync.test.ts b/packages/opencode/test/altimate/workspace/skill-sync.test.ts index 93d8fce13..2f7560df5 100644 --- a/packages/opencode/test/altimate/workspace/skill-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/skill-sync.test.ts @@ -582,6 +582,18 @@ describe("workspace skill sync", () => { expect(await lastSuccessfulSyncAt(project)).not.toBeNull() }) + test("a clean run that finds the snapshot up to date still advances the age", async () => { + // Publishing nothing is still a successful sync. Without a stamp here + // the age grew stale for as long as the workspace did not change. + serve({ "pub-1": { "SKILL.md": "one" } }) + await syncSkills(project) + const first = await lastSuccessfulSyncAt(project) + expect(first).not.toBeNull() + await new Promise((r) => setTimeout(r, 5)) + await syncSkills(project) + expect((await lastSuccessfulSyncAt(project)) as number).toBeGreaterThan(first as number) + }) + test("a removed snapshot has no last sync, whatever the process remembers", async () => { // The in-memory stamp survives the purge; the answer must not. After an // unlink or a rebind the root is gone, and "synced 2m ago" would describe From 09dc7bce843184c473ff55d9d8a59a69e91a735f Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 15 Sep 2026 15:47:56 +0530 Subject: [PATCH 5/5] fix(workspace): the sync marker carries its identity; the poller sits on an unknown; renames wake the tile - `.synced-at` is JSON carrying the workspace id, tenant and API URL it was written for, and `lastSuccessfulSyncAt` validates against the binding it reports under from the marker alone. Checking the manifest beside it was a second read, and another process could swap the tree between the two. Writing it is best-effort in the staged tree, so a failed marker write cannot cost a complete snapshot its publish. - The poller memoizes "unknown" for one tick. Not memoizing it meant every tick during an outage asked, and a queued re-run after a self-adoption asked twice in one tick. - A same-id adoption that changes the name or identifiers notifies, and the resolver's same-workspace answer carries the server's current name rather than the row as read before the write. - The sidebar clears what it rendered when the scope moved under the resolve (rather than only dropping the outcome), and re-checks the scope before committing the manage URL and the status, each of which takes a credentials read of its own. Verified: 550 pass across the workspace + plugin suites, typecheck clean. Mutation-checked: not memoizing unknown, not notifying on a rename, returning the stale name, and skipping the marker's identity check each fail a test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6 --- .../src/altimate/workspace/memory-sync.ts | 16 ++-- .../src/altimate/workspace/skill-sync.ts | 76 +++++++++++----- .../opencode/src/altimate/workspace/state.ts | 24 ++++- .../plugin/tui/altimate/workspace-sidebar.tsx | 31 +++++-- .../test/altimate/workspace/manage.test.ts | 89 +++++++++++++++++-- 5 files changed, 194 insertions(+), 42 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/memory-sync.ts b/packages/opencode/src/altimate/workspace/memory-sync.ts index b3c4af5d7..ca4240bbc 100644 --- a/packages/opencode/src/altimate/workspace/memory-sync.ts +++ b/packages/opencode/src/altimate/workspace/memory-sync.ts @@ -682,12 +682,17 @@ async function runQueue( * back to the network on every other tick — a steady drip of `/datamates` * requests for the life of the session. (cubic P2 on #1279.) */ const POLL_TTL_MS = 5 * 60 * 1000 +/** How long the poller sits on an "unknown" before asking again. Not + * memoizing it at all meant every tick during an outage asked — and a queued + * re-run after a self-adoption asked twice in one tick. One poll interval: + * the tile shows no counts until the next tick either way. */ +const POLL_UNKNOWN_TTL_MS = 30 * 1000 /** Keyed by tenant and API URL as well as workspace id. Workspace ids are * tenant-local, so a bare id let a same-numbered workspace in a NEWLY switched * account inherit the previous tenant's answer and hide its unsynced count for * the whole TTL. (cubic P2 on #1279.) */ -const pollMemo = new Map() +const pollMemo = new Map() const pollInFlight = new Map>() async function pollMemoKey(binding: CachedBinding): Promise { @@ -725,7 +730,8 @@ export async function memoryEnabledForPoller( // minutes per tenant is the price, and `memoryStatus` still warms both. const key = await pollMemoKey(binding) const memo = pollMemo.get(key) - if (memo && Date.now() - memo.at < POLL_TTL_MS) return memo.status + if (memo && Date.now() - memo.at < (memo.status === "unknown" ? POLL_UNKNOWN_TTL_MS : POLL_TTL_MS)) + return memo.status // The in-flight ask is memoized too, not only the settled answer. Two // refreshes overlapping on a cold memo — a remount while a slow one is // still out — both saw it empty and both put a request on the wire. @@ -734,9 +740,9 @@ export async function memoryEnabledForPoller( const ask = (async () => { try { const status = await memoryStatus(binding, { fresh: true }) - if (status === "error") return "unknown" as const - pollMemo.set(key, { at: Date.now(), status }) - return status + const answer = status === "error" ? ("unknown" as const) : status + pollMemo.set(key, { at: Date.now(), status: answer }) + return answer } finally { pollInFlight.delete(key) } diff --git a/packages/opencode/src/altimate/workspace/skill-sync.ts b/packages/opencode/src/altimate/workspace/skill-sync.ts index 72aacd530..d2ec146d6 100644 --- a/packages/opencode/src/altimate/workspace/skill-sync.ts +++ b/packages/opencode/src/altimate/workspace/skill-sync.ts @@ -265,23 +265,14 @@ export async function flushPendingSyncs(timeoutMs = 30_000): Promise { * marker on disk, which every thread and module realm sees alike. */ export async function lastSuccessfulSyncAt( directory: string, - /** The binding the age is being reported under. A marker beside a manifest - * for another workspace or account is not this binding's sync: after a - * rebind the sidebar can refresh before the detached sync has replaced the - * previous workspace's snapshot, and would otherwise show A's age under - * B's name. */ + /** The binding the age is being reported under. A marker for another + * workspace or account is not this binding's sync: after a rebind the + * sidebar can refresh before the detached sync has replaced the previous + * workspace's snapshot, and would otherwise show A's age under B's name. + * The marker carries its own identity — checking the manifest beside it was + * a second read, and another process could swap the tree between the two. */ binding?: { datamateId: number; tenant: string; apiUrl: string }, ): Promise { - if (binding) { - const manifest = await readManifest(directory) - if ( - !manifest || - manifest.datamateId !== binding.datamateId || - manifest.tenant !== binding.tenant || - manifest.apiUrl !== binding.apiUrl - ) - return null - } // From disk, not from the map. The map is on `globalThis`, which is shared // across module realms but NOT across threads — and the per-message sync // that does most of the stamping runs in the server worker, while the TUI @@ -291,12 +282,17 @@ export async function lastSuccessfulSyncAt( // agree; the manifest's mtime did not — a partial run publishes one, and a // clean up-to-date run publishes nothing. try { - // Validated, not coerced: `Number("")` is 0, and a truncated marker would - // otherwise render as a sync from 1970. - const raw = (await fs.readFile(path.join(managedRoot(directory), SYNCED_MARKER), "utf8")).trim() - if (!/^\d+$/.test(raw)) return null - const at = Number(raw) - return Number.isSafeInteger(at) && at > 0 ? at : null + // Validated, not coerced: a truncated or hand-edited marker reads as + // unknown, never as a sync from 1970 or as another workspace's. + const raw = await fs.readFile(path.join(managedRoot(directory), SYNCED_MARKER), "utf8") + const marker = parseMarker(raw) + if (!marker) return null + if ( + binding && + (marker.datamateId !== binding.datamateId || marker.tenant !== binding.tenant || marker.apiUrl !== binding.apiUrl) + ) + return null + return marker.at } catch (err) { // No snapshot is no sync: after an unlink, a rebind, or an empty // workspace, an in-memory stamp from before would report a sync that no @@ -307,6 +303,31 @@ export async function lastSuccessfulSyncAt( } } +/** What a clean sync leaves at the managed root: when, and for which binding. */ +interface SyncMarker { + at: number + datamateId: number + tenant: string + apiUrl: string +} + +function parseMarker(raw: string): SyncMarker | null { + try { + const m = JSON.parse(raw) as Partial | null + if (!m || typeof m !== "object") return null + if (typeof m.at !== "number" || !Number.isSafeInteger(m.at) || m.at <= 0) return null + if (typeof m.datamateId !== "number") return null + if (typeof m.tenant !== "string" || typeof m.apiUrl !== "string") return null + return { at: m.at, datamateId: m.datamateId, tenant: m.tenant, apiUrl: m.apiUrl } + } catch { + return null + } +} + +function markerFor(manifest: Pick, at: number): string { + return JSON.stringify({ at, datamateId: manifest.datamateId, tenant: manifest.tenant, apiUrl: manifest.apiUrl }) +} + /** Has this project's snapshot been checked within the poll interval? Callers * on a per-message path use this to skip the network entirely. */ export async function recentlySynced(directory: string): Promise { @@ -996,7 +1017,12 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean // marker and B was reported with A's sync age. `failed` is settled by // now — every skill has been fetched or skipped — so a partial publish // carries no marker, and the previous one went with the retired tree. - if (!failed) await fs.writeFile(path.join(staging, SYNCED_MARKER), String(Date.now())) + // Best-effort: the marker is status metadata, and a failure to write it + // must not cost a complete staged snapshot its publish. + if (!failed) + await fs.writeFile(path.join(staging, SYNCED_MARKER), markerFor(next, Date.now())).catch((err) => { + log.warn("could not write the workspace skill sync marker", { err: String(err) }) + }) // Move the live tree aside rather than deleting it first. `rm` then // `rename` leaves a window with no snapshot at all — a crash or a reader // inside it sees the skills vanish. The retired tree is removed only @@ -1048,7 +1074,11 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean // nothing: the manifest on disk is unchanged, so stamping beside it // cannot pair it with another workspace. Only where a snapshot exists — // a clean run against an empty workspace removed the root. - if (!changed) await fs.writeFile(path.join(managedRoot(canon), SYNCED_MARKER), String(now)).catch(() => {}) + if (!changed) { + const current = await readManifest(canon) + if (current) + await fs.writeFile(path.join(managedRoot(canon), SYNCED_MARKER), markerFor(current, now)).catch(() => {}) + } } return { changed } })() diff --git a/packages/opencode/src/altimate/workspace/state.ts b/packages/opencode/src/altimate/workspace/state.ts index 9c718aa06..2b320aae3 100644 --- a/packages/opencode/src/altimate/workspace/state.ts +++ b/packages/opencode/src/altimate/workspace/state.ts @@ -400,7 +400,19 @@ export async function resolveBindingOutcome(directory: string): Promise { setDetail(null) setManageUrl(null) setBinding(undefined) boundScope = null } + const scopeBefore = await currentScope() + if (boundScope !== null && scopeBefore !== null && scopeBefore !== boundScope) clearRendered() const outcome = await resolveBindingOutcome(dir).catch(() => ({ status: "unknown" }) as const) // Read again after the resolve. The credentials can change between the // two reads, and the resolver runs under whatever they were when it // ran; a scope that moved underneath it means this outcome cannot be - // trusted against the scope read first. Drop it: the next tick reads a - // settled pair. + // trusted against the scope read first. What WAS rendered belonged to + // the old scope and goes now; the outcome is dropped, and the next tick + // reads a settled pair. const scope = await currentScope() - if (scope !== scopeBefore) return + if (scope !== scopeBefore) { + if (scope !== null) clearRendered() + return + } + // Every later commit in this pass checks the scope again first: the + // manage base and the status each take a credentials read of their own, + // and a switch during either would otherwise pair one account's binding + // with another's URL or counts. + const stillThisScope = async () => (await currentScope()) === scope if (outcome.status === "bound") { // Counts and the manage URL belong to a SPECIFIC workspace. On a rebind // they would otherwise keep describing the old one until the new status @@ -176,6 +186,10 @@ function View(props: { api: TuiPluginApi }) { // cleared the manage URL, or never set one. if (!b) return const base = await resolveManageBase() + if (!(await stillThisScope())) { + clearRendered() + return + } setManageUrl(base ? buildManageUrl(base, b.datamateId) : null) // altimate_change start - status lines // `poll: true` marks this as the POLLER path: `status` then resolves the @@ -188,7 +202,12 @@ function View(props: { api: TuiPluginApi }) { // Handed the binding this pass resolved, so `status` does not resolve it // again — during an outage neither answer is memoized, and that was two // requests where one was already too many. - setDetail(await Manage.status(dir, { poll: true, binding: b }).catch(() => null)) + const detail = await Manage.status(dir, { poll: true, binding: b }).catch(() => null) + if (!(await stillThisScope())) { + clearRendered() + return + } + setDetail(detail) // altimate_change end } finally { refreshInFlight = false diff --git a/packages/opencode/test/altimate/workspace/manage.test.ts b/packages/opencode/test/altimate/workspace/manage.test.ts index a29b3cfc3..49eb74e41 100644 --- a/packages/opencode/test/altimate/workspace/manage.test.ts +++ b/packages/opencode/test/altimate/workspace/manage.test.ts @@ -941,6 +941,8 @@ describe("what /workspace status may cost and claim (review round 2)", () => { const manifestFor = (datamateId: number) => JSON.stringify({ version: 1, tenant: "acme", apiUrl: "https://api.example.com", datamateId, skills: {} }) +const markerFor = (datamateId: number, at: number) => + JSON.stringify({ at, datamateId, tenant: "acme", apiUrl: "https://api.example.com" }) describe("the sidebar's skills-synced age", () => { test("comes from the snapshot on disk, not a per-thread map", async () => { @@ -953,22 +955,23 @@ describe("the sidebar's skills-synced age", () => { mkdirSync(managed, { recursive: true }) writeFileSync(path.join(managed, ".manifest.json"), manifestFor(42)) const before = Date.now() - 5 * 60_000 - writeFileSync(path.join(managed, ".synced-at"), String(before)) + writeFileSync(path.join(managed, ".synced-at"), markerFor(42, before)) const report = await status(projectDir) expect(report.skillsSyncedAt).toBe(before) }) - test("is nothing when the snapshot beside the marker belongs to another workspace", async () => { + test("is nothing when the marker belongs to another workspace", async () => { // After a rebind the sidebar can refresh before the detached sync has // replaced the previous workspace's snapshot, and would otherwise render - // A's age under B's name. + // A's age under B's name. The marker carries its own identity, so this + // holds without a second read of the manifest beside it. await bind(projectDir) const managed = path.join(projectDir, ".altimate-code", "skill", "_workspace") mkdirSync(managed, { recursive: true }) - writeFileSync(path.join(managed, ".manifest.json"), manifestFor(7)) - writeFileSync(path.join(managed, ".synced-at"), String(Date.now() - 60_000)) + writeFileSync(path.join(managed, ".manifest.json"), manifestFor(42)) + writeFileSync(path.join(managed, ".synced-at"), markerFor(7, Date.now() - 60_000)) expect((await status(projectDir)).skillsSyncedAt).toBeNull() }) @@ -979,7 +982,7 @@ describe("the sidebar's skills-synced age", () => { const managed = path.join(projectDir, ".altimate-code", "skill", "_workspace") mkdirSync(managed, { recursive: true }) writeFileSync(path.join(managed, ".manifest.json"), manifestFor(42)) - for (const junk of ["", " \n", "soon", "12abc"]) { + for (const junk of ["", " \n", "soon", "12abc", "1700000000000", '{"at":0,"datamateId":42}', "{not json"]) { writeFileSync(path.join(managed, ".synced-at"), junk) expect((await status(projectDir)).skillsSyncedAt).toBeNull() } @@ -1092,6 +1095,80 @@ describe("status reuses a binding the caller resolved", () => { }) }) +describe("the poller during an outage", () => { + test("sits on an unknown for a tick rather than asking again at once", async () => { + // Not memoizing "unknown" at all meant every tick during an outage asked, + // and a queued re-run after a self-adoption asked twice in one tick. + await bind(projectDir) + resetPollMemoForTests() + const originalFetch3 = globalThis.fetch + globalThis.fetch = (async (input: any, init?: any) => { + const url = typeof input === "string" ? input : input.url + const method = (init?.method ?? "GET").toUpperCase() + requests.push({ method, url }) + if (method === "GET" && url.endsWith("/datamates/")) + return new Response(JSON.stringify({ detail: "down" }), { + status: 503, + headers: { "content-type": "application/json" }, + }) + return originalFetch3(input, init) + }) as typeof fetch + try { + requests = [] + const b = (await readLocalBinding(projectDir))! + expect(await memoryEnabledForPoller(b)).toBe("unknown") + expect(await memoryEnabledForPoller(b)).toBe("unknown") + expect(requests.filter((r) => r.method === "GET" && r.url.endsWith("/datamates/"))).toHaveLength(1) + } finally { + globalThis.fetch = originalFetch3 + } + }) +}) + +describe("a rename on the server", () => { + test("reaches the resolver's answer and wakes the sidebar", async () => { + // The same workspace, renamed in the SaaS. `lookupBinding` wrote the new + // name to the cache but the resolver handed back the row it had read + // before, and nothing woke the tile — so the old name stood. + await bind(projectDir) + expireValidationForTests(projectDir) + const originalFetch3 = globalThis.fetch + globalThis.fetch = (async (input: any, init?: any) => { + const url = typeof input === "string" ? input : input.url + const method = (init?.method ?? "GET").toUpperCase() + requests.push({ method, url }) + if (method === "GET" && url.includes("/datamate-project-bindings/by-")) + return new Response( + JSON.stringify({ + // Only the name differs; the identifiers match the recorded row, so + // the notification can be attributed to the rename alone. + binding: { + id: 1, + datamate_id: 42, + datamate_name: "Growth (renamed)", + repo_remote: "git@github.com:acme/app.git", + project_path: projectDir, + }, + datamate: { id: 42, name: "Growth (renamed)" }, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ) + return originalFetch3(input, init) + }) as typeof fetch + let notified = 0 + const unsubscribe = onBindingChanged(() => notified++) + try { + const outcome = await resolveBindingOutcome(projectDir) + expect(outcome.status).toBe("bound") + if (outcome.status === "bound") expect(outcome.binding.datamateName).toBe("Growth (renamed)") + expect(notified).toBe(1) + } finally { + unsubscribe() + globalThis.fetch = originalFetch3 + } + }) +}) + describe("the poller coalesces overlapping misses", () => { test("two refreshes on a cold memo put one request on the wire", async () => { // A remount while a slow refresh is still out started a second one; both