From 2efd7f30e38eade53e456de8e9048fb02559babd Mon Sep 17 00:00:00 2001 From: Sarav Date: Wed, 16 Sep 2026 10:49:27 +0530 Subject: [PATCH 1/2] fix: [AI-9171] create a quick workspace from an already-linked project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The picker's "＋ Create a quick workspace here" row always failed when the project was already linked, and told the user to re-run the command they were already inside. `createThenBindOrRebind` already had a correct rebind branch — it was simply unreachable. Step one called `createAndBind`, and the server's `create_and_bind` pre-checks both identifiers and 409s *before* creating anything, deliberately, so a binding conflict cannot strand a half-created workspace. On an already linked project that refuses the whole call, so nothing was created and the rebind below it was dead code. The row's own hint promised the opposite: "Creates a new workspace and repoints this project to it". Split the two cases: - unlinked: unchanged — `createAndBind` still creates and binds in one server-side transaction, which is what makes a stranded workspace impossible there. - already linked: create the workspace unbound via `POST /datamates/`, then repoint through the existing rebind path. `createWorkspaceUnbound` sends `memory_enabled` and `knowledge_engine_enabled` explicitly. `POST /datamates/` is the SaaS/extension creation path and defaults both to false, while the create-and-bind path sets both true — so without this the same menu row would hand back a differently configured workspace depending only on whether the project happened to be linked, with memory and the knowledge engine silently off. The comment names the coupling so the two move together. Also corrected the 409 message. After this change a conflict can only mean another workspace claimed the project mid-selection, so it says that instead of directing the user back into the command they are already running. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/altimate/workspace/api-client.ts | 40 +++++ packages/opencode/src/cli/cmd/link.ts | 69 ++++++--- .../create-workspace-unbound.test.ts | 143 ++++++++++++++++++ 3 files changed, 230 insertions(+), 22 deletions(-) create mode 100644 packages/opencode/test/altimate/workspace/create-workspace-unbound.test.ts diff --git a/packages/opencode/src/altimate/workspace/api-client.ts b/packages/opencode/src/altimate/workspace/api-client.ts index a1366c2cbc..4053d738a4 100644 --- a/packages/opencode/src/altimate/workspace/api-client.ts +++ b/packages/opencode/src/altimate/workspace/api-client.ts @@ -395,6 +395,46 @@ export namespace WorkspaceApi { }) } + /** Create a workspace WITHOUT binding anything to it. + * + * ``createAndBind`` is the right call for an unlinked project: it creates and + * binds in one server-side transaction, so a binding conflict cannot strand a + * workspace. But it pre-checks the identifiers and 409s *before* creating, + * which makes it unusable when the project is already linked — there is + * nothing to create, and the caller's rebind never gets a target (AI-9171). + * This is the two-step path for that case: create here, then rebind. + * + * The flags below deliberately mirror ``_create_datamate_flush_only`` in + * altimate-backend, which is what ``createAndBind`` reaches. ``POST + * /datamates/`` is the SaaS/extension creation path and defaults BOTH to + * false, so omitting them would hand a differently-configured workspace to + * whichever caller happened to be already linked — same menu row, memory and + * knowledge engine silently off. If the backend's workspace defaults move, + * this has to move with them; there is no endpoint that applies them without + * also binding. + */ + export async function createWorkspaceUnbound(input: { + name: string + description?: string + }): Promise<{ id: number; name: string }> { + const data = await req<{ id: number }>("POST", "/", { + base: "/datamates", + body: { + name: input.name, + description: input.description ?? null, + integrations: [], + memory_enabled: true, + knowledge_engine_enabled: true, + privacy: "private", + }, + }) + const id = Number(data?.id) + if (!Number.isSafeInteger(id) || id <= 0) { + throw new Error(`Workspace was created but the server returned no usable id (${String(data?.id)}).`) + } + return { id, name: input.name } + } + export async function bindExisting( datamateId: number, identifier: ProjectIdentifier, diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index 9da25ac1cf..3ad8b5af1f 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -22,6 +22,7 @@ import { NotConfiguredError, NotFoundError, PreconditionFailedError, + type Binding, type DatamateRef, type MatchedIdentifier, type ProjectBindingLookup, @@ -484,19 +485,36 @@ async function createThenBindOrRebind( ): Promise { const spin = prompts.spinner() spin.start(`Creating workspace "${name}"...`) - let created: Awaited> + let created: { datamate: DatamateRef; binding?: Binding; manage_url?: string } try { - created = await WorkspaceApi.createAndBind({ name, identifier }) + // Two different creates, because the server offers two different things. + // + // Unlinked: ``createAndBind`` creates and binds in ONE transaction, so a + // conflicting binding can never strand a half-created workspace. + // + // Already linked: that same atomicity makes it unusable. ``create_and_bind`` + // pre-checks the identifiers and 409s *before* creating anything, so the + // rebind below never got a target and this row simply always failed — with + // an error telling the user to re-run the command they were already inside + // (AI-9171). Create unbound first, then repoint, which is what the row's + // own hint promises. + if (existing) { + const ws = await WorkspaceApi.createWorkspaceUnbound({ name }) + created = { datamate: ws } + } else { + created = await WorkspaceApi.createAndBind({ name, identifier }) + } } catch (err) { spin.stop("Failed to create workspace.", 1) - // A 409 from create means someone else's binding on the same - // remote/path beat us. If the pre-check already knew about it, the user - // can pick from the list; if the pre-check missed it, this is the - // authoritative signal — surface it and hint the picker. + // A 409 here can only be a binding this caller did not know about: the + // already-linked path does not bind at all, and the unlinked path only + // reaches the server after a pre-check that said the project was free. So + // someone else's binding on the same remote/path landed in between. if (err instanceof ConflictError) { const existingName = conflictExistingName(err.detail) prompts.log.error( - `This project is already linked to "${existingName}". Re-run \`altimate-code link\` to switch to a different workspace.`, + `Another workspace, "${existingName}", claimed this project while you were choosing. ` + + `Nothing was created. Run \`altimate-code link\` again to see the current list.`, ) } else { prompts.log.error(err instanceof Error ? err.message : String(err)) @@ -510,10 +528,9 @@ async function createThenBindOrRebind( const safeCreatedName = stripControlChars(created.datamate.name) spin.stop(`Workspace "${safeCreatedName}" created.`) - // If the project was already linked, the new workspace exists but the - // binding still points at the OLD workspace — rebind so the project is - // now bound to the freshly-created one. Otherwise createAndBind already - // wrote the binding as part of the atomic create; we're done. + // The already-linked path created an unbound workspace above, so the binding + // still points at the OLD one — repoint it now. The unlinked path already got + // its binding from the atomic create, so there is nothing left to do. if (existing) { const rebindSpin = prompts.spinner() rebindSpin.start(`Repointing project at "${safeCreatedName}"...`) @@ -537,23 +554,31 @@ async function createThenBindOrRebind( // Prefer the canonicalized ``identifier.projectPath`` over the raw // ``--directory`` argument so ``altimate-code link -d ./myproj`` and its // symlink-resolved twin both write under the same cache key (Kilo cycle 6). + // The unbound path has no server binding row to echo, so fall back to the + // identifier we just rebound on — the same values the rebind wrote. await recordApprovedBinding(identifier.projectPath ?? directory, { datamateId: created.datamate.id, datamateName: created.datamate.name, - repoRemote: created.binding.repo_remote, - projectPath: created.binding.project_path, + repoRemote: created.binding?.repo_remote ?? identifier.repoRemote ?? null, + projectPath: created.binding?.project_path ?? identifier.projectPath ?? null, linkedAt: Date.now(), }, { awaitBackfill: true }) prompts.log.info("Saved memory blocks will sync to this workspace if memory is enabled for it.") - prompts.log.info(`Manage it at: ${created.manage_url}`) - // Guard against a server that hands back a non-http(s) manage_url — ``open`` - // delegates to the OS handler, so a rogue value could launch an unrelated - // application. Log a warning and skip the auto-open rather than trusting - // whatever protocol the URL parses to. - if (isSafeHttpUrl(created.manage_url)) { - await open(created.manage_url).catch(() => undefined) - } else { - prompts.log.warn(`Skipped auto-open: manage_url is not an http/https URL.`) + // ``createAndBind`` hands back a manage_url; the unbound create does not, so + // derive it from credentials exactly as the rest of this file does. Null on + // BYOK / unresolvable deployments — then there is simply nothing to show. + const manageUrl = created.manage_url ?? (await manageUrlFor(created.datamate.id)) + if (manageUrl) { + prompts.log.info(`Manage it at: ${manageUrl}`) + // Guard against a server that hands back a non-http(s) manage_url — ``open`` + // delegates to the OS handler, so a rogue value could launch an unrelated + // application. Log a warning and skip the auto-open rather than trusting + // whatever protocol the URL parses to. + if (isSafeHttpUrl(manageUrl)) { + await open(manageUrl).catch(() => undefined) + } else { + prompts.log.warn(`Skipped auto-open: manage_url is not an http/https URL.`) + } } prompts.outro("Done.") } diff --git a/packages/opencode/test/altimate/workspace/create-workspace-unbound.test.ts b/packages/opencode/test/altimate/workspace/create-workspace-unbound.test.ts new file mode 100644 index 0000000000..16e23e53d0 --- /dev/null +++ b/packages/opencode/test/altimate/workspace/create-workspace-unbound.test.ts @@ -0,0 +1,143 @@ +// altimate_change - new file +// Coverage for WorkspaceApi.createWorkspaceUnbound (AI-9171). +// +// This exists because `altimate link`'s "create a quick workspace" row always +// failed on an already-linked project: it went through `createAndBind`, whose +// server handler pre-checks the identifiers and 409s BEFORE creating, so the +// rebind that was supposed to follow never got a target. The fix creates the +// workspace unbound first, then repoints. +// +// The assertions worth having are about the REQUEST, not the response. Two +// creation paths now exist, and the silent failure mode is that they disagree: +// `POST /datamates/` defaults memory and the knowledge engine to false, while +// the create-and-bind path sets both true. If this drifts, the same menu row +// produces a differently-configured workspace depending only on whether the +// project happened to be linked — which nothing else would catch. +import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test" +import { mkdirSync, writeFileSync } from "node:fs" +import path from "node:path" +import os from "node:os" + +const ORIGINAL_TEST_HOME = process.env.OPENCODE_TEST_HOME +const SANDBOX = path.join(os.tmpdir(), `altimate-createunbound-${process.pid}-${Date.now()}`) +mkdirSync(path.join(SANDBOX, "home", ".altimate"), { recursive: true }) +process.env.OPENCODE_TEST_HOME = path.join(SANDBOX, "home") + +const API_URL = "https://api.example.test" +const TENANT = "acme" + +// A real credentials file, so the module resolves them the same way it does in +// production rather than through a stubbed export. +writeFileSync( + path.join(SANDBOX, "home", ".altimate", "altimate.json"), + JSON.stringify({ + altimateUrl: API_URL, + altimateInstanceName: TENANT, + altimateApiKey: "test-key", + }), +) + +const { WorkspaceApi } = await import("@/altimate/workspace/api-client") + +const ORIGINAL_FETCH = globalThis.fetch + +interface Captured { + url: string + method: string + body: Record +} + +let captured: Captured[] = [] + +/** Stub fetch, recording each request and replying with `reply`. */ +function respondWith(status: number, reply: unknown) { + globalThis.fetch = (async (input: string | URL, init?: RequestInit) => { + captured.push({ + url: String(input), + method: String(init?.method ?? "GET"), + body: init?.body ? JSON.parse(String(init.body)) : {}, + }) + return new Response(JSON.stringify(reply), { + status, + headers: { "content-type": "application/json" }, + }) + }) as typeof globalThis.fetch +} + +beforeEach(() => { + captured = [] +}) + +afterEach(() => { + globalThis.fetch = ORIGINAL_FETCH +}) + +afterAll(() => { + if (ORIGINAL_TEST_HOME === undefined) delete process.env.OPENCODE_TEST_HOME + else process.env.OPENCODE_TEST_HOME = ORIGINAL_TEST_HOME +}) + +describe("createWorkspaceUnbound", () => { + test("posts to /datamates/, NOT to the binding router", async () => { + respondWith(200, { id: 77 }) + await WorkspaceApi.createWorkspaceUnbound({ name: "jaffle_shop" }) + + expect(captured).toHaveLength(1) + expect(captured[0].method).toBe("POST") + // The whole point: this must not reach create_and_bind, which would 409. + expect(captured[0].url).not.toContain("datamate-project-bindings") + expect(captured[0].url).toContain("/datamates/") + }) + + test("sends no project identifier — binding is the caller's next step", async () => { + respondWith(200, { id: 77 }) + await WorkspaceApi.createWorkspaceUnbound({ name: "jaffle_shop" }) + + expect(captured[0].body).not.toHaveProperty("repo_remote") + expect(captured[0].body).not.toHaveProperty("project_path") + }) + + test("applies the workspace defaults, not the SaaS ones", async () => { + respondWith(200, { id: 77 }) + await WorkspaceApi.createWorkspaceUnbound({ name: "jaffle_shop" }) + + // `POST /datamates/` defaults both to false. Sending them explicitly is what + // keeps an already-linked project's new workspace configured like every + // other CLI-created one. Dropping either line is the regression. + expect(captured[0].body.memory_enabled).toBe(true) + expect(captured[0].body.knowledge_engine_enabled).toBe(true) + expect(captured[0].body.privacy).toBe("private") + // Required by CreateDatamateRequest — omitting it is a 422. + expect(captured[0].body.integrations).toEqual([]) + }) + + test("returns the created id and the caller's name", async () => { + respondWith(200, { id: 77 }) + const created = await WorkspaceApi.createWorkspaceUnbound({ name: "jaffle_shop" }) + expect(created).toEqual({ id: 77, name: "jaffle_shop" }) + }) + + test("passes a description through when given, null when not", async () => { + respondWith(200, { id: 77 }) + await WorkspaceApi.createWorkspaceUnbound({ name: "a", description: "from the CLI" }) + expect(captured[0].body.description).toBe("from the CLI") + + captured = [] + respondWith(200, { id: 78 }) + await WorkspaceApi.createWorkspaceUnbound({ name: "b" }) + expect(captured[0].body.description).toBeNull() + }) + + test("rejects a response with no usable id rather than returning NaN", async () => { + // A workspace the caller cannot then rebind to is worse than a clear error: + // `Number(undefined)` is NaN, which would reach the rebind as a garbage + // target id. + respondWith(200, { id: null }) + await expect(WorkspaceApi.createWorkspaceUnbound({ name: "x" })).rejects.toThrow(/no usable id/) + }) + + test("rejects a non-integer id", async () => { + respondWith(200, { id: "not-a-number" }) + await expect(WorkspaceApi.createWorkspaceUnbound({ name: "x" })).rejects.toThrow(/no usable id/) + }) +}) From 7b21606fcc22f7ee8381670460c2feb7765a2aba Mon Sep 17 00:00:00 2001 From: Sarav Date: Fri, 18 Sep 2026 08:21:02 +0530 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20[AI-9171]=20address=20review=20?= =?UTF-8?q?=E2=80=94=20fix=20the=20TUI's=20copy=20of=20the=20same=20bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round on PR #1314. The important one is sahrizvi's: the TUI had the identical defect and this PR did not touch it. **The TUI carried the same bug.** `createAndBindInline` called `createAndBind` unconditionally, so on an already-linked project the server 409'd before creating anything and the rebind below was unreachable — exactly what this PR fixes in `link.ts`. Its `rebindFrom` doc asserted the assumption the bug rested on ("createAndBind succeeds but leaves the binding pointing at the OLD workspace"); that is corrected too. Both surfaces now make the same split, so the CLI and the TUI cannot disagree about what that row does. **`Number()` coerced its way past the id guard.** `Number.isSafeInteger(Number(x))` accepts `true` as 1, `"7"` as 7 and `[5]` as 5, so a malformed body would have rebound the project to workspace 1 rather than failing. The type check now runs before the arithmetic, and it throws a typed `WorkspaceApiError` rather than a bare `Error`, matching every other failure in that module. **The account is pinned across the two-step.** Create and rebind resolve credentials independently, so a re-login in between created the workspace on one tenant and sent the rebind to another with an id local to the first. Both surfaces capture the account before the create and refuse the rebind if it changed. The API key is deliberately not part of the fingerprint: rotating a key for the same user on the same tenant is not an identity change. **The rebind's own binding row is what gets cached.** The response was discarded and the local identifiers cached instead, so a path-keyed row rebound through `/by-path` was cached carrying a `repo_remote` the server never stored. **The 409 message no longer guesses.** It was written for the bind path and claimed a binding race; the unbound create sends no identifiers and cannot produce one. Split by which call actually ran. **`created` is a discriminated union** rather than one shape with optional halves, so `binding` and `manage_url` are only reachable on the path that has them. Not changed, with reason: cubic asked for the nested `{datamate:{id}}` shape to be accepted. `POST /datamates/` declares `response_model=CreateDatamateResponse` (`{id: int}`) and FastAPI enforces it, so that shape cannot come back from this endpoint. The strict guard above turns a contract break into a loud failure, which is the better outcome than silently accepting an unexpected shape. Tests: `create-then-rebind.test.ts` asserts the *sequence of endpoints* for both surfaces, because the bug was never in a payload — it was in which request got sent. Covers unlinked, already-linked, a failed rebind (non-zero exit / error toast, not a claimed success), and a 409 that must not reach the rebind. Each one fails if the corresponding fix is reverted. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/altimate/workspace/api-client.ts | 37 ++- packages/opencode/src/cli/cmd/link.ts | 85 +++++-- .../src/plugin/tui/altimate/workspace.tsx | 71 ++++-- .../workspace/create-then-rebind.test.ts | 218 ++++++++++++++++++ .../create-workspace-unbound.test.ts | 51 +++- 5 files changed, 422 insertions(+), 40 deletions(-) create mode 100644 packages/opencode/test/altimate/workspace/create-then-rebind.test.ts diff --git a/packages/opencode/src/altimate/workspace/api-client.ts b/packages/opencode/src/altimate/workspace/api-client.ts index 4053d738a4..e45682f3e7 100644 --- a/packages/opencode/src/altimate/workspace/api-client.ts +++ b/packages/opencode/src/altimate/workspace/api-client.ts @@ -413,6 +413,28 @@ export namespace WorkspaceApi { * this has to move with them; there is no endpoint that applies them without * also binding. */ + /** Who the next call will act as. + * + * A create-then-rebind pair is two requests, and `req()` resolves credentials + * independently for each. If the account changes in between — a re-login, an + * edited `altimate.json` — the workspace is created in one tenant and the + * rebind is sent to another with an id that is local to the first. Callers + * capture this before the create and re-check it before the rebind. + * + * The API key is deliberately not part of it: rotating a key for the same + * user on the same tenant is not an identity change, and comparing it would + * abort a legitimate flow. */ + export async function accountFingerprint(): Promise<{ apiUrl: string; tenant: string }> { + const c = await creds() + return { apiUrl: c.url, tenant: c.instance } + } + + /** True when `before` still describes the account in effect. */ + export async function sameAccount(before: { apiUrl: string; tenant: string }): Promise { + const now = await accountFingerprint().catch(() => null) + return now !== null && now.apiUrl === before.apiUrl && now.tenant === before.tenant + } + export async function createWorkspaceUnbound(input: { name: string description?: string @@ -428,9 +450,18 @@ export namespace WorkspaceApi { privacy: "private", }, }) - const id = Number(data?.id) - if (!Number.isSafeInteger(id) || id <= 0) { - throw new Error(`Workspace was created but the server returned no usable id (${String(data?.id)}).`) + // `typeof` FIRST, before any arithmetic. `Number()` coerces, so the + // previous `Number.isSafeInteger(Number(data?.id))` accepted `true` as 1, + // `"7"` as 7 and `[5]` as 5 — a malformed body would have rebound the + // project to whatever those coerced to (workspace 1, in the boolean case) + // instead of failing. The server's `CreateDatamateResponse` is `{id: int}` + // and FastAPI enforces it, so anything else here is a contract break worth + // refusing loudly rather than guessing at. + const id: unknown = (data as { id?: unknown } | null | undefined)?.id + if (typeof id !== "number" || !Number.isSafeInteger(id) || id <= 0) { + throw new WorkspaceApiError( + `Workspace was created but the server returned no usable id (${JSON.stringify(id) ?? "undefined"}).`, + ) } return { id, name: input.name } } diff --git a/packages/opencode/src/cli/cmd/link.ts b/packages/opencode/src/cli/cmd/link.ts index 3ad8b5af1f..562a59aa1a 100644 --- a/packages/opencode/src/cli/cmd/link.ts +++ b/packages/opencode/src/cli/cmd/link.ts @@ -476,8 +476,12 @@ function handoffFailureMessage(result: Extract): s * real (billable) SaaS resource the CLI knows nothing about and the project * is still bound to the old workspace (M2 in the consensus review). When * rebind fails, the error message tells the user the workspace was created - * and how to recover; we do NOT silently swallow the orphan. */ -async function createThenBindOrRebind( + * and how to recover; we do NOT silently swallow the orphan. + * + * Exported for tests. The branch it picks — atomic create-and-bind when the + * project is free, unbound-create-then-rebind when it is already linked — is + * the whole of AI-9171, and nothing else in this file can assert it. */ +export async function createThenBindOrRebind( identifier: ProjectIdentifier, name: string, directory: string, @@ -485,7 +489,20 @@ async function createThenBindOrRebind( ): Promise { const spin = prompts.spinner() spin.start(`Creating workspace "${name}"...`) - let created: { datamate: DatamateRef; binding?: Binding; manage_url?: string } + // Discriminated on how the workspace was made, because the two creates return + // genuinely different things: only `bound` carries a server binding row and a + // manage_url. An optional-field shape let the rest of this function reach for + // `binding` on the path that never has one and silently fall through to a + // default. (review, PR #1314) + type Created = + | { via: "bound"; datamate: DatamateRef; binding: Binding; manage_url: string } + | { via: "unbound"; datamate: DatamateRef } + let created: Created + // Captured BEFORE the create and re-checked before the rebind. Each request + // resolves credentials on its own, so an account switch in between would + // create the workspace on one tenant and rebind on another using an id that + // is local to the first. (review, PR #1314) + const account = await WorkspaceApi.accountFingerprint().catch(() => null) try { // Two different creates, because the server offers two different things. // @@ -500,22 +517,31 @@ async function createThenBindOrRebind( // own hint promises. if (existing) { const ws = await WorkspaceApi.createWorkspaceUnbound({ name }) - created = { datamate: ws } + created = { via: "unbound", datamate: ws } } else { - created = await WorkspaceApi.createAndBind({ name, identifier }) + const res = await WorkspaceApi.createAndBind({ name, identifier }) + created = { via: "bound", datamate: res.datamate, binding: res.binding, manage_url: res.manage_url } } } catch (err) { spin.stop("Failed to create workspace.", 1) - // A 409 here can only be a binding this caller did not know about: the - // already-linked path does not bind at all, and the unlinked path only - // reaches the server after a pre-check that said the project was free. So - // someone else's binding on the same remote/path landed in between. + // Split by which call actually ran, because they cannot 409 for the same + // reason. `createAndBind` sends the project identifiers, so its conflict is + // a binding race. The unbound create sends none — it cannot produce an + // identity conflict at all, so attributing one there would have sent the + // user looking for a race that did not happen. (review, PR #1314) if (err instanceof ConflictError) { - const existingName = conflictExistingName(err.detail) - prompts.log.error( - `Another workspace, "${existingName}", claimed this project while you were choosing. ` + - `Nothing was created. Run \`altimate-code link\` again to see the current list.`, - ) + if (existing) { + prompts.log.error( + `The workspace could not be created: ${err.message}. Nothing was created, and this ` + + `project is still linked to "${stripControlChars(existing.datamate.name)}".`, + ) + } else { + const existingName = conflictExistingName(err.detail) + prompts.log.error( + `Another workspace, "${existingName}", claimed this project while you were choosing. ` + + `Nothing was created. Run \`altimate-code link\` again to see the current list.`, + ) + } } else { prompts.log.error(err instanceof Error ? err.message : String(err)) } @@ -531,16 +557,31 @@ async function createThenBindOrRebind( // The already-linked path created an unbound workspace above, so the binding // still points at the OLD one — repoint it now. The unlinked path already got // its binding from the atomic create, so there is nothing left to do. + let reboundBinding: Binding | null = null if (existing) { const rebindSpin = prompts.spinner() rebindSpin.start(`Repointing project at "${safeCreatedName}"...`) + // The workspace exists on the account that was in effect a moment ago, and + // its id means nothing anywhere else. Rebinding under a different account + // would point this project at whatever id collides there. + if (account && !(await WorkspaceApi.sameAccount(account))) { + rebindSpin.stop("Could not repoint the project.", 1) + prompts.log.error( + `The signed-in account changed while "${safeCreatedName}" was being created, so it was ` + + `not linked to this project. The workspace exists on the previous account. Re-run ` + + `\`altimate-code link\` to link this project on the account you are on now.`, + ) + process.exitCode = 1 + return + } try { - await rebindByMatchedIdentifier({ + const res = await rebindByMatchedIdentifier({ identifier, targetDatamateId: created.datamate.id, expectedCurrentDatamateId: existing.datamate.id, matchedBy: existing.matchedBy, }) + reboundBinding = res.binding rebindSpin.stop(`Project is now linked to "${safeCreatedName}".`) } catch (err) { rebindSpin.stop("Could not repoint the project.", 1) @@ -554,20 +595,24 @@ async function createThenBindOrRebind( // Prefer the canonicalized ``identifier.projectPath`` over the raw // ``--directory`` argument so ``altimate-code link -d ./myproj`` and its // symlink-resolved twin both write under the same cache key (Kilo cycle 6). - // The unbound path has no server binding row to echo, so fall back to the - // identifier we just rebound on — the same values the rebind wrote. + // Whichever call last wrote the row is what gets cached. `createAndBind` + // returns it directly; the unbound path gets it from the rebind. Caching the + // local identifiers instead would record fields the server never stored — a + // path-keyed row rebound through `/by-path` would be cached carrying a + // `repo_remote` that is not on the server's row. (review, PR #1314) + const serverBinding = created.via === "bound" ? created.binding : reboundBinding await recordApprovedBinding(identifier.projectPath ?? directory, { datamateId: created.datamate.id, datamateName: created.datamate.name, - repoRemote: created.binding?.repo_remote ?? identifier.repoRemote ?? null, - projectPath: created.binding?.project_path ?? identifier.projectPath ?? null, + repoRemote: serverBinding?.repo_remote ?? identifier.repoRemote ?? null, + projectPath: serverBinding?.project_path ?? identifier.projectPath ?? null, linkedAt: Date.now(), }, { awaitBackfill: true }) prompts.log.info("Saved memory blocks will sync to this workspace if memory is enabled for it.") // ``createAndBind`` hands back a manage_url; the unbound create does not, so // derive it from credentials exactly as the rest of this file does. Null on // BYOK / unresolvable deployments — then there is simply nothing to show. - const manageUrl = created.manage_url ?? (await manageUrlFor(created.datamate.id)) + const manageUrl = created.via === "bound" ? created.manage_url : await manageUrlFor(created.datamate.id) if (manageUrl) { prompts.log.info(`Manage it at: ${manageUrl}`) // Guard against a server that hands back a non-http(s) manage_url — ``open`` diff --git a/packages/opencode/src/plugin/tui/altimate/workspace.tsx b/packages/opencode/src/plugin/tui/altimate/workspace.tsx index 8c81c11b78..cb9d6ee532 100644 --- a/packages/opencode/src/plugin/tui/altimate/workspace.tsx +++ b/packages/opencode/src/plugin/tui/altimate/workspace.tsx @@ -37,6 +37,7 @@ import { NotFoundError, PreconditionFailedError, WorkspaceApi, + type Binding, type DatamateRef, type MatchedIdentifier, type ProjectBindingLookup, @@ -493,22 +494,48 @@ function toastHandoffFailure(api: TuiPluginApi, result: Extract { api.ui.dialog.clear() - let res: Awaited> + // The same split the CLI makes in `cli/cmd/link.ts` (AI-9171), for the same + // reason. `createAndBind` pre-checks the project identifiers server-side and + // 409s BEFORE creating anything, so on an already-linked project the catch + // below fired and the rebind further down was unreachable — this row never + // worked. The `rebindFrom` comment above described the opposite ("creates and + // binds, leaving the binding pointing at the OLD workspace"); that was the + // assumption the bug rested on. Create unbound first, then repoint. + type Created = + | { via: "bound"; datamate: DatamateRef; binding: Binding } + | { via: "unbound"; datamate: DatamateRef } + let res: Created + // Captured before the create and re-checked before the rebind: the two + // requests resolve credentials independently, and a workspace id means + // nothing on a different account. + const account = await WorkspaceApi.accountFingerprint().catch(() => null) try { - res = await WorkspaceApi.createAndBind({ name, identifier }) + if (rebindFrom) { + const ws = await WorkspaceApi.createWorkspaceUnbound({ name }) + res = { via: "unbound", datamate: ws } + } else { + const created = await WorkspaceApi.createAndBind({ name, identifier }) + res = { via: "bound", datamate: created.datamate, binding: created.binding } + } } catch (err) { - if (err instanceof ConflictError) { + // Only the bound path sends identifiers, so only it can lose an identity + // race. The unbound create sends none. + if (err instanceof ConflictError && !rebindFrom) { api.ui.toast({ variant: "warning", message: `This project is already linked to "${err.detail.existing_datamate_name ?? "another workspace"}". Use the palette's "Link this project to a workspace" to change.`, @@ -522,19 +549,27 @@ async function createAndBindInline( return } + let reboundBinding: Binding | null = null if (rebindFrom) { - // The atomic create-and-bind wrote a NEW binding for the new workspace, - // but the existing binding for THIS project's remote/path still points - // at the old workspace. Repoint via the matched-identifier rebind - // endpoint. If rebind fails, tell the user the workspace exists but - // the link didn't switch — do not silently orphan. + // The workspace above was created UNBOUND, so this project's binding still + // points at the old one. Repoint it. If this fails the workspace exists but + // the link did not switch — say so rather than silently orphan it. + if (account && !(await WorkspaceApi.sameAccount(account))) { + api.ui.toast({ + variant: "error", + message: `The signed-in account changed while "${res.datamate.name}" was being created, so it was not linked to this project. The workspace exists on the previous account.`, + duration: 15_000, + }) + return + } try { - await rebindByMatchedIdentifier({ + const rebound = await rebindByMatchedIdentifier({ identifier, targetDatamateId: res.datamate.id, expectedCurrentDatamateId: rebindFrom.expectedCurrentDatamateId, matchedBy: rebindFrom.matchedBy, }) + reboundBinding = rebound.binding } catch (err) { api.ui.toast({ variant: "error", @@ -545,6 +580,10 @@ async function createAndBindInline( } } + // Whichever call actually wrote the server row: the atomic create returns it, + // the unbound path gets it from the rebind. + const serverBinding = res.via === "bound" ? res.binding : reboundBinding + // Post-success tail — this function is invoked fire-and-forget // (``void createAndBindInline(...)``), so a bare rejection here would // surface as an unhandled promise and terminate the TUI. Contain the @@ -557,8 +596,8 @@ async function createAndBindInline( await recordApprovedBinding(api.state.path.directory, { datamateId: res.datamate.id, datamateName: res.datamate.name, - repoRemote: res.binding.repo_remote, - projectPath: res.binding.project_path, + repoRemote: serverBinding?.repo_remote ?? identifier.repoRemote ?? null, + projectPath: serverBinding?.project_path ?? identifier.projectPath ?? null, linkedAt: Date.now(), }) await showLinkedConfirmation(api, "Created", res.datamate.id, res.datamate.name) diff --git a/packages/opencode/test/altimate/workspace/create-then-rebind.test.ts b/packages/opencode/test/altimate/workspace/create-then-rebind.test.ts new file mode 100644 index 0000000000..b9f7b2dc7b --- /dev/null +++ b/packages/opencode/test/altimate/workspace/create-then-rebind.test.ts @@ -0,0 +1,218 @@ +// altimate_change - new file +// Control-flow coverage for the "create a quick workspace" row (AI-9171), on +// BOTH surfaces that offer it. +// +// The bug was never in a request shape — it was in which request got sent. +// `createAndBind` 409s before creating anything when the project is already +// linked, so the rebind that was meant to follow was unreachable and the row +// always failed. A test that only checks payloads cannot see that, which is why +// these assert the *sequence of endpoints* instead. +import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test" +import { mkdirSync, writeFileSync } from "node:fs" +import path from "node:path" +import os from "node:os" + +const ORIGINAL_TEST_HOME = process.env.OPENCODE_TEST_HOME +const ORIGINAL_XDG_STATE_HOME = process.env.XDG_STATE_HOME +const SANDBOX = path.join(os.tmpdir(), `altimate-createflow-${process.pid}-${Date.now()}`) +mkdirSync(path.join(SANDBOX, "home", ".altimate"), { recursive: true }) +mkdirSync(path.join(SANDBOX, "state"), { recursive: true }) +// Set before the modules under test are imported: they resolve `Global.Path` +// at import time, so this cannot move into `beforeEach`. Restored in +// `afterAll`, and the sandbox is per-pid so parallel files cannot collide. +process.env.OPENCODE_TEST_HOME = path.join(SANDBOX, "home") +process.env.XDG_STATE_HOME = path.join(SANDBOX, "state") + +const API_URL = "https://api.example.test" +writeFileSync( + path.join(SANDBOX, "home", ".altimate", "altimate.json"), + JSON.stringify({ altimateUrl: API_URL, altimateInstanceName: "acme", altimateApiKey: "test-key" }), +) + +const { createThenBindOrRebind } = await import("@/cli/cmd/link") +const { createAndBindInline } = await import("@/plugin/tui/altimate/workspace") + +const ORIGINAL_FETCH = globalThis.fetch + +interface Call { + method: string + path: string + body: Record +} +let calls: Call[] = [] +let routes: Array<{ match: RegExp; method?: string; status: number; body: unknown }> = [] + +function stubFetch() { + globalThis.fetch = (async (input: string | URL, init?: RequestInit) => { + const url = new URL(String(input)) + const method = String(init?.method ?? "GET") + calls.push({ + method, + path: url.pathname, + body: init?.body ? JSON.parse(String(init.body)) : {}, + }) + const route = routes.find( + (r) => r.match.test(url.pathname) && (r.method === undefined || r.method === method), + ) + const { status, body } = route ?? { status: 200, body: {} } + return new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }) + }) as typeof globalThis.fetch +} + +/** Endpoint sequence, ignoring the best-effort memory/skill traffic that + * `recordApprovedBinding` kicks off — this is about which create ran. */ +const sequence = () => + calls + .filter((c) => c.path.includes("/datamates") || c.path.includes("/datamate-project-bindings")) + .map((c) => `${c.method} ${c.path}`) + +const BINDING = { + id: 1, + datamate_id: 7, + datamate_name: "proj", + repo_remote: "https://github.com/acme/proj", + project_path: null, +} + +const IDENTIFIER = { repoRemote: "https://github.com/acme/proj", projectPath: "/tmp/proj" } +const EXISTING = { + datamate: { id: 3, name: "old-workspace" }, + matchedBy: "remote" as const, + binding: { ...BINDING, id: 9, datamate_id: 3, datamate_name: "old-workspace" }, +} + +beforeEach(() => { + calls = [] + routes = [] + stubFetch() +}) +afterEach(() => { + globalThis.fetch = ORIGINAL_FETCH + process.exitCode = undefined +}) +afterAll(() => { + if (ORIGINAL_TEST_HOME === undefined) delete process.env.OPENCODE_TEST_HOME + else process.env.OPENCODE_TEST_HOME = ORIGINAL_TEST_HOME + if (ORIGINAL_XDG_STATE_HOME === undefined) delete process.env.XDG_STATE_HOME + else process.env.XDG_STATE_HOME = ORIGINAL_XDG_STATE_HOME +}) + +describe("CLI: createThenBindOrRebind", () => { + test("unlinked project uses the atomic create-and-bind, and never rebinds", async () => { + routes = [ + { + match: /datamate-project-bindings\/$/, + method: "POST", + status: 200, + body: { datamate: { id: 7, name: "proj" }, binding: BINDING, manage_url: "https://x.test/w/7" }, + }, + ] + await createThenBindOrRebind(IDENTIFIER, "proj", "/tmp/proj", null) + + expect(sequence()).toEqual(["POST /datamate-project-bindings/"]) + expect(process.exitCode ?? 0).toBe(0) + }) + + test("already-linked project creates UNBOUND, then rebinds", async () => { + routes = [ + { match: /\/datamates\/$/, method: "POST", status: 200, body: { id: 7 } }, + { match: /by-remote/, method: "PUT", status: 200, body: { binding: BINDING } }, + ] + await createThenBindOrRebind(IDENTIFIER, "proj", "/tmp/proj", EXISTING) + + // The whole fix: /datamates/ (no identifiers) first, then the rebind. + // Before AI-9171 this was a single POST to the bindings router that 409'd. + expect(sequence()).toEqual(["POST /datamates/", "PUT /datamate-project-bindings/by-remote"]) + const create = calls.find((c) => c.path.endsWith("/datamates/"))! + expect(create.body).not.toHaveProperty("repo_remote") + expect(create.body).not.toHaveProperty("project_path") + expect(create.body.memory_enabled).toBe(true) + expect(create.body.knowledge_engine_enabled).toBe(true) + expect(process.exitCode ?? 0).toBe(0) + }) + + test("a failed rebind reports the orphan rather than claiming success", async () => { + routes = [ + { match: /\/datamates\/$/, method: "POST", status: 200, body: { id: 7 } }, + { match: /by-remote/, method: "PUT", status: 500, body: { detail: "boom" } }, + ] + await createThenBindOrRebind(IDENTIFIER, "proj", "/tmp/proj", EXISTING) + + // Created, not linked: the caller must exit non-zero so a script does not + // treat this as a successful link. + expect(sequence()).toEqual(["POST /datamates/", "PUT /datamate-project-bindings/by-remote"]) + expect(process.exitCode).toBe(1) + }) + + test("a 409 on the unbound create does not run the rebind", async () => { + routes = [{ match: /\/datamates\/$/, method: "POST", status: 409, body: { detail: "nope" } }] + await createThenBindOrRebind(IDENTIFIER, "proj", "/tmp/proj", EXISTING) + + expect(sequence()).toEqual(["POST /datamates/"]) + expect(process.exitCode).toBe(1) + }) +}) + +describe("TUI: createAndBindInline", () => { + const stubApi = () => { + const toasts: Array<{ variant?: string; message: string }> = [] + return { + toasts, + api: { + state: { path: { directory: "/tmp/proj" } }, + ui: { + toast: (t: { variant?: string; message: string }) => toasts.push(t), + dialog: { clear: () => {}, replace: () => {} }, + }, + } as never, + } + } + + test("already-linked project creates UNBOUND, then rebinds", async () => { + routes = [ + { match: /\/datamates\/$/, method: "POST", status: 200, body: { id: 7 } }, + { match: /by-remote/, method: "PUT", status: 200, body: { binding: BINDING } }, + ] + const { api } = stubApi() + await createAndBindInline(api, IDENTIFIER, "proj", { + expectedCurrentDatamateId: 3, + matchedBy: "remote", + }) + + // The Major issue on PR #1314: this surface kept calling the atomic + // create-and-bind, which 409s first, so the rebind below it never ran. + expect(sequence()).toEqual(["POST /datamates/", "PUT /datamate-project-bindings/by-remote"]) + }) + + test("unlinked project still uses the atomic create-and-bind", async () => { + routes = [ + { + match: /datamate-project-bindings\/$/, + method: "POST", + status: 200, + body: { datamate: { id: 7, name: "proj" }, binding: BINDING, manage_url: "https://x.test/w/7" }, + }, + ] + const { api } = stubApi() + await createAndBindInline(api, IDENTIFIER, "proj") + + expect(sequence()).toEqual(["POST /datamate-project-bindings/"]) + }) + + test("a failed rebind toasts the orphan instead of reporting success", async () => { + routes = [ + { match: /\/datamates\/$/, method: "POST", status: 200, body: { id: 7 } }, + { match: /by-remote/, method: "PUT", status: 500, body: { detail: "boom" } }, + ] + const { api, toasts } = stubApi() + await createAndBindInline(api, IDENTIFIER, "proj", { + expectedCurrentDatamateId: 3, + matchedBy: "remote", + }) + + expect(toasts.some((t) => t.variant === "error" && /CREATED but could not be linked/.test(t.message))).toBe(true) + }) +}) diff --git a/packages/opencode/test/altimate/workspace/create-workspace-unbound.test.ts b/packages/opencode/test/altimate/workspace/create-workspace-unbound.test.ts index 16e23e53d0..5bb5a8b366 100644 --- a/packages/opencode/test/altimate/workspace/create-workspace-unbound.test.ts +++ b/packages/opencode/test/altimate/workspace/create-workspace-unbound.test.ts @@ -18,6 +18,11 @@ import { mkdirSync, writeFileSync } from "node:fs" import path from "node:path" import os from "node:os" +// Set at module scope because the module under test resolves `Global.Path` at +// import time — moving this into `beforeEach` would be too late. The sandbox is +// keyed by pid and clock so parallel files cannot share it, the original value +// is restored in `afterAll`, and `globalThis.fetch` is restored after every +// test rather than left installed for whatever loads next. const ORIGINAL_TEST_HOME = process.env.OPENCODE_TEST_HOME const SANDBOX = path.join(os.tmpdir(), `altimate-createunbound-${process.pid}-${Date.now()}`) mkdirSync(path.join(SANDBOX, "home", ".altimate"), { recursive: true }) @@ -37,7 +42,7 @@ writeFileSync( }), ) -const { WorkspaceApi } = await import("@/altimate/workspace/api-client") +const { WorkspaceApi, WorkspaceApiError } = await import("@/altimate/workspace/api-client") const ORIGINAL_FETCH = globalThis.fetch @@ -140,4 +145,48 @@ describe("createWorkspaceUnbound", () => { respondWith(200, { id: "not-a-number" }) await expect(WorkspaceApi.createWorkspaceUnbound({ name: "x" })).rejects.toThrow(/no usable id/) }) + + // `Number()` coerces, so a guard written as `Number.isSafeInteger(Number(id))` + // accepts all three of these: `true` becomes 1, `"7"` becomes 7, `[5]` + // becomes 5. The first is the dangerous one — a malformed body would have + // rebound the project to workspace 1 rather than failing. The type check has + // to come before the arithmetic. + test.each([ + ["a boolean", true], + ["a numeric string", "7"], + ["a single-element array", [5]], + ["a float", 1.5], + ["zero", 0], + ["a negative", -1], + ])("rejects %s rather than coercing it", async (_label, value) => { + respondWith(200, { id: value }) + await expect(WorkspaceApi.createWorkspaceUnbound({ name: "x" })).rejects.toThrow(/no usable id/) + }) + + test("throws a typed WorkspaceApiError, not a bare Error", async () => { + // Every other failure in this module is typed; callers should be able to + // tell this apart programmatically. (review, PR #1314) + respondWith(200, { id: null }) + const err = await WorkspaceApi.createWorkspaceUnbound({ name: "x" }).catch((e: unknown) => e) + expect(err).toBeInstanceOf(WorkspaceApiError) + }) +}) + +describe("account fingerprint", () => { + test("reports the account the next call will act as", async () => { + const fp = await WorkspaceApi.accountFingerprint() + expect(fp).toEqual({ apiUrl: API_URL, tenant: TENANT }) + }) + + test("sameAccount is true for the account in effect", async () => { + expect(await WorkspaceApi.sameAccount({ apiUrl: API_URL, tenant: TENANT })).toBe(true) + }) + + test("sameAccount is false once the tenant or url differs", async () => { + // What the create-then-rebind pair guards against: a workspace id is local + // to the account that made it, so rebinding under another would point the + // project at whatever id collides there. + expect(await WorkspaceApi.sameAccount({ apiUrl: API_URL, tenant: "other" })).toBe(false) + expect(await WorkspaceApi.sameAccount({ apiUrl: "https://other.test", tenant: TENANT })).toBe(false) + }) })