diff --git a/mobile/src/components/NewWorktreeModal.tsx b/mobile/src/components/NewWorktreeModal.tsx index 3fca3261ecc..8881185d58c 100644 --- a/mobile/src/components/NewWorktreeModal.tsx +++ b/mobile/src/components/NewWorktreeModal.tsx @@ -20,6 +20,7 @@ import { useNewWorktreeDrawerNavigation } from './use-new-worktree-drawer-naviga import { PickerListDrawer } from './PickerListDrawer' import { MobileAgentIcon } from './MobileAgentIcon' import { getSuggestedCreatureName } from './worktree-name-suggestion' +import { useRetiredWorktreeNames } from '../worktree/use-retired-worktree-names' import { deriveWorkspaceSshGate, workspaceSshStatusLabel } from '../tasks/workspace-ssh-gate' import { isSetupHookTrusted, @@ -186,6 +187,9 @@ function NewWorktreeModalContent({ const [initialRepos] = useState(() => (hostId ? (getCachedRepos(hostId) as Repo[] | null) : null)) const [repos, setRepos] = useState(initialRepos ?? []) const [selectedRepo, setSelectedRepo] = useState(null) + // Why: a deleted workspace's directory can still hold agent conversation state keyed by cwd, so + // its name must never be suggested again. Fetched per selected repo while the sheet is open. + const retiredWorktreeNames = useRetiredWorktreeNames(client, selectedRepo?.id) const { drawerView, formSheetVisible, formSheetInteractive, transitionDrawer, openSourceDrawer } = useNewWorktreeDrawerNavigation(visible) const createInFlightRef = useRef(false) @@ -601,7 +605,9 @@ function NewWorktreeModalContent({ // the authoritative collision is checked server-side against git // branches/remotes/PRs, so we also retry-with-suffix on conflict. const trimmedName = composer.name.trim() - const baseName = trimmedName || getSuggestedCreatureName(existingWorktreePaths ?? []) + const baseName = + trimmedName || + getSuggestedCreatureName(existingWorktreePaths ?? [], undefined, retiredWorktreeNames) let setupDecision: SetupDecision = 'inherit' if (setupCommand) { diff --git a/mobile/src/components/worktree-name-suggestion.test.ts b/mobile/src/components/worktree-name-suggestion.test.ts new file mode 100644 index 00000000000..2569871c77a --- /dev/null +++ b/mobile/src/components/worktree-name-suggestion.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from 'vitest' +import { MARINE_CREATURES } from '../../../src/shared/marine-creatures' +import { getSuggestedCreatureName } from './worktree-name-suggestion' + +const pickFirst = () => 0 +const lower = (index: number) => MARINE_CREATURES[index].toLowerCase() + +describe('getSuggestedCreatureName (mobile)', () => { + it('picks an unused name', () => { + expect(getSuggestedCreatureName([], pickFirst)).toBe(lower(0)) + }) + + it('dedupes against the basename of a live worktree path', () => { + expect(getSuggestedCreatureName([`/home/u/worktrees/${MARINE_CREATURES[0]}`], pickFirst)).toBe( + lower(1) + ) + }) + + it('handles Windows separators and trailing separators like the desktop does', () => { + expect(getSuggestedCreatureName(['C:\\worktrees\\Nautilus\\\\'], pickFirst)).not.toBe( + 'nautilus' + ) + }) + + it('never reissues a retired name whose workspace is already deleted', () => { + // Parity with desktop: the deleted workspace's directory may still hold agent + // conversation state keyed by cwd, so the name must not come back. + expect(getSuggestedCreatureName([], pickFirst, [MARINE_CREATURES[0]])).toBe(lower(1)) + }) + + it('treats retired names case-insensitively', () => { + expect(getSuggestedCreatureName([], pickFirst, [MARINE_CREATURES[0].toUpperCase()])).toBe( + lower(1) + ) + }) + + it('applies retirement on top of live paths rather than instead of them', () => { + expect( + getSuggestedCreatureName([`/home/u/worktrees/${MARINE_CREATURES[0]}`], pickFirst, [ + MARINE_CREATURES[1] + ]) + ).toBe(lower(2)) + }) + + it('degrades to a suffixed tier when the pool is spent, skipping retired variants', () => { + const allPaths = MARINE_CREATURES.map((name) => `/home/u/worktrees/${name}`) + expect(getSuggestedCreatureName(allPaths, pickFirst, [`${lower(0)}-2`])).toBe(`${lower(1)}-2`) + }) + + it('falls back to live-only dedupe when the host sends no retired names', () => { + // Hosts predating the wire field omit it; behavior must match the pre-change app. + expect(getSuggestedCreatureName([`/home/u/worktrees/${MARINE_CREATURES[0]}`], pickFirst)).toBe( + lower(1) + ) + }) +}) diff --git a/mobile/src/components/worktree-name-suggestion.ts b/mobile/src/components/worktree-name-suggestion.ts index 4e84eaae097..585fb082181 100644 --- a/mobile/src/components/worktree-name-suggestion.ts +++ b/mobile/src/components/worktree-name-suggestion.ts @@ -1,55 +1,31 @@ -import { MARINE_CREATURES } from '../../../src/shared/marine-creatures' +import { + normalizeSuggestedName, + selectSuggestedCreatureName, + suggestionPathBasename +} from '../../../src/shared/worktree-name-suggestion' -// Why: matches the desktop fallback in -// src/renderer/src/components/sidebar/worktree-name-suggestions.ts. The -// "already exists locally" collision is on the on-disk worktree directory -// name (the path basename), not the user-facing displayName — so we derive -// the used set from path basenames just like the desktop does. +// Why: this used to hand-duplicate the desktop algorithm and the two drifted apart by +// construction. Both now call the shared core so a name suggested on a phone and one suggested on +// the desktop obey the same rules. The dedupe is on the on-disk directory basename, not the +// user-facing displayName, because that is what actually collides. -function stripTrailingSeparators(p: string): string { - return p.replace(/[\\/]+$/, '') -} - -// Why: cross-platform path basename — handles both POSIX ("/") and Windows -// ("\\") separators, mirroring src/renderer/src/lib/path.ts so the mobile -// suggestion logic agrees with the desktop's collision check. -function pathBasename(p: string): string { - const normalized = stripTrailingSeparators(p) - const idx = Math.max(normalized.lastIndexOf('/'), normalized.lastIndexOf('\\')) - return idx === -1 ? normalized : normalized.slice(idx + 1) -} - -function normalize(name: string): string { - return name.trim().toLowerCase() -} - -function pickRandom(items: readonly T[], random: () => number): T { - return items[Math.floor(random() * items.length)] -} - -// Why: pick randomly from the unused pool (not the first in list order) so -// fresh worktrees don't all default to "Nautilus" and collide across repos. +/** `retiredNames` are names already spent in the selected repo, including workspaces that have + * since been deleted — their directories may still hold agent conversation state keyed by cwd, so + * reissuing the name would hand it to the next occupant. + * + * Hosts older than this field send nothing, in which case this degrades to deduping against live + * paths only, exactly as it did before. */ export function getSuggestedCreatureName( existingPaths: readonly string[], - random: () => number = Math.random + random: () => number = Math.random, + retiredNames: readonly string[] = [] ): string { const used = new Set() - for (const p of existingPaths) { - used.add(normalize(pathBasename(p))) + for (const path of existingPaths) { + used.add(normalizeSuggestedName(suggestionPathBasename(path))) } - // Lowercased to match branch-name convention (fix/seahorse, not fix/Seahorse). - const available = MARINE_CREATURES.map(normalize).filter((name) => !used.has(name)) - if (available.length > 0) { - return pickRandom(available, random) - } - let suffix = 2 - while (true) { - const numbered = MARINE_CREATURES.map((name) => `${normalize(name)}-${suffix}`).filter( - (name) => !used.has(name) - ) - if (numbered.length > 0) { - return pickRandom(numbered, random) - } - suffix += 1 + for (const retiredName of retiredNames) { + used.add(normalizeSuggestedName(retiredName)) } + return selectSuggestedCreatureName(used, random) } diff --git a/mobile/src/worktree/use-retired-worktree-names.ts b/mobile/src/worktree/use-retired-worktree-names.ts new file mode 100644 index 00000000000..6aad4726756 --- /dev/null +++ b/mobile/src/worktree/use-retired-worktree-names.ts @@ -0,0 +1,48 @@ +import { useEffect, useState } from 'react' +import type { RpcClient } from '../transport/rpc-client' + +/** Names already spent in a repo, including workspaces that have since been deleted. + * + * Why a targeted request rather than the workspace catalog: the catalog is served by `worktree.ps`, + * which carries rows only. Retired names are needed just while the create sheet is open and only + * for one repo, so this mirrors the desktop hook and asks for exactly that. + * + * Hosts predating the field omit it, and any failure yields an empty list, so this degrades to + * deduping against live workspaces alone — the behavior before retirement existed. */ +export function useRetiredWorktreeNames( + client: RpcClient | null | undefined, + repoId: string | null | undefined +): readonly string[] { + const [retiredNames, setRetiredNames] = useState([]) + + useEffect(() => { + if (!client || !repoId) { + setRetiredNames([]) + return + } + let cancelled = false + void client + // limit 1 because only the retirement map is wanted; the rows are incidental. + .sendRequest('worktree.list', { repo: `id:${repoId}`, limit: 1 }) + .then((response) => { + if (cancelled) { + return + } + const result = (response as { result?: unknown }).result as + | { retiredNamesByRepo?: Record } + | undefined + const names = result?.retiredNamesByRepo?.[repoId] + setRetiredNames(Array.isArray(names) ? names : []) + }) + .catch(() => { + if (!cancelled) { + setRetiredNames([]) + } + }) + return () => { + cancelled = true + } + }, [client, repoId]) + + return retiredNames +} diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 568f3dc65c3..e2f6b635ba5 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1112,6 +1112,7 @@ type RuntimeStore = { /** Optional so older embedders of RuntimeStore keep type-checking; the create path skips * retirement when absent, which only means a name stays issuable. */ addRetiredWorktreeName?: Store['addRetiredWorktreeName'] + getRetiredWorktreeNames?: Store['getRetiredWorktreeNames'] addRepo: Store['addRepo'] updateRepo: Store['updateRepo'] getProjects?: Store['getProjects'] @@ -21027,11 +21028,37 @@ export class OrcaRuntimeService { visibilitySourceMatchersByRepoId.get(worktree.repoId) ) }) - return { - worktrees: worktrees.slice(0, limit), - totalCount: worktrees.length, - truncated: worktrees.length > limit + const page = worktrees.slice(0, limit) + const retirementRepoIds = new Set(page.map((worktree) => worktree.repoId)) + // Why: an explicitly selected repo must report its retirements even when the page is empty or + // truncated past it. A repo with no live workspaces is exactly the case where every generated + // name looks free, so omitting it would reissue names precisely where it matters most. + if (repoId) { + retirementRepoIds.add(repoId) } + return { + worktrees: page, + totalCount: worktrees.length, + truncated: worktrees.length > limit, + retiredNamesByRepo: this.collectRetiredNamesForRepos(retirementRepoIds) + } + } + + /** Why: scoped to the repos in the page plus any explicitly requested one, so the payload cannot + * grow with the number of repos on the host; a client only suggests names for repos it sees. */ + private collectRetiredNamesForRepos(repoIds: Set): Record { + const store = this.store + if (!store?.getRetiredWorktreeNames) { + return {} + } + const byRepo: Record = {} + for (const repoId of repoIds) { + const names = store.getRetiredWorktreeNames(repoId) + if (names.length > 0) { + byRepo[repoId] = names + } + } + return byRepo } async listDetectedManagedWorktrees( diff --git a/src/renderer/src/components/sidebar/worktree-name-suggestions.ts b/src/renderer/src/components/sidebar/worktree-name-suggestions.ts index 568e854b6cf..bf1ebebe5fa 100644 --- a/src/renderer/src/components/sidebar/worktree-name-suggestions.ts +++ b/src/renderer/src/components/sidebar/worktree-name-suggestions.ts @@ -1,5 +1,8 @@ -import { MARINE_CREATURES } from '@/constants/marine-creatures' import { basename } from '@/lib/path' +import { + normalizeSuggestedName, + selectSuggestedCreatureName +} from '../../../../shared/worktree-name-suggestion' type WorktreePathLike = { path: string @@ -17,14 +20,14 @@ function collectUsedNames(worktreesByRepo: Record): return usedNames } -function pickRandom(items: readonly T[], random: () => number): T { - return items[Math.floor(random() * items.length)] -} - /** `retiredNames` are names already spent in the active repo, including ones whose workspace was * deleted. Reissuing one would place the new workspace on the prior occupant's path, where agent * CLIs would hand it that workspace's conversation history — so spent names are never offered - * again, and the pool degrades to suffixed variants instead of recycling. */ + * again, and the pool degrades to suffixed variants instead of recycling. + * + * Live dedup stays cross-repo while retirement is per-repo: two live workspaces sharing a name + * are confusing in a flat sidebar, but a name retired under one repo says nothing about the same + * name under another, whose path never collided. */ export function getSuggestedCreatureName( worktreesByRepo: Record, random: () => number = Math.random, @@ -34,34 +37,11 @@ export function getSuggestedCreatureName( for (const retiredName of retiredNames) { usedNames.add(normalizeSuggestedName(retiredName)) } - - // Why: names are lowercased (branch names are conventionally lowercase, e.g. - // fix/seahorse), and a random pick keeps fresh worktrees from all starting at - // the same creature and marching down the list in lockstep. - const available = MARINE_CREATURES.map(normalizeSuggestedName).filter( - (name) => !usedNames.has(name) - ) - if (available.length > 0) { - return pickRandom(available, random) - } - - // Every base name is taken — fall back to numbered variants. - let suffix = 2 - while (true) { - const numbered = MARINE_CREATURES.map( - (name) => `${normalizeSuggestedName(name)}-${suffix}` - ).filter((name) => !usedNames.has(name)) - if (numbered.length > 0) { - return pickRandom(numbered, random) - } - suffix += 1 - } + return selectSuggestedCreatureName(usedNames, random) } export function shouldApplySuggestedName(name: string, previousSuggestedName: string): boolean { return !name.trim() || name === previousSuggestedName } -export function normalizeSuggestedName(name: string): string { - return name.trim().toLowerCase() -} +export { normalizeSuggestedName } diff --git a/src/shared/runtime-types.ts b/src/shared/runtime-types.ts index 079a416749a..fc414994577 100644 --- a/src/shared/runtime-types.ts +++ b/src/shared/runtime-types.ts @@ -894,6 +894,13 @@ export type RuntimeWorktreeListResult = { worktrees: RuntimeWorktreeRecord[] totalCount: number truncated: boolean + /** Generated workspace names already spent, keyed by repo id, so a client can avoid suggesting a + * name whose old directory may still hold agent conversation state. + * + * Optional on purpose (remote wire compatibility, Rule 1): hosts predating this field omit it, + * and every reader must fall back to deduping against live worktrees alone rather than + * requiring it. Scoped to the repos present in `worktrees` so the payload stays bounded. */ + retiredNamesByRepo?: Record } // ── Browser automation types ── diff --git a/src/shared/worktree-name-suggestion.test.ts b/src/shared/worktree-name-suggestion.test.ts new file mode 100644 index 00000000000..a24f9581903 --- /dev/null +++ b/src/shared/worktree-name-suggestion.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, it } from 'vitest' +import { MARINE_CREATURES } from './marine-creatures' +import { + normalizeSuggestedName, + selectSuggestedCreatureName, + suggestionPathBasename +} from './worktree-name-suggestion' + +const pickFirst = () => 0 +const lower = (index: number) => MARINE_CREATURES[index].toLowerCase() + +describe('suggestionPathBasename', () => { + it('handles POSIX, Windows, and trailing separators identically', () => { + expect(suggestionPathBasename('/tmp/worktrees/Nautilus')).toBe('Nautilus') + expect(suggestionPathBasename('C:\\worktrees\\Nautilus')).toBe('Nautilus') + expect(suggestionPathBasename('/tmp/worktrees/Nautilus///')).toBe('Nautilus') + expect(suggestionPathBasename('C:\\worktrees\\Nautilus\\\\')).toBe('Nautilus') + }) +}) + +describe('selectSuggestedCreatureName', () => { + it('returns a name absent from the used set', () => { + // Anchored on absence rather than pool position: asserting an index would keep passing + // against a filtered pool while testing nothing. + const used = [lower(0), lower(1), lower(2)] + const suggested = selectSuggestedCreatureName(used) + expect(used).not.toContain(suggested) + }) + + it('skips used names when picking the first available', () => { + expect(selectSuggestedCreatureName([lower(0)], pickFirst)).toBe(lower(1)) + }) + + it('normalizes the used set so case and padding cannot smuggle a name back in', () => { + const suggested = selectSuggestedCreatureName([` ${MARINE_CREATURES[0].toUpperCase()} `]) + expect(suggested).not.toBe(lower(0)) + }) + + it('degrades to a suffixed tier once every base name is spent', () => { + const allUsed = MARINE_CREATURES.map((name) => name.toLowerCase()) + expect(selectSuggestedCreatureName(allUsed, pickFirst)).toBe(`${lower(0)}-2`) + }) + + it('does not rewind onto a spent variant within a tier', () => { + const allUsed = [...MARINE_CREATURES.map((name) => name.toLowerCase()), `${lower(0)}-2`] + expect(selectSuggestedCreatureName(allUsed, pickFirst)).toBe(`${lower(1)}-2`) + }) + + it('advances to the next tier when a whole tier is spent', () => { + const allUsed = [ + ...MARINE_CREATURES.map((name) => name.toLowerCase()), + ...MARINE_CREATURES.map((name) => `${name.toLowerCase()}-2`) + ] + expect(selectSuggestedCreatureName(allUsed, pickFirst)).toBe(`${lower(0)}-3`) + }) + + it('returns a lowercase name to match branch convention', () => { + // Pool entries are capitalized; branch names are conventionally lowercase. + const suggested = selectSuggestedCreatureName([], pickFirst) + expect(suggested).toBe(suggested.toLowerCase()) + expect(MARINE_CREATURES).toContain(suggested.charAt(0).toUpperCase() + suggested.slice(1)) + }) +}) + +describe('normalizeSuggestedName', () => { + it('trims and lowercases', () => { + expect(normalizeSuggestedName(' Nautilus ')).toBe('nautilus') + }) +}) diff --git a/src/shared/worktree-name-suggestion.ts b/src/shared/worktree-name-suggestion.ts new file mode 100644 index 00000000000..c9c23838206 --- /dev/null +++ b/src/shared/worktree-name-suggestion.ts @@ -0,0 +1,64 @@ +import { MARINE_CREATURES } from './marine-creatures' + +/** Shared selection core for generated workspace names. + * + * Why this lives in shared: desktop and mobile each need to suggest a name before creating a + * workspace, and they previously hand-duplicated this algorithm. The two copies had to agree — + * both dedupe on the on-disk directory basename, both lowercase to match branch convention, and + * both degrade to suffixed tiers rather than recycling — so they are one implementation now. + * + * Callers assemble the used set themselves because they source it differently (a by-repo map on + * desktop, a flat path list on mobile) and because retired names arrive from different places. */ + +export function normalizeSuggestedName(name: string): string { + return name.trim().toLowerCase() +} + +function stripTrailingSeparators(path: string): string { + return path.replace(/[\\/]+$/, '') +} + +/** Cross-platform basename: worktree paths can be POSIX, Windows, or SSH-host paths, and the + * collision that matters is on the directory name rather than the user-facing display name. */ +export function suggestionPathBasename(path: string): string { + const normalized = stripTrailingSeparators(path) + const index = Math.max(normalized.lastIndexOf('/'), normalized.lastIndexOf('\\')) + return index === -1 ? normalized : normalized.slice(index + 1) +} + +function pickRandom(items: readonly T[], random: () => number): T { + return items[Math.floor(random() * items.length)] +} + +/** Picks a name no one has taken, at random rather than in list order so fresh workspaces don't + * all start at the same creature and march down the list together. + * + * `usedNames` must include retired names as well as live ones. A name whose workspace was deleted + * still owns its old directory path in any agent CLI that keys conversation state by cwd, so + * reissuing it hands the next occupant someone else's history. Once every base name is spent the + * pool degrades to `name-2`, `name-3`, and those variants are equally subject to retirement. */ +export function selectSuggestedCreatureName( + usedNames: Iterable, + random: () => number = Math.random +): string { + const used = new Set() + for (const name of usedNames) { + used.add(normalizeSuggestedName(name)) + } + + const available = MARINE_CREATURES.map(normalizeSuggestedName).filter((name) => !used.has(name)) + if (available.length > 0) { + return pickRandom(available, random) + } + + let suffix = 2 + while (true) { + const numbered = MARINE_CREATURES.map( + (name) => `${normalizeSuggestedName(name)}-${suffix}` + ).filter((name) => !used.has(name)) + if (numbered.length > 0) { + return pickRandom(numbered, random) + } + suffix += 1 + } +}