From 03a08ec8d535b8b6c1f1a9aea4197ae5f2210d10 Mon Sep 17 00:00:00 2001 From: Jonathan Deiss Date: Mon, 27 Jul 2026 15:02:09 -0500 Subject: [PATCH 1/7] Serve repo branches and create a workspace from an issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backend half of issue #12. The web client's create-workspace form needs real data behind its branch field and a way to start a workspace from an issue, so the bridge gains: - `GET /repos/:name/branches`, answered with `ls-remote` rather than the repo's provider: repos default to `provider: "custom"`, for which `providerFor` throws 501, so a provider-backed listing would be dead for most registered repos. - `RepoProvider.linkBranchToIssue`, which creates a branch and records it as the issue's linked development branch. GitHub exposes that linkage through GraphQL alone, so the provider grows a small `graphql` helper that also treats a 200 carrying `errors` as a failure. - `POST /workspaces` accepting `issueNumber` in place of `branch`, with the branch name derived by the new shared `issueBranchName` — shared so the client can preview the name the bridge will authoritatively pick. Pre-review: the review panel runs against this commit. --- packages/fleet-bridge/src/api/repos.ts | 9 + packages/fleet-bridge/src/api/workspaces.ts | 5 +- packages/fleet-bridge/src/fleet-manager.ts | 123 +++++++++- packages/fleet-bridge/src/providers/github.ts | 108 +++++++++ packages/fleet-bridge/src/providers/index.ts | 1 + .../fleet-bridge/src/providers/provider.ts | 16 ++ packages/fleet-bridge/src/types.ts | 6 + packages/fleet-bridge/tests/providers.test.ts | 150 +++++++++++++ .../fleet-bridge/tests/repo-branches.test.ts | 125 +++++++++++ .../tests/repo-provider-api.test.ts | 6 + .../tests/workspace-from-issue.test.ts | 212 ++++++++++++++++++ packages/fleet-protocol/index.ts | 1 + packages/fleet-protocol/src/issue-branch.ts | 43 ++++ .../fleet-protocol/tests/issue-branch.test.ts | 54 +++++ 14 files changed, 853 insertions(+), 6 deletions(-) create mode 100644 packages/fleet-bridge/tests/repo-branches.test.ts create mode 100644 packages/fleet-bridge/tests/workspace-from-issue.test.ts create mode 100644 packages/fleet-protocol/src/issue-branch.ts create mode 100644 packages/fleet-protocol/tests/issue-branch.test.ts diff --git a/packages/fleet-bridge/src/api/repos.ts b/packages/fleet-bridge/src/api/repos.ts index f5055a8..f751e30 100644 --- a/packages/fleet-bridge/src/api/repos.ts +++ b/packages/fleet-bridge/src/api/repos.ts @@ -58,6 +58,15 @@ export function reposPlugin(manager: FleetManager) { return mapped.body; } }) + .get("/repos/:name/branches", async ({ params, set }) => { + try { + return await manager.listRepoBranches(params.name); + } catch (err) { + const mapped = mapError(err); + set.status = mapped.status; + return mapped.body; + } + }) .get( "/repos/:name/issues", async ({ params, query, set }) => { diff --git a/packages/fleet-bridge/src/api/workspaces.ts b/packages/fleet-bridge/src/api/workspaces.ts index 396c2de..d835832 100644 --- a/packages/fleet-bridge/src/api/workspaces.ts +++ b/packages/fleet-bridge/src/api/workspaces.ts @@ -112,7 +112,10 @@ export function workspacesPlugin(manager: FleetManager) { ship: t.String(), repoName: t.String(), name: t.String(), - branch: t.String(), + // Either/or, enforced by the manager rather than the schema so the + // client gets a 400 with a reason instead of a shapeless 422. + branch: t.Optional(t.String()), + issueNumber: t.Optional(t.Numeric()), }), }, ) diff --git a/packages/fleet-bridge/src/fleet-manager.ts b/packages/fleet-bridge/src/fleet-manager.ts index cd3aba8..4ae69da 100644 --- a/packages/fleet-bridge/src/fleet-manager.ts +++ b/packages/fleet-bridge/src/fleet-manager.ts @@ -17,6 +17,7 @@ import { ARMORY_DIRECTORY, CreateRepoInputSchema, FleetIdentifierSchema, + issueBranchName, ShipSchema, WorkspaceRefsSchema, WorkspaceSummarySchema, @@ -32,7 +33,7 @@ import { type WorkspaceStatus, type WorkspaceSummary, } from "fleet-protocol"; -import type { DiffOptions } from "git-bun"; +import { Git, type DiffOptions, type RemoteRef } from "git-bun"; import { TERMINAL_TAKEOVER_QUERY } from "webterm/protocol"; import { ShipConnection, toWsUrl, type ShipConnectionDeps } from "./ship-connection"; import { defaultPublicUrl, type BridgeConfig } from "./config"; @@ -41,6 +42,7 @@ import { type BridgeWorkspaceEvent, type BridgeWorkspaceStatus, type BridgeWorkspaceSummary, + type RepoBranch, type ShipArmoryState, type ShipInfo, type ShipSystemResources, @@ -84,14 +86,22 @@ export class BridgeError extends Error { /** How long to wait for a ship's first `sync` before treating it as offline. */ const SYNC_TIMEOUT_MS = 5000; -/** Body of `POST /workspaces` on the bridge (ship-targeted, names a registered repo). */ +/** + * Body of `POST /workspaces` on the bridge (ship-targeted, names a registered + * repo). The branch comes either verbatim from `branch` or from the issue + * `issueNumber` names — exactly one of the two, never both. + */ export interface CreateWorkspaceInput { readonly ship: string; readonly repoName: string; readonly name: string; - readonly branch: string; + readonly branch?: string; + readonly issueNumber?: number; } +/** Which of the two mutually exclusive branch sources a create request chose. */ +type BranchSource = { readonly branch: string } | { readonly issueNumber: number }; + type EdenResult = { data: T | null; error: unknown }; interface CreateReservation { @@ -114,6 +124,8 @@ export class FleetManager { private readonly store: Store; /** Builds a `RepoProvider` for a registered repo; overridable in tests. */ private readonly makeProvider: (repo: Repo) => RepoProvider; + /** Probes a remote's refs; overridable so tests never shell out to git. */ + private readonly lsRemote: typeof Git.lsRemote; /** The bridge-owned file factory served from `/armory`. */ private readonly armory: ArmoryService; @@ -124,6 +136,7 @@ export class FleetManager { syncTimeoutMs?: number; store?: Store; providerFor?: (repo: Repo) => RepoProvider; + lsRemote?: typeof Git.lsRemote; armory?: ArmoryService; }, ) { @@ -131,6 +144,7 @@ export class FleetManager { this.syncTimeoutMs = opts?.syncTimeoutMs ?? SYNC_TIMEOUT_MS; this.store = opts?.store ?? new Store(config.dataDirectory); this.makeProvider = opts?.providerFor ?? providerFor; + this.lsRemote = opts?.lsRemote ?? Git.lsRemote; this.armory = opts?.armory ?? new ArmoryService(join(config.dataDirectory, ARMORY_DIRECTORY)); } @@ -347,6 +361,41 @@ export class FleetManager { if (!deleted) throw new BridgeError(`repo not found: ${name}`, 404); } + /** + * `GET /repos/:name/branches` — the branches the repo's remote advertises. + * + * Answered with `ls-remote` rather than through the repo's provider on purpose: + * `addRepo` defaults a repo to `provider: "custom"`, for which `providerFor` + * throws 501, so a provider-backed listing would be dead for most registered + * repos. `ls-remote` speaks to any git URL and needs no token. + */ + async listRepoBranches(name: string): Promise { + this.identifier(name, "repo"); + const repo = await this.store.getRepo(name); + if (!repo) throw new BridgeError(`repo not found: ${name}`, 404); + + let refs: RemoteRef[]; + try { + refs = await this.lsRemote(repo.url, { cwd: this.config.dataDirectory, heads: true }); + } catch (error) { + // Any failure here is the remote's or the network's, not the caller's — and + // a raw GitError must not reach the route, which would report it as a 500. + throw new BridgeError( + `could not list branches for repo "${name}": ${(error as Error).message}`, + 502, + ); + } + + const prefix = "refs/heads/"; + return refs + .filter((ref) => ref.ref.startsWith(prefix)) + .map((ref) => ({ name: ref.ref.slice(prefix.length), sha: ref.sha })) + // Plain codepoint order, not `localeCompare`: the listing must not reorder + // itself with the bridge host's locale. `--heads` hides HEAD, so which + // branch is the default one is not knowable here. + .sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0)); + } + /** * Look up a registered repo and run `fn` against its provider. `ProviderError` * from `fn` propagates unchanged so the API can surface its HTTP status. @@ -598,10 +647,15 @@ export class FleetManager { return parsed.data; } - /** `POST /workspaces {ship,repoName,name,branch}` — clones a registered repo. */ + /** + * `POST /workspaces {ship,repoName,name,branch|issueNumber}` — clones a + * registered repo onto a branch, either named outright or derived from an + * issue (which also creates and links that branch on the provider). + */ async createWorkspace(input: CreateWorkspaceInput): Promise { this.identifier(input.repoName, "repo"); this.identifier(input.name, "workspace"); + const source = this.branchSource(input); const conn = this.connections.get(input.ship); if (!conn) throw new BridgeError(`unknown ship: ${input.ship}`, 400); @@ -630,12 +684,22 @@ export class FleetManager { let retainReservation = false; try { + // Resolving the issue happens under the reservation, so two concurrent + // creates of the same workspace cannot both mint a linked branch. The + // converse — the branch gets created and linked and then the ship call + // fails — is accepted: a retry reuses that branch (the provider returns + // the existing linked branch, or a deduped name), the reservation is still + // cleared by the `finally` below, and the error the user sees is the + // ship's, because this call has already returned by then. + const branch = + "branch" in source ? source.branch : await this.branchForIssue(input.repoName, source.issueNumber); + const response = await this.call(conn, () => conn.client.workspaces.post({ url: repo.url, repoName: input.repoName, name: input.name, - branch: input.branch, + branch, }) as Promise>, { ambiguousEmptyResponse: true }, ); @@ -687,6 +751,55 @@ export class FleetManager { } } + /** + * Validate a create request's mutually exclusive branch source. The ship also + * rejects a blank branch, but doing it here saves the round trip and keeps the + * "one of the two" rule in a single place. + */ + private branchSource(input: CreateWorkspaceInput): BranchSource { + if (input.branch !== undefined && input.issueNumber !== undefined) { + throw new BridgeError("a workspace is created from a branch or an issue, not both", 400); + } + if (input.branch !== undefined) { + const branch = input.branch.trim(); + if (branch.length === 0) throw new BridgeError("branch must not be empty", 400); + return { branch }; + } + if (input.issueNumber !== undefined) { + // Guarded here so `issueBranchName` — which rejects the same values — can + // never be reached with a number a client made up. + if (!Number.isInteger(input.issueNumber) || input.issueNumber < 1) { + throw new BridgeError("issueNumber must be a positive integer", 400); + } + return { issueNumber: input.issueNumber }; + } + throw new BridgeError("a workspace needs either a branch or an issue to start from", 400); + } + + /** + * Turn an issue into the branch a new workspace sits on: read the issue, then + * have the provider create and link `-`. The name the provider + * returns wins over the computed one — it may have de-duplicated it. + */ + private async branchForIssue(repoName: string, issueNumber: number): Promise { + return this.withProvider(repoName, async (provider) => { + const issue = await provider.getIssue(issueNumber); + let computed: string; + try { + computed = issueBranchName(issue); + } catch (error) { + // The issue identity came from the provider, so a name that cannot be + // derived from it is an upstream fault, not a bad request. + throw new BridgeError( + `could not derive a branch name for issue ${issueNumber}: ${(error as Error).message}`, + 502, + ); + } + const linked = await provider.linkBranchToIssue(issueNumber, computed); + return linked.name; + }); + } + /** `POST /workspaces/:repo/:name/branch`. */ async switchBranch(repo: string, name: string, branch: string): Promise { const conn = this.routeFor(repo, name); diff --git a/packages/fleet-bridge/src/providers/github.ts b/packages/fleet-bridge/src/providers/github.ts index f4ed2ae..548cbb7 100644 --- a/packages/fleet-bridge/src/providers/github.ts +++ b/packages/fleet-bridge/src/providers/github.ts @@ -15,6 +15,7 @@ import type { Issue, IssueComment, IssueSummary, + LinkedBranch, ListOptions, PullRequest, PullRequestSummary, @@ -35,6 +36,22 @@ export interface GitHubProviderConfig { const DEFAULT_BASE_URL = "https://api.github.com"; +/** + * The only way to attach a branch to an issue: GitHub exposes the linkage + * through GraphQL alone, with no REST equivalent. The mutation creates the ref + * as well, so nothing has to push a branch beforehand. + */ +const CREATE_LINKED_BRANCH = `mutation CreateLinkedBranch($issueId: ID!, $oid: GitObjectID!, $name: String!) { + createLinkedBranch(input: { issueId: $issueId, oid: $oid, name: $name }) { + linkedBranch { + ref { + name + target { oid } + } + } + } +}`; + /** * Extract `{ owner, repo }` from a git clone URL. Handles the https web form * (with or without a `.git` suffix or trailing slash) and the `git@host:o/r.git` @@ -74,6 +91,8 @@ interface GitHubRepo { interface GitHubIssue { number: number; + /** GraphQL global id — the handle `createLinkedBranch` addresses the issue by. */ + node_id?: string; title: string; state: string; user: GitHubUser | null; @@ -140,6 +159,24 @@ interface GitHubJob { conclusion: string | null; } +interface GitHubRef { + object?: { sha?: string }; +} + +/** Envelope of a GraphQL reply; `errors` can be present alongside HTTP 200. */ +interface GraphQLResponse { + data?: T | null; + errors?: { message?: unknown }[]; +} + +interface CreateLinkedBranchResult { + createLinkedBranch?: { + linkedBranch?: { + ref?: { name?: string; target?: { oid?: string } | null } | null; + } | null; + } | null; +} + export class GitHubProvider implements RepoProvider { private readonly owner: string; private readonly repo: string; @@ -298,6 +335,51 @@ export class GitHubProvider implements RepoProvider { return logs; } + /** + * Create `branch` off the default branch's head and record it as issue + * `issueNumber`'s linked development branch, in one mutation. Needs a token + * with repo write scope: it writes a ref and an issue timeline entry. + * + * Three reads precede the write because the mutation is addressed by GraphQL + * node id and a commit oid, neither of which the caller has: the issue's + * `node_id`, the repo's default branch, and that branch's head SHA. + */ + async linkBranchToIssue(issueNumber: number, branch: string): Promise { + this.requireToken(); + + const issue = await this.request( + `/repos/${this.owner}/${this.repo}/issues/${issueNumber}`, + ); + if (!issue.node_id) { + throw new ProviderError(`GitHub issue ${issueNumber} carried no node id`, 502); + } + + const { defaultBranch } = await this.getInfo(); + // Not URL-encoded: a default branch may legitimately contain "/", which is a + // path separator in the ref endpoint rather than data. + const ref = await this.request( + `/repos/${this.owner}/${this.repo}/git/ref/heads/${defaultBranch}`, + ); + if (!ref.object?.sha) { + throw new ProviderError(`GitHub returned no head commit for branch ${defaultBranch}`, 502); + } + + const result = await this.graphql(CREATE_LINKED_BRANCH, { + issueId: issue.node_id, + oid: ref.object.sha, + name: branch, + }); + + const created = result.createLinkedBranch?.linkedBranch?.ref; + if (!created?.name || !created.target?.oid) { + throw new ProviderError( + `GitHub createLinkedBranch returned no branch for issue ${issueNumber}`, + 502, + ); + } + return { name: created.name, sha: created.target.oid }; + } + private async postComment(number: number, body: string): Promise { this.requireToken(); const comment = await this.request( @@ -374,6 +456,32 @@ export class GitHubProvider implements RepoProvider { return (await response.json()) as T; } + /** + * Run a GraphQL operation through `request`, so it shares the REST path's + * headers, auth and failure mapping. GraphQL reports *operation* failures with + * HTTP 200 and a populated `errors` array, so a transport success still has to + * be inspected before the payload can be trusted. + */ + private async graphql(query: string, variables: Record): Promise { + const payload = await this.request>("/graphql", { + method: "POST", + body: { query, variables }, + }); + + const first = payload.errors?.[0]; + if (first !== undefined) { + const message = typeof first.message === "string" ? first.message : "unknown GraphQL error"; + // GraphQL has no status of its own, so a name collision — the one failure a + // caller can act on — has to be recognized from the message text. + const status = /already exists/i.test(message) ? 422 : 502; + throw new ProviderError(`GitHub GraphQL request failed: ${message}`, status); + } + if (payload.data === undefined || payload.data === null) { + throw new ProviderError("GitHub GraphQL response carried no data", 502); + } + return payload.data; + } + /** * Fetch a non-JSON body (Actions job logs). GitHub answers the logs endpoint * with a 302 to a pre-signed storage URL that rejects the `Authorization` diff --git a/packages/fleet-bridge/src/providers/index.ts b/packages/fleet-bridge/src/providers/index.ts index ec37291..a62a8d8 100644 --- a/packages/fleet-bridge/src/providers/index.ts +++ b/packages/fleet-bridge/src/providers/index.ts @@ -43,6 +43,7 @@ export type { Issue, IssueComment, IssueSummary, + LinkedBranch, ListOptions, PullRequest, PullRequestSummary, diff --git a/packages/fleet-bridge/src/providers/provider.ts b/packages/fleet-bridge/src/providers/provider.ts index 15d76eb..001f94f 100644 --- a/packages/fleet-bridge/src/providers/provider.ts +++ b/packages/fleet-bridge/src/providers/provider.ts @@ -107,6 +107,12 @@ export interface ListOptions { export type ReviewEvent = "APPROVE" | "REQUEST_CHANGES" | "COMMENT"; +/** A branch the provider created and attached to an issue. */ +export interface LinkedBranch { + readonly name: string; + readonly sha: string; +} + export interface RepoProvider { getInfo(): Promise; listIssues(options?: ListOptions): Promise; @@ -120,4 +126,14 @@ export interface RepoProvider { listChecks(ref: string): Promise; /** Raw logs of the failed GitHub Actions jobs for a commit-ish. */ getFailedLogs(ref: string): Promise; + /** + * Create `branch` on the remote *and* record it as the issue's linked + * development branch (GitHub's "Development → create a branch for this issue" + * relationship). A write: it needs a token with repo write scope. + * + * Returns the ref the provider actually created — it may differ from the + * requested name, because a forge is free to de-duplicate against branches + * that already exist. + */ + linkBranchToIssue(issueNumber: number, branch: string): Promise; } diff --git a/packages/fleet-bridge/src/types.ts b/packages/fleet-bridge/src/types.ts index 94e4f8c..0a8f36f 100644 --- a/packages/fleet-bridge/src/types.ts +++ b/packages/fleet-bridge/src/types.ts @@ -44,6 +44,12 @@ export type BridgeWorkspaceEvent = readonly workspace: BridgeWorkspaceSummary; }; +/** A branch a registered repo's remote advertises — a row of `GET /repos/:name/branches`. */ +export interface RepoBranch { + readonly name: string; + readonly sha: string; +} + /** * One ship's entry in the aggregate `GET /system-resources`. `resources` is * present when the ship is online and responded; otherwise `error` explains why diff --git a/packages/fleet-bridge/tests/providers.test.ts b/packages/fleet-bridge/tests/providers.test.ts index fe52f55..c8a5014 100644 --- a/packages/fleet-bridge/tests/providers.test.ts +++ b/packages/fleet-bridge/tests/providers.test.ts @@ -447,6 +447,156 @@ describe("GitHubProvider", () => { } }); + /** + * Drive the three REST reads `linkBranchToIssue` makes, then hand the GraphQL + * mutation whatever `graphqlResponse` returns. + */ + function linkBranchFetch(graphqlResponse: () => Response): { fetch: typeof fetch; calls: FetchCall[] } { + const calls: FetchCall[] = []; + const fn = (async (input: string | URL | Request, init?: RequestInit) => { + const url = String(input); + calls.push({ + url, + method: init?.method ?? "GET", + headers: new Headers(init?.headers), + body: typeof init?.body === "string" ? init.body : undefined, + }); + if (url.endsWith("/issues/12")) { + return Response.json({ + number: 12, + node_id: "I_issue12", + title: "a bug", + state: "open", + user: { login: "alice" }, + html_url: "https://github.com/owner/repo/issues/12", + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + body: null, + comments: 0, + }); + } + if (url.endsWith("/repos/owner/repo")) return Response.json(repoPayload); + if (url.endsWith("/git/ref/heads/main")) return Response.json({ object: { sha: "basesha" } }); + if (url.endsWith("/graphql")) return graphqlResponse(); + throw new Error(`unexpected fetch: ${url}`); + }) as unknown as typeof globalThis.fetch; + return { fetch: fn, calls }; + } + + const linkedBranchPayload = () => + Response.json({ + data: { + createLinkedBranch: { + linkedBranch: { ref: { name: "12-a-bug", target: { oid: "newsha" } } }, + }, + }, + }); + + test("linkBranchToIssue posts the mutation with the issue node id, base oid and name", async () => { + const { fetch, calls } = linkBranchFetch(linkedBranchPayload); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + const linked = await provider.linkBranchToIssue(12, "12-a-bug"); + + expect(linked).toEqual({ name: "12-a-bug", sha: "newsha" }); + + const mutation = calls.find((c) => c.url.endsWith("/graphql"))!; + expect(mutation.url).toBe("https://api.github.com/graphql"); + expect(mutation.method).toBe("POST"); + expect(mutation.headers.get("Authorization")).toBe("Bearer t0ken"); + const body = JSON.parse(mutation.body!) as { query: string; variables: Record }; + expect(body.query).toContain("createLinkedBranch"); + expect(body.variables).toEqual({ issueId: "I_issue12", oid: "basesha", name: "12-a-bug" }); + }); + + test("linkBranchToIssue returns the name GitHub actually created, not the requested one", async () => { + const { fetch } = linkBranchFetch(() => + Response.json({ + data: { + createLinkedBranch: { + linkedBranch: { ref: { name: "12-a-bug-1", target: { oid: "newsha" } } }, + }, + }, + }), + ); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ name: "12-a-bug-1", sha: "newsha" }); + }); + + test("linkBranchToIssue throws ProviderError(401) without a token", async () => { + const { fetch, calls } = fakeFetch(Response.json({})); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", fetch }); + + try { + await provider.linkBranchToIssue(12, "12-a-bug"); + throw new Error("expected linkBranchToIssue to throw"); + } catch (error) { + expect(error).toBeInstanceOf(ProviderError); + expect((error as ProviderError).status).toBe(401); + } + expect(calls).toHaveLength(0); + }); + + test("a GraphQL error carried on an HTTP 200 becomes a ProviderError(502)", async () => { + const { fetch } = linkBranchFetch(() => + Response.json({ data: null, errors: [{ message: "Resource not accessible by integration" }] }), + ); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + try { + await provider.linkBranchToIssue(12, "12-a-bug"); + throw new Error("expected linkBranchToIssue to throw"); + } catch (error) { + expect(error).toBeInstanceOf(ProviderError); + expect((error as ProviderError).status).toBe(502); + expect((error as ProviderError).message).toContain("Resource not accessible by integration"); + } + }); + + test("a GraphQL 'already exists' error becomes a ProviderError(422)", async () => { + const { fetch } = linkBranchFetch(() => + Response.json({ errors: [{ message: "A branch with that name already exists" }] }), + ); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + try { + await provider.linkBranchToIssue(12, "12-a-bug"); + throw new Error("expected linkBranchToIssue to throw"); + } catch (error) { + expect((error as ProviderError).status).toBe(422); + } + }); + + test("a GraphQL payload missing the created ref becomes a ProviderError(502)", async () => { + const { fetch } = linkBranchFetch(() => Response.json({ data: { createLinkedBranch: null } })); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + try { + await provider.linkBranchToIssue(12, "12-a-bug"); + throw new Error("expected linkBranchToIssue to throw"); + } catch (error) { + expect(error).toBeInstanceOf(ProviderError); + expect((error as ProviderError).status).toBe(502); + expect((error as ProviderError).message).toContain("no branch"); + } + }); + + test("linkBranchToIssue reports an issue with no node id as a 502", async () => { + const fetch = (async (input: string | URL | Request) => { + if (String(input).endsWith("/issues/12")) return Response.json({ number: 12, title: "a bug" }); + throw new Error(`unexpected fetch: ${String(input)}`); + }) as unknown as typeof globalThis.fetch; + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + try { + await provider.linkBranchToIssue(12, "12-a-bug"); + throw new Error("expected linkBranchToIssue to throw"); + } catch (error) { + expect((error as ProviderError).status).toBe(502); + } + }); + test("getFailedLogs returns [] when nothing failed and there are no Actions runs", async () => { const fetch = (async (input: string | URL | Request) => { const url = String(input); diff --git a/packages/fleet-bridge/tests/repo-branches.test.ts b/packages/fleet-bridge/tests/repo-branches.test.ts new file mode 100644 index 0000000..568f5ae --- /dev/null +++ b/packages/fleet-bridge/tests/repo-branches.test.ts @@ -0,0 +1,125 @@ +/** + * repo-branches.test.ts — `GET /repos/:name/branches` driven in-process against + * a fake `Git.lsRemote`, so the route/manager mapping is exercised without a git + * binary or a reachable remote. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { GitError, type RemoteRef } from "git-bun"; +import { FleetManager } from "../src/fleet-manager"; +import { createApp } from "../src/api"; +import { Store } from "../src/store/store"; +import { makeDeps } from "./helpers"; + +/** What the fake `lsRemote` was asked, and what it answers with. */ +interface LsRemoteStub { + calls: { url: string; cwd: string; heads?: boolean }[]; + answer: () => RemoteRef[]; +} + +describe("GET /repos/:name/branches", () => { + let dir: string; + let manager: FleetManager; + let app: ReturnType; + let lsRemote: LsRemoteStub; + + async function call(method: string, path: string, body?: unknown) { + const res = await app.handle( + new Request(`http://bridge${path}`, { + method, + headers: body === undefined ? undefined : { "content-type": "application/json" }, + body: body === undefined ? undefined : JSON.stringify(body), + }), + ); + const text = await res.text(); + return { status: res.status, body: text ? JSON.parse(text) : undefined }; + } + + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), "fleet-bridge-branches-")); + lsRemote = { + calls: [], + answer: () => [ + { sha: "sha-main", ref: "refs/heads/main" }, + { sha: "sha-feature", ref: "refs/heads/feature/login" }, + { sha: "sha-alpha", ref: "refs/heads/alpha" }, + ], + }; + const config = { dataDirectory: dir, port: 4901, name: "bridge" }; + const store = new Store(dir); + await store.load(); + manager = new FleetManager(config, makeDeps(new Map()), { + syncTimeoutMs: 50, + store, + lsRemote: async (url, options) => { + lsRemote.calls.push({ url, cwd: options.cwd, heads: options.heads }); + return lsRemote.answer(); + }, + }); + await manager.init(); + app = createApp(manager, config); + expect((await call("POST", "/repos", { name: "repo1", url: "git@fake/repo1.git" })).status).toBe(201); + }); + afterEach(async () => { + manager.shutdown(); + await rm(dir, { recursive: true, force: true }); + }); + + test("maps refs/heads/* to {name, sha}, sorted ascending", async () => { + const res = await call("GET", "/repos/repo1/branches"); + + expect(res.status).toBe(200); + expect(res.body).toEqual([ + { name: "alpha", sha: "sha-alpha" }, + { name: "feature/login", sha: "sha-feature" }, + { name: "main", sha: "sha-main" }, + ]); + }); + + test("probes the repo's url from the bridge's data directory, branches only", async () => { + await call("GET", "/repos/repo1/branches"); + + expect(lsRemote.calls).toEqual([{ url: "git@fake/repo1.git", cwd: dir, heads: true }]); + }); + + test("drops refs that are not branches", async () => { + lsRemote.answer = () => [ + { sha: "sha-main", ref: "refs/heads/main" }, + { sha: "sha-tag", ref: "refs/tags/v1.0.0" }, + { sha: "sha-pull", ref: "refs/pull/7/head" }, + { sha: "sha-head", ref: "HEAD" }, + ]; + + const res = await call("GET", "/repos/repo1/branches"); + + expect(res.body).toEqual([{ name: "main", sha: "sha-main" }]); + }); + + test("an unregistered repo returns 404", async () => { + expect((await call("GET", "/repos/ghost/branches")).status).toBe(404); + expect(lsRemote.calls).toHaveLength(0); + }); + + test("an invalid repo identifier returns 400", async () => { + expect((await call("GET", "/repos/..%2Fescape/branches")).status).toBe(400); + }); + + test("an unreachable remote surfaces as 502 naming the repo", async () => { + lsRemote.answer = () => { + throw new GitError(["ls-remote", "--heads", "--", "git@fake/repo1.git"], { + stdout: "", + stderr: "fatal: repository not found", + exitCode: 128, + }); + }; + + const res = await call("GET", "/repos/repo1/branches"); + + expect(res.status).toBe(502); + expect(res.body.error).toContain("repo1"); + expect(res.body.error).toContain("repository not found"); + }); +}); diff --git a/packages/fleet-bridge/tests/repo-provider-api.test.ts b/packages/fleet-bridge/tests/repo-provider-api.test.ts index 57b5110..9d763e9 100644 --- a/packages/fleet-bridge/tests/repo-provider-api.test.ts +++ b/packages/fleet-bridge/tests/repo-provider-api.test.ts @@ -38,6 +38,7 @@ interface Recorder { reviewPr?: { number: number; review: { event: ReviewEvent; body?: string } }; checksRef?: string; failedLogsRef?: string; + linkBranch?: { issueNumber: number; branch: string }; } const info: RepoInfo = { @@ -172,6 +173,11 @@ describe("repo provider API", () => { recorder.failedLogsRef = ref; return [failedLog]; }, + async linkBranchToIssue(issueNumber: number, branch: string) { + guard(); + recorder.linkBranch = { issueNumber, branch }; + return { name: branch, sha: "sha-of-linked-branch" }; + }, }; } diff --git a/packages/fleet-bridge/tests/workspace-from-issue.test.ts b/packages/fleet-bridge/tests/workspace-from-issue.test.ts new file mode 100644 index 0000000..cf21336 --- /dev/null +++ b/packages/fleet-bridge/tests/workspace-from-issue.test.ts @@ -0,0 +1,212 @@ +/** + * workspace-from-issue.test.ts — `POST /workspaces` driven in-process with both + * a fake ship and a fake provider, covering the `issueNumber` branch source: + * which provider calls it makes, what branch the ship ends up being handed, and + * the validation and reservation behaviour around a provider failure. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { Repo } from "fleet-protocol"; +import { FleetManager } from "../src/fleet-manager"; +import { createApp } from "../src/api"; +import { Store } from "../src/store/store"; +import { ProviderError, type Issue, type RepoProvider } from "../src/providers"; +import { FakeSocket, makeDeps, type FakeShip } from "./helpers"; + +/** Records what the fake provider was asked to do. */ +interface Recorder { + getIssueNumber?: number; + linkBranch?: { issueNumber: number; branch: string }; +} + +const issue: Issue = { + number: 12, + title: "Better create workspace issue", + state: "open", + author: "octocat", + url: "https://github.com/acme/repo1/issues/12", + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-02T00:00:00.000Z", + body: "details", + comments: 1, +}; + +describe("POST /workspaces from an issue", () => { + let dir: string; + let manager: FleetManager; + let app: ReturnType; + let ships: Map; + let recorder: Recorder; + /** When set, `linkBranchToIssue` throws this instead of succeeding. */ + let linkFailsWith: ProviderError | undefined; + /** Name the provider claims to have created; defaults to the requested one. */ + let linkReturns: string | undefined; + + function makeProvider(_repo: Repo): RepoProvider { + const unused = () => { + throw new Error("not used by these tests"); + }; + return { + getInfo: unused, + listIssues: unused, + listPullRequests: unused, + getPullRequest: unused, + commentOnIssue: unused, + commentOnPullRequest: unused, + reviewPullRequest: unused, + listChecks: unused, + getFailedLogs: unused, + async getIssue(number: number) { + recorder.getIssueNumber = number; + return issue; + }, + async linkBranchToIssue(issueNumber: number, branch: string) { + recorder.linkBranch = { issueNumber, branch }; + if (linkFailsWith) throw linkFailsWith; + return { name: linkReturns ?? branch, sha: "sha-of-linked-branch" }; + }, + } as RepoProvider; + } + + async function call(method: string, path: string, body?: unknown) { + const res = await app.handle( + new Request(`http://bridge${path}`, { + method, + headers: body === undefined ? undefined : { "content-type": "application/json" }, + body: body === undefined ? undefined : JSON.stringify(body), + }), + ); + const text = await res.text(); + return { status: res.status, body: text ? JSON.parse(text) : undefined }; + } + + /** The branch the fake ship recorded for a workspace it was asked to create. */ + function branchTheShipReceived(name: string): string | undefined { + return ships.get("http://ship-a")!.workspaces.find((w) => w.name === name)?.branch; + } + + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), "fleet-bridge-issue-ws-")); + FakeSocket.byBase.clear(); + recorder = {}; + linkFailsWith = undefined; + linkReturns = undefined; + ships = new Map([["http://ship-a", { name: "ship-a", workspaces: [] }]]); + const config = { dataDirectory: dir, port: 4902, name: "bridge" }; + const store = new Store(dir); + await store.load(); + await store.createShip({ name: "ship-a", url: "http://ship-a" }); + manager = new FleetManager(config, makeDeps(ships), { + syncTimeoutMs: 50, + store, + providerFor: makeProvider, + }); + await manager.init(); + app = createApp(manager, config); + expect( + (await call("POST", "/repos", { name: "repo1", url: "https://github.com/acme/repo1", provider: "github" })) + .status, + ).toBe(201); + }); + afterEach(async () => { + manager.shutdown(); + await rm(dir, { recursive: true, force: true }); + }); + + test("derives the branch name from the issue and links it before cloning", async () => { + const res = await call("POST", "/workspaces", { + ship: "ship-a", + repoName: "repo1", + name: "twelve", + issueNumber: 12, + }); + + expect(res.status).toBe(201); + expect(recorder.getIssueNumber).toBe(12); + expect(recorder.linkBranch).toEqual({ issueNumber: 12, branch: "12-better-create-workspace-issue" }); + expect(branchTheShipReceived("twelve")).toBe("12-better-create-workspace-issue"); + expect(res.body).toMatchObject({ + repoName: "repo1", + name: "twelve", + branch: "12-better-create-workspace-issue", + ship: "ship-a", + }); + }); + + test("hands the ship the name the provider returned, not the computed one", async () => { + linkReturns = "12-better-create-workspace-issue-1"; + + const res = await call("POST", "/workspaces", { + ship: "ship-a", + repoName: "repo1", + name: "twelve", + issueNumber: 12, + }); + + expect(res.status).toBe(201); + expect(recorder.linkBranch!.branch).toBe("12-better-create-workspace-issue"); + expect(branchTheShipReceived("twelve")).toBe("12-better-create-workspace-issue-1"); + expect(res.body.branch).toBe("12-better-create-workspace-issue-1"); + }); + + test("a plain branch create never touches the provider", async () => { + const res = await call("POST", "/workspaces", { + ship: "ship-a", + repoName: "repo1", + name: "plain", + branch: "main", + }); + + expect(res.status).toBe(201); + expect(res.body.branch).toBe("main"); + expect(recorder).toEqual({}); + }); + + test.each([ + ["both a branch and an issue", { branch: "main", issueNumber: 12 }], + ["neither", {}], + ["a blank branch", { branch: " " }], + ["a non-integer issue number", { issueNumber: 1.5 }], + ["a zero issue number", { issueNumber: 0 }], + ])("rejects %s with 400", async (_label, extra) => { + const res = await call("POST", "/workspaces", { + ship: "ship-a", + repoName: "repo1", + name: "rejected", + ...extra, + }); + + expect(res.status).toBe(400); + expect(recorder).toEqual({}); + expect(ships.get("http://ship-a")!.createCalls).toBeUndefined(); + }); + + test("a failed link surfaces its status and leaves no reservation behind", async () => { + linkFailsWith = new ProviderError("authentication required", 401); + const body = { ship: "ship-a", repoName: "repo1", name: "twelve", issueNumber: 12 }; + + const first = await call("POST", "/workspaces", body); + expect(first.status).toBe(401); + expect(ships.get("http://ship-a")!.createCalls).toBeUndefined(); + + // A retry must fail the same way, not 409 on a reservation the first attempt left. + const second = await call("POST", "/workspaces", body); + expect(second.status).toBe(401); + expect(second.body.error).not.toContain("already in progress"); + }); + + test("a ship failure after a successful link clears the reservation too", async () => { + ships.get("http://ship-a")!.errorResponse = { status: 409, message: "clone destination exists" }; + const body = { ship: "ship-a", repoName: "repo1", name: "twelve", issueNumber: 12 }; + + expect((await call("POST", "/workspaces", body)).status).toBe(409); + + ships.get("http://ship-a")!.errorResponse = undefined; + const retry = await call("POST", "/workspaces", body); + expect(retry.status).toBe(201); + expect(retry.body.branch).toBe("12-better-create-workspace-issue"); + }); +}); diff --git a/packages/fleet-protocol/index.ts b/packages/fleet-protocol/index.ts index ee71327..4092b6f 100644 --- a/packages/fleet-protocol/index.ts +++ b/packages/fleet-protocol/index.ts @@ -8,6 +8,7 @@ export { parseFleetIdentifier, type FleetIdentifier, } from "./src/identifier"; +export { issueBranchName } from "./src/issue-branch"; export { DEFAULT_PORT, ATLAS_FILENAME, diff --git a/packages/fleet-protocol/src/issue-branch.ts b/packages/fleet-protocol/src/issue-branch.ts new file mode 100644 index 0000000..d16f851 --- /dev/null +++ b/packages/fleet-protocol/src/issue-branch.ts @@ -0,0 +1,43 @@ +/** + * src/issue-branch.ts — the canonical branch name for an issue. + * + * Reproduces the convention GitHub itself uses when you create a branch from an + * issue ("Development → create a branch"): `-`. + * + * It lives in the shared protocol package, rather than in the bridge, so that a + * client can render a preview of the name *before* submitting a create request + * while the bridge computes the same name authoritatively — one implementation, + * so the two can never disagree about what the user was shown. + */ + +/** + * Longest name this produces. Not a git limit (git allows far more): it keeps + * the resulting workspace/branch readable in a list and well inside the + * `FleetIdentifier` byte budget, since the output is pure ASCII. + */ +const MAX_LENGTH = 60; + +/** + * `-`, e.g. `12-better-create-workspace-issue`. The slug + * lowercases the title and collapses every run of non-`[a-z0-9]` into a single + * `-`; a title with no Latin alphanumerics at all yields just ``. + * + * The result is always a legal git branch name and a valid `FleetIdentifier`. + * Throws when `number` is not a positive integer — nothing downstream can make + * sense of a branch pointing at an issue that cannot exist. + */ +export function issueBranchName(issue: { number: number; title: string }): string { + if (!Number.isInteger(issue.number) || issue.number < 1) { + throw new Error(`issue number must be a positive integer, got ${issue.number}`); + } + + const slug = issue.title + .toLowerCase() + .replace(/[^a-z0-9]+/g, "-") + .replace(/^-+|-+$/g, ""); + const name = slug.length === 0 ? String(issue.number) : `${issue.number}-${slug}`; + if (name.length <= MAX_LENGTH) return name; + // Truncation can land mid-word and leave the separator dangling; a trailing + // "-" is legal in git but reads as a mistake, so it goes. + return name.slice(0, MAX_LENGTH).replace(/-+$/, ""); +} diff --git a/packages/fleet-protocol/tests/issue-branch.test.ts b/packages/fleet-protocol/tests/issue-branch.test.ts new file mode 100644 index 0000000..9a3e5ed --- /dev/null +++ b/packages/fleet-protocol/tests/issue-branch.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, test } from "bun:test"; +import { FleetIdentifierSchema, issueBranchName } from ".."; + +describe("issueBranchName", () => { + test("joins the number with a slug of the title", () => { + expect(issueBranchName({ number: 12, title: "Better create workspace issue" })).toBe( + "12-better-create-workspace-issue", + ); + }); + + test("collapses punctuation, emoji and non-Latin runs into single dashes", () => { + expect(issueBranchName({ number: 3, title: "Fix: the __thing__ (again)!" })).toBe( + "3-fix-the-thing-again", + ); + expect(issueBranchName({ number: 4, title: "🔥 hot 🔥 path 🔥" })).toBe("4-hot-path"); + expect(issueBranchName({ number: 5, title: "café / naïve" })).toBe("5-caf-na-ve"); + }); + + test("falls back to the bare number when the title has no Latin alphanumerics", () => { + expect(issueBranchName({ number: 7, title: "" })).toBe("7"); + expect(issueBranchName({ number: 8, title: "!!! ??? ---" })).toBe("8"); + expect(issueBranchName({ number: 9, title: "日本語のタイトル" })).toBe("9"); + }); + + test("caps the total length at 60 characters and never ends on a dash", () => { + const long = issueBranchName({ + number: 123, + title: "a very long issue title that keeps going and going and going well past the cap", + }); + expect(long.length).toBeLessThanOrEqual(60); + expect(long).toBe("123-a-very-long-issue-title-that-keeps-going-and-going-and-g"); + expect(long.endsWith("-")).toBe(false); + }); + + test("strips the separator when the cap lands right after one", () => { + // The 60th character is the dash before "cut", so the dash goes with the tail. + const name = issueBranchName({ number: 1, title: `${"a".repeat(57)} cut here` }); + expect(name).toBe(`1-${"a".repeat(57)}`); + }); + + test("output is a valid fleet identifier", () => { + for (const title of ["Better create workspace issue", "", "🔥".repeat(40), "x".repeat(500)]) { + const name = issueBranchName({ number: 42, title }); + expect(FleetIdentifierSchema.safeParse(name).success).toBe(true); + expect(name).toMatch(/^[0-9][a-z0-9-]*$/); + } + }); + + test("rejects a number that is not a positive integer", () => { + for (const number of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + expect(() => issueBranchName({ number, title: "t" })).toThrow(); + } + }); +}); From a4c9aa8077b7b86b642435fc65e898706d0bef18 Mon Sep 17 00:00:00 2001 From: Jonathan Deiss Date: Mon, 27 Jul 2026 15:27:32 -0500 Subject: [PATCH 2/7] Make issue linking idempotent and stop git prompting on a probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for 03a08ec. The two that matter were each found by two reviewers independently: - `ls-remote` ran with no prompt suppression and no deadline. git opens /dev/tty directly for credentials, ssh passphrases and unknown host keys, bypassing the pipes it was handed, and the bridge normally runs in an operator's foreground terminal — so a private repo parked the probe on a prompt nobody answers and hung the request behind it for good. Probes now run non-interactively under a 15s deadline. - `linkBranchToIssue` had no "already linked" path, so the retry the create-site comment promised did not exist: any failure after a successful link left that issue permanently uncreatable. GitHub signals a duplicate two different ways — a populated `errors` array, or HTTP 200 with a null `linkedBranch` — and both are now read as "already there", falling back to whatever the issue is linked to, or to the branch of that name. Only a genuine absence is an error, and it is a 409 naming the collision rather than a 502. Also: pin the reservation-before-provider ordering with a gated concurrency test (hoisting the provider call now fails it), raise the issue listing off GitHub's 30-item default, keep credentials embedded in a repo URL out of the 502 body, and correct the bridge API reference, which this feature had made false. --- .../src/content/docs/reference/bridge-api.md | 63 +++++++- packages/fleet-bridge/src/fleet-manager.ts | 96 +++++++++-- packages/fleet-bridge/src/providers/github.ts | 124 ++++++++++++-- packages/fleet-bridge/tests/providers.test.ts | 153 +++++++++++++----- .../fleet-bridge/tests/repo-branches.test.ts | 67 +++++++- .../tests/repo-provider-api.test.ts | 5 +- .../tests/workspace-from-issue.test.ts | 45 +++++- packages/fleet-protocol/src/issue-branch.ts | 8 +- 8 files changed, 469 insertions(+), 92 deletions(-) diff --git a/apps/docs/src/content/docs/reference/bridge-api.md b/apps/docs/src/content/docs/reference/bridge-api.md index 992749a..c4591a0 100644 --- a/apps/docs/src/content/docs/reference/bridge-api.md +++ b/apps/docs/src/content/docs/reference/bridge-api.md @@ -20,7 +20,7 @@ adds ship management, a repo registry, and an aggregate system-resources view. | `GET /workspaces` | Same path. Merged across ships, deduped, each row gains `ship`. | | `GET /workspaces/:repo/:name` | Same path. Proxied live to the owning ship; response gains `ship` on **both** the `active` and `inactive` variants. | | `GET /workspaces/:repo/:name/diff` | Same path and query. Proxied verbatim. | -| `POST /workspaces` | Same path, **different body**: `{ship, repoName, name, branch}` instead of `{url, repoName, name, branch}`. The clone URL comes from the bridge's repo registry. Response gains `ship`. | +| `POST /workspaces` | Same path, **different body**: `{ship, repoName, name, branch \| issueNumber}` instead of `{url, repoName, name, branch}`. The clone URL comes from the bridge's repo registry, and the branch may be named outright or derived from an issue. Response gains `ship`. | | `POST /workspaces/:repo/:name/branch` | Same. | | `POST /workspaces/:repo/:name/activate` | Same. | | `POST /workspaces/:repo/:name/deactivate` | Same. | @@ -36,7 +36,8 @@ adds ship management, a repo registry, and an aggregate system-resources view. Bridge-only routes: `GET`/`POST /ships`, `DELETE /ships/:name`, `GET /ships/:ship/system-resources`, `GET`/`POST /repos`, -`DELETE /repos/:name`, `GET /armory/file`, `GET /armory/ships`. +`DELETE /repos/:name`, `GET /repos/:name/branches`, `GET /armory/file`, +`GET /armory/ships`. ## Routes at a glance @@ -50,6 +51,7 @@ Bridge-only routes: `GET`/`POST /ships`, `DELETE /ships/:name`, | GET | `/repos` | 200 | `Repo[]` | | POST | `/repos` | 201 | `Repo` | | DELETE | `/repos/:name` | 200 | `{ ok: true }` | +| GET | `/repos/:name/branches` | 200 | `RepoBranch[]` | | GET | `/armory` | 200 | `ArmoryManifest` | | GET | `/armory/file` | 200 | `ArmoryFile` | | GET | `/armory/ships` | 200 | `ShipArmoryState[]` | @@ -76,11 +78,11 @@ The status comes from the thrown `BridgeError`; anything else is a `500`. | Status | Raised when | | --- | --- | -| `400` | Invalid repo/workspace/ship identifier; `unknown ship: ` (create, or per-ship resources); `unknown repo: `; `invalid repo`. | +| `400` | Invalid repo/workspace/ship identifier; `unknown ship: ` (create, or per-ship resources); `unknown repo: `; `invalid repo`; a create naming both a `branch` and an `issueNumber`, neither, a blank `branch`, or an `issueNumber` that is not a positive integer. | | `404` | `workspace not found: /` — no ship in the ownership index holds it; `ship not found: `; `repo not found: `. | | `409` | `ship already registered: `; a registering ship holds workspaces already hosted elsewhere; `workspace already exists: /`; a create already in progress or of indeterminate outcome for that key; a ship removed mid-request. | | `422` | Elysia schema validation on the request body. | -| `502` | `ship at did not respond: ` (`POST /ships`); a ship returned no data, an invalid summary/status, or a workspace identity that was not requested. | +| `502` | `ship at did not respond: ` (`POST /ships`); a ship returned no data, an invalid summary/status, or a workspace identity that was not requested; `GET /repos/:name/branches` could not reach the remote. | | `503` | `ship "" hosting / is offline`; `ship "" is offline` (create, per-ship resources); `ship "" unreachable: `. | | ship's status | Any error the owning ship returned is passed through with the ship's own status and message. | @@ -218,6 +220,30 @@ Responds `{ ok: true }`. Deleting a repo does not touch any workspace already cloned from it. +### `GET /repos/:name/branches` + +The branches the repo's remote currently advertises, sorted by name. + +```ts +{ name: string; sha: string }[] +``` + +Answered with `git ls-remote --heads` against the registered clone URL, **not** +through the repo's provider: `provider` defaults to `"custom"`, for which no +provider exists, so a provider-backed listing would be unavailable for most +repos. `ls-remote` works against any git URL and needs no token. The probe runs +non-interactively (git never prompts for credentials or host keys) and is +abandoned after 15 s. + +`refs/heads/` is stripped from each name; tags and other refs are omitted, so a +tag the ship would happily clone does not appear here. + +| Status | Cause | +| --- | --- | +| `400` | Invalid repo identifier. | +| `404` | `repo not found: `. | +| `502` | `could not list branches for repo "": ` — unreachable, unauthenticated, or timed out. Credentials embedded in the repo URL are redacted from this message. | + ## Armory The read side of the [armory](/guides/the-armory/): the manifest of the bridge's @@ -374,8 +400,10 @@ text. ### `POST /workspaces` ```ts -// request body — all four fields required -{ ship: string; repoName: string; name: string; branch: string } +// request body — ship, repoName and name are required; +// exactly one of branch / issueNumber must be present +{ ship: string; repoName: string; name: string; + branch?: string; issueNumber?: number } ``` `ship` names the target host and `repoName` must be a **registered repo**; the @@ -387,10 +415,29 @@ bridge looks up its clone URL and calls the ship's `POST /workspaces` with created in the new workspace rather than rejected — see [ship API](/reference/ship-api/). +With `issueNumber` instead, the bridge resolves the branch itself before calling +the ship: + +1. it reads the issue through the repo's [provider](#repo-registry) — so this + form needs `provider: "github"` and a token with repo write scope; +2. it computes the issue's canonical branch name, `-` + capped at 60 characters (`12-better-create-workspace-issue`) — the same + function a client can use to preview the name; +3. it asks the provider to create that branch and record it as the issue's + linked development branch (GitHub's "Development → create a branch"); +4. the **name the provider returns** is what the ship is told to check out, which + may differ from the computed one if the provider de-duplicated it. + +Step 3 is idempotent: an issue that already has a linked branch, or a branch of +that name created by hand, resolves to the existing branch instead of failing. +The branch is created before the clone and is *not* removed if the clone then +fails — a retry reuses it. + | Status | Cause | | --- | --- | -| `422` | A body field is missing. | -| `400` | Invalid repo/workspace identifier; `unknown ship: `; `unknown repo: `. | +| `422` | `ship`, `repoName` or `name` is missing. | +| `400` | Invalid repo/workspace identifier; `unknown ship: `; `unknown repo: `; both `branch` and `issueNumber`, or neither; a blank `branch`; an `issueNumber` that is not a positive integer. | +| provider's status | Any error resolving or linking the issue is passed through with the provider's own status — e.g. `401` with no token, `404` for an unknown issue, `409` when the branch could be neither created nor found. | | `503` | `ship "" is offline`. | | `409` | `workspace already exists: /`; a create for that key is already in progress; the key's create outcome is indeterminate; the target ship was removed mid-request. | | `502` | The ship returned no data, an invalid summary, or a different workspace identity. | diff --git a/packages/fleet-bridge/src/fleet-manager.ts b/packages/fleet-bridge/src/fleet-manager.ts index 4ae69da..67855ca 100644 --- a/packages/fleet-bridge/src/fleet-manager.ts +++ b/packages/fleet-bridge/src/fleet-manager.ts @@ -33,7 +33,7 @@ import { type WorkspaceStatus, type WorkspaceSummary, } from "fleet-protocol"; -import { Git, type DiffOptions, type RemoteRef } from "git-bun"; +import { Git, GitError, type DiffOptions, type RemoteRef } from "git-bun"; import { TERMINAL_TAKEOVER_QUERY } from "webterm/protocol"; import { ShipConnection, toWsUrl, type ShipConnectionDeps } from "./ship-connection"; import { defaultPublicUrl, type BridgeConfig } from "./config"; @@ -86,10 +86,60 @@ export class BridgeError extends Error { /** How long to wait for a ship's first `sync` before treating it as offline. */ const SYNC_TIMEOUT_MS = 5000; +/** How long a remote ref probe may run before the bridge stops waiting on it. */ +const LS_REMOTE_TIMEOUT_MS = 15000; + +/** + * Environment for every git invocation the bridge makes. + * + * git asks for credentials, ssh passphrases and unknown host keys by opening + * `/dev/tty` directly, which bypasses the pipes it was given — and the bridge + * normally runs in an operator's foreground terminal, so that tty exists. Left + * to itself a private repo would therefore park the git process on a prompt + * nobody answers and hang the HTTP request behind it for good. These three tell + * git, `GIT_ASKPASS`' callers and ssh to fail instead of asking. + */ +const NON_INTERACTIVE_GIT_ENV: Record = { + GIT_TERMINAL_PROMPT: "0", + GIT_ASKPASS: "", + GIT_SSH_COMMAND: "ssh -oBatchMode=yes", +}; + +/** + * Reject with `message` if `promise` has not settled within `ms`. + * + * Only the *waiting* is bounded: `git-bun` exposes no way to abort a running + * command, so a git process talking to a blackholed remote is abandoned to its + * own network timeout rather than killed. That still keeps the request — and the + * client waiting on it — from hanging indefinitely. + */ +async function withTimeout(promise: Promise, ms: number, message: string): Promise { + let timer: ReturnType | undefined; + try { + return await Promise.race([ + promise, + new Promise((_resolve, reject) => { + timer = setTimeout(() => reject(new Error(message)), ms); + }), + ]); + } finally { + clearTimeout(timer); + } +} + +/** + * Blank out `user:secret@` userinfo in any URL a message carries. A repo may be + * registered with an embedded token, and git echoes the URL it was given back in + * the command line it reports on failure — this keeps that out of an API response. + */ +function redactUrlCredentials(text: string): string { + return text.replace(/([a-z][a-z0-9+.-]*:\/\/)[^\s/@]+@/gi, "$1***@"); +} + /** * Body of `POST /workspaces` on the bridge (ship-targeted, names a registered - * repo). The branch comes either verbatim from `branch` or from the issue - * `issueNumber` names — exactly one of the two, never both. + * repo). The branch comes either verbatim from `branch`, or from the issue that + * `issueNumber` identifies — exactly one of the two, never both. */ export interface CreateWorkspaceInput { readonly ship: string; @@ -126,6 +176,8 @@ export class FleetManager { private readonly makeProvider: (repo: Repo) => RepoProvider; /** Probes a remote's refs; overridable so tests never shell out to git. */ private readonly lsRemote: typeof Git.lsRemote; + /** Deadline for one `lsRemote` probe (overridable in tests). */ + private readonly lsRemoteTimeoutMs: number; /** The bridge-owned file factory served from `/armory`. */ private readonly armory: ArmoryService; @@ -137,6 +189,7 @@ export class FleetManager { store?: Store; providerFor?: (repo: Repo) => RepoProvider; lsRemote?: typeof Git.lsRemote; + lsRemoteTimeoutMs?: number; armory?: ArmoryService; }, ) { @@ -145,6 +198,7 @@ export class FleetManager { this.store = opts?.store ?? new Store(config.dataDirectory); this.makeProvider = opts?.providerFor ?? providerFor; this.lsRemote = opts?.lsRemote ?? Git.lsRemote; + this.lsRemoteTimeoutMs = opts?.lsRemoteTimeoutMs ?? LS_REMOTE_TIMEOUT_MS; this.armory = opts?.armory ?? new ArmoryService(join(config.dataDirectory, ARMORY_DIRECTORY)); } @@ -376,12 +430,23 @@ export class FleetManager { let refs: RemoteRef[]; try { - refs = await this.lsRemote(repo.url, { cwd: this.config.dataDirectory, heads: true }); + refs = await withTimeout( + this.lsRemote(repo.url, { + cwd: this.config.dataDirectory, + heads: true, + env: NON_INTERACTIVE_GIT_ENV, + }), + this.lsRemoteTimeoutMs, + `timed out after ${this.lsRemoteTimeoutMs}ms`, + ); } catch (error) { // Any failure here is the remote's or the network's, not the caller's — and // a raw GitError must not reach the route, which would report it as a 500. + // git's own stderr is preferred over the GitError message because the + // message replays the command line, credentials in the URL included. + const detail = error instanceof GitError ? error.stderr.trim() || error.message : (error as Error).message; throw new BridgeError( - `could not list branches for repo "${name}": ${(error as Error).message}`, + redactUrlCredentials(`could not list branches for repo "${name}": ${detail}`), 502, ); } @@ -685,12 +750,14 @@ export class FleetManager { try { // Resolving the issue happens under the reservation, so two concurrent - // creates of the same workspace cannot both mint a linked branch. The - // converse — the branch gets created and linked and then the ship call - // fails — is accepted: a retry reuses that branch (the provider returns - // the existing linked branch, or a deduped name), the reservation is still - // cleared by the `finally` below, and the error the user sees is the - // ship's, because this call has already returned by then. + // creates of the same workspace cannot both ask the provider for a branch; + // the second is turned away with a 409 before it gets here. + // + // The branch outliving a failed create is accepted rather than undone: if + // the ship call below fails, the linked branch stays on the remote with no + // workspace behind it. Nothing is corrupted — the `finally` still clears + // the reservation, the error the user sees is the ship's, and a retry + // resolves the same branch because `linkBranchToIssue` is idempotent. const branch = "branch" in source ? source.branch : await this.branchForIssue(input.repoName, source.issueNumber); @@ -766,9 +833,10 @@ export class FleetManager { return { branch }; } if (input.issueNumber !== undefined) { - // Guarded here so `issueBranchName` — which rejects the same values — can - // never be reached with a number a client made up. - if (!Number.isInteger(input.issueNumber) || input.issueNumber < 1) { + // `t.Numeric()` admits 0, 1.5 and -3; rejecting them here costs nothing, + // where letting them through costs a provider round trip to learn that no + // such issue exists. + if (!Number.isSafeInteger(input.issueNumber) || input.issueNumber < 1) { throw new BridgeError("issueNumber must be a positive integer", 400); } return { issueNumber: input.issueNumber }; diff --git a/packages/fleet-bridge/src/providers/github.ts b/packages/fleet-bridge/src/providers/github.ts index 548cbb7..83a2cc9 100644 --- a/packages/fleet-bridge/src/providers/github.ts +++ b/packages/fleet-bridge/src/providers/github.ts @@ -52,6 +52,26 @@ const CREATE_LINKED_BRANCH = `mutation CreateLinkedBranch($issueId: ID!, $oid: G } }`; +/** + * The branches already attached to an issue, used to make `linkBranchToIssue` + * idempotent. `first: 10` is generous: GitHub's own UI links one branch per + * issue, and only a human linking branches by hand pushes it past that. + */ +const LINKED_BRANCHES = `query LinkedBranches($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + issue(number: $number) { + linkedBranches(first: 10) { + nodes { + ref { + name + target { oid } + } + } + } + } + } +}`; + /** * Extract `{ owner, repo }` from a git clone URL. Handles the https web form * (with or without a `.git` suffix or trailing slash) and the `git@host:o/r.git` @@ -169,14 +189,32 @@ interface GraphQLResponse { errors?: { message?: unknown }[]; } +/** The `{ name, target { oid } }` selection both linked-branch operations share. */ +interface GraphQLRef { + name?: string; + target?: { oid?: string } | null; +} + interface CreateLinkedBranchResult { createLinkedBranch?: { - linkedBranch?: { - ref?: { name?: string; target?: { oid?: string } | null } | null; + linkedBranch?: { ref?: GraphQLRef | null } | null; + } | null; +} + +interface LinkedBranchesResult { + repository?: { + issue?: { + linkedBranches?: { nodes?: ({ ref?: GraphQLRef | null } | null)[] | null } | null; } | null; } | null; } +/** A ref selection that carries both fields, or `undefined` if it does not. */ +function toLinkedBranch(ref: GraphQLRef | null | undefined): LinkedBranch | undefined { + if (!ref?.name || !ref.target?.oid) return undefined; + return { name: ref.name, sha: ref.target.oid }; +} + export class GitHubProvider implements RepoProvider { private readonly owner: string; private readonly repo: string; @@ -208,8 +246,11 @@ export class GitHubProvider implements RepoProvider { async listIssues(options?: ListOptions): Promise { const state = options?.state ?? "open"; + // GitHub pages at 30 by default and says nothing about the truncation. The + // list feeds a search-over-all-issues picker, so 100 — the maximum a single + // page allows — is the difference between "no matches" and the issue. const issues = await this.request( - `/repos/${this.owner}/${this.repo}/issues?state=${state}`, + `/repos/${this.owner}/${this.repo}/issues?state=${state}&per_page=100`, ); return issues .filter((issue) => issue.pull_request === undefined) @@ -343,6 +384,13 @@ export class GitHubProvider implements RepoProvider { * Three reads precede the write because the mutation is addressed by GraphQL * node id and a commit oid, neither of which the caller has: the issue's * `node_id`, the repo's default branch, and that branch's head SHA. + * + * Idempotent by design — asking twice for the same branch is ordinary (a + * second workspace off one issue, a retry after a failed clone, or a branch + * someone already made with `gh issue develop`). GitHub signals "that is + * already linked" in two different ways depending on the case, and neither is + * a real failure, so both fall back to {@link linkedBranch}: whatever is + * attached to the issue now is what the caller wanted. */ async linkBranchToIssue(issueNumber: number, branch: string): Promise { this.requireToken(); @@ -357,27 +405,68 @@ export class GitHubProvider implements RepoProvider { const { defaultBranch } = await this.getInfo(); // Not URL-encoded: a default branch may legitimately contain "/", which is a // path separator in the ref endpoint rather than data. - const ref = await this.request( + const base = await this.request( `/repos/${this.owner}/${this.repo}/git/ref/heads/${defaultBranch}`, ); - if (!ref.object?.sha) { + if (!base.object?.sha) { throw new ProviderError(`GitHub returned no head commit for branch ${defaultBranch}`, 502); } - const result = await this.graphql(CREATE_LINKED_BRANCH, { - issueId: issue.node_id, - oid: ref.object.sha, - name: branch, + let result: CreateLinkedBranchResult; + try { + result = await this.graphql(CREATE_LINKED_BRANCH, { + issueId: issue.node_id, + oid: base.object.sha, + name: branch, + }); + } catch (error) { + // Shape 1: a populated `errors` array saying the name is taken. + if (error instanceof ProviderError && error.status === 422) { + return this.linkedBranch(issueNumber, branch); + } + throw error; + } + + const created = toLinkedBranch(result.createLinkedBranch?.linkedBranch?.ref); + // Shape 2: HTTP 200, no `errors` at all, and a null `linkedBranch`. + return created ?? (await this.linkedBranch(issueNumber, branch)); + } + + /** + * The branch already standing in for a failed `createLinkedBranch`: whichever + * branch the issue is linked to (preferring `requested`, since a second caller + * asking for the same name should get that one back), or — for a branch that + * exists but was never linked — the ref itself. + * + * Only when neither turns anything up has nothing actually been created, and + * the 409 says which name collided rather than blaming the upstream. + */ + private async linkedBranch(issueNumber: number, requested: string): Promise { + const result = await this.graphql(LINKED_BRANCHES, { + owner: this.owner, + repo: this.repo, + number: issueNumber, }); + const linked = (result.repository?.issue?.linkedBranches?.nodes ?? []) + .map((node) => toLinkedBranch(node?.ref)) + .filter((ref): ref is LinkedBranch => ref !== undefined); + const match = linked.find((ref) => ref.name === requested) ?? linked[0]; + if (match) return match; - const created = result.createLinkedBranch?.linkedBranch?.ref; - if (!created?.name || !created.target?.oid) { - throw new ProviderError( - `GitHub createLinkedBranch returned no branch for issue ${issueNumber}`, - 502, + try { + const ref = await this.request( + `/repos/${this.owner}/${this.repo}/git/ref/heads/${requested}`, ); + if (ref.object?.sha) return { name: requested, sha: ref.object.sha }; + } catch { + // A 404 here just means the branch is not there either; fall through to + // the collision report, which is the more useful message. } - return { name: created.name, sha: created.target.oid }; + + throw new ProviderError( + `GitHub would not create branch "${requested}" for issue ${issueNumber}, and neither that branch nor a branch linked to the issue exists`, + 409, + ); } private async postComment(number: number, body: string): Promise { @@ -471,8 +560,9 @@ export class GitHubProvider implements RepoProvider { const first = payload.errors?.[0]; if (first !== undefined) { const message = typeof first.message === "string" ? first.message : "unknown GraphQL error"; - // GraphQL has no status of its own, so a name collision — the one failure a - // caller can act on — has to be recognized from the message text. + // GraphQL has no status of its own, so a name collision has to be + // recognized from the message text. 422 is what `linkBranchToIssue` reads + // as "already there, go and find it"; it never reaches a client. const status = /already exists/i.test(message) ? 422 : 502; throw new ProviderError(`GitHub GraphQL request failed: ${message}`, status); } diff --git a/packages/fleet-bridge/tests/providers.test.ts b/packages/fleet-bridge/tests/providers.test.ts index c8a5014..bf99fde 100644 --- a/packages/fleet-bridge/tests/providers.test.ts +++ b/packages/fleet-bridge/tests/providers.test.ts @@ -216,7 +216,7 @@ describe("GitHubProvider", () => { expect(issues).toHaveLength(1); expect(issues[0]!.number).toBe(1); - expect(calls[0]!.url).toBe("https://api.github.com/repos/owner/repo/issues?state=open"); + expect(calls[0]!.url).toBe("https://api.github.com/repos/owner/repo/issues?state=open&per_page=100"); }); test("listIssues passes through an explicit state", async () => { @@ -225,7 +225,16 @@ describe("GitHubProvider", () => { await provider.listIssues({ state: "all" }); - expect(calls[0]!.url).toBe("https://api.github.com/repos/owner/repo/issues?state=all"); + expect(calls[0]!.url).toBe("https://api.github.com/repos/owner/repo/issues?state=all&per_page=100"); + }); + + test("listIssues asks for a full page, not GitHub's default 30", async () => { + const { fetch, calls } = fakeFetch(Response.json([])); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", fetch }); + + await provider.listIssues(); + + expect(new URL(calls[0]!.url).searchParams.get("per_page")).toBe("100"); }); test("a 404 upstream response surfaces as a ProviderError with status 404", async () => { @@ -447,20 +456,28 @@ describe("GitHubProvider", () => { } }); + /** The GraphQL operation a fake saw, so a test can answer per operation. */ + interface GraphQLCall { + query: string; + variables: Record; + } + /** - * Drive the three REST reads `linkBranchToIssue` makes, then hand the GraphQL - * mutation whatever `graphqlResponse` returns. + * Drive every REST read `linkBranchToIssue` makes, and route each GraphQL + * operation to `graphql`. Refs other than `main` 404 unless `extraRefs` names + * them — that is how the "branch exists but was never linked" path is set up. */ - function linkBranchFetch(graphqlResponse: () => Response): { fetch: typeof fetch; calls: FetchCall[] } { + function linkBranchFetch( + graphql: (call: GraphQLCall) => Response, + extraRefs: Record = {}, + ): { fetch: typeof fetch; calls: FetchCall[] } { const calls: FetchCall[] = []; + const refs: Record = { main: "basesha", ...extraRefs }; const fn = (async (input: string | URL | Request, init?: RequestInit) => { const url = String(input); - calls.push({ - url, - method: init?.method ?? "GET", - headers: new Headers(init?.headers), - body: typeof init?.body === "string" ? init.body : undefined, - }); + const body = typeof init?.body === "string" ? init.body : undefined; + calls.push({ url, method: init?.method ?? "GET", headers: new Headers(init?.headers), body }); + if (url.endsWith("/issues/12")) { return Response.json({ number: 12, @@ -476,24 +493,48 @@ describe("GitHubProvider", () => { }); } if (url.endsWith("/repos/owner/repo")) return Response.json(repoPayload); - if (url.endsWith("/git/ref/heads/main")) return Response.json({ object: { sha: "basesha" } }); - if (url.endsWith("/graphql")) return graphqlResponse(); + + const ref = /\/git\/ref\/heads\/(.+)$/.exec(url)?.[1]; + if (ref !== undefined) { + const sha = refs[ref]; + return sha === undefined + ? Response.json({ message: "Not Found" }, { status: 404 }) + : Response.json({ object: { sha } }); + } + if (url.endsWith("/graphql")) return graphql(JSON.parse(body!) as GraphQLCall); throw new Error(`unexpected fetch: ${url}`); }) as unknown as typeof globalThis.fetch; return { fetch: fn, calls }; } - const linkedBranchPayload = () => + const isMutation = (call: GraphQLCall) => call.query.includes("createLinkedBranch("); + + const createdRef = (name: string, oid: string) => + Response.json({ data: { createLinkedBranch: { linkedBranch: { ref: { name, target: { oid } } } } } }); + + const linkedRefs = (...refs: { name: string; oid: string }[]) => Response.json({ data: { - createLinkedBranch: { - linkedBranch: { ref: { name: "12-a-bug", target: { oid: "newsha" } } }, + repository: { + issue: { + linkedBranches: { + nodes: refs.map((ref) => ({ ref: { name: ref.name, target: { oid: ref.oid } } })), + }, + }, }, }, }); + /** The documented duplicate reply: HTTP 200, no `errors`, null `linkedBranch`. */ + const duplicateByNull = () => + Response.json({ data: { createLinkedBranch: { clientMutationId: null, issue: null, linkedBranch: null } } }); + + /** The other duplicate reply: a populated `errors` array. */ + const duplicateByError = () => + Response.json({ errors: [{ message: "A ref named 12-a-bug already exists in the repository" }] }); + test("linkBranchToIssue posts the mutation with the issue node id, base oid and name", async () => { - const { fetch, calls } = linkBranchFetch(linkedBranchPayload); + const { fetch, calls } = linkBranchFetch(() => createdRef("12-a-bug", "newsha")); const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); const linked = await provider.linkBranchToIssue(12, "12-a-bug"); @@ -510,15 +551,7 @@ describe("GitHubProvider", () => { }); test("linkBranchToIssue returns the name GitHub actually created, not the requested one", async () => { - const { fetch } = linkBranchFetch(() => - Response.json({ - data: { - createLinkedBranch: { - linkedBranch: { ref: { name: "12-a-bug-1", target: { oid: "newsha" } } }, - }, - }, - }), - ); + const { fetch } = linkBranchFetch(() => createdRef("12-a-bug-1", "newsha")); const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ name: "12-a-bug-1", sha: "newsha" }); @@ -554,22 +587,64 @@ describe("GitHubProvider", () => { } }); - test("a GraphQL 'already exists' error becomes a ProviderError(422)", async () => { - const { fetch } = linkBranchFetch(() => - Response.json({ errors: [{ message: "A branch with that name already exists" }] }), + // GitHub reports "that issue already has this branch" in two different ways + // depending on the case, and neither is a failure the caller can act on, so + // both have to resolve to the branch that is already there. + test.each([ + ["a null linkedBranch with no errors", duplicateByNull], + ["an 'already exists' error", duplicateByError], + ])("a duplicate reported as %s resolves to the issue's existing linked branch", async (_label, duplicate) => { + const { fetch, calls } = linkBranchFetch((call) => + isMutation(call) ? duplicate() : linkedRefs({ name: "12-a-bug", oid: "existingsha" }), ); const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); - try { - await provider.linkBranchToIssue(12, "12-a-bug"); - throw new Error("expected linkBranchToIssue to throw"); - } catch (error) { - expect((error as ProviderError).status).toBe(422); - } + expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ name: "12-a-bug", sha: "existingsha" }); + + const query = calls.filter((c) => c.url.endsWith("/graphql")).at(-1)!; + const body = JSON.parse(query.body!) as GraphQLCall; + expect(body.query).toContain("linkedBranches"); + expect(body.variables).toEqual({ owner: "owner", repo: "repo", number: 12 }); + }); + + test.each([ + ["a null linkedBranch with no errors", duplicateByNull], + ["an 'already exists' error", duplicateByError], + ])("a duplicate reported as %s prefers the requested name among several links", async (_label, duplicate) => { + const { fetch } = linkBranchFetch((call) => + isMutation(call) + ? duplicate() + : linkedRefs({ name: "12-something-else", oid: "othersha" }, { name: "12-a-bug", oid: "existingsha" }), + ); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ name: "12-a-bug", sha: "existingsha" }); }); - test("a GraphQL payload missing the created ref becomes a ProviderError(502)", async () => { - const { fetch } = linkBranchFetch(() => Response.json({ data: { createLinkedBranch: null } })); + test("a duplicate falls back to any branch the issue is linked to", async () => { + const { fetch } = linkBranchFetch((call) => + isMutation(call) ? duplicateByNull() : linkedRefs({ name: "12-renamed-by-hand", oid: "handsha" }), + ); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ + name: "12-renamed-by-hand", + sha: "handsha", + }); + }); + + test("a duplicate with no linked branch falls back to the branch of that name", async () => { + const { fetch } = linkBranchFetch( + (call) => (isMutation(call) ? duplicateByError() : linkedRefs()), + { "12-a-bug": "unlinkedsha" }, + ); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ name: "12-a-bug", sha: "unlinkedsha" }); + }); + + test("a refused create with nothing to fall back on is a ProviderError(409) naming the branch", async () => { + const { fetch } = linkBranchFetch((call) => (isMutation(call) ? duplicateByNull() : linkedRefs())); const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); try { @@ -577,8 +652,8 @@ describe("GitHubProvider", () => { throw new Error("expected linkBranchToIssue to throw"); } catch (error) { expect(error).toBeInstanceOf(ProviderError); - expect((error as ProviderError).status).toBe(502); - expect((error as ProviderError).message).toContain("no branch"); + expect((error as ProviderError).status).toBe(409); + expect((error as ProviderError).message).toContain("12-a-bug"); } }); diff --git a/packages/fleet-bridge/tests/repo-branches.test.ts b/packages/fleet-bridge/tests/repo-branches.test.ts index 568f5ae..cdc39e5 100644 --- a/packages/fleet-bridge/tests/repo-branches.test.ts +++ b/packages/fleet-bridge/tests/repo-branches.test.ts @@ -16,8 +16,8 @@ import { makeDeps } from "./helpers"; /** What the fake `lsRemote` was asked, and what it answers with. */ interface LsRemoteStub { - calls: { url: string; cwd: string; heads?: boolean }[]; - answer: () => RemoteRef[]; + calls: { url: string; cwd: string; heads?: boolean; env?: Record }[]; + answer: () => RemoteRef[] | Promise; } describe("GET /repos/:name/branches", () => { @@ -54,8 +54,9 @@ describe("GET /repos/:name/branches", () => { manager = new FleetManager(config, makeDeps(new Map()), { syncTimeoutMs: 50, store, + lsRemoteTimeoutMs: 50, lsRemote: async (url, options) => { - lsRemote.calls.push({ url, cwd: options.cwd, heads: options.heads }); + lsRemote.calls.push({ url, cwd: options.cwd, heads: options.heads, env: options.env }); return lsRemote.answer(); }, }); @@ -82,7 +83,30 @@ describe("GET /repos/:name/branches", () => { test("probes the repo's url from the bridge's data directory, branches only", async () => { await call("GET", "/repos/repo1/branches"); - expect(lsRemote.calls).toEqual([{ url: "git@fake/repo1.git", cwd: dir, heads: true }]); + expect(lsRemote.calls).toHaveLength(1); + expect(lsRemote.calls[0]).toMatchObject({ url: "git@fake/repo1.git", cwd: dir, heads: true }); + }); + + test("runs git non-interactively, so a credential prompt cannot hang the request", async () => { + await call("GET", "/repos/repo1/branches"); + + // Without these, git opens /dev/tty for the prompt — the bridge runs in an + // operator's terminal, so the request would block until someone typed. + expect(lsRemote.calls[0]!.env).toEqual({ + GIT_TERMINAL_PROMPT: "0", + GIT_ASKPASS: "", + GIT_SSH_COMMAND: "ssh -oBatchMode=yes", + }); + }); + + test("a probe that never settles is abandoned as a 502 instead of hanging", async () => { + lsRemote.answer = () => new Promise(() => {}); + + const res = await call("GET", "/repos/repo1/branches"); + + expect(res.status).toBe(502); + expect(res.body.error).toContain("timed out"); + expect(res.body.error).toContain("repo1"); }); test("drops refs that are not branches", async () => { @@ -107,6 +131,41 @@ describe("GET /repos/:name/branches", () => { expect((await call("GET", "/repos/..%2Fescape/branches")).status).toBe(400); }); + test("a token embedded in the repo url never reaches the error response", async () => { + const url = "https://x-access-token:ghp_SECRET@github.com/acme/private.git"; + expect((await call("POST", "/repos", { name: "private", url })).status).toBe(201); + lsRemote.answer = () => { + // git replays the command line — token and all — in the GitError message, + // and redacts it only in its own stderr. + throw new GitError(["ls-remote", "--heads", "--", url], { + stdout: "", + stderr: "fatal: Authentication failed for 'https://github.com/acme/private.git/'", + exitCode: 128, + }); + }; + + const res = await call("GET", "/repos/private/branches"); + + expect(res.status).toBe(502); + expect(res.body.error).not.toContain("ghp_SECRET"); + expect(res.body.error).toContain("Authentication failed"); + }); + + test("credentials git echoes back in its own stderr are redacted too", async () => { + lsRemote.answer = () => { + throw new GitError(["ls-remote"], { + stdout: "", + stderr: "fatal: could not read from 'https://user:ghp_SECRET@github.com/acme/private.git'", + exitCode: 128, + }); + }; + + const res = await call("GET", "/repos/repo1/branches"); + + expect(res.body.error).not.toContain("ghp_SECRET"); + expect(res.body.error).toContain("https://***@github.com/acme/private.git"); + }); + test("an unreachable remote surfaces as 502 naming the repo", async () => { lsRemote.answer = () => { throw new GitError(["ls-remote", "--heads", "--", "git@fake/repo1.git"], { diff --git a/packages/fleet-bridge/tests/repo-provider-api.test.ts b/packages/fleet-bridge/tests/repo-provider-api.test.ts index 9d763e9..863951f 100644 --- a/packages/fleet-bridge/tests/repo-provider-api.test.ts +++ b/packages/fleet-bridge/tests/repo-provider-api.test.ts @@ -38,7 +38,6 @@ interface Recorder { reviewPr?: { number: number; review: { event: ReviewEvent; body?: string } }; checksRef?: string; failedLogsRef?: string; - linkBranch?: { issueNumber: number; branch: string }; } const info: RepoInfo = { @@ -173,9 +172,9 @@ describe("repo provider API", () => { recorder.failedLogsRef = ref; return [failedLog]; }, - async linkBranchToIssue(issueNumber: number, branch: string) { + // Unused by these routes, but the interface requires it. + async linkBranchToIssue(_issueNumber: number, branch: string) { guard(); - recorder.linkBranch = { issueNumber, branch }; return { name: branch, sha: "sha-of-linked-branch" }; }, }; diff --git a/packages/fleet-bridge/tests/workspace-from-issue.test.ts b/packages/fleet-bridge/tests/workspace-from-issue.test.ts index cf21336..c365a1d 100644 --- a/packages/fleet-bridge/tests/workspace-from-issue.test.ts +++ b/packages/fleet-bridge/tests/workspace-from-issue.test.ts @@ -16,10 +16,19 @@ import { Store } from "../src/store/store"; import { ProviderError, type Issue, type RepoProvider } from "../src/providers"; import { FakeSocket, makeDeps, type FakeShip } from "./helpers"; +function deferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; +} + /** Records what the fake provider was asked to do. */ interface Recorder { getIssueNumber?: number; linkBranch?: { issueNumber: number; branch: string }; + linkCalls: number; } const issue: Issue = { @@ -44,6 +53,8 @@ describe("POST /workspaces from an issue", () => { let linkFailsWith: ProviderError | undefined; /** Name the provider claims to have created; defaults to the requested one. */ let linkReturns: string | undefined; + /** When set, `linkBranchToIssue` reports arrival and blocks until released. */ + let linkGate: { entered: () => void; wait: Promise } | undefined; function makeProvider(_repo: Repo): RepoProvider { const unused = () => { @@ -65,10 +76,13 @@ describe("POST /workspaces from an issue", () => { }, async linkBranchToIssue(issueNumber: number, branch: string) { recorder.linkBranch = { issueNumber, branch }; + recorder.linkCalls += 1; + linkGate?.entered(); + await linkGate?.wait; if (linkFailsWith) throw linkFailsWith; return { name: linkReturns ?? branch, sha: "sha-of-linked-branch" }; }, - } as RepoProvider; + }; } async function call(method: string, path: string, body?: unknown) { @@ -91,9 +105,10 @@ describe("POST /workspaces from an issue", () => { beforeEach(async () => { dir = await mkdtemp(join(tmpdir(), "fleet-bridge-issue-ws-")); FakeSocket.byBase.clear(); - recorder = {}; + recorder = { linkCalls: 0 }; linkFailsWith = undefined; linkReturns = undefined; + linkGate = undefined; ships = new Map([["http://ship-a", { name: "ship-a", workspaces: [] }]]); const config = { dataDirectory: dir, port: 4902, name: "bridge" }; const store = new Store(dir); @@ -162,7 +177,7 @@ describe("POST /workspaces from an issue", () => { expect(res.status).toBe(201); expect(res.body.branch).toBe("main"); - expect(recorder).toEqual({}); + expect(recorder).toEqual({ linkCalls: 0 }); }); test.each([ @@ -171,6 +186,7 @@ describe("POST /workspaces from an issue", () => { ["a blank branch", { branch: " " }], ["a non-integer issue number", { issueNumber: 1.5 }], ["a zero issue number", { issueNumber: 0 }], + ["an issue number past the safe integer range", { issueNumber: 1e21 }], ])("rejects %s with 400", async (_label, extra) => { const res = await call("POST", "/workspaces", { ship: "ship-a", @@ -180,7 +196,7 @@ describe("POST /workspaces from an issue", () => { }); expect(res.status).toBe(400); - expect(recorder).toEqual({}); + expect(recorder).toEqual({ linkCalls: 0 }); expect(ships.get("http://ship-a")!.createCalls).toBeUndefined(); }); @@ -209,4 +225,25 @@ describe("POST /workspaces from an issue", () => { expect(retry.status).toBe(201); expect(retry.body.branch).toBe("12-better-create-workspace-issue"); }); + + test("the reservation is taken before the provider call, so a concurrent create links once", async () => { + const entered = deferred(); + const release = deferred(); + linkGate = { entered: entered.resolve, wait: release.promise }; + const body = { ship: "ship-a", repoName: "repo1", name: "twelve", issueNumber: 12 }; + + const first = call("POST", "/workspaces", body); + await entered.promise; + const second = await call("POST", "/workspaces", body); + + expect(second.status).toBe(409); + expect(second.body.error).toContain("already in progress"); + // The second request must have been turned away before reaching the provider: + // two linked branches for one workspace is exactly what the ordering prevents. + expect(recorder.linkCalls).toBe(1); + + release.resolve(); + expect((await first).status).toBe(201); + expect(recorder.linkCalls).toBe(1); + }); }); diff --git a/packages/fleet-protocol/src/issue-branch.ts b/packages/fleet-protocol/src/issue-branch.ts index d16f851..6be75fa 100644 --- a/packages/fleet-protocol/src/issue-branch.ts +++ b/packages/fleet-protocol/src/issue-branch.ts @@ -23,11 +23,13 @@ const MAX_LENGTH = 60; * `-`; a title with no Latin alphanumerics at all yields just ``. * * The result is always a legal git branch name and a valid `FleetIdentifier`. - * Throws when `number` is not a positive integer — nothing downstream can make - * sense of a branch pointing at an issue that cannot exist. + * Throws when `number` is not a positive *safe* integer — nothing downstream can + * make sense of a branch pointing at an issue that cannot exist, and past + * `Number.MAX_SAFE_INTEGER` the number stringifies to exponential notation + * (`1e+21`), which is not the documented `-` shape at all. */ export function issueBranchName(issue: { number: number; title: string }): string { - if (!Number.isInteger(issue.number) || issue.number < 1) { + if (!Number.isSafeInteger(issue.number) || issue.number < 1) { throw new Error(`issue number must be a positive integer, got ${issue.number}`); } From a4f10b8b379c225bbfcacdadd1c896ec0f82d594 Mon Sep 17 00:00:00 2001 From: Jonathan Deiss Date: Mon, 27 Jul 2026 15:50:43 -0500 Subject: [PATCH 3/7] Pick a branch or an issue when creating a workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Client half of issue #12. The branch field was a bare text box; it is now a type-to-filter combobox over the repo's remote branches, and it says `On branch ` or `Creating new branch ` underneath so the outcome is visible before you commit to it. A "Create from issue" checkbox swaps it for a picker over the repo's open issues, previewing the branch name the bridge will derive and link. Both lists are allowed to be missing, and neither is on the critical path: a repo whose remote refuses to list branches falls back to exactly the text field this form used to be, and issues are not requested until the checkbox is ticked, since for a `custom` repo that request is a guaranteed 501. When the list is unknown the form says nothing about the branch rather than guessing, so it cannot claim "Creating new branch" for one that already exists. The fuzzy matcher is hand-rolled — the client carries no search dependency and this is the only place that wants one. It and the form's branch-state logic are pure and tested; the repo has no DOM harness, so interaction behaviour is not under test. Pre-review: the review panel runs against this commit. --- .../src/components/CreateWorkspaceModal.tsx | 193 +++++++++++++++- .../src/components/ui/checkbox.tsx | 40 ++++ .../src/components/ui/combobox.tsx | 200 ++++++++++++++++ .../fleet-client/src/data/FleetContext.tsx | 29 ++- packages/fleet-client/src/data/eden.ts | 20 +- packages/fleet-client/src/data/mock.ts | 133 ++++++++++- packages/fleet-client/src/data/provider.ts | 20 +- packages/fleet-client/src/data/types.ts | 19 ++ .../fleet-client/src/lib/create-workspace.ts | 59 +++++ packages/fleet-client/src/lib/fuzzy.ts | 112 +++++++++ .../tests/create-workspace.test.ts | 71 ++++++ packages/fleet-client/tests/fuzzy.test.ts | 115 ++++++++++ .../tests/workspace-mutations.test.ts | 215 +++++++++++++++++- 13 files changed, 1198 insertions(+), 28 deletions(-) create mode 100644 packages/fleet-client/src/components/ui/checkbox.tsx create mode 100644 packages/fleet-client/src/components/ui/combobox.tsx create mode 100644 packages/fleet-client/src/lib/create-workspace.ts create mode 100644 packages/fleet-client/src/lib/fuzzy.ts create mode 100644 packages/fleet-client/tests/create-workspace.test.ts create mode 100644 packages/fleet-client/tests/fuzzy.test.ts diff --git a/packages/fleet-client/src/components/CreateWorkspaceModal.tsx b/packages/fleet-client/src/components/CreateWorkspaceModal.tsx index 8f000b8..b97f383 100644 --- a/packages/fleet-client/src/components/CreateWorkspaceModal.tsx +++ b/packages/fleet-client/src/components/CreateWorkspaceModal.tsx @@ -1,9 +1,19 @@ -import { useState, type FormEvent } from "react"; +import { useEffect, useState, type FormEvent, type ReactNode } from "react"; import { useFleet } from "@/data/FleetContext"; +import type { RepoBranch, RepoIssue } from "@/data/types"; import { Modal } from "@/components/ui/modal"; import { Input } from "@/components/ui/input"; +import { Checkbox } from "@/components/ui/checkbox"; +import { Combobox, highlight } from "@/components/ui/combobox"; +import { cn } from "@/lib/utils"; +import { splitRanges } from "@/lib/fuzzy"; +import { branchState, issueBranchPreview, issueText } from "@/lib/create-workspace"; import { Field, ModalActions } from "@/routes/ReposRoute"; +// Module-level so the pickers' memoised filtering is not invalidated every render. +const branchName = (branch: RepoBranch) => branch.name; +const issueKey = (issue: RepoIssue) => String(issue.number); + interface Props { repoName: string; /** When set, the ship is fixed (Bridge cell entry); otherwise a dropdown is shown. */ @@ -11,22 +21,98 @@ interface Props { onClose: () => void; } +/** + * The create-workspace form. The branch can be picked two ways: by name, out of + * the repo's remote branches (or typed, which creates it), or by open issue, in + * which case the bridge derives and links the branch and the client only previews + * the name it will pick. + * + * Both lists are allowed to be missing — `GET /repos/:name/branches` needs to + * reach the remote and the issues route needs a provider and a token — so + * neither is on the critical path: a repo whose branches cannot be listed falls + * back to the plain text field this form used to be, and issues are not even + * requested until the checkbox is ticked, since for a `custom` repo that request + * is a guaranteed 501. + */ export function CreateWorkspaceModal({ repoName, ship, onClose }: Props) { - const { ships, createWorkspace } = useFleet(); + const { ships, createWorkspace, listRepoBranches, listRepoIssues } = useFleet(); const [name, setName] = useState(""); const [branch, setBranch] = useState("main"); const [selectedShip, setSelectedShip] = useState(ship ?? ships[0]?.name ?? ""); const [error, setError] = useState(null); const [pending, setPending] = useState(false); + // null while unknown — loading or failed. `branchState` reads it as "cannot say". + const [branches, setBranches] = useState(null); + const [branchesError, setBranchesError] = useState(null); + + const [fromIssue, setFromIssue] = useState(false); + const [issues, setIssues] = useState([]); + const [issuesLoaded, setIssuesLoaded] = useState(false); + const [issuesLoading, setIssuesLoading] = useState(false); + const [issuesError, setIssuesError] = useState(null); + const [issueQuery, setIssueQuery] = useState(""); + const [issue, setIssue] = useState(null); + const shipName = ship ?? selectedShip; + useEffect(() => { + let cancelled = false; + void (async () => { + try { + const loaded = await listRepoBranches(repoName); + if (!cancelled) setBranches(loaded); + } catch (e) { + if (!cancelled) setBranchesError((e as Error).message); + } + })(); + return () => { + cancelled = true; + }; + }, [listRepoBranches, repoName]); + + // Lazy on purpose: see the component's note about `custom` repos. A failed load + // leaves `issuesLoaded` false, so unticking and re-ticking retries. + useEffect(() => { + if (!fromIssue || issuesLoaded) return; + let cancelled = false; + setIssuesLoading(true); + setIssuesError(null); + void (async () => { + try { + const loaded = await listRepoIssues(repoName); + if (cancelled) return; + setIssues(loaded); + setIssuesLoaded(true); + } catch (e) { + if (!cancelled) setIssuesError((e as Error).message); + } finally { + if (!cancelled) setIssuesLoading(false); + } + })(); + return () => { + cancelled = true; + }; + }, [fromIssue, issuesLoaded, listRepoIssues, repoName]); + + const state = branchState(branch, branches); + const ready = Boolean(name.trim()) && Boolean(shipName) && (fromIssue ? issue !== null : Boolean(branch.trim())); + const submit = async (e: FormEvent) => { e.preventDefault(); + // The Create button is disabled in this state, but implicit submission can + // still reach here — and issue mode without a selection has no payload to + // send, only a branch-mode one that would silently create the wrong thing. + if (!ready || pending) return; setPending(true); setError(null); try { - await createWorkspace({ ship: shipName, repoName, name: name.trim(), branch: branch.trim() }); + // Exactly one branch source goes on the wire: the bridge 400s on both keys. + await createWorkspace( + fromIssue && issue + ? { ship: shipName, repoName, name: name.trim(), issueNumber: issue.number } + : { ship: shipName, repoName, name: name.trim(), branch: branch.trim() }, + ); onClose(); } catch (err) { setError((err as Error).message); @@ -63,18 +149,101 @@ export function CreateWorkspaceModal({ repoName, ship, onClose }: Props) { setName(e.target.value)} placeholder="feature-x" autoFocus /> - - setBranch(e.target.value)} placeholder="main" /> - + + + Create from issue + + + {fromIssue ? ( + + { + setIssueQuery(value); + // Editing the text abandons the selection: what the field says and + // what would be submitted must never disagree. + setIssue(null); + }} + items={issues} + toText={issueText} + toKey={issueKey} + renderItem={(i, ranges) => } + onSelect={(i) => { + setIssue(i); + setIssueQuery(issueText(i)); + }} + placeholder="Search open issues" + disabled={issuesError !== null} + emptyMessage={issuesLoading ? "listing issues…" : "No open issue matches."} + /> + {issuesError ? ( + Issues could not be listed: {issuesError} + ) : issue ? ( + + ) : null} + + ) : ( + + setBranch(b.name)} + placeholder="main" + allowFreeText + /> + {state.kind === "existing" && On branch {state.branch}} + {state.kind === "new" && Creating new branch {state.branch}} + {branchesError && Branches could not be listed — type a branch name.} + {!branchesError && branches === null && listing branches…} + + )} {error &&

{error}

} - + ); } + +/** One line of small mono text under a field. */ +function Note({ children, className }: { children: ReactNode; className: string }) { + return {children}; +} + +function IssueRow({ issue, ranges }: { issue: RepoIssue; ranges: [number, number][] }) { + const text = issueText(issue); + const pivot = `#${issue.number}`.length; + const [numberRanges, titleRanges] = splitRanges(ranges, pivot); + return ( + + {highlight(text.slice(0, pivot), numberRanges)} + {highlight(text.slice(pivot), titleRanges)} + {issue.author && @{issue.author}} + + ); +} + +function SelectedIssue({ issue }: { issue: RepoIssue }) { + const preview = issueBranchPreview(issue); + return ( + + + {issueText(issue)} ↗ + + {preview && ( + + Creating new branch {preview}{" "} + (the provider may adjust the name) + + )} + + ); +} diff --git a/packages/fleet-client/src/components/ui/checkbox.tsx b/packages/fleet-client/src/components/ui/checkbox.tsx new file mode 100644 index 0000000..9a7d3ed --- /dev/null +++ b/packages/fleet-client/src/components/ui/checkbox.tsx @@ -0,0 +1,40 @@ +import { cn } from "@/lib/utils"; + +/** + * ui/checkbox.tsx — a real `` with a label, styled to the + * Bridge design language. Native rather than a composed widget: `accent-color` + * is enough to tint the box, and the browser's own control keeps keyboard and + * screen-reader behaviour for free. + */ +export function Checkbox({ + checked, + onChange, + disabled, + children, + className, +}: { + checked: boolean; + onChange: (checked: boolean) => void; + disabled?: boolean; + children: React.ReactNode; + className?: string; +}) { + return ( + + ); +} diff --git a/packages/fleet-client/src/components/ui/combobox.tsx b/packages/fleet-client/src/components/ui/combobox.tsx new file mode 100644 index 0000000..8baac13 --- /dev/null +++ b/packages/fleet-client/src/components/ui/combobox.tsx @@ -0,0 +1,200 @@ +import { useCallback, useEffect, useId, useMemo, useRef, useState, type KeyboardEvent, type ReactNode } from "react"; +import { cn } from "@/lib/utils"; +import { fuzzySearch } from "@/lib/fuzzy"; +import { Input } from "@/components/ui/input"; + +/** + * ui/combobox.tsx — a controlled "type to filter, pick from a list" input. + * + * Generic over the item because the create-workspace modal needs the same + * behaviour twice, over branches and over issues. Filtering is + * {@link fuzzySearch}, done once per render and handed to `renderItem` as the + * ranges to highlight, so the list rendering never re-derives the match. + * + * It lives inside a `
` and inside a `Modal`, both of which claim the keys a + * dropdown needs: Enter would submit the form and Escape would close the modal + * (whose listener is on `window`, so stopping the native event's propagation + * inside React's handler — React dispatches from the root container, below + * `window` — is what keeps it from firing). Both are intercepted here only while + * the list is on screen; otherwise the form and the modal keep their own keys. + * + * Deliberately not a full ARIA dialog/listbox widget: like `Modal` it stays + * proportionate to an app with no competing overlays. + */ + +interface ComboboxProps { + /** The text in the input. Free text is the user's, not a mirror of the selection. */ + value: string; + onValueChange: (value: string) => void; + items: T[]; + /** What to fuzzy-match against, and what the default row renders. */ + toText: (item: T) => string; + toKey: (item: T) => string; + renderItem?: (item: T, ranges: [number, number][]) => ReactNode; + onSelect: (item: T) => void; + placeholder?: string; + disabled?: boolean; + /** Shown in place of the list when nothing matches; suppressed under `allowFreeText`. */ + emptyMessage?: ReactNode; + /** + * Typing something no item matches is a legal value in its own right (a branch + * name that does not exist yet), so an empty result set closes the list quietly + * instead of reporting that nothing was found. + */ + allowFreeText?: boolean; +} + +export function Combobox({ + value, + onValueChange, + items, + toText, + toKey, + renderItem, + onSelect, + placeholder, + disabled, + emptyMessage, + allowFreeText, +}: ComboboxProps) { + const [open, setOpen] = useState(false); + const [active, setActive] = useState(0); + const listId = useId(); + const listRef = useRef(null); + + const matches = useMemo(() => fuzzySearch(items, value, toText), [items, value, toText]); + const activeIndex = matches.length === 0 ? -1 : Math.min(active, matches.length - 1); + const showList = open && (matches.length > 0 || (!allowFreeText && emptyMessage !== undefined)); + + useEffect(() => { + listRef.current?.querySelector('[data-active="true"]')?.scrollIntoView({ block: "nearest" }); + }, [activeIndex, showList]); + + const select = useCallback( + (item: T) => { + setOpen(false); + onSelect(item); + }, + [onSelect], + ); + + const onKeyDown = (event: KeyboardEvent) => { + if (event.key === "Tab") { + setOpen(false); + return; + } + // Escape and Enter are only claimed while the list is actually on screen, so + // that a key with nothing to act on still reaches the modal and the form. + if (event.key === "Escape") { + if (!showList) return; + event.preventDefault(); + event.stopPropagation(); + setOpen(false); + return; + } + if (event.key === "ArrowDown" || event.key === "ArrowUp") { + event.preventDefault(); + if (!open) { + setOpen(true); + return; + } + if (matches.length === 0) return; + const step = event.key === "ArrowDown" ? 1 : -1; + setActive((current) => { + const from = Math.min(current, matches.length - 1); + return (from + step + matches.length) % matches.length; + }); + return; + } + if (event.key === "Enter" && showList) { + event.preventDefault(); + const item = matches[activeIndex]; + if (item) select(item.item); + else setOpen(false); + } + }; + + return ( +
+ = 0 ? `${listId}-${activeIndex}` : undefined} + autoComplete="off" + value={value} + placeholder={placeholder} + disabled={disabled} + onChange={(event) => { + onValueChange(event.target.value); + setActive(0); + setOpen(true); + }} + onFocus={() => setOpen(true)} + onBlur={() => setOpen(false)} + onKeyDown={onKeyDown} + // Mono only: the size stays whatever `Input` sets, so the field lines up + // with the plain inputs above it. + className="font-mono" + /> + + {showList && ( +
    + {matches.length === 0 ? ( +
  • + {emptyMessage} +
  • + ) : ( + matches.map((match, index) => ( +
  • { + event.preventDefault(); + select(match.item); + }} + onMouseEnter={() => setActive(index)} + className={cn( + "cursor-pointer px-3 py-[5px] font-mono text-[11.5px] text-dim", + index === activeIndex && "bg-panel2 text-text", + )} + > + {renderItem ? renderItem(match.item, match.ranges) : highlight(toText(match.item), match.ranges)} +
  • + )) + )} +
+ )} +
+ ); +} + +/** Text with the matched ranges emphasised — the default row, and reusable in a custom one. */ +export function highlight(text: string, ranges: [number, number][]): ReactNode { + if (ranges.length === 0) return text; + const parts: ReactNode[] = []; + let at = 0; + for (const [start, end] of ranges) { + if (start > at) parts.push(text.slice(at, start)); + parts.push( + + {text.slice(start, end)} + , + ); + at = end; + } + if (at < text.length) parts.push(text.slice(at)); + return parts; +} diff --git a/packages/fleet-client/src/data/FleetContext.tsx b/packages/fleet-client/src/data/FleetContext.tsx index f759937..f3bd8cb 100644 --- a/packages/fleet-client/src/data/FleetContext.tsx +++ b/packages/fleet-client/src/data/FleetContext.tsx @@ -7,6 +7,8 @@ import type { ArmoryManifest, ArmoryShipState, Repo, + RepoBranch, + RepoIssue, Ship, Workspace, WorkspaceDetail, @@ -33,8 +35,22 @@ interface FleetValue { getWorkspaceDiff: (repo: string, name: string, query: DiffQuery) => Promise; /** Branches and recent commits a workspace's diff can be taken against. */ getWorkspaceRefs: (repo: string, name: string) => Promise; - /** Create a workspace, then refresh the workspace list. Rejects on failure. */ - createWorkspace: (input: { ship: string; repoName: string; name: string; branch: string }) => Promise; + /** + * Create a workspace, then refresh the workspace list. Rejects on failure. + * Exactly one of `branch` and `issueNumber` may be given — the bridge rejects + * both, and derives the branch itself from an issue. + */ + createWorkspace: (input: { + ship: string; + repoName: string; + name: string; + branch?: string; + issueNumber?: number; + }) => Promise; + /** The branches a repo's remote advertises. Fetched on demand by the create form. */ + listRepoBranches: (name: string) => Promise; + /** A repo's open issues, from its provider. Fetched on demand by the create form. */ + listRepoIssues: (name: string) => Promise; /** Register a repo, then refresh the repo list. Rejects on failure. */ createRepo: (input: { name: string; url: string; provider?: string }) => Promise; /** Remove a repo, then refresh the repo list. Rejects on failure. */ @@ -178,13 +194,18 @@ export function FleetProvider({ children }: { children: ReactNode }) { ); const createWorkspace = useCallback( - async (input: { ship: string; repoName: string; name: string; branch: string }) => { + async (input: { ship: string; repoName: string; name: string; branch?: string; issueNumber?: number }) => { await bridge.createWorkspace(input); await refresh(); }, [refresh], ); + // Branches and issues belong to the create form alone, so — like the armory — + // they stay out of the boot snapshot and are fetched when that form opens. + const listRepoBranches = useCallback((name: string) => bridge.listRepoBranches(name), []); + const listRepoIssues = useCallback((name: string) => bridge.listRepoIssues(name), []); + // Like the repo/ship mutations, these rethrow so the driving modal can show the // failure inline rather than swallowing it into the global banner. const switchBranch = useCallback( @@ -222,6 +243,8 @@ export function FleetProvider({ children }: { children: ReactNode }) { createShip, deleteShip, createWorkspace, + listRepoBranches, + listRepoIssues, getArmory, getArmoryFile, listArmoryShips, diff --git a/packages/fleet-client/src/data/eden.ts b/packages/fleet-client/src/data/eden.ts index d1050c6..aeb314e 100644 --- a/packages/fleet-client/src/data/eden.ts +++ b/packages/fleet-client/src/data/eden.ts @@ -7,6 +7,8 @@ import type { ArmoryManifest, ArmoryShipState, Repo, + RepoBranch, + RepoIssue, Ship, Workspace, WorkspaceDetail, @@ -101,6 +103,21 @@ export class EdenFleetBridge implements FleetBridge { if (error) throw edenError(error); } + async listRepoBranches(name: string): Promise { + const { data, error } = await this.client.repos({ name }).branches.get(); + if (error) throw edenError(error); + // The handler can also surface an in-band `{ error }` body on a 200. + if (!Array.isArray(data)) throw edenError({ value: data }); + return data; + } + + async listRepoIssues(name: string): Promise { + const { data, error } = await this.client.repos({ name }).issues.get({ query: { state: "open" } }); + if (error) throw edenError(error); + if (!Array.isArray(data)) throw edenError({ value: data }); + return data; + } + async createShip(url: string): Promise { const { data, error } = await this.client.ships.post({ url }); if (error) throw edenError(error); @@ -164,7 +181,8 @@ export class EdenFleetBridge implements FleetBridge { ship: string; repoName: string; name: string; - branch: string; + branch?: string; + issueNumber?: number; }): Promise { const { data, error } = await this.client.workspaces.post(input); if (error) throw edenError(error); diff --git a/packages/fleet-client/src/data/mock.ts b/packages/fleet-client/src/data/mock.ts index dbbbc1e..661ab00 100644 --- a/packages/fleet-client/src/data/mock.ts +++ b/packages/fleet-client/src/data/mock.ts @@ -1,4 +1,4 @@ -import type { AgentState, AgentStatus, WorkspaceDiff, WorkspaceRefs } from "fleet-protocol"; +import { issueBranchName, type AgentState, type AgentStatus, type WorkspaceDiff, type WorkspaceRefs } from "fleet-protocol"; import type { DiffQuery } from "@/lib/diff/diff-target"; import type { FleetBridge } from "./provider"; import type { @@ -8,6 +8,8 @@ import type { ArmoryShipState, ArmorySyncState, Repo, + RepoBranch, + RepoIssue, Ship, Workspace, WorkspaceDetail, @@ -122,6 +124,61 @@ const MOCK_BRANCHES: WorkspaceRefs["branches"] = [ { name: "origin/develop", remote: true }, ]; +/** + * What a repo's remote advertises, for the create-workspace branch picker. Kept + * apart from `MOCK_BRANCHES`, which answers a *workspace's* refs and so carries + * local/remote pairs rather than the `{ name, sha }` rows of the repo route. + */ +const MOCK_REPO_BRANCHES: RepoBranch[] = [ + { name: "develop", sha: "1f0a2b3c4d5e6f708192a3b4c5d6e7f809102132" }, + { name: "feat/oauth-pkce", sha: "2a1b3c4d5e6f708192a3b4c5d6e7f80910213243" }, + { name: "feat/redesign", sha: "3b2c4d5e6f708192a3b4c5d6e7f8091021324354" }, + { name: "fix/rate-limit", sha: "4c3d5e6f708192a3b4c5d6e7f80910213243546a" }, + { name: "hotfix/csp", sha: "5d4e6f708192a3b4c5d6e7f8091021324354657b" }, + { name: "main", sha: "6e5f708192a3b4c5d6e7f8091021324354657b8c" }, + { name: "release/2.3", sha: "7f60819a2b3c4d5e6f708192a3b4c5d6e7f80910" }, + { name: "spike/backfill", sha: "80719a2b3c4d5e6f708192a3b4c5d6e7f8091021" }, +]; + +/** + * Open issues for any provider-backed repo. The titles are deliberately varied — + * punctuation that the slug has to collapse, and one long enough to be truncated — + * so mock mode exercises the derived branch name the picker previews. + */ +const MOCK_ISSUES: RepoIssue[] = [ + { + number: 12, + title: "Better create workspace issue", + author: "firesquid", + url: "https://github.com/orchestra/repo/issues/12", + }, + { + number: 47, + title: "Rate limiter drops the first request after a restart", + author: "avery", + url: "https://github.com/orchestra/repo/issues/47", + }, + { + number: 103, + title: "Support OAuth 2.1 / PKCE (and drop the implicit flow)", + author: "kai", + url: "https://github.com/orchestra/repo/issues/103", + }, + { + number: 118, + title: + "Workspace terminal should reconnect automatically when the ship restarts instead of leaving a dead pane", + author: "morgan", + url: "https://github.com/orchestra/repo/issues/118", + }, + { + number: 204, + title: "Docs: document the armory dotfile map", + author: null, + url: "https://github.com/orchestra/repo/issues/204", + }, +]; + const MOCK_COMMITS: WorkspaceRefs["commits"] = [ { sha: "9f1c0a2b3d4e5f60718293a4b5c6d7e8f9012345", shortSha: "9f1c0a2", subject: "Add the /version route" }, { sha: "8e0b9a1c2d3e4f5061728394a5b6c7d8e9f01234", shortSha: "8e0b9a1", subject: "Return structured health output" }, @@ -335,17 +392,25 @@ const SEED_ARMORY_SHIP_STATES: Record = { }, }; -/** Seed the repo registry from the distinct repo names in the seed workspaces. */ +/** The one seed repo that is not provider-backed; see {@link seedRepos}. */ +const CUSTOM_REPO = "notifier"; + +/** + * Seed the repo registry from the distinct repo names in the seed workspaces. + * All but one are `github`, so the issue picker has something to show; the + * odd one out is a plain git remote, which is what makes the picker's + * "this provider cannot list issues" path reachable in mock mode. + */ function seedRepos(): Repo[] { const names: string[] = []; for (const w of SEED_WORKSPACES) { if (!names.includes(w.repoName)) names.push(w.repoName); } - return names.map((name) => ({ - name, - url: `git@github.com:orchestra/${name}.git`, - provider: "custom", - })); + return names.map((name) => + name === CUSTOM_REPO + ? { name, url: `git@git.internal:orchestra/${name}.git`, provider: "custom" } + : { name, url: `git@github.com:orchestra/${name}.git`, provider: "github" }, + ); } export class MockFleetBridge implements FleetBridge { @@ -365,6 +430,28 @@ export class MockFleetBridge implements FleetBridge { return w; } + private repo(name: string): Repo { + const repo = this.repos.find((r) => r.name === name); + if (!repo) throw new Error(`repo not found: ${name}`); + return repo; + } + + /** A repo whose forge can answer issue queries — the rest 501 on the bridge. */ + private providerRepo(name: string): Repo { + const repo = this.repo(name); + if (repo.provider.toLowerCase() !== "github") { + throw new Error(`provider "${repo.provider}" is not supported yet`); + } + return repo; + } + + private issue(repoName: string, number: number): RepoIssue { + this.providerRepo(repoName); + const issue = MOCK_ISSUES.find((i) => i.number === number); + if (!issue) throw new Error(`issue not found: #${number}`); + return issue; + } + async listShips(): Promise { return this.ships.map((s) => ({ ...s })); } @@ -388,6 +475,16 @@ export class MockFleetBridge implements FleetBridge { this.repos.splice(i, 1); } + async listRepoBranches(name: string): Promise { + this.repo(name); + return MOCK_REPO_BRANCHES.map((b) => ({ ...b })); + } + + async listRepoIssues(name: string): Promise { + this.providerRepo(name); + return MOCK_ISSUES.map((i) => ({ ...i })); + } + async createShip(url: string): Promise { // The real bridge learns the ship's name from its first sync; approximate // that here by deriving a name from the URL host. @@ -426,14 +523,32 @@ export class MockFleetBridge implements FleetBridge { ship: string; repoName: string; name: string; - branch: string; + branch?: string; + issueNumber?: number; }): Promise { + if (input.branch !== undefined && input.issueNumber !== undefined) { + throw new Error("a workspace is created from a branch or an issue, not both"); + } + if (input.branch === undefined && input.issueNumber === undefined) { + throw new Error("a workspace needs either a branch or an issue to start from"); + } if (!this.ships.some((s) => s.name === input.ship)) throw new Error(`unknown ship: ${input.ship}`); if (!this.repos.some((r) => r.name === input.repoName)) throw new Error(`unknown repo: ${input.repoName}`); if (this.workspaces.some((w) => w.repoName === input.repoName && w.name === input.name)) { throw new Error(`workspace already exists: ${key(input.repoName, input.name)}`); } - const ws: Workspace = { ...input, active: false, agent: null }; + // The bridge derives the branch from the issue and links it on the provider + // before the ship ever sees the request; the workspace it returns is on that + // branch, so the mock has to do the same or issue mode looks like a no-op. + const branch = input.branch ?? issueBranchName(this.issue(input.repoName, input.issueNumber!)); + const ws: Workspace = { + ship: input.ship, + repoName: input.repoName, + name: input.name, + branch, + active: false, + agent: null, + }; this.workspaces.push(ws); this.emit({ type: "workspace.created", at: new Date().toISOString(), workspace: { ...ws } }); return { ...ws }; diff --git a/packages/fleet-client/src/data/provider.ts b/packages/fleet-client/src/data/provider.ts index e628c3d..4e5c0f8 100644 --- a/packages/fleet-client/src/data/provider.ts +++ b/packages/fleet-client/src/data/provider.ts @@ -5,6 +5,8 @@ import type { ArmoryManifest, ArmoryShipState, Repo, + RepoBranch, + RepoIssue, Ship, Workspace, WorkspaceDetail, @@ -35,6 +37,10 @@ export interface FleetBridge { createRepo(input: { name: string; url: string; provider?: string }): Promise; /** `DELETE /repos/:name` — remove a registered repo. */ deleteRepo(name: string): Promise; + /** `GET /repos/:name/branches` — the branches the repo's remote advertises. */ + listRepoBranches(name: string): Promise; + /** `GET /repos/:name/issues?state=open` — the repo's open issues. */ + listRepoIssues(name: string): Promise; /** `POST /ships` — register a ship by URL; the bridge discovers its name. */ createShip(url: string): Promise; /** `DELETE /ships/:name` — deregister a ship. */ @@ -43,8 +49,18 @@ export interface FleetBridge { listWorkspaces(): Promise; /** `WS /events` — live workspace snapshots and changes. */ subscribeWorkspaces(listener: (event: WorkspaceEvent) => void, onError?: (error: Error) => void): () => void; - /** `POST /workspaces` — create a workspace on a given ship for a repo. */ - createWorkspace(input: { ship: string; repoName: string; name: string; branch: string }): Promise; + /** + * `POST /workspaces` — create a workspace on a given ship for a repo. The + * branch is either named outright or derived by the bridge from an issue; + * exactly one of `branch` and `issueNumber` may be sent. + */ + createWorkspace(input: { + ship: string; + repoName: string; + name: string; + branch?: string; + issueNumber?: number; + }): Promise; /** `GET /workspaces/:repo/:name` — detailed status (diff, ship, …). */ getWorkspace(repo: string, name: string): Promise; /** `GET /workspaces/:repo/:name/diff` — raw `git diff` text for a {@link DiffQuery}. */ diff --git a/packages/fleet-client/src/data/types.ts b/packages/fleet-client/src/data/types.ts index 9161710..84f0627 100644 --- a/packages/fleet-client/src/data/types.ts +++ b/packages/fleet-client/src/data/types.ts @@ -49,6 +49,25 @@ export type WorkspaceEvent = readonly workspace: Workspace; }; +/** A branch a repo's remote advertises — a row of `GET /repos/:name/branches`. */ +export interface RepoBranch { + readonly name: string; + readonly sha: string; +} + +/** + * An issue on a repo's provider — a row of `GET /repos/:name/issues`. The bridge's + * `IssueSummary` carries more than this (`state`, `createdAt`, `updatedAt`); only + * the fields the picker renders are mirrored, so the shape stays honest about what + * the UI depends on. + */ +export interface RepoIssue { + readonly number: number; + readonly title: string; + readonly author: string | null; + readonly url: string; +} + /** Detail: `WorkspaceStatus` with `ship` guaranteed on both variants. */ export type WorkspaceDetail = WorkspaceStatus & { readonly ship: string }; diff --git a/packages/fleet-client/src/lib/create-workspace.ts b/packages/fleet-client/src/lib/create-workspace.ts new file mode 100644 index 0000000..53072b1 --- /dev/null +++ b/packages/fleet-client/src/lib/create-workspace.ts @@ -0,0 +1,59 @@ +/** + * lib/create-workspace.ts — the decisions behind the create-workspace form. + * + * The form's one piece of real logic is what to tell the user about the branch + * they typed, and that depends on a list that is allowed to be missing. It lives + * here so it can be tested without a DOM, and so the component is left with + * rendering. + */ + +import { issueBranchName } from "fleet-protocol"; +import type { RepoBranch, RepoIssue } from "@/data/types"; + +/** + * What the form knows about the branch the user typed. + * + * `unknown` is the branch list being unavailable — still loading, or the remote + * refused to list it. It is deliberately distinct from `new`: without the list + * there is no way to tell an existing branch from one about to be created, and + * guessing wrong in either direction is worse than saying nothing. + */ +export type BranchState = + | { readonly kind: "empty" } + | { readonly kind: "unknown" } + | { readonly kind: "existing"; readonly branch: string } + | { readonly kind: "new"; readonly branch: string }; + +/** + * Classify the typed branch against the repo's branches, `null` when that list + * could not be loaded. The comparison is case-sensitive on the trimmed input, + * because git refs are: `Main` and `main` are two different branches. + */ +export function branchState(input: string, branches: RepoBranch[] | null): BranchState { + const branch = input.trim(); + if (branch.length === 0) return { kind: "empty" }; + if (branches === null) return { kind: "unknown" }; + return branches.some((b) => b.name === branch) ? { kind: "existing", branch } : { kind: "new", branch }; +} + +/** + * The text an issue is matched and displayed as. The number is part of it so + * that typing `12` and typing `workspace` both find issue #12. + */ +export function issueText(issue: Pick): string { + return `#${issue.number} ${issue.title}`; +} + +/** + * The branch the bridge will derive for an issue, or null if it cannot be + * derived. `issueBranchName` throws on an issue number the convention cannot + * express; that is a preview, not the create request, so it must degrade to + * showing nothing rather than take the modal down with it. + */ +export function issueBranchPreview(issue: Pick): string | null { + try { + return issueBranchName(issue); + } catch { + return null; + } +} diff --git a/packages/fleet-client/src/lib/fuzzy.ts b/packages/fleet-client/src/lib/fuzzy.ts new file mode 100644 index 0000000..7960723 --- /dev/null +++ b/packages/fleet-client/src/lib/fuzzy.ts @@ -0,0 +1,112 @@ +/** + * lib/fuzzy.ts — the subsequence matcher behind every type-to-filter list. + * + * Hand-rolled because the client carries no search dependency and this is the + * only place that needs one. The matcher is deliberately greedy — each query + * character takes the first position that can still hold it — rather than the + * dynamic-programming search fzf runs. Greedy can score an alignment lower than + * the best one that exists (`ab` against `a-b ab` matches the scattered pair), + * but the lists it ranks here are a repo's branches and its open issues: tens of + * short strings, where the cost of being occasionally one rank off is far below + * the cost of the machinery that avoids it. + */ + +/** One item that matched, with the ranges to highlight and its ranking score. */ +export interface FuzzyMatch { + readonly item: T; + readonly score: number; + /** `[start, end)` index pairs over the *original* text, adjacent runs merged. */ + readonly ranges: [number, number][]; +} + +/** Characters after which a match reads as the start of a word. */ +const SEPARATORS = new Set(["-", "_", "/", ".", " "]); + +const MATCH_SCORE = 1; +/** Paid when a match directly follows the previous one — the strongest signal. */ +const CONTIGUOUS_BONUS = 8; +const START_BONUS = 12; +const SEPARATOR_BONUS = 6; +const GAP_PENALTY = 1; +/** Beyond this, extra distance says nothing more than "far away". */ +const MAX_GAP = 10; +/** Per haystack character, so a short name outranks a long one that matches as well. */ +const LENGTH_PENALTY = 0.01; + +/** + * Rank `items` against `query` by subsequence match, best first. Items that do + * not contain the query as a subsequence are dropped; an empty query returns + * every item in input order with no ranges. + * + * Ties keep their input order — `Array.prototype.sort` is stable — so a list + * already in a meaningful order (branches sorted by name) stays that way. + */ +export function fuzzySearch(items: T[], query: string, toText: (item: T) => string): FuzzyMatch[] { + if (query.length === 0) return items.map((item) => ({ item, score: 0, ranges: [] })); + + const needle = query.toLowerCase(); + const matches: FuzzyMatch[] = []; + for (const item of items) { + const scored = scoreText(toText(item), needle); + if (scored) matches.push({ item, ...scored }); + } + return matches.sort((a, b) => b.score - a.score); +} + +/** Score one haystack against an already-lowercased needle; null when it does not match. */ +function scoreText(text: string, needle: string): { score: number; ranges: [number, number][] } | null { + // Indices are taken on the lowercased copy but reported against `text`, which + // holds because case folding is length-preserving for the ASCII-ish branch + // names and issue titles these lists carry. + const haystack = text.toLowerCase(); + const indices: number[] = []; + let score = -text.length * LENGTH_PENALTY; + let from = 0; + let previous = -1; + + for (const character of needle) { + const at = haystack.indexOf(character, from); + if (at === -1) return null; + + score += MATCH_SCORE; + if (previous !== -1 && at === previous + 1) score += CONTIGUOUS_BONUS; + else score -= Math.min(at - from, MAX_GAP) * GAP_PENALTY; + if (at === 0) score += START_BONUS; + else if (SEPARATORS.has(haystack[at - 1]!)) score += SEPARATOR_BONUS; + + indices.push(at); + previous = at; + from = at + character.length; + } + + return { score, ranges: mergeRanges(indices) }; +} + +/** + * Cut `ranges` at index `at`, rebasing the right-hand side to 0 — for a row that + * renders one matched string as two differently styled pieces. A range straddling + * the cut is split across both sides. + */ +export function splitRanges( + ranges: [number, number][], + at: number, +): [[number, number][], [number, number][]] { + const left: [number, number][] = []; + const right: [number, number][] = []; + for (const [start, end] of ranges) { + if (start < at) left.push([start, Math.min(end, at)]); + if (end > at) right.push([Math.max(start, at) - at, end - at]); + } + return [left, right]; +} + +/** Collapse ascending match indices into `[start, end)` runs. */ +function mergeRanges(indices: number[]): [number, number][] { + const ranges: [number, number][] = []; + for (const index of indices) { + const last = ranges[ranges.length - 1]; + if (last && last[1] === index) last[1] = index + 1; + else ranges.push([index, index + 1]); + } + return ranges; +} diff --git a/packages/fleet-client/tests/create-workspace.test.ts b/packages/fleet-client/tests/create-workspace.test.ts new file mode 100644 index 0000000..abe2415 --- /dev/null +++ b/packages/fleet-client/tests/create-workspace.test.ts @@ -0,0 +1,71 @@ +/** + * create-workspace.test.ts — the decisions the create-workspace form makes about + * the branch the user typed. There is no DOM harness in this package, so this is + * where the form's logic is held to account; the component around it only renders + * what these return. + */ + +import { describe, expect, test } from "bun:test"; +import { branchState, issueBranchPreview, issueText } from "../src/lib/create-workspace"; +import type { RepoBranch } from "../src/data/types"; + +const BRANCHES: RepoBranch[] = [ + { name: "main", sha: "a".repeat(40) }, + { name: "feat/oauth-pkce", sha: "b".repeat(40) }, +]; + +describe("branchState", () => { + test("an exact name is an existing branch", () => { + expect(branchState("main", BRANCHES)).toEqual({ kind: "existing", branch: "main" }); + expect(branchState("feat/oauth-pkce", BRANCHES)).toEqual({ kind: "existing", branch: "feat/oauth-pkce" }); + }); + + test("surrounding whitespace is trimmed before the comparison", () => { + expect(branchState(" main ", BRANCHES)).toEqual({ kind: "existing", branch: "main" }); + }); + + test("a name differing only in case is a new branch, because git refs are case-sensitive", () => { + expect(branchState("Main", BRANCHES)).toEqual({ kind: "new", branch: "Main" }); + expect(branchState("MAIN", BRANCHES)).toEqual({ kind: "new", branch: "MAIN" }); + }); + + test("an unlisted name is a new branch", () => { + expect(branchState("feat/new-thing", BRANCHES)).toEqual({ kind: "new", branch: "feat/new-thing" }); + }); + + test("empty and whitespace-only input say nothing", () => { + expect(branchState("", BRANCHES)).toEqual({ kind: "empty" }); + expect(branchState(" ", BRANCHES)).toEqual({ kind: "empty" }); + expect(branchState("", null)).toEqual({ kind: "empty" }); + }); + + test("without a branch list the state is unknown, never a guess", () => { + expect(branchState("main", null)).toEqual({ kind: "unknown" }); + expect(branchState("anything", null)).toEqual({ kind: "unknown" }); + }); + + test("an empty branch list is knowledge: every name is new", () => { + expect(branchState("main", [])).toEqual({ kind: "new", branch: "main" }); + }); +}); + +describe("issueText", () => { + test("carries the number so it can be matched as well as the title", () => { + expect(issueText({ number: 12, title: "Better create workspace issue" })).toBe( + "#12 Better create workspace issue", + ); + }); +}); + +describe("issueBranchPreview", () => { + test("previews the name the bridge derives", () => { + expect(issueBranchPreview({ number: 12, title: "Better create workspace issue" })).toBe( + "12-better-create-workspace-issue", + ); + }); + + test("a number the convention cannot express previews as nothing instead of throwing", () => { + expect(issueBranchPreview({ number: 0, title: "Impossible" })).toBeNull(); + expect(issueBranchPreview({ number: 1.5, title: "Impossible" })).toBeNull(); + }); +}); diff --git a/packages/fleet-client/tests/fuzzy.test.ts b/packages/fleet-client/tests/fuzzy.test.ts new file mode 100644 index 0000000..6ae11b7 --- /dev/null +++ b/packages/fleet-client/tests/fuzzy.test.ts @@ -0,0 +1,115 @@ +/** + * fuzzy.test.ts — the subsequence matcher behind the branch and issue pickers. + * + * Scores are never asserted absolutely, only as orderings: the constants are an + * implementation detail, but "a prefix beats a scattered match" is the contract + * the pickers are built on. Ranges, which drive the highlighting, are asserted + * exactly. + */ + +import { describe, expect, test } from "bun:test"; +import { fuzzySearch, splitRanges } from "../src/lib/fuzzy"; + +const ranked = (items: string[], query: string): string[] => + fuzzySearch(items, query, (item) => item).map((match) => match.item); + +describe("fuzzySearch", () => { + test("an empty query returns every item, in order, unmatched", () => { + const items = ["main", "develop", "feat/oauth-pkce"]; + + const matches = fuzzySearch(items, "", (item) => item); + + expect(matches.map((m) => m.item)).toEqual(items); + expect(matches.every((m) => m.ranges.length === 0)).toBe(true); + expect(matches.every((m) => m.score === 0)).toBe(true); + }); + + test("a query no item contains as a subsequence returns nothing", () => { + expect(ranked(["main", "develop"], "zzz")).toEqual([]); + // Right characters, wrong order — a subsequence is ordered. + expect(ranked(["main"], "nima")).toEqual([]); + }); + + test("matches a scattered subsequence, not just a substring", () => { + expect(ranked(["feat/oauth-pkce", "main"], "fpk")).toEqual(["feat/oauth-pkce"]); + }); + + test("matching is case-insensitive in both directions", () => { + expect(ranked(["main"], "MAIN")).toEqual(["main"]); + expect(ranked(["MAIN"], "main")).toEqual(["MAIN"]); + expect(fuzzySearch(["Release/2.3"], "r2", (i) => i)[0]?.ranges).toEqual([ + [0, 1], + [8, 9], + ]); + }); + + test("ranges are half-open indices into the original text, adjacent ones merged", () => { + const [match] = fuzzySearch(["rate-limit"], "rali", (item) => item); + + expect(match?.ranges).toEqual([ + [0, 2], + [5, 7], + ]); + }); + + test("a fully contiguous match is one range covering the query", () => { + const [match] = fuzzySearch(["feat/oauth-pkce"], "feat", (item) => item); + + expect(match?.ranges).toEqual([[0, 4]]); + }); + + test("a match at the start outranks the same match in the middle", () => { + expect(ranked(["domain", "main"], "main")).toEqual(["main", "domain"]); + }); + + test("a contiguous match outranks a gappy one", () => { + expect(ranked(["f-e-a-t", "feat"], "feat")).toEqual(["feat", "f-e-a-t"]); + }); + + test("a match after a separator outranks a closer one mid-word", () => { + expect(ranked(["firebrand", "fix-rate"], "fr")).toEqual(["fix-rate", "firebrand"]); + }); + + test("the shorter of two equally good matches wins", () => { + expect(ranked(["main-branch", "main"], "main")).toEqual(["main", "main-branch"]); + }); + + test("ties keep the input order", () => { + expect(ranked(["alpha-one", "alpha-two"], "alpha")).toEqual(["alpha-one", "alpha-two"]); + expect(ranked(["alpha-two", "alpha-one"], "alpha")).toEqual(["alpha-two", "alpha-one"]); + }); + + test("matches over the projected text, not the item itself", () => { + const issues = [ + { number: 12, title: "Better create workspace issue" }, + { number: 47, title: "Rate limiter drops requests" }, + ]; + const text = (issue: { number: number; title: string }) => `#${issue.number} ${issue.title}`; + + expect(fuzzySearch(issues, "12", text).map((m) => m.item.number)).toEqual([12]); + expect(fuzzySearch(issues, "workspace", text).map((m) => m.item.number)).toEqual([12]); + }); +}); + +describe("splitRanges", () => { + test("sends each range to its side and rebases the right one", () => { + expect( + splitRanges( + [ + [0, 2], + [5, 7], + ], + 3, + ), + ).toEqual([[[0, 2]], [[2, 4]]]); + }); + + test("a range straddling the cut appears on both sides", () => { + expect(splitRanges([[1, 5]], 3)).toEqual([[[1, 3]], [[0, 2]]]); + }); + + test("a cut past the end or at zero leaves one side empty", () => { + expect(splitRanges([[1, 3]], 10)).toEqual([[[1, 3]], []]); + expect(splitRanges([[1, 3]], 0)).toEqual([[], [[1, 3]]]); + }); +}); diff --git a/packages/fleet-client/tests/workspace-mutations.test.ts b/packages/fleet-client/tests/workspace-mutations.test.ts index ee02a27..aada803 100644 --- a/packages/fleet-client/tests/workspace-mutations.test.ts +++ b/packages/fleet-client/tests/workspace-mutations.test.ts @@ -1,8 +1,9 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { issueBranchName } from "fleet-protocol"; import { EdenFleetBridge } from "../src/data/eden"; import { makeBridgeClient } from "../src/data/client"; import { MockFleetBridge } from "../src/data/mock"; -import type { WorkspaceEvent } from "../src/data/types"; +import type { RepoBranch, RepoIssue, WorkspaceEvent } from "../src/data/types"; describe("EdenFleetBridge workspace mutations hit the right routes", () => { let server: ReturnType; @@ -85,3 +86,215 @@ describe("MockFleetBridge workspace mutations", () => { await expect(mock.deleteWorkspace("nope", "nope")).rejects.toThrow("workspace not found"); }); }); + +/** + * The create-workspace form's own surface: the two lists it fills its pickers + * from, and the create itself. The bridge 400s a create that names both a branch + * and an issue, so *which keys are present* in the body is a contract rather than + * a detail — hence the exact-body assertions below. + */ + +const BRANCHES: RepoBranch[] = [ + { name: "develop", sha: "a".repeat(40) }, + { name: "main", sha: "b".repeat(40) }, +]; + +const ISSUES: RepoIssue[] = [ + { number: 12, title: "Better create workspace issue", author: "firesquid", url: "https://example.test/12" }, +]; + +const CREATED = { + repoName: "api-gateway", + name: "ws-1", + branch: "main", + active: false, + agent: null, + ship: "forge-01", +}; + +describe("EdenFleetBridge create-workspace reads and writes", () => { + let server: ReturnType; + let requests: { method: string; path: string; query: Record; body: unknown }[]; + let bridge: EdenFleetBridge; + /** Overrides the canned body for a path, to drive the failure cases. */ + let override: Map; + + beforeEach(() => { + requests = []; + override = new Map(); + server = Bun.serve({ + port: 0, + async fetch(request) { + const url = new URL(request.url); + requests.push({ + method: request.method, + path: url.pathname, + query: Object.fromEntries(url.searchParams), + body: request.method === "POST" ? await request.json() : undefined, + }); + if (override.has(url.pathname)) return Response.json(override.get(url.pathname)); + if (url.pathname === "/repos/api-gateway/branches") return Response.json(BRANCHES); + if (url.pathname === "/repos/api-gateway/issues") return Response.json(ISSUES); + if (url.pathname === "/workspaces") return Response.json(CREATED, { status: 201 }); + return Response.json({ error: "unexpected route" }, { status: 404 }); + }, + }); + bridge = new EdenFleetBridge(makeBridgeClient(`http://localhost:${server.port}`)); + }); + + afterEach(() => server.stop(true)); + + test("listRepoBranches GETs /repos/:name/branches", async () => { + expect(await bridge.listRepoBranches("api-gateway")).toEqual(BRANCHES); + expect(requests).toEqual([ + { method: "GET", path: "/repos/api-gateway/branches", query: {}, body: undefined }, + ]); + }); + + test("listRepoIssues GETs /repos/:name/issues filtered to the open ones", async () => { + expect(await bridge.listRepoIssues("api-gateway")).toEqual(ISSUES); + expect(requests).toEqual([ + { method: "GET", path: "/repos/api-gateway/issues", query: { state: "open" }, body: undefined }, + ]); + }); + + test("a branch-mode create sends branch and no issueNumber", async () => { + await bridge.createWorkspace({ ship: "forge-01", repoName: "api-gateway", name: "ws-1", branch: "main" }); + + expect(requests[0]).toEqual({ + method: "POST", + path: "/workspaces", + query: {}, + body: { ship: "forge-01", repoName: "api-gateway", name: "ws-1", branch: "main" }, + }); + }); + + test("an issue-mode create sends issueNumber and no branch key at all", async () => { + await bridge.createWorkspace({ ship: "forge-01", repoName: "api-gateway", name: "ws-1", issueNumber: 12 }); + + expect(requests[0]!.body).toEqual({ + ship: "forge-01", + repoName: "api-gateway", + name: "ws-1", + issueNumber: 12, + }); + expect(Object.keys(requests[0]!.body as object)).not.toContain("branch"); + }); + + test("a branch left undefined never reaches the wire", async () => { + await bridge.createWorkspace({ + ship: "forge-01", + repoName: "api-gateway", + name: "ws-1", + branch: undefined, + issueNumber: 12, + }); + + expect(Object.keys(requests[0]!.body as object)).not.toContain("branch"); + }); + + test("an in-band { error } body on a 200 throws rather than returning it", async () => { + override.set("/repos/api-gateway/branches", { error: "could not list branches for repo" }); + override.set("/repos/api-gateway/issues", { error: "provider is not supported yet" }); + + await expect(bridge.listRepoBranches("api-gateway")).rejects.toThrow("fleet-bridge request failed"); + await expect(bridge.listRepoIssues("api-gateway")).rejects.toThrow("fleet-bridge request failed"); + }); + + test("an error status is surfaced as a rejection", async () => { + await expect(bridge.listRepoBranches("missing")).rejects.toThrow("fleet-bridge request failed"); + }); +}); + +describe("MockFleetBridge create-workspace surface", () => { + /** A seeded repo whose provider can answer issue queries. */ + const REPO = "api-gateway"; + + test("listRepoBranches answers for a registered repo and rejects an unknown one", async () => { + const mock = new MockFleetBridge(); + + const names = (await mock.listRepoBranches(REPO)).map((b) => b.name); + expect(names).toContain("main"); + // Sorted by name, the way the bridge's `ls-remote` listing is. + expect(names).toEqual([...names].sort()); + await expect(mock.listRepoBranches("nope")).rejects.toThrow("repo not found"); + }); + + test("listRepoIssues answers for a provider-backed repo and refuses a custom one", async () => { + const mock = new MockFleetBridge(); + + expect((await mock.listRepoIssues(REPO)).length).toBeGreaterThan(0); + await expect(mock.listRepoIssues("notifier")).rejects.toThrow("is not supported yet"); + }); + + test("a create from an issue lands on the branch that issue derives", async () => { + const mock = new MockFleetBridge(); + const issue = (await mock.listRepoIssues(REPO))[0]!; + + const workspace = await mock.createWorkspace({ + ship: "forge-01", + repoName: REPO, + name: "ws-from-issue", + issueNumber: issue.number, + }); + + expect(workspace.branch).toBe(issueBranchName(issue)); + expect(await mock.listWorkspaces()).toContainEqual(workspace); + }); + + test("the issue fixture exercises punctuation and the 60-character cap", async () => { + const issues = await new MockFleetBridge().listRepoIssues(REPO); + const names = issues.map((issue) => issueBranchName(issue)); + + expect(names.every((name) => /^[0-9a-z-]+$/.test(name))).toBe(true); + expect(issues.some((issue) => /[^\w ]/.test(issue.title))).toBe(true); + expect(names.some((name) => name.length === 60)).toBe(true); + }); + + test("a create from an issue on a custom repo is refused, as on the bridge", async () => { + await expect( + new MockFleetBridge().createWorkspace({ + ship: "forge-01", + repoName: "notifier", + name: "ws-custom", + issueNumber: 12, + }), + ).rejects.toThrow("is not supported yet"); + }); + + test("a create naming both a branch and an issue is refused", async () => { + await expect( + new MockFleetBridge().createWorkspace({ + ship: "forge-01", + repoName: REPO, + name: "ws-both", + branch: "main", + issueNumber: 12, + }), + ).rejects.toThrow("not both"); + }); + + test("a create naming neither is refused", async () => { + await expect( + new MockFleetBridge().createWorkspace({ ship: "forge-01", repoName: REPO, name: "ws-neither" }), + ).rejects.toThrow("either a branch or an issue"); + }); + + test("a plain branch create still works and keeps the branch verbatim", async () => { + const workspace = await new MockFleetBridge().createWorkspace({ + ship: "forge-01", + repoName: REPO, + name: "ws-plain", + branch: "feature/x", + }); + + expect(workspace).toEqual({ + ship: "forge-01", + repoName: REPO, + name: "ws-plain", + branch: "feature/x", + active: false, + agent: null, + }); + }); +}); From f420884efe2e7d71df84f5159ccbb342bc442134 Mon Sep 17 00:00:00 2001 From: Jonathan Deiss Date: Mon, 27 Jul 2026 16:02:11 -0500 Subject: [PATCH 4/7] Resolve an issue branch on evidence, and let git give up on its own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round on the create-from-issue path. The idempotency fallback preferred any branch linked to the issue over one actually named as requested, so an issue linked to `42-retries` by an earlier `gh issue develop` would quietly put the workspace on that stale branch instead of the name the form previewed. The fallback is now exact-linked-match, then a ref of that name, then a 409 — a branch the caller never asked for is no longer a candidate at all. Which failure means "already there" is no longer guessed from an English substring of GitHub's error text, which a rewording or another locale would defeat. Anything short of 401/403/404 is now checked against what exists before it is called a failure, and a null `linkedBranch` is distinguished structurally from an unreadable payload, so a malformed reply can no longer discard a branch that was just created. `GIT_HTTP_LOW_SPEED_*` and ssh's `ConnectTimeout` make git abort a stalled probe by itself, which is what actually reaps the process — the outer timer never could, and 8 probes at a tarpit previously left 16 git processes alive. The outer bound moves to 20s so git's own diagnosis wins the race. Credential redaction now parses rather than pattern-matches, since a password may itself contain `@`. --- .../src/content/docs/reference/bridge-api.md | 21 +-- packages/fleet-bridge/src/fleet-manager.ts | 85 +++++++++--- packages/fleet-bridge/src/providers/github.ts | 117 ++++++++++------ .../fleet-bridge/src/providers/provider.ts | 8 +- packages/fleet-bridge/tests/providers.test.ts | 125 +++++++++++++++--- .../fleet-bridge/tests/repo-branches.test.ts | 27 +++- 6 files changed, 290 insertions(+), 93 deletions(-) diff --git a/apps/docs/src/content/docs/reference/bridge-api.md b/apps/docs/src/content/docs/reference/bridge-api.md index c4591a0..43281a7 100644 --- a/apps/docs/src/content/docs/reference/bridge-api.md +++ b/apps/docs/src/content/docs/reference/bridge-api.md @@ -232,8 +232,8 @@ Answered with `git ls-remote --heads` against the registered clone URL, **not** through the repo's provider: `provider` defaults to `"custom"`, for which no provider exists, so a provider-backed listing would be unavailable for most repos. `ls-remote` works against any git URL and needs no token. The probe runs -non-interactively (git never prompts for credentials or host keys) and is -abandoned after 15 s. +non-interactively — git never prompts for credentials or host keys, aborts an +http transfer that stalls for 15 s, and is given up on entirely after 20 s. `refs/heads/` is stripped from each name; tags and other refs are omitted, so a tag the ship would happily clone does not appear here. @@ -425,19 +425,22 @@ the ship: function a client can use to preview the name; 3. it asks the provider to create that branch and record it as the issue's linked development branch (GitHub's "Development → create a branch"); -4. the **name the provider returns** is what the ship is told to check out, which - may differ from the computed one if the provider de-duplicated it. +4. the **name the provider returns** is what the ship is told to check out. Use + it rather than the computed name — a provider may hand back a different ref. -Step 3 is idempotent: an issue that already has a linked branch, or a branch of -that name created by hand, resolves to the existing branch instead of failing. -The branch is created before the clone and is *not* removed if the clone then -fails — a retry reuses it. +Step 3 is safe to repeat. If the provider will not create the branch, the bridge +looks for one of the *same name* — first among the issue's linked branches, then +as a plain ref (someone may have pushed it by hand) — and uses that. A branch +linked to the issue under a **different** name is never substituted; that case +fails with `409`, since silently checking out a branch the caller never named is +worse than refusing. The branch is created before the clone and is not removed if +the clone then fails, so a retry reuses it. | Status | Cause | | --- | --- | | `422` | `ship`, `repoName` or `name` is missing. | | `400` | Invalid repo/workspace identifier; `unknown ship: `; `unknown repo: `; both `branch` and `issueNumber`, or neither; a blank `branch`; an `issueNumber` that is not a positive integer. | -| provider's status | Any error resolving or linking the issue is passed through with the provider's own status — e.g. `401` with no token, `404` for an unknown issue, `409` when the branch could be neither created nor found. | +| provider's status | Any error resolving or linking the issue is passed through with the provider's own status — e.g. `401` with no token, `403` for a token without repo write scope, `404` for an unknown issue, `409` when the branch could be neither created nor found under the requested name. | | `503` | `ship "" is offline`. | | `409` | `workspace already exists: /`; a create for that key is already in progress; the key's create outcome is indeterminate; the target ship was removed mid-request. | | `502` | The ship returned no data, an invalid summary, or a different workspace identity. | diff --git a/packages/fleet-bridge/src/fleet-manager.ts b/packages/fleet-bridge/src/fleet-manager.ts index 67855ca..647d336 100644 --- a/packages/fleet-bridge/src/fleet-manager.ts +++ b/packages/fleet-bridge/src/fleet-manager.ts @@ -86,32 +86,48 @@ export class BridgeError extends Error { /** How long to wait for a ship's first `sync` before treating it as offline. */ const SYNC_TIMEOUT_MS = 5000; -/** How long a remote ref probe may run before the bridge stops waiting on it. */ -const LS_REMOTE_TIMEOUT_MS = 15000; +/** + * Outer bound on a remote ref probe. Deliberately longer than the 15 s of + * stalled transfer `NON_INTERACTIVE_GIT_ENV` gives git, so that on a dead remote + * git aborts itself first and the client gets git's own diagnosis instead of a + * bare "timed out". This is the backstop for a git that somehow does neither. + */ +const LS_REMOTE_TIMEOUT_MS = 20000; /** - * Environment for every git invocation the bridge makes. + * Environment for every git invocation the bridge makes. Two failure modes, both + * of which would otherwise leave a git process alive after the HTTP request that + * spawned it has gone: + * + * - **Prompts.** git asks for credentials, ssh passphrases and unknown host keys + * by opening `/dev/tty` directly, bypassing the pipes it was given — and the + * bridge normally runs in an operator's foreground terminal, so that tty + * exists. A private repo would park git on a prompt nobody answers forever. + * - **Tarpits.** A remote that completes the TCP handshake and then says nothing + * holds git open indefinitely; git only inherits a deadline if it is given + * one. The low-speed pair makes git abort a transfer moving under 1 byte/s for + * 15 s, and `ConnectTimeout` bounds the ssh handshake. Both make git exit on + * its own, which is what actually reaps the process — killing the request that + * is waiting on it would not. * - * git asks for credentials, ssh passphrases and unknown host keys by opening - * `/dev/tty` directly, which bypasses the pipes it was given — and the bridge - * normally runs in an operator's foreground terminal, so that tty exists. Left - * to itself a private repo would therefore park the git process on a prompt - * nobody answers and hang the HTTP request behind it for good. These three tell - * git, `GIT_ASKPASS`' callers and ssh to fail instead of asking. + * `GIT_HTTP_LOW_SPEED_*` only covers the http(s) transport; an ssh remote that + * connects and then stalls is still only bounded by `LS_REMOTE_TIMEOUT_MS`. */ const NON_INTERACTIVE_GIT_ENV: Record = { GIT_TERMINAL_PROMPT: "0", GIT_ASKPASS: "", - GIT_SSH_COMMAND: "ssh -oBatchMode=yes", + GIT_SSH_COMMAND: "ssh -oBatchMode=yes -oConnectTimeout=10", + GIT_HTTP_LOW_SPEED_LIMIT: "1", + GIT_HTTP_LOW_SPEED_TIME: "15", }; /** * Reject with `message` if `promise` has not settled within `ms`. * - * Only the *waiting* is bounded: `git-bun` exposes no way to abort a running - * command, so a git process talking to a blackholed remote is abandoned to its - * own network timeout rather than killed. That still keeps the request — and the - * client waiting on it — from hanging indefinitely. + * This bounds the *waiting*, not the work: `git-bun` exposes no handle to abort + * a running command, so a git process that outlives its deadline is left to exit + * on its own. That is why `NON_INTERACTIVE_GIT_ENV` configures git to give up by + * itself — this timer is the backstop, not the mechanism. */ async function withTimeout(promise: Promise, ms: number, message: string): Promise { let timer: ReturnType | undefined; @@ -127,13 +143,46 @@ async function withTimeout(promise: Promise, ms: number, message: string): } } +/** A URL-shaped run of text; quotes and whitespace end it, as they do in git's messages. */ +const URL_IN_TEXT = /[a-z][a-z0-9+.-]*:\/\/[^\s'"`]+/gi; + /** - * Blank out `user:secret@` userinfo in any URL a message carries. A repo may be - * registered with an embedded token, and git echoes the URL it was given back in - * the command line it reports on failure — this keeps that out of an API response. + * Blank out `user:secret@` userinfo in every URL a message carries. A repo may + * be registered with an embedded token, and git echoes the URL it was given back + * in the command line it reports on failure — this keeps that out of an API + * response. + * + * Parsing beats pattern-matching here: a password may itself contain `@` + * (`https://user:p@ssw0rd@host/r`), and userinfo runs to the *last* `@` before + * the path, which a leftmost-shortest regex gets wrong. `URL` implements that + * rule; the regex below is only for candidates it refuses to parse. + * + * Not covered, deliberately: credentials in a query string + * (`?access_token=…`) and scp-style `token@host:org/repo.git`, neither of which + * is a URL this code can recognize as one. */ function redactUrlCredentials(text: string): string { - return text.replace(/([a-z][a-z0-9+.-]*:\/\/)[^\s/@]+@/gi, "$1***@"); + return text.replace(URL_IN_TEXT, (candidate) => { + try { + const url = new URL(candidate); + if (!url.username && !url.password) return candidate; + url.username = "***"; + url.password = ""; + return url.toString(); + } catch { + return redactAuthority(candidate); + } + }); +} + +/** Fallback for an unparseable URL: strip through the last `@` of the authority. */ +function redactAuthority(candidate: string): string { + const parts = /^([a-z][a-z0-9+.-]*:\/\/)([^/?#]*)([\s\S]*)$/i.exec(candidate); + if (!parts) return candidate; + const [, scheme, authority = "", rest = ""] = parts; + const at = authority.lastIndexOf("@"); + if (at === -1) return candidate; + return `${scheme}***@${authority.slice(at + 1)}${rest}`; } /** diff --git a/packages/fleet-bridge/src/providers/github.ts b/packages/fleet-bridge/src/providers/github.ts index 83a2cc9..675ab6a 100644 --- a/packages/fleet-bridge/src/providers/github.ts +++ b/packages/fleet-bridge/src/providers/github.ts @@ -209,6 +209,14 @@ interface LinkedBranchesResult { } | null; } +/** + * Statuses that mean the create was refused for a reason no amount of looking + * around will change. Everything else is treated as "the branch may already + * exist" — the safe default, since a wrong guess there costs one extra read + * while the opposite dead-ends the issue permanently. + */ +const REFUSALS_TO_SURFACE = new Set([401, 403, 404]); + /** A ref selection that carries both fields, or `undefined` if it does not. */ function toLinkedBranch(ref: GraphQLRef | null | undefined): LinkedBranch | undefined { if (!ref?.name || !ref.target?.oid) return undefined; @@ -385,12 +393,15 @@ export class GitHubProvider implements RepoProvider { * node id and a commit oid, neither of which the caller has: the issue's * `node_id`, the repo's default branch, and that branch's head SHA. * - * Idempotent by design — asking twice for the same branch is ordinary (a - * second workspace off one issue, a retry after a failed clone, or a branch - * someone already made with `gh issue develop`). GitHub signals "that is - * already linked" in two different ways depending on the case, and neither is - * a real failure, so both fall back to {@link linkedBranch}: whatever is - * attached to the issue now is what the caller wanted. + * Idempotent as far as it can be — asking twice for the same branch is + * ordinary (a second workspace off one issue, a retry after a failed clone, or + * a branch someone already made with `gh issue develop`). What GitHub does + * with a duplicate is **not verified here**: it has been reported both as a + * null `linkedBranch` on an HTTP 200 and as a refusal, and the API docs + * describe de-duplicating the name instead. So rather than encoding a guess, + * anything short of a clear auth/permission/not-found failure is treated as + * "it may already be there" and handed to {@link existingBranch}, which + * decides on evidence. */ async linkBranchToIssue(issueNumber: number, branch: string): Promise { this.requireToken(); @@ -420,28 +431,50 @@ export class GitHubProvider implements RepoProvider { name: branch, }); } catch (error) { - // Shape 1: a populated `errors` array saying the name is taken. - if (error instanceof ProviderError && error.status === 422) { - return this.linkedBranch(issueNumber, branch); - } - throw error; + // Refusals worth reporting as-is: no token, no scope, no such issue. They + // cannot mean "already there", and looking would only mask them. Anything + // else — including a refusal whose wording nothing here can anticipate — + // is checked against what exists before it is called a failure. + if (error instanceof ProviderError && REFUSALS_TO_SURFACE.has(error.status)) throw error; + return this.existingBranch(issueNumber, branch, error); } + // A `linkedBranch` that is explicitly null is GitHub declining to create + // one; a *missing* payload is a reply this code cannot read. Only the first + // means "look for what is already there" — conflating them would throw away + // a branch that had just been created and silently use an older one. + if (result.createLinkedBranch?.linkedBranch === null) { + return this.existingBranch(issueNumber, branch); + } const created = toLinkedBranch(result.createLinkedBranch?.linkedBranch?.ref); - // Shape 2: HTTP 200, no `errors` at all, and a null `linkedBranch`. - return created ?? (await this.linkedBranch(issueNumber, branch)); + if (!created) { + throw new ProviderError( + `GitHub createLinkedBranch returned an unusable payload for issue ${issueNumber}`, + 502, + ); + } + return created; } /** - * The branch already standing in for a failed `createLinkedBranch`: whichever - * branch the issue is linked to (preferring `requested`, since a second caller - * asking for the same name should get that one back), or — for a branch that - * exists but was never linked — the ref itself. + * What to use when `createLinkedBranch` would not create anything, in order of + * how well it matches what the caller asked for: + * + * 1. a branch of exactly `requested` already linked to the issue — the retry + * and second-workspace cases, and the only outright happy answer; + * 2. a branch named `requested` that exists but was never linked — someone + * pushed it by hand; it is still the branch the caller named. * - * Only when neither turns anything up has nothing actually been created, and - * the 409 says which name collided rather than blaming the upstream. + * A branch linked to the issue under some *other* name is deliberately not + * used: it is not what was asked for, and returning it would silently put a + * workspace on a branch the user never saw. That case fails instead, carrying + * `cause` so the reason the create was refused is not lost. */ - private async linkedBranch(issueNumber: number, requested: string): Promise { + private async existingBranch( + issueNumber: number, + requested: string, + cause?: unknown, + ): Promise { const result = await this.graphql(LINKED_BRANCHES, { owner: this.owner, repo: this.repo, @@ -449,26 +482,35 @@ export class GitHubProvider implements RepoProvider { }); const linked = (result.repository?.issue?.linkedBranches?.nodes ?? []) .map((node) => toLinkedBranch(node?.ref)) - .filter((ref): ref is LinkedBranch => ref !== undefined); - const match = linked.find((ref) => ref.name === requested) ?? linked[0]; - if (match) return match; + .find((ref) => ref?.name === requested); + if (linked) return linked; - try { - const ref = await this.request( - `/repos/${this.owner}/${this.repo}/git/ref/heads/${requested}`, - ); - if (ref.object?.sha) return { name: requested, sha: ref.object.sha }; - } catch { - // A 404 here just means the branch is not there either; fall through to - // the collision report, which is the more useful message. - } + const unlinked = await this.branchRef(requested); + if (unlinked) return unlinked; + const detail = cause instanceof Error ? `: ${cause.message}` : ""; throw new ProviderError( - `GitHub would not create branch "${requested}" for issue ${issueNumber}, and neither that branch nor a branch linked to the issue exists`, + `GitHub would not create branch "${requested}" for issue ${issueNumber}, and no branch of that name exists${detail}`, 409, ); } + /** One branch by name, or `undefined` when the remote has no such ref. */ + private async branchRef(branch: string): Promise { + try { + const ref = await this.request( + `/repos/${this.owner}/${this.repo}/git/ref/heads/${branch}`, + ); + return ref.object?.sha ? { name: branch, sha: ref.object.sha } : undefined; + } catch (error) { + // Only a 404 carries information — it means the ref is genuinely absent. A + // 403 or a 502 says nothing about whether the branch exists, and reporting + // either as "no such branch" would be a lie. + if (error instanceof ProviderError && error.status === 404) return undefined; + throw error; + } + } + private async postComment(number: number, body: string): Promise { this.requireToken(); const comment = await this.request( @@ -560,11 +602,10 @@ export class GitHubProvider implements RepoProvider { const first = payload.errors?.[0]; if (first !== undefined) { const message = typeof first.message === "string" ? first.message : "unknown GraphQL error"; - // GraphQL has no status of its own, so a name collision has to be - // recognized from the message text. 422 is what `linkBranchToIssue` reads - // as "already there, go and find it"; it never reaches a client. - const status = /already exists/i.test(message) ? 422 : 502; - throw new ProviderError(`GitHub GraphQL request failed: ${message}`, status); + // A GraphQL error has no status of its own. Callers that care what kind of + // failure it was must decide from context — matching on the wording would + // break the first time GitHub rephrases it, or answers in another locale. + throw new ProviderError(`GitHub GraphQL request failed: ${message}`, 502); } if (payload.data === undefined || payload.data === null) { throw new ProviderError("GitHub GraphQL response carried no data", 502); diff --git a/packages/fleet-bridge/src/providers/provider.ts b/packages/fleet-bridge/src/providers/provider.ts index 001f94f..8a92943 100644 --- a/packages/fleet-bridge/src/providers/provider.ts +++ b/packages/fleet-bridge/src/providers/provider.ts @@ -131,9 +131,11 @@ export interface RepoProvider { * development branch (GitHub's "Development → create a branch for this issue" * relationship). A write: it needs a token with repo write scope. * - * Returns the ref the provider actually created — it may differ from the - * requested name, because a forge is free to de-duplicate against branches - * that already exist. + * Callers must use the returned `name`, never the one they passed in: a forge + * may hand back a different ref. What any given forge does when the name is + * already taken — de-duplicate it, refuse, or return the existing link — is + * not established here, so an implementation is expected to cope with the name + * coming back changed and to be safe to call twice for the same branch. */ linkBranchToIssue(issueNumber: number, branch: string): Promise; } diff --git a/packages/fleet-bridge/tests/providers.test.ts b/packages/fleet-bridge/tests/providers.test.ts index bf99fde..6f920eb 100644 --- a/packages/fleet-bridge/tests/providers.test.ts +++ b/packages/fleet-bridge/tests/providers.test.ts @@ -478,14 +478,15 @@ describe("GitHubProvider", () => { const body = typeof init?.body === "string" ? init.body : undefined; calls.push({ url, method: init?.method ?? "GET", headers: new Headers(init?.headers), body }); - if (url.endsWith("/issues/12")) { + const issue = /\/issues\/(\d+)$/.exec(url)?.[1]; + if (issue !== undefined) { return Response.json({ - number: 12, - node_id: "I_issue12", + number: Number(issue), + node_id: `I_issue${issue}`, title: "a bug", state: "open", user: { login: "alice" }, - html_url: "https://github.com/owner/repo/issues/12", + html_url: `https://github.com/owner/repo/issues/${issue}`, created_at: "2026-01-01T00:00:00Z", updated_at: "2026-01-01T00:00:00Z", body: null, @@ -529,7 +530,7 @@ describe("GitHubProvider", () => { const duplicateByNull = () => Response.json({ data: { createLinkedBranch: { clientMutationId: null, issue: null, linkedBranch: null } } }); - /** The other duplicate reply: a populated `errors` array. */ + /** The other duplicate reply: a refusal. The wording is not what makes it one. */ const duplicateByError = () => Response.json({ errors: [{ message: "A ref named 12-a-bug already exists in the repository" }] }); @@ -571,7 +572,7 @@ describe("GitHubProvider", () => { expect(calls).toHaveLength(0); }); - test("a GraphQL error carried on an HTTP 200 becomes a ProviderError(502)", async () => { + test("a GraphQL error on an HTTP 200 keeps its message when nothing can be resolved", async () => { const { fetch } = linkBranchFetch(() => Response.json({ data: null, errors: [{ message: "Resource not accessible by integration" }] }), ); @@ -587,13 +588,13 @@ describe("GitHubProvider", () => { } }); - // GitHub reports "that issue already has this branch" in two different ways - // depending on the case, and neither is a failure the caller can act on, so - // both have to resolve to the branch that is already there. + // A refused create may only mean the branch is already there, and GitHub has + // been seen reporting that both as a null `linkedBranch` on an HTTP 200 and as + // an outright refusal — so both go looking before failing. test.each([ ["a null linkedBranch with no errors", duplicateByNull], - ["an 'already exists' error", duplicateByError], - ])("a duplicate reported as %s resolves to the issue's existing linked branch", async (_label, duplicate) => { + ["a refusal", duplicateByError], + ])("%s resolves to the branch of the requested name linked to the issue", async (_label, duplicate) => { const { fetch, calls } = linkBranchFetch((call) => isMutation(call) ? duplicate() : linkedRefs({ name: "12-a-bug", oid: "existingsha" }), ); @@ -609,8 +610,8 @@ describe("GitHubProvider", () => { test.each([ ["a null linkedBranch with no errors", duplicateByNull], - ["an 'already exists' error", duplicateByError], - ])("a duplicate reported as %s prefers the requested name among several links", async (_label, duplicate) => { + ["a refusal", duplicateByError], + ])("%s ignores links under other names and takes the requested one", async (_label, duplicate) => { const { fetch } = linkBranchFetch((call) => isMutation(call) ? duplicate() @@ -621,15 +622,19 @@ describe("GitHubProvider", () => { expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ name: "12-a-bug", sha: "existingsha" }); }); - test("a duplicate falls back to any branch the issue is linked to", async () => { - const { fetch } = linkBranchFetch((call) => - isMutation(call) ? duplicateByNull() : linkedRefs({ name: "12-renamed-by-hand", oid: "handsha" }), + test("a branch of the requested name beats an unrelated branch linked to the issue", async () => { + // Issue 42 was linked to "42-retries" by `gh issue develop`, and the name the + // caller previewed exists too. Answering with "42-retries" would put the + // workspace on a branch the user never saw. + const { fetch } = linkBranchFetch( + (call) => (isMutation(call) ? duplicateByNull() : linkedRefs({ name: "42-retries", oid: "OLDSHA" })), + { "42-add-retries-to-the-sync-loop": "requestedsha" }, ); const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); - expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ - name: "12-renamed-by-hand", - sha: "handsha", + expect(await provider.linkBranchToIssue(42, "42-add-retries-to-the-sync-loop")).toEqual({ + name: "42-add-retries-to-the-sync-loop", + sha: "requestedsha", }); }); @@ -643,8 +648,22 @@ describe("GitHubProvider", () => { expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ name: "12-a-bug", sha: "unlinkedsha" }); }); - test("a refused create with nothing to fall back on is a ProviderError(409) naming the branch", async () => { - const { fetch } = linkBranchFetch((call) => (isMutation(call) ? duplicateByNull() : linkedRefs())); + test("a refusal whose wording nothing anticipated still resolves the existing branch", async () => { + const { fetch } = linkBranchFetch( + (call) => + isMutation(call) + ? Response.json({ errors: [{ message: "Referenz existiert bereits" }] }) + : linkedRefs({ name: "12-a-bug", oid: "existingsha" }), + ); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + expect(await provider.linkBranchToIssue(12, "12-a-bug")).toEqual({ name: "12-a-bug", sha: "existingsha" }); + }); + + test("a refused create with no branch of that name is a 409 carrying the reason", async () => { + const { fetch } = linkBranchFetch((call) => + isMutation(call) ? duplicateByError() : linkedRefs({ name: "12-something-else", oid: "othersha" }), + ); const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); try { @@ -654,6 +673,70 @@ describe("GitHubProvider", () => { expect(error).toBeInstanceOf(ProviderError); expect((error as ProviderError).status).toBe(409); expect((error as ProviderError).message).toContain("12-a-bug"); + // The branch linked under another name is reported, never returned. + expect((error as ProviderError).message).toContain("already exists in the repository"); + } + }); + + test.each([ + ["a payload with no createLinkedBranch", { data: { createLinkedBranch: null } }], + [ + "a created ref missing its target oid", + { data: { createLinkedBranch: { linkedBranch: { ref: { name: "12-a-bug" } } } } }, + ], + ])("%s is a ProviderError(502), not a duplicate", async (_label, payload) => { + const { fetch, calls } = linkBranchFetch((call) => + isMutation(call) ? Response.json(payload) : linkedRefs({ name: "12-older-link", oid: "oldsha" }), + ); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + try { + await provider.linkBranchToIssue(12, "12-a-bug"); + throw new Error("expected linkBranchToIssue to throw"); + } catch (error) { + expect(error).toBeInstanceOf(ProviderError); + expect((error as ProviderError).status).toBe(502); + } + // An unreadable reply must not be answered with some older branch: the + // mutation may well have created the one that was asked for. + expect(calls.filter((c) => c.url.endsWith("/graphql"))).toHaveLength(1); + }); + + test.each([401, 403, 404])( + "an HTTP %i from the mutation is surfaced without looking for an existing branch", + async (status) => { + const { fetch, calls } = linkBranchFetch(() => Response.json({ message: "nope" }, { status })); + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch }); + + try { + await provider.linkBranchToIssue(12, "12-a-bug"); + throw new Error("expected linkBranchToIssue to throw"); + } catch (error) { + expect((error as ProviderError).status).toBe(status); + } + expect(calls.filter((c) => c.url.endsWith("/graphql"))).toHaveLength(1); + }, + ); + + test("a non-404 from the branch lookup is surfaced instead of 'no such branch'", async () => { + const { fetch } = linkBranchFetch((call) => { + if (isMutation(call)) return duplicateByNull(); + return linkedRefs(); + }); + // Re-wrap so the ref lookup 403s rather than 404s. + const forbidding = (async (input: string | URL | Request, init?: RequestInit) => { + if (/\/git\/ref\/heads\/12-a-bug$/.test(String(input))) { + return Response.json({ message: "Resource not accessible" }, { status: 403 }); + } + return fetch(input as string, init); + }) as unknown as typeof globalThis.fetch; + const provider = new GitHubProvider({ owner: "owner", repo: "repo", token: "t0ken", fetch: forbidding }); + + try { + await provider.linkBranchToIssue(12, "12-a-bug"); + throw new Error("expected linkBranchToIssue to throw"); + } catch (error) { + expect((error as ProviderError).status).toBe(403); } }); diff --git a/packages/fleet-bridge/tests/repo-branches.test.ts b/packages/fleet-bridge/tests/repo-branches.test.ts index cdc39e5..831d42e 100644 --- a/packages/fleet-bridge/tests/repo-branches.test.ts +++ b/packages/fleet-bridge/tests/repo-branches.test.ts @@ -87,15 +87,19 @@ describe("GET /repos/:name/branches", () => { expect(lsRemote.calls[0]).toMatchObject({ url: "git@fake/repo1.git", cwd: dir, heads: true }); }); - test("runs git non-interactively, so a credential prompt cannot hang the request", async () => { + test("runs git non-interactively and under its own deadlines", async () => { await call("GET", "/repos/repo1/branches"); - // Without these, git opens /dev/tty for the prompt — the bridge runs in an - // operator's terminal, so the request would block until someone typed. + // Without the first three git opens /dev/tty for the prompt — the bridge runs + // in an operator's terminal, so the request would block until someone typed. + // The last two make git abandon a stalled transfer itself, which is the only + // thing that reaps the process: giving up on the promise does not. expect(lsRemote.calls[0]!.env).toEqual({ GIT_TERMINAL_PROMPT: "0", GIT_ASKPASS: "", - GIT_SSH_COMMAND: "ssh -oBatchMode=yes", + GIT_SSH_COMMAND: "ssh -oBatchMode=yes -oConnectTimeout=10", + GIT_HTTP_LOW_SPEED_LIMIT: "1", + GIT_HTTP_LOW_SPEED_TIME: "15", }); }); @@ -151,6 +155,21 @@ describe("GET /repos/:name/branches", () => { expect(res.body.error).toContain("Authentication failed"); }); + test("a password containing @ is redacted whole, not up to its first @", async () => { + lsRemote.answer = () => { + throw new GitError(["ls-remote"], { + stdout: "", + stderr: "fatal: could not read from 'https://user:p@ssw0rd@github.com/o/r.git'", + exitCode: 128, + }); + }; + + const res = await call("GET", "/repos/repo1/branches"); + + expect(res.body.error).not.toContain("ssw0rd"); + expect(res.body.error).toContain("https://***@github.com/o/r.git"); + }); + test("credentials git echoes back in its own stderr are redacted too", async () => { lsRemote.answer = () => { throw new GitError(["ls-remote"], { From 39208c9674db03c5e81c109cd358183f4ae1a4b7 Mon Sep 17 00:00:00 2001 From: Jonathan Deiss Date: Mon, 27 Jul 2026 16:24:38 -0500 Subject: [PATCH 5/7] Stop the branch dropdown being clipped, and Enter eating a new branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for a4f10b8. The dropdown was absolutely positioned inside a modal whose panel is `overflow-hidden` and whose body is `overflow-y-auto`, and a non-visible overflow ancestor always clips an out-of-flow descendant — so most of an eight-branch list was unreachable, and arrowing down dragged the input off the top. The list is now in normal flow: there is nothing out of flow left to clip, and it simply lengthens a modal body that already scrolls. Pressing Enter to submit a newly typed branch name silently replaced it with the top fuzzy match, because the list pre-highlighted its first row — typing `fix` where `fix/rate-limit` exists then created the workspace on the existing branch, which is precisely the case the new-branch flow is for. Nothing is highlighted now until the user arrows into the list, so Enter commits what they typed. Matching restarts at every occurrence of the query's first character and keeps the best alignment, because pure greedy scanning got the headline case wrong: `main` consumed the `m` of `chore/re*m*ove-main-shim` and ranked that branch below one merely containing the letters scattered. The form's payload choice moves into a single pure function that returns the request or null, so "what gets sent" and "is it submittable" cannot disagree — the one decision here with a 400 behind it, previously the only one no test could reach. --- .../src/components/CreateWorkspaceModal.tsx | 34 +++-- .../src/components/ui/combobox.tsx | 123 +++++++++++++----- packages/fleet-client/src/data/mock.ts | 35 ++++- .../fleet-client/src/lib/create-workspace.ts | 54 +++++++- packages/fleet-client/src/lib/fuzzy.ts | 94 ++++++------- packages/fleet-client/tests/combobox.test.ts | 85 ++++++++++++ .../tests/create-workspace.test.ts | 76 ++++++++++- packages/fleet-client/tests/fuzzy.test.ts | 80 +++++++----- .../tests/workspace-mutations.test.ts | 28 +++- 9 files changed, 465 insertions(+), 144 deletions(-) create mode 100644 packages/fleet-client/tests/combobox.test.ts diff --git a/packages/fleet-client/src/components/CreateWorkspaceModal.tsx b/packages/fleet-client/src/components/CreateWorkspaceModal.tsx index b97f383..1e696a8 100644 --- a/packages/fleet-client/src/components/CreateWorkspaceModal.tsx +++ b/packages/fleet-client/src/components/CreateWorkspaceModal.tsx @@ -4,10 +4,9 @@ import type { RepoBranch, RepoIssue } from "@/data/types"; import { Modal } from "@/components/ui/modal"; import { Input } from "@/components/ui/input"; import { Checkbox } from "@/components/ui/checkbox"; -import { Combobox, highlight } from "@/components/ui/combobox"; +import { Combobox, highlight, splitRanges } from "@/components/ui/combobox"; import { cn } from "@/lib/utils"; -import { splitRanges } from "@/lib/fuzzy"; -import { branchState, issueBranchPreview, issueText } from "@/lib/create-workspace"; +import { branchState, createWorkspaceInput, issueBranchPreview, issueText } from "@/lib/create-workspace"; import { Field, ModalActions } from "@/routes/ReposRoute"; // Module-level so the pickers' memoised filtering is not invalidated every render. @@ -96,23 +95,17 @@ export function CreateWorkspaceModal({ repoName, ship, onClose }: Props) { }, [fromIssue, issuesLoaded, listRepoIssues, repoName]); const state = branchState(branch, branches); - const ready = Boolean(name.trim()) && Boolean(shipName) && (fromIssue ? issue !== null : Boolean(branch.trim())); + const input = createWorkspaceInput({ ship: shipName, repoName, name, fromIssue, branch, issue }); const submit = async (e: FormEvent) => { e.preventDefault(); - // The Create button is disabled in this state, but implicit submission can - // still reach here — and issue mode without a selection has no payload to - // send, only a branch-mode one that would silently create the wrong thing. - if (!ready || pending) return; + // `input` being null is what disables the Create button, but implicit + // submission can still reach here. + if (!input || pending) return; setPending(true); setError(null); try { - // Exactly one branch source goes on the wire: the bridge 400s on both keys. - await createWorkspace( - fromIssue && issue - ? { ship: shipName, repoName, name: name.trim(), issueNumber: issue.number } - : { ship: shipName, repoName, name: name.trim(), branch: branch.trim() }, - ); + await createWorkspace(input); onClose(); } catch (err) { setError((err as Error).message); @@ -174,10 +167,12 @@ export function CreateWorkspaceModal({ repoName, ship, onClose }: Props) { }} placeholder="Search open issues" disabled={issuesError !== null} - emptyMessage={issuesLoading ? "listing issues…" : "No open issue matches."} + emptyMessage="No open issue matches." /> {issuesError ? ( Issues could not be listed: {issuesError} + ) : issuesLoading ? ( + listing issues… ) : issue ? ( ) : null} @@ -196,13 +191,16 @@ export function CreateWorkspaceModal({ repoName, ship, onClose }: Props) { /> {state.kind === "existing" && On branch {state.branch}} {state.kind === "new" && Creating new branch {state.branch}} - {branchesError && Branches could not be listed — type a branch name.} - {!branchesError && branches === null && listing branches…} + {state.kind === "unknown" && ( + + {branchesError ? "Branches could not be listed — type a branch name." : "listing branches…"} + + )} )} {error &&

{error}

} - + ); diff --git a/packages/fleet-client/src/components/ui/combobox.tsx b/packages/fleet-client/src/components/ui/combobox.tsx index 8baac13..41a1e64 100644 --- a/packages/fleet-client/src/components/ui/combobox.tsx +++ b/packages/fleet-client/src/components/ui/combobox.tsx @@ -11,12 +11,19 @@ import { Input } from "@/components/ui/input"; * {@link fuzzySearch}, done once per render and handed to `renderItem` as the * ranges to highlight, so the list rendering never re-derives the match. * - * It lives inside a `
` and inside a `Modal`, both of which claim the keys a - * dropdown needs: Enter would submit the form and Escape would close the modal - * (whose listener is on `window`, so stopping the native event's propagation - * inside React's handler — React dispatches from the root container, below - * `window` — is what keeps it from firing). Both are intercepted here only while - * the list is on screen; otherwise the form and the modal keep their own keys. + * The list is rendered **in normal flow**, not absolutely positioned. Its only + * home is inside a `Modal`, whose panel is `overflow-hidden` and whose body is + * `overflow-y-auto`: any non-`visible` overflow ancestor clips an out-of-flow + * descendant, so a floating list would be cut off with no way to reach the rest + * of it. In flow it simply lengthens the modal body, which already knows how to + * scroll. It costs the actions below being pushed down while the list is open. + * + * It lives inside a `` and inside that `Modal`, both of which claim the + * keys a dropdown needs: Enter would submit the form and Escape would close the + * modal (whose listener is on `window`, so stopping the native event's + * propagation inside React's handler — React dispatches from the root container, + * below `window` — is what keeps it from firing). Both are intercepted here only + * while the list is on screen; otherwise the form and the modal keep their keys. * * Deliberately not a full ARIA dialog/listbox widget: like `Modal` it stays * proportionate to an app with no competing overlays. @@ -38,8 +45,8 @@ interface ComboboxProps { emptyMessage?: ReactNode; /** * Typing something no item matches is a legal value in its own right (a branch - * name that does not exist yet), so an empty result set closes the list quietly - * instead of reporting that nothing was found. + * name that does not exist yet). It suppresses the empty message, and it stops + * the list from pre-highlighting a row — see {@link enterAction}. */ allowFreeText?: boolean; } @@ -58,15 +65,21 @@ export function Combobox({ allowFreeText, }: ComboboxProps) { const [open, setOpen] = useState(false); - const [active, setActive] = useState(0); + const [active, setActive] = useState(() => defaultActive(allowFreeText)); const listId = useId(); const listRef = useRef(null); + /** Whether the current active row was reached by keyboard; see the scroll effect. */ + const arrowedRef = useRef(false); const matches = useMemo(() => fuzzySearch(items, value, toText), [items, value, toText]); - const activeIndex = matches.length === 0 ? -1 : Math.min(active, matches.length - 1); + const activeIndex = active < 0 || matches.length === 0 ? -1 : Math.min(active, matches.length - 1); const showList = open && (matches.length > 0 || (!allowFreeText && emptyMessage !== undefined)); useEffect(() => { + // Only arrow keys scroll. Doing it on hover too makes a partially visible row + // slide out from under a stationary pointer, whose `mouseenter` on the row + // that replaced it scrolls again — the list walks itself to the end. + if (!arrowedRef.current) return; listRef.current?.querySelector('[data-active="true"]')?.scrollIntoView({ block: "nearest" }); }, [activeIndex, showList]); @@ -98,24 +111,26 @@ export function Combobox({ setOpen(true); return; } - if (matches.length === 0) return; - const step = event.key === "ArrowDown" ? 1 : -1; - setActive((current) => { - const from = Math.min(current, matches.length - 1); - return (from + step + matches.length) % matches.length; - }); + arrowedRef.current = true; + setActive((current) => moveActive(current, matches.length, event.key === "ArrowDown" ? 1 : -1)); return; } if (event.key === "Enter" && showList) { - event.preventDefault(); - const item = matches[activeIndex]; - if (item) select(item.item); - else setOpen(false); + const action = enterAction(activeIndex, matches.length, Boolean(allowFreeText)); + if (action === "select") { + event.preventDefault(); + select(matches[activeIndex]!.item); + return; + } + // "submit" falls through with the list closed: the typed text is the value, + // and swallowing the key would make the user press Enter twice. + if (action === "dismiss") event.preventDefault(); + setOpen(false); } }; return ( -
+
({ disabled={disabled} onChange={(event) => { onValueChange(event.target.value); - setActive(0); + setActive(defaultActive(allowFreeText)); setOpen(true); }} onFocus={() => setOpen(true)} @@ -144,7 +159,11 @@ export function Combobox({ id={listId} role="listbox" ref={listRef} - className="absolute left-0 right-0 top-full z-10 mt-1 max-h-[200px] overflow-y-auto rounded-md border border-line bg-panel py-1 shadow-xl" + // On the list rather than each row: a mousedown anywhere in it — + // including on its scrollbar, which is needed past ~7 rows — would + // otherwise blur the input and close the list mid-drag. + onMouseDown={(event) => event.preventDefault()} + className="max-h-[200px] overflow-y-auto rounded-md border border-line bg-panel py-1" > {matches.length === 0 ? (
  • @@ -158,14 +177,11 @@ export function Combobox({ role="option" aria-selected={index === activeIndex} data-active={index === activeIndex ? "true" : undefined} - // mousedown, not click: it fires before the input's blur, and - // preventing its default keeps focus (and so the list) in place - // long enough for the selection to register. - onMouseDown={(event) => { - event.preventDefault(); - select(match.item); + onMouseDown={() => select(match.item)} + onMouseEnter={() => { + arrowedRef.current = false; + setActive(index); }} - onMouseEnter={() => setActive(index)} className={cn( "cursor-pointer px-3 py-[5px] font-mono text-[11.5px] text-dim", index === activeIndex && "bg-panel2 text-text", @@ -181,6 +197,34 @@ export function Combobox({ ); } +/** + * Where the highlight starts, and returns after every edit. Free text mode starts + * at nothing highlighted (`-1`): with a row pre-selected, typing a new branch name + * and pressing Enter would replace it with the closest existing branch instead of + * committing what was typed. + */ +function defaultActive(allowFreeText: boolean | undefined): number { + return allowFreeText ? -1 : 0; +} + +/** Where ArrowDown/ArrowUp move from `current`; `-1` means nothing is highlighted. */ +export function moveActive(current: number, count: number, step: 1 | -1): number { + if (count === 0) return -1; + if (current < 0) return step === 1 ? 0 : count - 1; + return (Math.min(current, count - 1) + step + count) % count; +} + +/** + * What Enter means with the list on screen: take the highlighted row, let the + * form submit the text as typed, or just close a list that has nothing to offer. + */ +export type EnterAction = "select" | "submit" | "dismiss"; + +export function enterAction(activeIndex: number, matchCount: number, allowFreeText: boolean): EnterAction { + if (activeIndex >= 0 && matchCount > 0) return "select"; + return allowFreeText ? "submit" : "dismiss"; +} + /** Text with the matched ranges emphasised — the default row, and reusable in a custom one. */ export function highlight(text: string, ranges: [number, number][]): ReactNode { if (ranges.length === 0) return text; @@ -198,3 +242,22 @@ export function highlight(text: string, ranges: [number, number][]): ReactNode { if (at < text.length) parts.push(text.slice(at)); return parts; } + +/** + * Cut `ranges` at index `at`, rebasing the right-hand side to 0 — for a row that + * renders one matched string as two differently styled pieces. A range straddling + * the cut is split across both sides; one ending or starting exactly on it stays + * whole, so neither side is handed a zero-width range to render. + */ +export function splitRanges( + ranges: [number, number][], + at: number, +): [[number, number][], [number, number][]] { + const left: [number, number][] = []; + const right: [number, number][] = []; + for (const [start, end] of ranges) { + if (start < at) left.push([start, Math.min(end, at)]); + if (end > at) right.push([Math.max(start, at) - at, end - at]); + } + return [left, right]; +} diff --git a/packages/fleet-client/src/data/mock.ts b/packages/fleet-client/src/data/mock.ts index 661ab00..15627cd 100644 --- a/packages/fleet-client/src/data/mock.ts +++ b/packages/fleet-client/src/data/mock.ts @@ -395,6 +395,31 @@ const SEED_ARMORY_SHIP_STATES: Record = { /** The one seed repo that is not provider-backed; see {@link seedRepos}. */ const CUSTOM_REPO = "notifier"; +/** + * Validate a create request's mutually exclusive branch source, mirroring the + * bridge's own `branchSource` (`fleet-manager.ts`) — including the checks past + * "one or the other": a blank branch and a non-integral issue number are 400s + * there, and a mock that quietly accepts them would let a form ship a bug that + * only the real bridge would catch. + */ +function branchSource(input: { branch?: string; issueNumber?: number }): { branch: string } | { issueNumber: number } { + if (input.branch !== undefined && input.issueNumber !== undefined) { + throw new Error("a workspace is created from a branch or an issue, not both"); + } + if (input.branch !== undefined) { + const branch = input.branch.trim(); + if (branch.length === 0) throw new Error("branch must not be empty"); + return { branch }; + } + if (input.issueNumber !== undefined) { + if (!Number.isSafeInteger(input.issueNumber) || input.issueNumber < 1) { + throw new Error("issueNumber must be a positive integer"); + } + return { issueNumber: input.issueNumber }; + } + throw new Error("a workspace needs either a branch or an issue to start from"); +} + /** * Seed the repo registry from the distinct repo names in the seed workspaces. * All but one are `github`, so the issue picker has something to show; the @@ -526,12 +551,7 @@ export class MockFleetBridge implements FleetBridge { branch?: string; issueNumber?: number; }): Promise { - if (input.branch !== undefined && input.issueNumber !== undefined) { - throw new Error("a workspace is created from a branch or an issue, not both"); - } - if (input.branch === undefined && input.issueNumber === undefined) { - throw new Error("a workspace needs either a branch or an issue to start from"); - } + const source = branchSource(input); if (!this.ships.some((s) => s.name === input.ship)) throw new Error(`unknown ship: ${input.ship}`); if (!this.repos.some((r) => r.name === input.repoName)) throw new Error(`unknown repo: ${input.repoName}`); if (this.workspaces.some((w) => w.repoName === input.repoName && w.name === input.name)) { @@ -540,7 +560,8 @@ export class MockFleetBridge implements FleetBridge { // The bridge derives the branch from the issue and links it on the provider // before the ship ever sees the request; the workspace it returns is on that // branch, so the mock has to do the same or issue mode looks like a no-op. - const branch = input.branch ?? issueBranchName(this.issue(input.repoName, input.issueNumber!)); + const branch = + "branch" in source ? source.branch : issueBranchName(this.issue(input.repoName, source.issueNumber)); const ws: Workspace = { ship: input.ship, repoName: input.repoName, diff --git a/packages/fleet-client/src/lib/create-workspace.ts b/packages/fleet-client/src/lib/create-workspace.ts index 53072b1..7462af1 100644 --- a/packages/fleet-client/src/lib/create-workspace.ts +++ b/packages/fleet-client/src/lib/create-workspace.ts @@ -1,10 +1,9 @@ /** - * lib/create-workspace.ts — the decisions behind the create-workspace form. - * - * The form's one piece of real logic is what to tell the user about the branch - * they typed, and that depends on a list that is allowed to be missing. It lives - * here so it can be tested without a DOM, and so the component is left with - * rendering. + * lib/create-workspace.ts — the decisions behind the create-workspace form: + * what to tell the user about the branch they typed, and what request that form + * state amounts to. Both live here rather than in the component so they can be + * tested without a DOM — there is no DOM harness in this package — leaving the + * component with rendering. */ import { issueBranchName } from "fleet-protocol"; @@ -57,3 +56,46 @@ export function issueBranchPreview(issue: Pick): return null; } } + +/** Everything the form holds that decides what gets sent. */ +export interface CreateWorkspaceForm { + readonly ship: string; + readonly repoName: string; + readonly name: string; + /** Whether the "Create from issue" checkbox is ticked. */ + readonly fromIssue: boolean; + readonly branch: string; + readonly issue: RepoIssue | null; +} + +/** The body of `POST /workspaces`; the two branch sources are mutually exclusive. */ +export interface CreateWorkspaceInput { + readonly ship: string; + readonly repoName: string; + readonly name: string; + readonly branch?: string; + readonly issueNumber?: number; +} + +/** + * The request a filled-in form amounts to, or `null` when it is not submittable. + * + * One function rather than a payload builder plus a `canSubmit` predicate: they + * would be two statements of the same rules, free to disagree, and the disagreement + * would show up as a request the bridge rejects. In particular the issue-mode + * result carries **no `branch` key at all** — the bridge answers 400 to a body + * naming both sources, and an empty string is naming one. + */ +export function createWorkspaceInput(form: CreateWorkspaceForm): CreateWorkspaceInput | null { + const name = form.name.trim(); + if (name.length === 0 || form.ship.length === 0) return null; + + if (form.fromIssue) { + if (!form.issue) return null; + return { ship: form.ship, repoName: form.repoName, name, issueNumber: form.issue.number }; + } + + const branch = form.branch.trim(); + if (branch.length === 0) return null; + return { ship: form.ship, repoName: form.repoName, name, branch }; +} diff --git a/packages/fleet-client/src/lib/fuzzy.ts b/packages/fleet-client/src/lib/fuzzy.ts index 7960723..1cd16b8 100644 --- a/packages/fleet-client/src/lib/fuzzy.ts +++ b/packages/fleet-client/src/lib/fuzzy.ts @@ -2,13 +2,17 @@ * lib/fuzzy.ts — the subsequence matcher behind every type-to-filter list. * * Hand-rolled because the client carries no search dependency and this is the - * only place that needs one. The matcher is deliberately greedy — each query - * character takes the first position that can still hold it — rather than the - * dynamic-programming search fzf runs. Greedy can score an alignment lower than - * the best one that exists (`ab` against `a-b ab` matches the scattered pair), - * but the lists it ranks here are a repo's branches and its open issues: tens of - * short strings, where the cost of being occasionally one rank off is far below - * the cost of the machinery that avoids it. + * only place that needs one. Matching is greedy from a fixed starting point, but + * every occurrence of the query's first character is tried as that starting + * point and the best-scoring alignment wins. Purely greedy scanning gets the + * headline case of this feature wrong — `main` against `chore/remove-main-shim` + * consumes the `m` of "re*m*ove" and scores the branch below one that merely + * contains those letters scattered — while restarting costs one pass per + * occurrence of a single character over lists of tens of short strings. + * + * It is still not fzf: the alignment after each start is greedy, so a query + * whose *later* characters could align better elsewhere can be scored low + * (`ab` against `a-b ab`). That residue is accepted. */ /** One item that matched, with the ranges to highlight and its ranking score. */ @@ -22,8 +26,11 @@ export interface FuzzyMatch { /** Characters after which a match reads as the start of a word. */ const SEPARATORS = new Set(["-", "_", "/", ".", " "]); -const MATCH_SCORE = 1; -/** Paid when a match directly follows the previous one — the strongest signal. */ +/** + * Every candidate is scored against the same query, so a per-character reward + * would be the same constant on all of them and could not order anything. There + * is deliberately none: only the bonuses and penalties below decide. + */ const CONTIGUOUS_BONUS = 8; const START_BONUS = 12; const SEPARATOR_BONUS = 6; @@ -55,58 +62,57 @@ export function fuzzySearch(items: T[], query: string, toText: (item: T) => s /** Score one haystack against an already-lowercased needle; null when it does not match. */ function scoreText(text: string, needle: string): { score: number; ranges: [number, number][] } | null { - // Indices are taken on the lowercased copy but reported against `text`, which - // holds because case folding is length-preserving for the ASCII-ish branch - // names and issue titles these lists carry. const haystack = text.toLowerCase(); - const indices: number[] = []; - let score = -text.length * LENGTH_PENALTY; + const first = [...needle][0]!; + + let best: { score: number; spans: [number, number][] } | null = null; + for (let at = haystack.indexOf(first); at !== -1; at = haystack.indexOf(first, at + 1)) { + const candidate = alignFrom(haystack, needle, at); + if (candidate && (best === null || candidate.score > best.score)) best = candidate; + } + if (!best) return null; + + // Spans index the lowercased copy, which addresses `text` only while case + // folding preserves length — U+0130 lowercases to two units and shifts + // everything after it. Where it does not, the item still ranks; it just + // highlights nothing, rather than emphasising the wrong characters. + const ranges = haystack.length === text.length ? mergeSpans(best.spans) : []; + return { score: best.score - text.length * LENGTH_PENALTY, ranges }; +} + +/** Greedily place `needle` with its first character pinned at `start`. */ +function alignFrom(haystack: string, needle: string, start: number): { score: number; spans: [number, number][] } | null { + const spans: [number, number][] = []; + let score = 0; + // Distance is measured from the last consumed position, starting at the head of + // the string, so a match far into the text pays for the run-up as well. let from = 0; - let previous = -1; for (const character of needle) { - const at = haystack.indexOf(character, from); + const at = spans.length === 0 ? start : haystack.indexOf(character, from); if (at === -1) return null; - score += MATCH_SCORE; - if (previous !== -1 && at === previous + 1) score += CONTIGUOUS_BONUS; + if (at === from && spans.length > 0) score += CONTIGUOUS_BONUS; else score -= Math.min(at - from, MAX_GAP) * GAP_PENALTY; if (at === 0) score += START_BONUS; else if (SEPARATORS.has(haystack[at - 1]!)) score += SEPARATOR_BONUS; - indices.push(at); - previous = at; + // A span, not an index: an astral character is two code units, and half of a + // surrogate pair renders as U+FFFD. + spans.push([at, at + character.length]); from = at + character.length; } - return { score, ranges: mergeRanges(indices) }; -} - -/** - * Cut `ranges` at index `at`, rebasing the right-hand side to 0 — for a row that - * renders one matched string as two differently styled pieces. A range straddling - * the cut is split across both sides. - */ -export function splitRanges( - ranges: [number, number][], - at: number, -): [[number, number][], [number, number][]] { - const left: [number, number][] = []; - const right: [number, number][] = []; - for (const [start, end] of ranges) { - if (start < at) left.push([start, Math.min(end, at)]); - if (end > at) right.push([Math.max(start, at) - at, end - at]); - } - return [left, right]; + return { score, spans }; } -/** Collapse ascending match indices into `[start, end)` runs. */ -function mergeRanges(indices: number[]): [number, number][] { +/** Collapse ascending, non-overlapping spans into `[start, end)` runs. */ +function mergeSpans(spans: [number, number][]): [number, number][] { const ranges: [number, number][] = []; - for (const index of indices) { + for (const [start, end] of spans) { const last = ranges[ranges.length - 1]; - if (last && last[1] === index) last[1] = index + 1; - else ranges.push([index, index + 1]); + if (last && last[1] === start) last[1] = end; + else ranges.push([start, end]); } return ranges; } diff --git a/packages/fleet-client/tests/combobox.test.ts b/packages/fleet-client/tests/combobox.test.ts new file mode 100644 index 0000000..fbf3e63 --- /dev/null +++ b/packages/fleet-client/tests/combobox.test.ts @@ -0,0 +1,85 @@ +/** + * combobox.test.ts — the pure parts of the picker: where the highlight moves, + * what Enter means, and how a matched string is cut in two for a row that styles + * its halves differently. The component around them cannot be rendered here — + * this package has no DOM harness — so everything that can be a function is one. + */ + +import { describe, expect, test } from "bun:test"; +import { enterAction, moveActive, splitRanges } from "../src/components/ui/combobox"; + +describe("moveActive", () => { + test("arrives at the first row going down and the last going up", () => { + expect(moveActive(-1, 4, 1)).toBe(0); + expect(moveActive(-1, 4, -1)).toBe(3); + }); + + test("wraps at both ends", () => { + expect(moveActive(3, 4, 1)).toBe(0); + expect(moveActive(0, 4, -1)).toBe(3); + }); + + test("steps one row at a time in between", () => { + expect(moveActive(1, 4, 1)).toBe(2); + expect(moveActive(2, 4, -1)).toBe(1); + }); + + test("an index left over from a longer list is clamped before it moves", () => { + expect(moveActive(9, 3, -1)).toBe(1); + expect(moveActive(9, 3, 1)).toBe(0); + }); + + test("an empty list has nothing to highlight", () => { + expect(moveActive(-1, 0, 1)).toBe(-1); + expect(moveActive(2, 0, -1)).toBe(-1); + }); +}); + +describe("enterAction", () => { + test("takes the highlighted row when there is one", () => { + expect(enterAction(0, 3, true)).toBe("select"); + expect(enterAction(2, 3, false)).toBe("select"); + }); + + test("with nothing highlighted, free text submits what was typed", () => { + // The bug this prevents: with a row pre-highlighted, typing a branch name + // that does not exist yet and pressing Enter silently swaps in the closest + // existing branch, and the next Enter creates a workspace on *that*. + expect(enterAction(-1, 3, true)).toBe("submit"); + expect(enterAction(-1, 0, true)).toBe("submit"); + }); + + test("with nothing highlighted and no free text, Enter only dismisses", () => { + expect(enterAction(-1, 3, false)).toBe("dismiss"); + expect(enterAction(-1, 0, false)).toBe("dismiss"); + }); +}); + +describe("splitRanges", () => { + test("sends each range to its side and rebases the right one", () => { + expect( + splitRanges( + [ + [0, 2], + [5, 7], + ], + 3, + ), + ).toEqual([[[0, 2]], [[2, 4]]]); + }); + + test("a range straddling the cut appears on both sides", () => { + expect(splitRanges([[1, 5]], 3)).toEqual([[[1, 3]], [[0, 2]]]); + }); + + test("a range touching the cut stays whole, with no zero-width leftover", () => { + // What `IssueRow` hits when the query matches the space after the number. + expect(splitRanges([[3, 5]], 3)).toEqual([[], [[0, 2]]]); + expect(splitRanges([[1, 3]], 3)).toEqual([[[1, 3]], []]); + }); + + test("a cut past the end or at zero leaves one side empty", () => { + expect(splitRanges([[1, 3]], 10)).toEqual([[[1, 3]], []]); + expect(splitRanges([[1, 3]], 0)).toEqual([[], [[1, 3]]]); + }); +}); diff --git a/packages/fleet-client/tests/create-workspace.test.ts b/packages/fleet-client/tests/create-workspace.test.ts index abe2415..d1baeaa 100644 --- a/packages/fleet-client/tests/create-workspace.test.ts +++ b/packages/fleet-client/tests/create-workspace.test.ts @@ -1,19 +1,42 @@ /** - * create-workspace.test.ts — the decisions the create-workspace form makes about - * the branch the user typed. There is no DOM harness in this package, so this is - * where the form's logic is held to account; the component around it only renders - * what these return. + * create-workspace.test.ts — the decisions the create-workspace form makes: what + * it says about the branch the user typed, and what request the form amounts to. + * There is no DOM harness in this package, so this is where the form's logic is + * held to account; the component around it only renders what these return. */ import { describe, expect, test } from "bun:test"; -import { branchState, issueBranchPreview, issueText } from "../src/lib/create-workspace"; -import type { RepoBranch } from "../src/data/types"; +import { + branchState, + createWorkspaceInput, + issueBranchPreview, + issueText, + type CreateWorkspaceForm, +} from "../src/lib/create-workspace"; +import type { RepoBranch, RepoIssue } from "../src/data/types"; const BRANCHES: RepoBranch[] = [ { name: "main", sha: "a".repeat(40) }, { name: "feat/oauth-pkce", sha: "b".repeat(40) }, ]; +const ISSUE: RepoIssue = { + number: 12, + title: "Better create workspace issue", + author: "firesquid", + url: "https://example.test/12", +}; + +const form = (patch: Partial = {}): CreateWorkspaceForm => ({ + ship: "forge-01", + repoName: "api-gateway", + name: "ws-1", + fromIssue: false, + branch: "main", + issue: null, + ...patch, +}); + describe("branchState", () => { test("an exact name is an existing branch", () => { expect(branchState("main", BRANCHES)).toEqual({ kind: "existing", branch: "main" }); @@ -69,3 +92,44 @@ describe("issueBranchPreview", () => { expect(issueBranchPreview({ number: 1.5, title: "Impossible" })).toBeNull(); }); }); + +describe("createWorkspaceInput", () => { + test("branch mode sends the trimmed branch and no issueNumber key", () => { + const input = createWorkspaceInput(form({ branch: " feat/x ", name: " ws-1 " })); + + expect(input).toEqual({ ship: "forge-01", repoName: "api-gateway", name: "ws-1", branch: "feat/x" }); + expect(input && "issueNumber" in input).toBe(false); + }); + + test("issue mode sends the issue number and no branch key at all", () => { + // Not merely a falsy branch: the bridge answers 400 to a body naming both + // sources, and `branch: ""` names one. + const input = createWorkspaceInput(form({ fromIssue: true, branch: "main", issue: ISSUE })); + + expect(input).toEqual({ ship: "forge-01", repoName: "api-gateway", name: "ws-1", issueNumber: 12 }); + expect(input && "branch" in input).toBe(false); + }); + + test("issue mode with nothing selected is not submittable", () => { + // Even with a perfectly good branch sitting in the other mode's field. + expect(createWorkspaceInput(form({ fromIssue: true, branch: "main", issue: null }))).toBeNull(); + }); + + test("a blank name or ship is not submittable in either mode", () => { + expect(createWorkspaceInput(form({ name: " " }))).toBeNull(); + expect(createWorkspaceInput(form({ ship: "" }))).toBeNull(); + expect(createWorkspaceInput(form({ fromIssue: true, issue: ISSUE, name: "" }))).toBeNull(); + expect(createWorkspaceInput(form({ fromIssue: true, issue: ISSUE, ship: "" }))).toBeNull(); + }); + + test("a blank branch is not submittable in branch mode", () => { + expect(createWorkspaceInput(form({ branch: "" }))).toBeNull(); + expect(createWorkspaceInput(form({ branch: " " }))).toBeNull(); + }); + + test("an unticked checkbox ignores a previously selected issue", () => { + const input = createWorkspaceInput(form({ fromIssue: false, branch: "main", issue: ISSUE })); + + expect(input).toEqual({ ship: "forge-01", repoName: "api-gateway", name: "ws-1", branch: "main" }); + }); +}); diff --git a/packages/fleet-client/tests/fuzzy.test.ts b/packages/fleet-client/tests/fuzzy.test.ts index 6ae11b7..ab524de 100644 --- a/packages/fleet-client/tests/fuzzy.test.ts +++ b/packages/fleet-client/tests/fuzzy.test.ts @@ -3,16 +3,21 @@ * * Scores are never asserted absolutely, only as orderings: the constants are an * implementation detail, but "a prefix beats a scattered match" is the contract - * the pickers are built on. Ranges, which drive the highlighting, are asserted - * exactly. + * the pickers are built on. Each ordering case is built so that the property it + * names is the only thing that can decide it — same haystack length, same + * contiguity, same separators — because a case several rules could settle pins + * none of them. Ranges, which drive the highlighting, are asserted exactly. */ import { describe, expect, test } from "bun:test"; -import { fuzzySearch, splitRanges } from "../src/lib/fuzzy"; +import { fuzzySearch } from "../src/lib/fuzzy"; const ranked = (items: string[], query: string): string[] => fuzzySearch(items, query, (item) => item).map((match) => match.item); +const rangesOf = (text: string, query: string): [number, number][] | undefined => + fuzzySearch([text], query, (item) => item)[0]?.ranges; + describe("fuzzySearch", () => { test("an empty query returns every item, in order, unmatched", () => { const items = ["main", "develop", "feat/oauth-pkce"]; @@ -37,31 +42,33 @@ describe("fuzzySearch", () => { test("matching is case-insensitive in both directions", () => { expect(ranked(["main"], "MAIN")).toEqual(["main"]); expect(ranked(["MAIN"], "main")).toEqual(["MAIN"]); - expect(fuzzySearch(["Release/2.3"], "r2", (i) => i)[0]?.ranges).toEqual([ + expect(rangesOf("Release/2.3", "r2")).toEqual([ [0, 1], [8, 9], ]); }); test("ranges are half-open indices into the original text, adjacent ones merged", () => { - const [match] = fuzzySearch(["rate-limit"], "rali", (item) => item); - - expect(match?.ranges).toEqual([ + expect(rangesOf("rate-limit", "rali")).toEqual([ [0, 2], [5, 7], ]); }); test("a fully contiguous match is one range covering the query", () => { - const [match] = fuzzySearch(["feat/oauth-pkce"], "feat", (item) => item); - - expect(match?.ranges).toEqual([[0, 4]]); + expect(rangesOf("feat/oauth-pkce", "feat")).toEqual([[0, 4]]); }); test("a match at the start outranks the same match in the middle", () => { expect(ranked(["domain", "main"], "main")).toEqual(["main", "domain"]); }); + test("starting at index 0 beats starting after a separator, all else equal", () => { + // Same length, same four-character contiguous run: only the start bonus — + // which has to outweigh the separator bonus it gives up — can order these. + expect(ranked(["xx-main-xx", "main-xxxxx"], "main")).toEqual(["main-xxxxx", "xx-main-xx"]); + }); + test("a contiguous match outranks a gappy one", () => { expect(ranked(["f-e-a-t", "feat"], "feat")).toEqual(["feat", "f-e-a-t"]); }); @@ -70,6 +77,21 @@ describe("fuzzySearch", () => { expect(ranked(["firebrand", "fix-rate"], "fr")).toEqual(["fix-rate", "firebrand"]); }); + test("the closer of two equally gappy matches wins", () => { + // Identical length, identical start, neither contiguous, no separators: the + // distance between the two matched characters is all that differs. + expect(ranked(["axxxxbxxxx", "axbxxxxxxx"], "ab")).toEqual(["axbxxxxxxx", "axxxxbxxxx"]); + }); + + test("distance stops counting once it is simply far", () => { + // Gaps of 12 and 20 are both past the cap, so these tie and keep input order. + const far = `a${"x".repeat(12)}b${"x".repeat(10)}`; + const farther = `a${"x".repeat(20)}b${"x".repeat(2)}`; + + expect(ranked([far, farther], "ab")).toEqual([far, farther]); + expect(ranked([farther, far], "ab")).toEqual([farther, far]); + }); + test("the shorter of two equally good matches wins", () => { expect(ranked(["main-branch", "main"], "main")).toEqual(["main", "main-branch"]); }); @@ -79,6 +101,16 @@ describe("fuzzySearch", () => { expect(ranked(["alpha-two", "alpha-one"], "alpha")).toEqual(["alpha-two", "alpha-one"]); }); + test("the whole word wins even when an earlier character could start the match", () => { + // Scanning purely left to right consumes the `m` of "re*m*ove" and ranks the + // branch that really contains "main" below one that merely has the letters. + expect(ranked(["renovate/lock-file-maintenance", "chore/remove-main-shim"], "main")).toEqual([ + "chore/remove-main-shim", + "renovate/lock-file-maintenance", + ]); + expect(rangesOf("chore/remove-main-shim", "main")).toEqual([[13, 17]]); + }); + test("matches over the projected text, not the item itself", () => { const issues = [ { number: 12, title: "Better create workspace issue" }, @@ -89,27 +121,17 @@ describe("fuzzySearch", () => { expect(fuzzySearch(issues, "12", text).map((m) => m.item.number)).toEqual([12]); expect(fuzzySearch(issues, "workspace", text).map((m) => m.item.number)).toEqual([12]); }); -}); - -describe("splitRanges", () => { - test("sends each range to its side and rebases the right one", () => { - expect( - splitRanges( - [ - [0, 2], - [5, 7], - ], - 3, - ), - ).toEqual([[[0, 2]], [[2, 4]]]); - }); - test("a range straddling the cut appears on both sides", () => { - expect(splitRanges([[1, 5]], 3)).toEqual([[[1, 3]], [[0, 2]]]); + test("an astral character is one whole range, and still counts as contiguous", () => { + expect(rangesOf("Fix 🐛 crash", "🐛")).toEqual([[4, 6]]); + expect(rangesOf("Fix 🐛 crash", "🐛 c")).toEqual([[4, 8]]); }); - test("a cut past the end or at zero leaves one side empty", () => { - expect(splitRanges([[1, 3]], 10)).toEqual([[[1, 3]], []]); - expect(splitRanges([[1, 3]], 0)).toEqual([[], [[1, 3]]]); + test("text whose case folding changes length still ranks, but highlights nothing", () => { + // U+0130 lowercases to two code units, so every index past it addresses the + // wrong character; no highlight beats the wrong one. + expect(ranked(["fix İO errors"], "errors")).toEqual(["fix İO errors"]); + expect(rangesOf("fix İO errors", "errors")).toEqual([]); + expect(rangesOf("İİİ abc", "abc")).toEqual([]); }); }); diff --git a/packages/fleet-client/tests/workspace-mutations.test.ts b/packages/fleet-client/tests/workspace-mutations.test.ts index aada803..6aebf6c 100644 --- a/packages/fleet-client/tests/workspace-mutations.test.ts +++ b/packages/fleet-client/tests/workspace-mutations.test.ts @@ -215,8 +215,6 @@ describe("MockFleetBridge create-workspace surface", () => { const names = (await mock.listRepoBranches(REPO)).map((b) => b.name); expect(names).toContain("main"); - // Sorted by name, the way the bridge's `ls-remote` listing is. - expect(names).toEqual([...names].sort()); await expect(mock.listRepoBranches("nope")).rejects.toThrow("repo not found"); }); @@ -242,13 +240,16 @@ describe("MockFleetBridge create-workspace surface", () => { expect(await mock.listWorkspaces()).toContainEqual(workspace); }); - test("the issue fixture exercises punctuation and the 60-character cap", async () => { + test("the issue fixture exercises punctuation and truncation", async () => { const issues = await new MockFleetBridge().listRepoIssues(REPO); const names = issues.map((issue) => issueBranchName(issue)); expect(names.every((name) => /^[0-9a-z-]+$/.test(name))).toBe(true); expect(issues.some((issue) => /[^\w ]/.test(issue.title))).toBe(true); - expect(names.some((name) => name.length === 60)).toBe(true); + // One title outgrows its branch name, so the picker's preview shows a + // truncated one. Asserted against the derived name rather than a length, + // which would pin `fleet-protocol`'s cap from another package's test. + expect(issues.some((issue, i) => issue.title.length > names[i]!.length)).toBe(true); }); test("a create from an issue on a custom repo is refused, as on the bridge", async () => { @@ -280,6 +281,25 @@ describe("MockFleetBridge create-workspace surface", () => { ).rejects.toThrow("either a branch or an issue"); }); + test("a blank branch is refused rather than recorded verbatim", async () => { + const mock = new MockFleetBridge(); + + await expect( + mock.createWorkspace({ ship: "forge-01", repoName: REPO, name: "ws-blank", branch: " " }), + ).rejects.toThrow("branch must not be empty"); + expect((await mock.listWorkspaces()).some((w) => w.name === "ws-blank")).toBe(false); + }); + + test("an issue number that cannot identify an issue is refused before it is looked up", async () => { + const mock = new MockFleetBridge(); + + for (const issueNumber of [0, -3, 1.5, Number.MAX_SAFE_INTEGER + 2]) { + await expect( + mock.createWorkspace({ ship: "forge-01", repoName: REPO, name: "ws-bad", issueNumber }), + ).rejects.toThrow("issueNumber must be a positive integer"); + } + }); + test("a plain branch create still works and keeps the branch verbatim", async () => { const workspace = await new MockFleetBridge().createWorkspace({ ship: "forge-01", From 96a4a2b447ddddd3208065c57c3a8ac1641bdfa9 Mon Sep 17 00:00:00 2001 From: Jonathan Deiss Date: Mon, 27 Jul 2026 16:46:06 -0500 Subject: [PATCH 6/7] Portal the branch dropdown out of the modal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The list has now failed in two places, for opposite reasons. Absolutely positioned inside the panel it was clipped: the panel is `overflow-hidden` and its body `overflow-y-auto`, and a non-visible overflow ancestor always clips an out-of-flow descendant, so most of an eight-branch list was unreachable. In normal flow it was not clipped, but its presence set the height of everything below it — pressing Create closed the list on blur, the panel re-centred, and the button moved out from under the pointer before mouseup, so the click either missed or landed on the backdrop and discarded the form. Portalled into `document.body` and positioned `fixed`, it is subject to neither: no overflow ancestor to clip it, and out of flow so mounting and unmounting move nothing. Placement is a pure function, so the flip and clamp are unit-tested rather than reasoned about. React routes portal events through the React tree, so a row click still meets the panel's `stopPropagation` on its way up. Rows now select on click rather than mousedown — the focus guard keeps the list mounted through mouseup — and hover reacts only to genuine pointer movement, so a list opening under a resting pointer can no longer capture the Enter that was meant to submit a newly typed branch name. --- .../src/components/CreateWorkspaceModal.tsx | 4 +- .../src/components/ui/combobox.tsx | 269 ++++++++++++++---- packages/fleet-client/tests/combobox.test.ts | 64 ++++- .../tests/workspace-mutations.test.ts | 4 + 4 files changed, 278 insertions(+), 63 deletions(-) diff --git a/packages/fleet-client/src/components/CreateWorkspaceModal.tsx b/packages/fleet-client/src/components/CreateWorkspaceModal.tsx index 1e696a8..c3f0482 100644 --- a/packages/fleet-client/src/components/CreateWorkspaceModal.tsx +++ b/packages/fleet-client/src/components/CreateWorkspaceModal.tsx @@ -167,7 +167,9 @@ export function CreateWorkspaceModal({ repoName, ship, onClose }: Props) { }} placeholder="Search open issues" disabled={issuesError !== null} - emptyMessage="No open issue matches." + // No message while the issues are still on their way: the list + // would claim nothing matches over the "listing issues…" below it. + emptyMessage={issuesLoading ? undefined : "No open issue matches."} /> {issuesError ? ( Issues could not be listed: {issuesError} diff --git a/packages/fleet-client/src/components/ui/combobox.tsx b/packages/fleet-client/src/components/ui/combobox.tsx index 41a1e64..7d2b526 100644 --- a/packages/fleet-client/src/components/ui/combobox.tsx +++ b/packages/fleet-client/src/components/ui/combobox.tsx @@ -1,4 +1,16 @@ -import { useCallback, useEffect, useId, useMemo, useRef, useState, type KeyboardEvent, type ReactNode } from "react"; +import { + useCallback, + useEffect, + useId, + useMemo, + useRef, + useState, + type CSSProperties, + type KeyboardEvent, + type MouseEvent, + type ReactNode, +} from "react"; +import { createPortal } from "react-dom"; import { cn } from "@/lib/utils"; import { fuzzySearch } from "@/lib/fuzzy"; import { Input } from "@/components/ui/input"; @@ -11,12 +23,27 @@ import { Input } from "@/components/ui/input"; * {@link fuzzySearch}, done once per render and handed to `renderItem` as the * ranges to highlight, so the list rendering never re-derives the match. * - * The list is rendered **in normal flow**, not absolutely positioned. Its only - * home is inside a `Modal`, whose panel is `overflow-hidden` and whose body is - * `overflow-y-auto`: any non-`visible` overflow ancestor clips an out-of-flow - * descendant, so a floating list would be cut off with no way to reach the rest - * of it. In flow it simply lengthens the modal body, which already knows how to - * scroll. It costs the actions below being pushed down while the list is open. + * The list is **portalled into `document.body` and positioned `fixed`** against + * the input's viewport rect. Its only home is inside a `Modal`, and the two + * simpler options each break there: absolutely positioned inside the panel it is + * clipped, because the panel is `overflow-hidden` and its body `overflow-y-auto` + * and any non-`visible` overflow ancestor clips an out-of-flow descendant; laid + * out in normal flow it is not clipped, but then its *presence* sets the height + * of everything below it, so unmounting it on blur reflows the panel between a + * mousedown and its mouseup and the click lands somewhere else entirely — on the + * backdrop, in the worst case, discarding the form. Portalled and fixed, it is + * neither clipped nor part of any layout. + * + * Two consequences of the portal, both relied upon: + * - React routes events through the *React* tree, not the DOM tree, so a click on + * a row still passes through `Modal`'s panel handler and is stopped there — it + * cannot reach the backdrop's `onClose`. + * - The rows are no longer inside the `
  • select(match.item)} + onMouseMove={(event) => hover(event, index)} + className={cn( + "cursor-pointer px-3 py-[5px] font-mono text-[11.5px] text-dim", + index === activeIndex && "bg-panel2 text-text", + )} + > + {renderItem ? renderItem(match.item, match.ranges) : highlight(toText(match.item), match.ranges)} +
  • + )) + )} + , + document.body, + )} + + ); +} + +/** Tallest the list gets before it scrolls. */ +const LIST_MAX_HEIGHT = 200; +/** Shortest it is allowed to be squeezed to, rather than vanish in a cramped viewport. */ +const LIST_MIN_HEIGHT = 96; +/** Breathing room between the input and the list, and against the viewport edge. */ +const LIST_GAP = 6; +const VIEWPORT_MARGIN = 8; + +/** Where a portalled list sits: `offset` is from the viewport's top, or its bottom when flipped. */ +export interface ListPlacement { + readonly anchor: "below" | "above"; + readonly offset: number; + readonly left: number; + readonly width: number; + readonly maxHeight: number; +} + +/** + * Place the list against the input's viewport rect. It goes below unless there is + * both too little room there and more room above — flipping on the second + * condition alone would move the list for no gain. + */ +export function placeList( + rect: { top: number; bottom: number; left: number; width: number }, + viewportHeight: number, +): ListPlacement { + const below = viewportHeight - rect.bottom - LIST_GAP - VIEWPORT_MARGIN; + const above = rect.top - LIST_GAP - VIEWPORT_MARGIN; + const fit = (space: number) => Math.max(LIST_MIN_HEIGHT, Math.min(LIST_MAX_HEIGHT, space)); + + if (below < LIST_MAX_HEIGHT && above > below) { + return { + anchor: "above", + offset: viewportHeight - rect.top + LIST_GAP, + left: rect.left, + width: rect.width, + maxHeight: fit(above), + }; + } + return { + anchor: "below", + offset: rect.bottom + LIST_GAP, + left: rect.left, + width: rect.width, + maxHeight: fit(below), + }; +} + +function samePlacement(a: ListPlacement, b: ListPlacement): boolean { + return ( + a.anchor === b.anchor && + a.offset === b.offset && + a.left === b.left && + a.width === b.width && + a.maxHeight === b.maxHeight ); } +/** + * A placement as inline style. `offset` is a different edge in each direction, so + * which property it lands on is the whole point — the flipped list must be + * anchored by its *bottom*, since its height is not known until it has rendered. + */ +export function placementStyle(placement: ListPlacement): CSSProperties { + const box = { left: placement.left, width: placement.width, maxHeight: placement.maxHeight }; + return placement.anchor === "below" ? { ...box, top: placement.offset } : { ...box, bottom: placement.offset }; +} + /** * Where the highlight starts, and returns after every edit. Free text mode starts * at nothing highlighted (`-1`): with a row pre-selected, typing a new branch name diff --git a/packages/fleet-client/tests/combobox.test.ts b/packages/fleet-client/tests/combobox.test.ts index fbf3e63..ce21589 100644 --- a/packages/fleet-client/tests/combobox.test.ts +++ b/packages/fleet-client/tests/combobox.test.ts @@ -1,12 +1,13 @@ /** * combobox.test.ts — the pure parts of the picker: where the highlight moves, - * what Enter means, and how a matched string is cut in two for a row that styles - * its halves differently. The component around them cannot be rendered here — - * this package has no DOM harness — so everything that can be a function is one. + * what Enter means, where the portalled list is placed against the input, and how + * a matched string is cut in two for a row that styles its halves differently. + * The component around them cannot be rendered here — this package has no DOM + * harness — so everything that can be a function is one. */ import { describe, expect, test } from "bun:test"; -import { enterAction, moveActive, splitRanges } from "../src/components/ui/combobox"; +import { enterAction, moveActive, placeList, placementStyle, splitRanges } from "../src/components/ui/combobox"; describe("moveActive", () => { test("arrives at the first row going down and the last going up", () => { @@ -55,6 +56,61 @@ describe("enterAction", () => { }); }); +describe("placeList", () => { + /** An input 300px wide, 36px tall, with its top edge at `top`. */ + const inputAt = (top: number) => ({ top, bottom: top + 36, left: 40, width: 300 }); + + test("sits below the input, matching its width and left edge", () => { + const placement = placeList(inputAt(100), 800); + + expect(placement).toEqual({ anchor: "below", offset: 142, left: 40, width: 300, maxHeight: 200 }); + }); + + test("flips above when the room below is short and there is more above", () => { + // 700px down a 800px viewport: 58px below, 686px above. + const placement = placeList(inputAt(700), 800); + + expect(placement.anchor).toBe("above"); + // Measured from the viewport's bottom edge, so the list ends above the input. + expect(placement.offset).toBe(106); + expect(placement.maxHeight).toBe(200); + }); + + test("stays below when neither side fits but below is the roomier one", () => { + const placement = placeList(inputAt(120), 300); + + expect(placement.anchor).toBe("below"); + expect(placement.maxHeight).toBeGreaterThan(0); + }); + + test("takes only the room it has, and never less than a usable minimum", () => { + // Near the top of a short viewport: flipping would be worse, so it takes + // what is below — 170px of it, not the full 200. + expect(placeList(inputAt(20), 240)).toMatchObject({ anchor: "below", maxHeight: 170 }); + + // Cramped on both sides: still a usable list rather than a zero-height one. + expect(placeList(inputAt(20), 100).maxHeight).toBe(96); + }); + + test("style anchors by the top going down and by the bottom going up", () => { + // A flipped list cannot be anchored by its top: its height is not known + // until it has rendered. + const below = placementStyle(placeList(inputAt(100), 800)); + expect(below).toEqual({ left: 40, width: 300, maxHeight: 200, top: 142 }); + expect("bottom" in below).toBe(false); + + const above = placementStyle(placeList(inputAt(700), 800)); + expect(above).toEqual({ left: 40, width: 300, maxHeight: 200, bottom: 106 }); + expect("top" in above).toBe(false); + }); + + test("a tall viewport never flips", () => { + for (let top = 0; top < 900; top += 50) { + expect(placeList(inputAt(top), 1000).anchor).toBe(top + 36 + 14 + 200 <= 1000 ? "below" : "above"); + } + }); +}); + describe("splitRanges", () => { test("sends each range to its side and rebases the right one", () => { expect( diff --git a/packages/fleet-client/tests/workspace-mutations.test.ts b/packages/fleet-client/tests/workspace-mutations.test.ts index 6aebf6c..ef08996 100644 --- a/packages/fleet-client/tests/workspace-mutations.test.ts +++ b/packages/fleet-client/tests/workspace-mutations.test.ts @@ -215,6 +215,10 @@ describe("MockFleetBridge create-workspace surface", () => { const names = (await mock.listRepoBranches(REPO)).map((b) => b.name); expect(names).toContain("main"); + // The bridge answers this route from `ls-remote` sorted by name, and the + // picker's tie-breaking inherits that order; a fixture appended out of order + // would quietly diverge from it. + expect(names).toEqual([...names].sort()); await expect(mock.listRepoBranches("nope")).rejects.toThrow("repo not found"); }); From 1883de8ad7db62240d8e278e12a75c3266c832bc Mon Sep 17 00:00:00 2001 From: Jonathan Deiss Date: Sun, 2 Aug 2026 23:22:02 -0500 Subject: [PATCH 7/7] Theme the portalled combobox list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `.dark` class carrying every dark token value sat on a `div` inside the Shell, so the custom properties only inherited down the app's own subtree. The combobox list is portalled to `document.body`, which is outside it, and resolved the `:root` light values instead — a white dropdown over the dark modal. Toggling the class on `document.documentElement` puts every token above both the app and any portal. `@custom-variant dark (&:is(.dark *))` is unaffected: `html` is an ancestor of everything the variant targets. Co-Authored-By: Claude Opus 5 (1M context) --- packages/fleet-client/src/App.tsx | 8 +++++++- packages/fleet-client/src/layouts/Shell.tsx | 7 +------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/fleet-client/src/App.tsx b/packages/fleet-client/src/App.tsx index 6d1bc66..15efc57 100644 --- a/packages/fleet-client/src/App.tsx +++ b/packages/fleet-client/src/App.tsx @@ -1,6 +1,6 @@ import "./index.css"; -import { useState } from "react"; +import { useEffect, useState } from "react"; import { BrowserRouter, Route, Routes } from "react-router-dom"; import { FleetProvider } from "./data/FleetContext"; import { Shell } from "./layouts/Shell"; @@ -17,6 +17,12 @@ export function App() { const [theme, setTheme] = useState("dark"); const toggleTheme = () => setTheme((t) => (t === "dark" ? "light" : "dark")); + // On the document root rather than a wrapper element so that content + // portalled to `document.body` — the combobox list — is themed too. + useEffect(() => { + document.documentElement.classList.toggle("dark", theme === "dark"); + }, [theme]); + return ( diff --git a/packages/fleet-client/src/layouts/Shell.tsx b/packages/fleet-client/src/layouts/Shell.tsx index 0dffda3..1d99eba 100644 --- a/packages/fleet-client/src/layouts/Shell.tsx +++ b/packages/fleet-client/src/layouts/Shell.tsx @@ -1,20 +1,15 @@ import { useState } from "react"; import { Outlet } from "react-router-dom"; -import { cn } from "@/lib/utils"; import type { Theme } from "@/App"; import { useFleet } from "@/data/FleetContext"; import { Sidebar } from "./Sidebar"; import { TopBar } from "./TopBar"; -/** - * The theme is applied here by toggling the `.dark` class that switches every - * Bridge design token (see styles/globals.css). - */ export function Shell({ theme, onToggleTheme }: { theme: Theme; onToggleTheme: () => void }) { const { error } = useFleet(); const [sidebarOpen, setSidebarOpen] = useState(false); return ( -
    +
    setSidebarOpen(false)} /> {sidebarOpen && (