-
Notifications
You must be signed in to change notification settings - Fork 134
fix: [AI-9171] create a quick workspace from an already-linked project #1314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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) | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: When Prompt for AI agents
Suggested change
|
||||||||||
| if (!Number.isSafeInteger(id) || id <= 0) { | ||||||||||
|
Comment on lines
+431
to
+432
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Reject non-number workspace IDs before applying the integer check. Prompt for AI agents
Suggested change
|
||||||||||
| 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, | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import { | |
| NotConfiguredError, | ||
| NotFoundError, | ||
| PreconditionFailedError, | ||
| type Binding, | ||
| type DatamateRef, | ||
| type MatchedIdentifier, | ||
| type ProjectBindingLookup, | ||
|
|
@@ -484,19 +485,36 @@ async function createThenBindOrRebind( | |
| ): Promise<void> { | ||
| const spin = prompts.spinner() | ||
| spin.start(`Creating workspace "${name}"...`) | ||
| let created: Awaited<ReturnType<typeof WorkspaceApi.createAndBind>> | ||
| 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 }) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Pin the account across the two-step operation
Reply with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Pin the credential or account scope across the unbound creation and rebind calls. If credentials change between these requests, the workspace can be created in one tenant and rebound under another tenant's workspace ID. Prompt for AI agents |
||
| 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, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Cache the authoritative binding returned by rebind The rebind response is discarded, so this fallback stores both fields from the current checkout even when only one identifies the server row. For example, a path-keyed binding whose remote changed is rebound through Reply with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: After an existing project is rebound, this fallback records identifiers that the rebind request did not send instead of the server's returned binding row. A moved checkout or a path-keyed binding can therefore leave the cache with the wrong remote/path, causing later offline resolution and cleanup to use stale binding identity. Persist the Prompt for AI agents |
||
| 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.") | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Isolate process-global test state This assignment persists for the entire loaded suite, while this file also replaces Reply with
Comment on lines
+21
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '1,180p' packages/opencode/test/altimate/workspace/create-workspace-unbound.test.ts
rg -n --glob '*.{ts,tsx}' 'tmpdir\(|OPENCODE_TEST_HOME|globalThis\.fetch' packages/opencode/test/altimate/workspace | head -160
rg -n 'bun test|concurrent|parallel' packages/opencode/package.json package.json bunfig.toml 2>/dev/nullRepository: AltimateAI/altimate-code Length of output: 20761 🤖 get_repo_knowledge executed:
Length of output: 30848 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- test package/config references ---'
sed -n '1,100p' packages/opencode/package.json
for f in bunfig.toml packages/opencode/bunfig.toml; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
cat -n "$f"
fi
done
printf '%s\n' '--- API client imports and environment-sensitive definitions ---'
rg -n -C 4 'OPENCODE_TEST_HOME|altimateUrl|read.*config|credentials|process\.env|WorkspaceApi|export' packages/opencode/src/altimate/workspace/api-client.ts packages/opencode/src/altimate -g '*.ts' | head -240
printf '%s\n' '--- tmpdir definitions/usages ---'
rg -n -C 5 'function tmpdir|const tmpdir|export .*tmpdir|tmpdir\(' packages/opencode/test packages/opencode/src -g '*.ts' -g '*.tsx' | head -240
printf '%s\n' '--- file line count ---'
wc -l packages/opencode/test/altimate/workspace/create-workspace-unbound.test.tsRepository: AltimateAI/altimate-code Length of output: 46915 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- fixture helper ---'
fd -i 'fixture.ts' packages/opencode/test packages/opencode/src
for f in $(fd -i 'fixture.ts' packages/opencode/test packages/opencode/src); do
if rg -q 'tmpdir' "$f"; then
printf '%s\n' "--- $f ---"
rg -n -C 8 'tmpdir|class Tmp|interface Tmp' "$f" | head -180
fi
done
printf '%s\n' '--- AltimateApi binding ---'
rg -n -C 8 'export (const|class|namespace) AltimateApi|namespace AltimateApi|function isConfigured|function getCredentials|OPENCODE_TEST_HOME' packages/opencode/src/altimate packages/opencode/src -g '*.ts' | head -260Repository: AltimateAI/altimate-code Length of output: 13570 Make the test fixture per-test and parallel-safe.
Use 🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Do not mutate Prompt for AI agents |
||
|
|
||
| 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<string, unknown> | ||
| } | ||
|
|
||
| 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(() => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The test writes a temp credentials file into Prompt for AI agents |
||
| 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/) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject non-number workspace IDs.
Number(data?.id)converts malformed JSON values. For example,{ "id": true }becomes1and passes validation. The link flow can then rebind the project to workspace1instead of rejecting the invalid response.Require
data.idto be a number before the integer check.Proposed fix
🤖 Prompt for AI Agents