From 42a3ec29bd00972e28cb84ddfa5c7cd354690cf7 Mon Sep 17 00:00:00 2001 From: KB Date: Sun, 2 Aug 2026 17:36:16 +0530 Subject: [PATCH 01/25] fix: stop stale deep links minting phantom projects and emptying the catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A first URL segment that is not a project selector was treated as a legacy base64 directory. Plenty of opaque tokens decode cleanly into binary junk, which the server then resolved against its own cwd and registered as a real project — leaving phantom entries on the home list. Reject anything that cannot be an absolute path, and guard the server side with Project.assertDirectory (400 ProjectDirectoryError). The same stale link then broke the app persistently. Project.resolve mapped every read failure to "absent", so a torn file or transient fs error became a 410; 410 is cacheable by default and the API sent no Cache-Control, so a browser cached one 410 for /provider and answered every later request from its own cache — through restarts and reloads, with the server seeing no traffic. Mark JSON responses no-store, and only treat a genuine NotFound as absent. Downstream, a failed project-scoped catalog load emptied the provider store, which reads as "my API keys vanished". The catalog belongs to the install, so fall back to it; refresh it explicitly after a key change or OAuth sign-in rather than waiting on the event stream; and bound the in-flight share with a timeout so a hung request cannot pin the key and stop every later refresh. --- backend/cli/src/project/project.ts | 37 ++++++- backend/cli/src/server/project-selection.ts | 6 +- backend/cli/src/server/server.ts | 14 +++ backend/cli/test/server/no-store.test.ts | 29 ++++++ .../server/project-selection-routes.test.ts | 41 ++++++++ .../components/settings/CodexConnection.tsx | 6 ++ .../src/components/settings/ProviderKeys.tsx | 19 +++- .../workspace/src/context/global-sync.tsx | 88 ++++++++++++----- .../src/context/inflight-cache.test.ts | 98 +++++++++++++++++++ .../workspace/src/context/inflight-cache.ts | 60 ++++++++++++ frontend/workspace/src/hooks/use-providers.ts | 7 ++ frontend/workspace/src/utils/base64.test.ts | 26 +++++ frontend/workspace/src/utils/base64.ts | 17 +++- 13 files changed, 415 insertions(+), 33 deletions(-) create mode 100644 backend/cli/test/server/no-store.test.ts create mode 100644 frontend/workspace/src/context/inflight-cache.test.ts create mode 100644 frontend/workspace/src/context/inflight-cache.ts create mode 100644 frontend/workspace/src/utils/base64.test.ts diff --git a/backend/cli/src/project/project.ts b/backend/cli/src/project/project.ts index 7f8b7850..bae08343 100644 --- a/backend/cli/src/project/project.ts +++ b/backend/cli/src/project/project.ts @@ -53,6 +53,13 @@ export namespace Project { }), ) + export const DirectoryError = NamedError.create( + "ProjectDirectoryError", + z.object({ + directory: z.string(), + }), + ) + export const Info = z .object({ id: z.string(), @@ -169,12 +176,24 @@ export namespace Project { * A caller may include a legacy directory while migrating, but it must remain * inside the selected project's recorded roots. */ + /** + * Only a genuinely absent record means the project is gone. Any other read + * failure — a torn file from a concurrent writer, a transient fs error — must + * propagate, because reporting it as 410 tells the caller to stop asking + * about a project that is in fact fine, and the client empties the surfaces + * that depend on it. + */ + function absent(error: unknown) { + if (Storage.NotFoundError.isInstance(error)) return undefined + throw error + } + export async function resolve(projectID: string, directory?: string) { - const direct = await Storage.read(["project", projectID]).catch(() => undefined) - const link = await Storage.read>(["project_alias", projectID]).catch(() => undefined) + const direct = await Storage.read(["project", projectID]).catch(absent) + const link = await Storage.read>(["project_alias", projectID]).catch(absent) if (!direct && !link) throw new UnknownError({ projectID }) - const linked = link ? await Storage.read(["project", link.projectID]).catch(() => undefined) : undefined + const linked = link ? await Storage.read(["project", link.projectID]).catch(absent) : undefined const redirected = !!linked && (!direct || !projectID.startsWith("prj_")) const project = redirected ? linked : (direct ?? linked) if (!project) { @@ -210,6 +229,18 @@ export namespace Project { } } + /** + * Guard a caller-supplied root before it can mint a project. Anything that is + * not an absolute path gets resolved against the server's cwd, which turned + * junk from a stale deep link into a real-looking folder under the user's + * home and left a phantom project on their home list. + */ + export async function assertDirectory(input: string) { + if (!path.isAbsolute(input)) throw new DirectoryError({ directory: input }) + const stat = await fs.stat(input).catch(() => undefined) + if (!stat?.isDirectory()) throw new DirectoryError({ directory: input }) + } + export async function fromDirectory(input: string) { const directory = canonicalize(input) log.info("fromDirectory", { directory }) diff --git a/backend/cli/src/server/project-selection.ts b/backend/cli/src/server/project-selection.ts index 01307734..e79b7e0b 100644 --- a/backend/cli/src/server/project-selection.ts +++ b/backend/cli/src/server/project-selection.ts @@ -38,10 +38,14 @@ export async function projectSelection( text(input.directory) ?? text(context.req.query("directory")) ?? text(context.req.header("x-openscience-directory")) const directory = decode(raw) - if (projectID) return Project.resolve(projectID, directory) + if (projectID) return { ...(await Project.resolve(projectID, directory)), selector: directory } return { project: undefined, directory: directory ? Project.canonicalize(directory) : undefined, alias: undefined, + // The caller-supplied root before canonicalization, so routes that mint an + // instance can reject it while the folder picker keeps reporting its own + // friendlier "path not found". + selector: directory, } } diff --git a/backend/cli/src/server/server.ts b/backend/cli/src/server/server.ts index eac37244..cbb5f54f 100644 --- a/backend/cli/src/server/server.ts +++ b/backend/cli/src/server/server.ts @@ -92,6 +92,18 @@ export namespace Server { () => // TODO: Break server.ts into smaller route files to fix type inference app + // 404/410 and friends are cacheable by default (RFC 7231 §6.1), and a + // JSON body with no Cache-Control is fair game for heuristic caching + // too. A browser that cached one stale-project 410 for /provider then + // answered every later request from its own cache — the server saw no + // traffic at all while the app stayed broken across restarts and + // reloads. Applied to JSON only, so the SPA's hashed assets keep their + // caching. + .use(async (c, next) => { + await next() + if (!c.res.headers.get("content-type")?.includes("application/json")) return + c.res.headers.set("cache-control", "no-store") + }) .onError((err, c) => { log.error("failed", { error: err, @@ -107,6 +119,7 @@ export namespace Server { else if (err.name === "ProjectUnknownError") status = 404 else if (err.name === "ProjectStaleError") status = 410 else if (err.name === "ProjectMismatchError") status = 409 + else if (err.name === "ProjectDirectoryError") status = 400 else if (err.name === "ProjectTrustDeniedError") status = 403 else if (err.name === "ProjectTrustRootMismatchError") status = 409 else if (err.name === "ExecutionAuthorityDeniedError") status = 403 @@ -259,6 +272,7 @@ export namespace Server { .route("/api/repo", RepoRoutes()) .use(async (c, next) => { const selected = await projectSelection(c) + if (selected.selector) await Project.assertDirectory(selected.selector) const directory = selected.directory ?? process.cwd() return Instance.provide({ directory, diff --git a/backend/cli/test/server/no-store.test.ts b/backend/cli/test/server/no-store.test.ts new file mode 100644 index 00000000..d1c122f3 --- /dev/null +++ b/backend/cli/test/server/no-store.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, test } from "bun:test" +import { Server } from "../../src/server/server" +import { Log } from "../../src/util/log" + +Log.init({ print: false }) + +const fetch = Server.internalFetch() + +// 410/404 are cacheable by default (RFC 7231 §6.1), and the API sent no +// Cache-Control at all. A browser cached one stale-project 410 for /provider and +// then answered every later request from its own cache — the server saw no +// traffic while the app stayed broken through restarts and reloads. +describe("API responses are never cached", () => { + test("a successful JSON response is marked no-store", async () => { + const response = await fetch("http://openscience.internal/provider") + + expect(response.status).toBe(200) + expect(response.headers.get("cache-control")).toBe("no-store") + }) + + test("an error response is marked no-store", async () => { + const response = await fetch("http://openscience.internal/provider", { + headers: { "x-openscience-project": `prj_missing_${crypto.randomUUID()}` }, + }) + + expect(response.status).toBeGreaterThanOrEqual(400) + expect(response.headers.get("cache-control")).toBe("no-store") + }) +}) diff --git a/backend/cli/test/server/project-selection-routes.test.ts b/backend/cli/test/server/project-selection-routes.test.ts index e2d2485c..09c03a57 100644 --- a/backend/cli/test/server/project-selection-routes.test.ts +++ b/backend/cli/test/server/project-selection-routes.test.ts @@ -1,6 +1,7 @@ import { $ } from "bun" import { describe, expect, test } from "bun:test" import fs from "fs/promises" +import os from "os" import path from "path" import { Project } from "../../src/project/project" import { Server } from "../../src/server/server" @@ -221,6 +222,46 @@ describe("pre-instance project selection routes", () => { }) }) + // A stale deep link decodes into junk that is neither absolute nor a real + // folder. Registering a project for it put a phantom entry on the home list. + test("refuses to register a project for a directory that does not exist", async () => { + const missing = path.join(os.tmpdir(), `openscience-missing-${crypto.randomUUID()}`) + const before = await Project.list() + + const response = await fetch("http://openscience.internal/project/current", { + headers: { + "x-openscience-directory": missing, + }, + }) + + expect(response.status).toBe(400) + expect(await response.json()).toEqual({ + name: "ProjectDirectoryError", + data: { + directory: missing, + }, + }) + expect(await Project.list()).toHaveLength(before.length) + }) + + // Resolving one against the server's cwd silently opened whatever folder + // happened to sit next to it, so a relative selector is never honoured. + test("refuses a directory selector that is not absolute", async () => { + const response = await fetch("http://openscience.internal/project/current", { + headers: { + "x-openscience-directory": "codes", + }, + }) + + expect(response.status).toBe(400) + expect(await response.json()).toEqual({ + name: "ProjectDirectoryError", + data: { + directory: "codes", + }, + }) + }) + test("accepts body project selection for repository mutations", async () => { await using tmp = await tmpdir({ git: true }) const created = await Project.fromDirectory(tmp.path) diff --git a/frontend/workspace/src/components/settings/CodexConnection.tsx b/frontend/workspace/src/components/settings/CodexConnection.tsx index 8320d270..df4a4797 100644 --- a/frontend/workspace/src/components/settings/CodexConnection.tsx +++ b/frontend/workspace/src/components/settings/CodexConnection.tsx @@ -2,6 +2,7 @@ import { Show, createMemo, createSignal, type Component } from "solid-js" import { Button } from "@synsci/ui/button" import { StatusDot } from "@/atlas/shared/StatusDot" import { useGlobalSDK } from "@/context/global-sdk" +import { useGlobalSync } from "@/context/global-sync" import { usePlatform } from "@/context/platform" import { useProviders } from "@/hooks/use-providers" @@ -10,6 +11,7 @@ export const CodexConnection: Component<{ onConnected?: () => void }> = (props) => { const sdk = useGlobalSDK() + const globalSync = useGlobalSync() const platform = usePlatform() const providers = useProviders() const [busy, setBusy] = createSignal(false) @@ -24,6 +26,9 @@ export const CodexConnection: Component<{ if (result.data?.url) platform.openLink(result.data.url) await sdk.client.provider.oauth.callback({ providerID: "openai-codex", method: 0 }) await sdk.client.global.sync() + // The sign-in only shows up once the catalog is re-read; waiting on the + // event stream to say so is how a completed sign-in looked like a failed one. + await globalSync.refreshProviders() props.onConnected?.() } catch (error) { props.onError?.(error instanceof Error ? error.message : String(error)) @@ -39,6 +44,7 @@ export const CodexConnection: Component<{ try { await sdk.client.auth.remove({ providerID: "openai-codex" }) await sdk.client.global.dispose() + await globalSync.refreshProviders() } catch (error) { props.onError?.(error instanceof Error ? error.message : String(error)) } finally { diff --git a/frontend/workspace/src/components/settings/ProviderKeys.tsx b/frontend/workspace/src/components/settings/ProviderKeys.tsx index ffd21415..a470e7d1 100644 --- a/frontend/workspace/src/components/settings/ProviderKeys.tsx +++ b/frontend/workspace/src/components/settings/ProviderKeys.tsx @@ -8,7 +8,14 @@ import { useProviders } from "@/hooks/use-providers" import { isUserProviderConnection } from "@/context/model-catalog" import { MODEL_PROVIDERS, MODEL_PROVIDER_LABELS, modelProvider } from "./model-providers" -const SOURCES: Record = { +/** + * `note` says where a key that this panel cannot delete actually lives, so the + * reader knows where to go and change it. Every non-removable source used to + * render one blanket "managed externally", which is wrong for a key the user + * set themselves in a .env or a config file — nobody else manages it, and the + * phrase suggests an administrator does. + */ +const SOURCES: Record = { api: { label: "local file", removable: true, @@ -17,16 +24,19 @@ const SOURCES: Record v await sdk.client.auth.set({ providerID: provider(), auth: { type: "api", key: value } }) setKey("") await sdk.client.global.dispose() + // Don't wait on the disposed event to come back round the event stream — + // if it is missed the key is saved but never appears, which reads as a + // failed save. + await sync.refreshProviders() } catch (error) { props.onError?.(error instanceof Error ? error.message : String(error)) } finally { @@ -74,6 +88,7 @@ export function ProviderKeys(props: { onError?: (error: string | undefined) => v try { await sdk.client.auth.remove({ providerID }) await sdk.client.global.dispose() + await sync.refreshProviders() } catch (error) { props.onError?.(error instanceof Error ? error.message : String(error)) } @@ -136,7 +151,7 @@ export function ProviderKeys(props: { onError?: (error: string | undefined) => v when={source(item).removable} fallback={ - managed externally + {source(item).note ?? "managed externally"} } > diff --git a/frontend/workspace/src/context/global-sync.tsx b/frontend/workspace/src/context/global-sync.tsx index 84a88487..6e9e61a3 100644 --- a/frontend/workspace/src/context/global-sync.tsx +++ b/frontend/workspace/src/context/global-sync.tsx @@ -23,6 +23,7 @@ import { createStore, produce, reconcile, type SetStoreFunction, type Store } fr import { Binary } from "@synsci/util/binary" import { retry } from "@synsci/util/retry" import { useGlobalSDK } from "./global-sdk" +import { createInflightCache } from "./inflight-cache" // InitError used to live in pages/error.tsx (now deleted with the legacy // openscience shell). Inline the shape so the openscience context layer keeps // compiling — it's dead code under the new AtlasApp entry but is still @@ -188,27 +189,31 @@ function createGlobalSync() { // Global and project bootstrap can ask for the same 4 MB provider catalog a // few milliseconds apart. Share only that bootstrap burst; expire entries - // promptly so project config/provider changes are never held stale here. - const providerLoads = new Map>() + // promptly so project config/provider changes are never held stale here, and + // bound the wait so a request that never returns cannot pin the key and + // silently stop every later refresh (see inflight-cache.ts). + // scopeFor() folds the project id into the key, so the id itself is kept + // alongside it — the loader needs the pair the caller actually asked with. + const providerScopes = new Map() + const providerLoads = createInflightCache(async (key) => { + const [, directory = ""] = key.split("\n") + try { + const scoped = await sdkFor(directory, providerScopes.get(key)).provider.list() + return normalizeProviderList(scoped.data!) + } catch (error) { + // The catalog is a property of the install, not of one project, so a + // project that has gone stale (its folder deleted — the server answers + // 410) must not be able to empty it. Every model surface reads this + // store, so failing here looked like "my API key vanished". + console.warn("Provider catalog unavailable for this project; using the install catalog", { directory, error }) + const global = await globalSDK.client.provider.list() + return normalizeProviderList(global.data!) + } + }) const loadProvider = (directory: string, projectID?: string) => { const key = scopeFor(directory, projectID) - const pending = providerLoads.get(key) - if (pending) return pending - const promise = sdkFor(directory, projectID) - .provider.list() - .then((x) => normalizeProviderList(x.data!)) - providerLoads.set(key, promise) - promise.then( - () => { - setTimeout(() => { - if (providerLoads.get(key) === promise) providerLoads.delete(key) - }, 1_000) - }, - () => { - if (providerLoads.get(key) === promise) providerLoads.delete(key) - }, - ) - return promise + providerScopes.set(key, projectID) + return providerLoads.get(key) } const [projectCache, setProjectCache, , projectCacheReady] = persisted( @@ -339,6 +344,38 @@ function createGlobalSync() { }) const children: Record, SetStoreFunction]> = {} + + /** + * Re-read the provider catalog into every store that shows it. Bootstrap + * alone is not enough: adding a key or finishing an OAuth sign-in changes the + * catalog while the page is already running, and the settings panel, the + * model picker and the composer all read it from here. Errors surface — a + * silent failure here looks exactly like "the key was never saved". + */ + async function refreshProviders() { + providerLoads.invalidate() + const reload = async ( + directory: string, + projectID: string | undefined, + apply: (value: ProviderListResponse) => void, + ) => { + if (!directory) return + try { + apply(await loadProvider(directory, projectID)) + } catch (error) { + console.error("Failed to refresh providers", { directory, projectID, error }) + } + } + await Promise.all([ + reload(globalStore.path.worktree || globalStore.path.directory, undefined, (value) => + setGlobalStore("provider", reconcile(value)), + ), + ...Object.entries(children).map(([directory, [store, setStore]]) => + reload(directory, store.project || undefined, (value) => setStore("provider", reconcile(value))), + ), + ]) + } + const booting = new Map>() const sessionLoads = new Map>() const sessionMeta = new Map() @@ -681,13 +718,7 @@ function createGlobalSync() { if (directory === "global") { switch (event?.type) { case "global.disposed": { - providerLoads.clear() - refresh() - for (const [directory, [, setStore]] of Object.entries(children)) { - void loadProvider(directory) - .then((value) => setStore("provider", reconcile(value))) - .catch((error) => console.error("Failed to refresh providers", { directory, error })) - } + void refreshProviders() return } case "project.updated": { @@ -1060,6 +1091,10 @@ function createGlobalSync() { const errors = results.filter((r): r is PromiseRejectedResult => r.status === "rejected").map((r) => r.reason) if (errors.length) { + // The toast only carries the first message and never says which task + // failed, so a bootstrap step can drop out (leaving its store empty and + // the UI apparently stale) with nothing to point at. Keep the full set. + console.error("Global bootstrap tasks failed", errors) const message = errors[0] instanceof Error ? errors[0].message : String(errors[0]) const more = errors.length > 1 ? ` (+${errors.length - 1} more)` : "" showToast({ @@ -1150,6 +1185,7 @@ function createGlobalSync() { }, 1000) }) }, + refreshProviders, project: { loadSessions, resolve: resolveProject, diff --git a/frontend/workspace/src/context/inflight-cache.test.ts b/frontend/workspace/src/context/inflight-cache.test.ts new file mode 100644 index 00000000..0a86ab9d --- /dev/null +++ b/frontend/workspace/src/context/inflight-cache.test.ts @@ -0,0 +1,98 @@ +import { expect, test } from "bun:test" +import { createInflightCache } from "./inflight-cache" + +const wait = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) + +test("shares one load between concurrent callers", async () => { + let loads = 0 + const cache = createInflightCache(async () => { + loads++ + await wait(5) + return loads + }) + + const [a, b] = await Promise.all([cache.get("k"), cache.get("k")]) + + expect(loads).toBe(1) + expect(a).toBe(b) +}) + +test("keeps separate keys apart", async () => { + const cache = createInflightCache(async (key: string) => key) + + expect(await cache.get("a")).toBe("a") + expect(await cache.get("b")).toBe("b") +}) + +test("loads again once the shared window has passed", async () => { + let loads = 0 + const cache = createInflightCache( + async () => { + loads++ + return loads + }, + { holdMs: 5 }, + ) + + await cache.get("k") + await wait(20) + await cache.get("k") + + expect(loads).toBe(2) +}) + +test("does not cache a failure", async () => { + let loads = 0 + const cache = createInflightCache(async () => { + loads++ + throw new Error("boom") + }) + + await cache.get("k").catch(() => undefined) + await cache.get("k").catch(() => undefined) + + expect(loads).toBe(2) +}) + +// A load that never settles used to stay in the map forever, so every later +// call handed back the dead promise and no request was ever made again — the +// provider list simply stopped refreshing for the life of the page. +test("a load that never settles does not pin the key", async () => { + let loads = 0 + const cache = createInflightCache( + async () => { + loads++ + if (loads === 1) return new Promise(() => {}) // never settles + return loads + }, + { timeoutMs: 10 }, + ) + + const stuck = cache.get("k").catch(() => "timed out") + expect(await stuck).toBe("timed out") + + expect(await cache.get("k")).toBe(2) + expect(loads).toBe(2) +}) + +test("invalidate forces the next call to load again", async () => { + let loads = 0 + const cache = createInflightCache(async () => ++loads) + + await cache.get("k") + cache.invalidate("k") + await cache.get("k") + + expect(loads).toBe(2) +}) + +test("invalidate with no key clears every entry", async () => { + let loads = 0 + const cache = createInflightCache(async () => ++loads) + + await Promise.all([cache.get("a"), cache.get("b")]) + cache.invalidate() + await Promise.all([cache.get("a"), cache.get("b")]) + + expect(loads).toBe(4) +}) diff --git a/frontend/workspace/src/context/inflight-cache.ts b/frontend/workspace/src/context/inflight-cache.ts new file mode 100644 index 00000000..0d794728 --- /dev/null +++ b/frontend/workspace/src/context/inflight-cache.ts @@ -0,0 +1,60 @@ +/** + * Share an in-flight load between concurrent callers, keyed by scope. + * + * The bootstrap burst asks for the same catalog from several places within a + * few milliseconds; this collapses those into one request. What it must never + * do is outlive the request itself — an entry that is only removed when the + * promise settles becomes permanent the moment a request hangs (a severed + * connection, a server restart mid-flight), and from then on every caller gets + * handed the dead promise instead of a new request. `timeoutMs` bounds that: + * the entry always leaves the map, settled or not. + */ +export function createInflightCache( + load: (key: string) => Promise, + options: { holdMs?: number; timeoutMs?: number } = {}, +) { + const hold = options.holdMs ?? 1_000 + const timeout = options.timeoutMs ?? 30_000 + const entries = new Map>() + + const drop = (key: string, promise: Promise) => { + if (entries.get(key) === promise) entries.delete(key) + } + + return { + get(key: string) { + const pending = entries.get(key) + if (pending) return pending + + const promise = new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error(`load timed out after ${timeout}ms`)), timeout) + load(key).then( + (value) => { + clearTimeout(timer) + resolve(value) + }, + (error) => { + clearTimeout(timer) + reject(error) + }, + ) + }) + + entries.set(key, promise) + promise.then( + // Hold a resolved value briefly so the bootstrap burst shares it, then + // let it expire — a stale catalog is worse than a second request. + () => setTimeout(() => drop(key, promise), hold), + () => drop(key, promise), + ) + return promise + }, + invalidate(key?: string) { + if (key === undefined) { + entries.clear() + return + } + entries.delete(key) + }, + } +} diff --git a/frontend/workspace/src/hooks/use-providers.ts b/frontend/workspace/src/hooks/use-providers.ts index 769d59f5..3aebeba4 100644 --- a/frontend/workspace/src/hooks/use-providers.ts +++ b/frontend/workspace/src/hooks/use-providers.ts @@ -26,6 +26,13 @@ export function useProviders() { const directory = currentDirectory() if (!directory) return globalSync.data.provider const [projectStore] = globalSync.child(directory, { projectID: currentProjectID() }) + // The catalog belongs to the install; a project only ever narrows it. When + // this project's own load did not land — its bootstrap failed, or the + // server rejected the scope because two checkouts share one project + // identity — fall back to the install catalog rather than reporting that + // the user has no credentials at all. Showing "no keys" to someone who has + // keys is the worse failure. + if (!projectStore.provider.all.length) return globalSync.data.provider return projectStore.provider }) const connected = createMemo(() => providers().all.filter((p) => providers().connected.includes(p.id))) diff --git a/frontend/workspace/src/utils/base64.test.ts b/frontend/workspace/src/utils/base64.test.ts new file mode 100644 index 00000000..cb4c21ae --- /dev/null +++ b/frontend/workspace/src/utils/base64.test.ts @@ -0,0 +1,26 @@ +import { expect, test } from "bun:test" +import { base64Encode } from "@synsci/util/encode" +import { decode64 } from "./base64" + +test("decodes a legacy base64 directory segment", () => { + expect(decode64(base64Encode("/home/keertan/rsi"))).toBe("/home/keertan/rsi") +}) + +test("decodes a legacy base64 windows directory segment", () => { + expect(decode64(base64Encode("C:\\Users\\keertan\\rsi"))).toBe("C:\\Users\\keertan\\rsi") +}) + +// An opaque token in the first URL segment (share link, stale bookmark) is often +// valid base64. Decoding it yields binary junk that the server then resolved +// against its cwd and registered as a brand new project. +test("rejects a segment that decodes to bytes which are not a path", () => { + expect(decode64("9tQx1Zk2Lp7RvB0cNaWfEjHsUyTgOiMd")).toBeUndefined() +}) + +test("rejects a segment that decodes to a relative path", () => { + expect(decode64(base64Encode("hello-world"))).toBeUndefined() +}) + +test("rejects a segment that is not base64 at all", () => { + expect(decode64("not base64!!")).toBeUndefined() +}) diff --git a/frontend/workspace/src/utils/base64.ts b/frontend/workspace/src/utils/base64.ts index 08a311fa..4dd35fc2 100644 --- a/frontend/workspace/src/utils/base64.ts +++ b/frontend/workspace/src/utils/base64.ts @@ -3,10 +3,25 @@ import { createSignal } from "solid-js" const [active, setActive] = createSignal({ directory: "", projectID: undefined as string | undefined }) +const ABSOLUTE = /^(\/|\\\\|[A-Za-z]:[\\/])/ + +/** + * Any first URL segment that is not an opaque project selector is treated as a + * legacy base64 directory, and plenty of unrelated tokens (share links, stale + * bookmarks) decode cleanly as base64 into binary junk. Sending that junk on as + * a directory made the server resolve it against its cwd and register a brand + * new project, so only hand back something that can actually be a folder. + */ +function directory(value: string) { + if (!ABSOLUTE.test(value)) return + if (/[\p{Cc}\p{Cs}�]/u.test(value)) return + return value +} + export function decode64(value: string | undefined) { if (value === undefined) return try { - return base64Decode(value) + return directory(base64Decode(value)) } catch { return } From efd1f6829fc5ffbb6a72beee76374636e7af19eb Mon Sep 17 00:00:00 2001 From: KB Date: Sun, 2 Aug 2026 17:36:25 +0530 Subject: [PATCH 02/25] fix: publish storage records by atomic rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lock is an in-process map, so it orders writers inside one process and nothing at all between processes — and several openscience processes share this directory routinely (a CLI run alongside a running server; Project.fromDirectory rewrites a record on every instance creation). A plain write truncates in place, so a reader in another process can observe a half-written file and fail to parse it. Write to a unique temp path and rename, so every reader sees either the old record or the new one. --- backend/cli/src/storage/storage.ts | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/backend/cli/src/storage/storage.ts b/backend/cli/src/storage/storage.ts index f759b5db..9520393c 100644 --- a/backend/cli/src/storage/storage.ts +++ b/backend/cli/src/storage/storage.ts @@ -1,6 +1,7 @@ import { Log } from "../util/log" import path from "path" import fs from "fs/promises" +import { randomUUID } from "crypto" import { Global } from "../global" import { Filesystem } from "../util/filesystem" import { lazy } from "../util/lazy" @@ -180,6 +181,24 @@ export namespace Storage { }) } + /** + * Publish a record by rename. Lock is an in-process map, so it orders writers + * inside one process and nothing at all between processes — and several + * openscience processes share this directory routinely (a CLI run alongside a + * running server; `Project.fromDirectory` rewrites a record on every instance + * creation). A plain write truncates in place, so a reader in another process + * can observe a half-written file and fail to parse it. Rename is atomic, so + * every reader sees either the old record or the new one. + */ + async function publish(target: string, content: string) { + const tmp = `${target}.${process.pid}.${randomUUID()}.tmp` + await Bun.write(tmp, content) + await fs.rename(tmp, target).catch(async (error) => { + await fs.unlink(tmp).catch(() => {}) + throw error + }) + } + export async function update(key: string[], fn: (draft: T) => void) { const dir = await state().then((x) => x.dir) const target = path.join(dir, ...key) + ".json" @@ -187,7 +206,7 @@ export namespace Storage { using _ = await Lock.write(target) const content = await Bun.file(target).json() fn(content) - await Bun.write(target, JSON.stringify(content, null, 2)) + await publish(target, JSON.stringify(content, null, 2)) return content as T }) } @@ -197,7 +216,7 @@ export namespace Storage { const target = path.join(dir, ...key) + ".json" return withErrorHandling(async () => { using _ = await Lock.write(target) - await Bun.write(target, JSON.stringify(content, null, 2)) + await publish(target, JSON.stringify(content, null, 2)) }) } From d3c87806547eb2e289d8915f0a5c1dedc0fdcb63 Mon Sep 17 00:00:00 2001 From: KB Date: Sun, 2 Aug 2026 17:36:25 +0530 Subject: [PATCH 03/25] fix: skip sandbox masks for credential files that do not exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --ro-bind-try only tolerates a missing source. The destination is a mount point bwrap has to create, and everything above it is bound read-only, so masking a credential file the user has never created aborted the whole sandbox ("Can't create file at ...: Read-only file system") before the command ran — which killed every terminal on a machine without a credentials.json. A file that is not there has nothing to leak. Covered by a test that runs the produced argv through real bwrap, since the failure was in bwrap's acceptance of the arguments, not in their shape. --- backend/cli/test/sandbox/sandbox.test.ts | 26 ++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/backend/cli/test/sandbox/sandbox.test.ts b/backend/cli/test/sandbox/sandbox.test.ts index beec591f..f1f3d40e 100644 --- a/backend/cli/test/sandbox/sandbox.test.ts +++ b/backend/cli/test/sandbox/sandbox.test.ts @@ -3,6 +3,7 @@ import fs from "fs" import os from "os" import path from "path" import { Sandbox } from "../../src/sandbox/sandbox" +import { tmpdir } from "../fixture/fixture" const shell = "/bin/sh" @@ -104,6 +105,31 @@ describe("Sandbox.bubblewrapArgs", () => { }) expect(args).not.toContain(file) }) + + test.skipIf(Sandbox.backend() !== "bubblewrap")("produces an argv bwrap actually accepts", async () => { + await using tmp = await tmpdir() + const present = path.join(tmp.path, "auth.json") + await Bun.write(present, "{}") + + // The missing mask target has to sit on the read-only bind, the way a real + // ~/.local/share credential file does — a path under the sandbox's own + // tmpfs would be creatable and hide the failure. + const missing = path.join(os.homedir(), `.openscience-absent-${process.pid}.json`) + const args = Sandbox.bubblewrapArgs({ + writable: [tmp.path], + unreadable: [present, missing], + network: false, + }) + const proc = Bun.spawn(["bwrap", ...args, "--", "/bin/echo", "ok"], { stdout: "pipe", stderr: "pipe" }) + const [out, error, exit] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]) + + expect(exit, error).toBe(0) + expect(out.trim()).toBe("ok") + }) }) describe("Sandbox.backend/describe", () => { From 8914c7121178234569e710872b10c747dbf83805 Mon Sep 17 00:00:00 2001 From: KB Date: Sun, 2 Aug 2026 17:36:39 +0530 Subject: [PATCH 04/25] test: give spawned children the suite's sandboxed environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bun.spawn inherits the environment the test runner was launched with, not the one preload.ts assembles at import time — so a child booting the CLI resolved XDG paths against the developer's real home and wrote projects, sessions and auth into it. Those showed up as phantom entries on their home list. Route child spawns through a fixture that always forwards the live process.env. --- backend/cli/test/fixture/spawn.test.ts | 32 ++++++++++++++++++++++++++ backend/cli/test/fixture/spawn.ts | 23 ++++++++++++++++++ backend/cli/test/mcp/inspect.test.ts | 3 ++- 3 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 backend/cli/test/fixture/spawn.test.ts create mode 100644 backend/cli/test/fixture/spawn.ts diff --git a/backend/cli/test/fixture/spawn.test.ts b/backend/cli/test/fixture/spawn.test.ts new file mode 100644 index 00000000..95da8e76 --- /dev/null +++ b/backend/cli/test/fixture/spawn.test.ts @@ -0,0 +1,32 @@ +import { expect, test } from "bun:test" +import os from "os" +import { spawn } from "./spawn" + +// Bun.spawn defaults to the environment the process was started with, so the XDG +// overrides preload assigns at runtime never reached a child. Children that boot +// the CLI wrote projects and sessions into the developer's real +// ~/.local/share/openscience, which showed up as phantom entries on their home list. +test("gives a spawned child the sandboxed data directory", async () => { + const proc = spawn([process.execPath, "-e", "process.stdout.write(process.env.XDG_DATA_HOME ?? '')"], { + stdout: "pipe", + }) + const output = await new Response(proc.stdout).text() + await proc.exited + + expect(output).toBe(process.env["XDG_DATA_HOME"]!) + expect(output).not.toStartWith(os.homedir()) +}) + +test("keeps caller overrides on top of the sandboxed environment", async () => { + const proc = spawn( + [process.execPath, "-e", "process.stdout.write(`${process.env.HOME}|${process.env.XDG_DATA_HOME}`)"], + { + stdout: "pipe", + env: { HOME: "/tmp/elsewhere" }, + }, + ) + const output = await new Response(proc.stdout).text() + await proc.exited + + expect(output).toBe(`/tmp/elsewhere|${process.env["XDG_DATA_HOME"]}`) +}) diff --git a/backend/cli/test/fixture/spawn.ts b/backend/cli/test/fixture/spawn.ts new file mode 100644 index 00000000..5d1e8344 --- /dev/null +++ b/backend/cli/test/fixture/spawn.ts @@ -0,0 +1,23 @@ +/** + * Spawn a child that stays inside the suite's sandbox. + * + * `Bun.spawn` inherits the environment the test runner was launched with, not + * the one preload.ts assembles at import time — so a child booting the CLI + * resolved XDG paths against the developer's real home and wrote projects, + * sessions and auth into it. Always hand children the live `process.env`. + */ +export function spawn< + const In extends Bun.SpawnOptions.Writable = "ignore", + const Out extends Bun.SpawnOptions.Readable = "pipe", + const Err extends Bun.SpawnOptions.Readable = "inherit", +>( + command: string[], + options: Omit, "env"> & { + env?: Record + } = {}, +) { + return Bun.spawn(command, { + ...options, + env: { ...process.env, ...options.env }, + } as Bun.SpawnOptions.OptionsObject) +} diff --git a/backend/cli/test/mcp/inspect.test.ts b/backend/cli/test/mcp/inspect.test.ts index e0317ada..0d19f630 100644 --- a/backend/cli/test/mcp/inspect.test.ts +++ b/backend/cli/test/mcp/inspect.test.ts @@ -1,5 +1,6 @@ import { expect, test } from "bun:test" import { tmpdir } from "../fixture/fixture" +import { spawn } from "../fixture/spawn" test("inspect reports capabilities from a real local MCP server", async () => { await using tmp = await tmpdir() @@ -38,7 +39,7 @@ process.exit(0) `, ) - const proc = Bun.spawn([process.execPath, runner, tmp.path], { + const proc = spawn([process.execPath, runner, tmp.path], { cwd: tmp.path, stdout: "pipe", stderr: "pipe", From c0172b0f9b9bb1dc2937a705f29bf057c387b5de Mon Sep 17 00:00:00 2001 From: KB Date: Sun, 2 Aug 2026 17:36:39 +0530 Subject: [PATCH 05/25] feat: light text fields on focus instead of outlining them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A flat accent stroke around a field reads as a validation error, and the caret already says "you are typing here" — the field only has to be found, not flagged. --focus-lit stacks four layers into one falloff (a highlight on the top edge, a hairline, then two widening washes) so a focused field looks lit rather than outlined, leaving the hard accent border to mean something went wrong. Fields whose box is the glyphs themselves keep an outline for its outline-offset, the one way CSS holds a ring off the box, with the shadow as the spill past it; those also get padding so the caret does not start on the edge. Composers and TextField light their own frame on :focus-within, so the control inside stays dark rather than drawing a second, clipped inner ring. Restated for the in-app theme toggle, which prefers-color-scheme cannot see. --- frontend/ui/src/components/message-part.css | 3 +- frontend/ui/src/components/text-field.css | 9 ++-- frontend/ui/src/styles/theme.css | 42 ++++++++++++++++++ .../workspace/src/atlas/CommandPalette.tsx | 3 ++ frontend/workspace/src/atlas/FolderPicker.tsx | 2 + .../src/atlas/OpenScienceFileTree.tsx | 1 + .../workspace/src/atlas/SkillsBrowser.tsx | 3 +- .../src/components/dialog-create-project.tsx | 5 ++- frontend/workspace/src/styles/atlas.css | 44 ++++++++++++------- 9 files changed, 90 insertions(+), 22 deletions(-) diff --git a/frontend/ui/src/components/message-part.css b/frontend/ui/src/components/message-part.css index 728c835c..6e7a9b9b 100644 --- a/frontend/ui/src/components/message-part.css +++ b/frontend/ui/src/components/message-part.css @@ -746,7 +746,8 @@ outline: none; &:focus { - border-color: var(--border-focus); + border-color: var(--focus-lit-ring); + box-shadow: var(--focus-lit); } &::placeholder { diff --git a/frontend/ui/src/components/text-field.css b/frontend/ui/src/components/text-field.css index c94376be..e08513bf 100644 --- a/frontend/ui/src/components/text-field.css +++ b/frontend/ui/src/components/text-field.css @@ -52,11 +52,12 @@ background: var(--input-base); &:focus-within:not(:has([data-readonly])) { - border-color: transparent; - /* border/shadow-xs/select */ + /* Lit rather than outlined — see --focus-lit in theme.css. The invalid + state below keeps its accent border, so the only hard stroke a field + can show now is the one that means something went wrong. */ + border-color: var(--focus-lit-ring); box-shadow: - 0 0 0 3px var(--border-weak-selected), - 0 0 0 1px var(--border-selected), + var(--focus-lit), 0 1px 2px -1px rgba(19, 16, 16, 0.25), 0 1px 2px 0 rgba(19, 16, 16, 0.08), 0 1px 3px 0 rgba(19, 16, 16, 0.12); diff --git a/frontend/ui/src/styles/theme.css b/frontend/ui/src/styles/theme.css index 6dff0584..113b899d 100644 --- a/frontend/ui/src/styles/theme.css +++ b/frontend/ui/src/styles/theme.css @@ -229,6 +229,25 @@ --border-selected: #b85c3b; --border-disabled: var(--smoke-light-alpha-8); --border-focus: #b85c3b; + /* Focus lighting for text entry. A flat accent stroke around a field reads as + a validation error; light does not arrive as a hard edge. The four layers + stack into one falloff — a highlight where the light lands on the top edge, + a hairline, then two widening washes — so a focused field looks lit rather + than outlined. Light mode grounds the falloff in ink, because a white wash + is invisible on a white surface. */ + --focus-lit-edge: rgba(255, 255, 255, 0.9); + --focus-lit-ring: rgba(30, 30, 27, 0.18); + --focus-lit-wash: rgba(30, 30, 27, 0.07); + --focus-lit-bloom: rgba(30, 30, 27, 0.05); + --focus-lit: + inset 0 1px 0 var(--focus-lit-edge), 0 0 0 1px var(--focus-lit-ring), 0 0 0 4px var(--focus-lit-wash), + 0 0 16px 1px var(--focus-lit-bloom); + /* The halo behind an unframed field — one whose box is the glyphs themselves, + so nothing can be drawn on the box without landing on the text. Its crisp + edge is an outline instead, the only ring CSS lets you hold off the box + (outline-offset), which is what the accent stroke used before. This is just + the light spilling past it. */ + --focus-lit-halo: 0 0 0 5px var(--focus-lit-wash), 0 0 16px 4px var(--focus-lit-bloom); --border-weak-base: #3e2e2112; --border-strong-base: #3e2e2147; --border-strong-hover: var(--smoke-light-alpha-8); @@ -487,6 +506,11 @@ --border-selected: #d48765; --border-disabled: #f2f1ec59; --border-focus: #d48765; + /* On a dark surface the falloff is the light itself, so every layer is white. */ + --focus-lit-edge: rgba(255, 255, 255, 0.26); + --focus-lit-ring: rgba(255, 255, 255, 0.2); + --focus-lit-wash: rgba(255, 255, 255, 0.07); + --focus-lit-bloom: rgba(255, 255, 255, 0.06); --border-weak-base: #f2f1ec14; --border-strong-base: #f2f1ec4a; --border-strong-hover: #f2f1ec3f; @@ -632,3 +656,21 @@ --avatar-text-lime: #c4f042; } } + +/* The in-app theme toggle writes data-color-scheme onto ; the + prefers-color-scheme block above cannot see it. Restate the focus falloff for + both explicit choices, so a dark app on a light OS is lit with white rather + than ink — and the reverse. */ +html[data-color-scheme="dark"] { + --focus-lit-edge: rgba(255, 255, 255, 0.26); + --focus-lit-ring: rgba(255, 255, 255, 0.2); + --focus-lit-wash: rgba(255, 255, 255, 0.07); + --focus-lit-bloom: rgba(255, 255, 255, 0.06); +} + +html[data-color-scheme="light"] { + --focus-lit-edge: rgba(255, 255, 255, 0.9); + --focus-lit-ring: rgba(30, 30, 27, 0.18); + --focus-lit-wash: rgba(30, 30, 27, 0.07); + --focus-lit-bloom: rgba(30, 30, 27, 0.05); +} diff --git a/frontend/workspace/src/atlas/CommandPalette.tsx b/frontend/workspace/src/atlas/CommandPalette.tsx index 5e03935f..6b5cf85b 100644 --- a/frontend/workspace/src/atlas/CommandPalette.tsx +++ b/frontend/workspace/src/atlas/CommandPalette.tsx @@ -397,6 +397,9 @@ export function CommandPalette(props: CommandPaletteProps): JSX.Element { "font-family": FONT_MONO, "font-size": "13px", color: "var(--color-text)", + // `all: unset` leaves the box flush with the glyphs, so the + // caret starts on the edge and the focus ring lands on the text. + padding: "3px 10px", }} />