mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
fix(worktrees): retire a created pool name whatever the client claims (STA-4471)
#14350 gated the whole of generated-name retirement on the new optional `nameWasGenerated` bit, conflating two decisions that pull in opposite directions: - *Which cwd a create may land on* genuinely belongs to the client. A generated name is one of ours — nothing was chosen — so folding it onto the canonical tier ladder and stepping over cwds this host already spent costs the user nothing. A name the user typed is a request; `nautilus` stays `nautilus` even where that cwd is retired, because silently handing back `nautilus-2` is the worse surprise. This half stays gated, and clients that predate the bit are read as typing users, exactly as before #14350. - *Whether the created name is recorded as spent* never belonged to the client. An older paired or mobile client omits the bit but still hands back pool-shaped names, and a registry that never learns about them lets the very next generated create collide with one. Recording is only ever a write — `addRetiredWorktreeName` self-gates on pool shape, so a coined name like `fix-login` is never recorded, and no recorded name ever redirects a typed one. The three create paths — `OrcaRuntimeService.createManagedWorktree` (runtime, mobile, CLI), `createRemoteWorktree` (SSH) and `createLocalWorktree` (desktop IPC) — each carried their own copy of the gate plus candidate selection, which is how one rule ended up applied inconsistently. They now share `planWorktreeCreateNames`, which is unit-tested directly. No wire change: no new field, no new opcode, nothing removed from what the host publishes. A name outside the creature pool never loads the registry, and neither does a typed name, so neither pays for the deadline-less backfill scan.
This commit is contained in:
@@ -134,17 +134,14 @@ import {
|
||||
} from '../project-runtime-git-options'
|
||||
import {
|
||||
getBranchNameOverrideCandidate,
|
||||
getGeneratedWorktreeCreateCandidate,
|
||||
getWorktreeCreateCandidate,
|
||||
isGeneratedWorktreeCreateName,
|
||||
WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS
|
||||
} from '../worktree-create-candidates'
|
||||
import { planWorktreeCreateNames } from '../worktree-create-name-plan'
|
||||
import {
|
||||
failedWorktreeCreationNeedsRetirement,
|
||||
getRetiredNameRegistryForRepo,
|
||||
retireGeneratedWorktreeName
|
||||
} from '../worktree-name-retirement'
|
||||
import { createRetiredNameLookup } from '../../shared/worktree/retired-name-registry'
|
||||
|
||||
const SSH_WORKTREE_CREATE_FETCH_FRESHNESS_MS = 30_000
|
||||
const SSH_WORKTREE_CREATE_FETCH_CACHE_MAX = 512
|
||||
@@ -1589,29 +1586,20 @@ export async function createRemoteWorktree(
|
||||
let selectedExistingLocalBranchName: string | null = null
|
||||
let lastBranchConflictKind: 'local' | 'remote' | null = null
|
||||
let remotePathResolved = false
|
||||
const shouldRetireGeneratedName =
|
||||
args.nameWasGenerated === true && isGeneratedWorktreeCreateName(sanitizedName)
|
||||
const retiredNameRegistry = shouldRetireGeneratedName
|
||||
? await getRetiredNameRegistryForRepo(store, repo, store.getRepos(), settings)
|
||||
: null
|
||||
const isRetiredName = retiredNameRegistry ? createRetiredNameLookup(retiredNameRegistry) : null
|
||||
const namePlan = await planWorktreeCreateNames({
|
||||
sanitizedName,
|
||||
requestedName: args.name,
|
||||
nameWasGenerated: args.nameWasGenerated,
|
||||
loadRetiredNames: () => getRetiredNameRegistryForRepo(store, repo, store.getRepos(), settings)
|
||||
})
|
||||
// Why: duplicate PR/MR checkouts still need a workspace; suffix branch/path while preserving review metadata and push target.
|
||||
for (let suffix = 1, attempts = 0; attempts < WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS; suffix += 1) {
|
||||
effectiveSanitizedName = shouldRetireGeneratedName
|
||||
? getGeneratedWorktreeCreateCandidate(
|
||||
sanitizedName,
|
||||
suffix,
|
||||
retiredNameRegistry?.exhaustedTiers
|
||||
)
|
||||
: getWorktreeCreateCandidate(sanitizedName, suffix)
|
||||
effectiveRequestedName = shouldRetireGeneratedName
|
||||
? effectiveSanitizedName
|
||||
: args.name.trim()
|
||||
? getWorktreeCreateCandidate(args.name, suffix)
|
||||
: effectiveSanitizedName
|
||||
if (isRetiredName?.(effectiveSanitizedName)) {
|
||||
const nameCandidate = namePlan.candidateAt(suffix)
|
||||
if (!nameCandidate) {
|
||||
continue
|
||||
}
|
||||
effectiveSanitizedName = nameCandidate.sanitizedName
|
||||
effectiveRequestedName = nameCandidate.requestedName
|
||||
attempts += 1
|
||||
branchName = await resolveCreateBranchNameSsh(
|
||||
provider,
|
||||
@@ -1813,7 +1801,7 @@ export async function createRemoteWorktree(
|
||||
} catch (rollbackError) {
|
||||
console.warn('[worktree-create] Failed to roll back remote sparse worktree:', rollbackError)
|
||||
}
|
||||
if (!rollbackSucceeded && shouldRetireGeneratedName) {
|
||||
if (!rollbackSucceeded && namePlan.retiresCreatedName) {
|
||||
await retireGeneratedWorktreeName(store, repo, settings, effectiveSanitizedName)
|
||||
}
|
||||
throw err
|
||||
@@ -1821,7 +1809,7 @@ export async function createRemoteWorktree(
|
||||
}
|
||||
|
||||
// Why: fallible metadata work after creation must not leave a real workspace name reusable.
|
||||
if (shouldRetireGeneratedName) {
|
||||
if (namePlan.retiresCreatedName) {
|
||||
await retireGeneratedWorktreeName(store, repo, settings, effectiveSanitizedName)
|
||||
}
|
||||
|
||||
@@ -2165,29 +2153,20 @@ export async function createLocalWorktree(
|
||||
let lastBranchConflictKind: 'local' | 'remote' | null = null
|
||||
let lastExistingPR: Awaited<ReturnType<typeof getPRForBranch>> | null = null
|
||||
let lastExistingReviewNumber: number | null = null
|
||||
const shouldRetireGeneratedName =
|
||||
args.nameWasGenerated === true && isGeneratedWorktreeCreateName(sanitizedName)
|
||||
const retiredNameRegistry = shouldRetireGeneratedName
|
||||
? await getRetiredNameRegistryForRepo(store, repo, store.getRepos(), settings)
|
||||
: null
|
||||
const isRetiredName = retiredNameRegistry ? createRetiredNameLookup(retiredNameRegistry) : null
|
||||
const namePlan = await planWorktreeCreateNames({
|
||||
sanitizedName,
|
||||
requestedName,
|
||||
nameWasGenerated: args.nameWasGenerated,
|
||||
loadRetiredNames: () => getRetiredNameRegistryForRepo(store, repo, store.getRepos(), settings)
|
||||
})
|
||||
// Why: a create-from-review branch override may already exist locally; suffix both branch and path instead of blocking the user.
|
||||
for (let suffix = 1, attempts = 0; attempts < WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS; suffix += 1) {
|
||||
effectiveSanitizedName = shouldRetireGeneratedName
|
||||
? getGeneratedWorktreeCreateCandidate(
|
||||
sanitizedName,
|
||||
suffix,
|
||||
retiredNameRegistry?.exhaustedTiers
|
||||
)
|
||||
: getWorktreeCreateCandidate(sanitizedName, suffix)
|
||||
effectiveRequestedName = shouldRetireGeneratedName
|
||||
? effectiveSanitizedName
|
||||
: requestedName.trim()
|
||||
? getWorktreeCreateCandidate(requestedName, suffix)
|
||||
: effectiveSanitizedName
|
||||
if (isRetiredName?.(effectiveSanitizedName)) {
|
||||
const nameCandidate = namePlan.candidateAt(suffix)
|
||||
if (!nameCandidate) {
|
||||
continue
|
||||
}
|
||||
effectiveSanitizedName = nameCandidate.sanitizedName
|
||||
effectiveRequestedName = nameCandidate.requestedName
|
||||
attempts += 1
|
||||
lastExistingReviewNumber = null
|
||||
|
||||
@@ -2450,14 +2429,14 @@ export async function createLocalWorktree(
|
||||
)
|
||||
})) ?? {}
|
||||
} catch (error) {
|
||||
if (shouldRetireGeneratedName && failedWorktreeCreationNeedsRetirement(error)) {
|
||||
if (namePlan.retiresCreatedName && failedWorktreeCreationNeedsRetirement(error)) {
|
||||
await retireGeneratedWorktreeName(store, repo, settings, effectiveSanitizedName)
|
||||
}
|
||||
throw error
|
||||
}
|
||||
|
||||
// Why: fallible metadata work after creation must not leave a real workspace name reusable.
|
||||
if (shouldRetireGeneratedName) {
|
||||
if (namePlan.retiresCreatedName) {
|
||||
await retireGeneratedWorktreeName(store, repo, settings, effectiveSanitizedName)
|
||||
}
|
||||
|
||||
|
||||
@@ -464,8 +464,10 @@ describe('registerWorktreeHandlers', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('leaves a user-typed name reusable on the SSH create path', async () => {
|
||||
// Why: `nautilus` is retired here, yet the user typed it — it must neither be skipped nor burned.
|
||||
it('keeps a typed name on its retired cwd yet still records it as spent on the SSH create path', async () => {
|
||||
// Why: a name the user typed is a request, so `nautilus` stays `nautilus` even where this host
|
||||
// spent that cwd. The write half is unconditional — otherwise a client that predates the
|
||||
// provenance bit leaves the registry blind and the next generated create collides here.
|
||||
const repo = {
|
||||
id: 'repo-ssh',
|
||||
path: '/remote/repo',
|
||||
@@ -517,9 +519,70 @@ describe('registerWorktreeHandlers', () => {
|
||||
name: 'nautilus'
|
||||
})
|
||||
|
||||
expect(store.addRetiredWorktreeName).not.toHaveBeenCalled()
|
||||
expect(store.getRetiredWorktreeNameRegistry).not.toHaveBeenCalled()
|
||||
expect(store.addRetiredWorktreeName).toHaveBeenCalledWith('repo-ssh', 'nautilus')
|
||||
expect(result).toMatchObject({
|
||||
worktree: expect.objectContaining({ path: '/remote/repo-nautilus' })
|
||||
})
|
||||
})
|
||||
|
||||
it('never consults the retirement registry for a name outside the creature pool', async () => {
|
||||
// Why: the pool holds ordinary words, so only pool-shaped names may be redirected. Everything
|
||||
// else must skip the registry entirely — including its backfill scan.
|
||||
const repo = {
|
||||
id: 'repo-ssh',
|
||||
path: '/remote/repo',
|
||||
displayName: 'ssh',
|
||||
badgeColor: '#000',
|
||||
addedAt: 0,
|
||||
connectionId: 'conn-1',
|
||||
worktreeBaseRef: 'origin/main'
|
||||
}
|
||||
const provider = {
|
||||
exec: vi.fn().mockImplementation(async (args: string[]) => {
|
||||
if (args[0] === 'remote') {
|
||||
return { stdout: 'origin\n', stderr: '' }
|
||||
}
|
||||
return { stdout: '', stderr: '' }
|
||||
}),
|
||||
fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined),
|
||||
addWorktree: vi.fn().mockResolvedValue(undefined),
|
||||
listWorktrees: vi.fn().mockResolvedValue([
|
||||
{
|
||||
path: '/remote/repo',
|
||||
head: 'base123',
|
||||
branch: 'refs/heads/main',
|
||||
isBare: false,
|
||||
isMainWorktree: true
|
||||
},
|
||||
{
|
||||
path: '/remote/repo-improve-dashboard',
|
||||
head: 'abc123',
|
||||
branch: 'refs/heads/improve-dashboard',
|
||||
isBare: false,
|
||||
isMainWorktree: false
|
||||
}
|
||||
]),
|
||||
worktreeIsClean: vi.fn().mockResolvedValue({ clean: true })
|
||||
}
|
||||
store.getRepos.mockReturnValue([repo])
|
||||
store.getRepo.mockReturnValue(repo)
|
||||
getSshGitProviderMock.mockReturnValue(provider)
|
||||
getActiveMultiplexerMock.mockReturnValue({
|
||||
request: vi.fn().mockResolvedValue(undefined),
|
||||
notify: vi.fn()
|
||||
})
|
||||
store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta)
|
||||
|
||||
const result = await handlers['worktrees:create'](null, {
|
||||
repoId: 'repo-ssh',
|
||||
name: 'improve-dashboard'
|
||||
})
|
||||
|
||||
expect(store.getRetiredWorktreeNameRegistry).not.toHaveBeenCalled()
|
||||
expect(store.addRetiredWorktreeName).not.toHaveBeenCalled()
|
||||
expect(result).toMatchObject({
|
||||
worktree: expect.objectContaining({ path: '/remote/repo-improve-dashboard' })
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -358,9 +358,10 @@ describe('registerWorktreeHandlers – Windows path handling', () => {
|
||||
expect(store.addRetiredWorktreeName).toHaveBeenCalledWith('repo-1', 'nautilus-2')
|
||||
})
|
||||
|
||||
it('leaves a user-typed name reusable even when the same name is retired', async () => {
|
||||
// Why: the creature pool contains ordinary words ("orca", "runner", "molly"). Silently
|
||||
// renaming a deliberate `nautilus` to `nautilus-2` — and burning it — is the wrong trade.
|
||||
it('keeps a typed name on its retired cwd yet still records it as spent', async () => {
|
||||
// Why: a name the user typed is a request, so `nautilus` stays `nautilus` even where this host
|
||||
// spent that cwd. The write half is unconditional — otherwise a client that predates the
|
||||
// provenance bit leaves the registry blind and the next generated create collides here.
|
||||
store.getRetiredWorktreeNameRegistry.mockReturnValue({ exhaustedTiers: 0, names: ['nautilus'] })
|
||||
computeWorktreePathMock.mockReturnValue('C:\\workspaces\\nautilus')
|
||||
ensurePathWithinWorkspaceMock.mockReturnValue('C:\\workspaces\\nautilus')
|
||||
@@ -383,6 +384,28 @@ describe('registerWorktreeHandlers – Windows path handling', () => {
|
||||
'origin/main',
|
||||
false
|
||||
)
|
||||
expect(store.getRetiredWorktreeNameRegistry).not.toHaveBeenCalled()
|
||||
expect(store.addRetiredWorktreeName).toHaveBeenCalledWith('repo-1', 'nautilus')
|
||||
})
|
||||
|
||||
it('never consults the retirement registry for a name outside the creature pool', async () => {
|
||||
// Why: the pool holds ordinary words, so only pool-shaped names may be redirected. Everything
|
||||
// else must skip the registry entirely — including its backfill scan.
|
||||
computeWorktreePathMock.mockReturnValue('C:\\workspaces\\fix-login')
|
||||
ensurePathWithinWorkspaceMock.mockReturnValue('C:\\workspaces\\fix-login')
|
||||
listWorktreesMock.mockResolvedValue([
|
||||
{
|
||||
path: 'C:/workspaces/fix-login',
|
||||
head: 'abc123',
|
||||
branch: 'refs/heads/fix-login',
|
||||
isBare: false,
|
||||
isMainWorktree: false
|
||||
}
|
||||
])
|
||||
|
||||
await handlers['worktrees:create'](null, { repoId: 'repo-1', name: 'fix-login' })
|
||||
|
||||
expect(store.getRetiredWorktreeNameRegistry).not.toHaveBeenCalled()
|
||||
expect(store.addRetiredWorktreeName).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
|
||||
@@ -4592,13 +4592,61 @@ describe('OrcaRuntimeService', () => {
|
||||
expect(addRetiredWorktreeName).toHaveBeenCalledWith(TEST_REPO_ID, 'nautilus')
|
||||
})
|
||||
|
||||
it('neither skips nor retires a name the user typed', async () => {
|
||||
// Why: the pool contains ordinary words. Retirement only ever applies to generated names.
|
||||
it('retires a pool-shaped name a failed create left behind even with no provenance bit', async () => {
|
||||
// Why: the failure paths are where a blind registry bites hardest — the directory survives the
|
||||
// rollback, so a later generated create that reuses it inherits the previous agent history.
|
||||
// Recording is shape-decided, so it must fire here for a client that never sent the bit.
|
||||
const addRetiredWorktreeName = vi.fn()
|
||||
const runtime = new OrcaRuntimeService({ ...store, addRetiredWorktreeName })
|
||||
computeWorktreePathMock.mockReturnValue('/tmp/workspaces/nautilus')
|
||||
ensurePathWithinWorkspaceMock.mockReturnValue('/tmp/workspaces/nautilus')
|
||||
vi.mocked(addSparseWorktree).mockRejectedValueOnce(
|
||||
Object.assign(new Error('sparse setup failed'), { cleanupFailed: true })
|
||||
)
|
||||
|
||||
await expect(
|
||||
runtime.createManagedWorktree({
|
||||
repoSelector: 'id:repo-1',
|
||||
name: 'nautilus',
|
||||
sparseCheckout: { directories: ['packages/web'] }
|
||||
})
|
||||
).rejects.toThrow('sparse setup failed')
|
||||
|
||||
expect(addRetiredWorktreeName).toHaveBeenCalledWith(TEST_REPO_ID, 'nautilus')
|
||||
})
|
||||
|
||||
it('leaves a coined name unrecorded when a create fails', async () => {
|
||||
// Why: the shape gate has to hold on the failure paths too, or a failed `fix-login` burns a
|
||||
// name the generator could never have produced.
|
||||
const addRetiredWorktreeName = vi.fn()
|
||||
const runtime = new OrcaRuntimeService({ ...store, addRetiredWorktreeName })
|
||||
computeWorktreePathMock.mockReturnValue('/tmp/workspaces/fix-login')
|
||||
ensurePathWithinWorkspaceMock.mockReturnValue('/tmp/workspaces/fix-login')
|
||||
vi.mocked(addSparseWorktree).mockRejectedValueOnce(
|
||||
Object.assign(new Error('sparse setup failed'), { cleanupFailed: true })
|
||||
)
|
||||
|
||||
await expect(
|
||||
runtime.createManagedWorktree({
|
||||
repoSelector: 'id:repo-1',
|
||||
name: 'fix-login',
|
||||
sparseCheckout: { directories: ['packages/web'] }
|
||||
})
|
||||
).rejects.toThrow('sparse setup failed')
|
||||
|
||||
expect(addRetiredWorktreeName).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('keeps a typed name on its retired cwd yet still records it as spent', async () => {
|
||||
// Why: a name the user typed is a request, so `nautilus` stays `nautilus` even where this host
|
||||
// spent that cwd. The write half is unconditional — otherwise a client that predates the
|
||||
// provenance bit leaves the registry blind and the next generated create collides here.
|
||||
const addRetiredWorktreeName = vi.fn()
|
||||
const getRetiredWorktreeNameRegistry = vi.fn(() => ({ exhaustedTiers: 0, names: ['nautilus'] }))
|
||||
const runtime = new OrcaRuntimeService({
|
||||
...store,
|
||||
addRetiredWorktreeName,
|
||||
getRetiredWorktreeNameRegistry: () => ({ exhaustedTiers: 0, names: ['nautilus'] })
|
||||
getRetiredWorktreeNameRegistry
|
||||
})
|
||||
const createdWorktree = {
|
||||
path: '/tmp/workspaces/nautilus',
|
||||
@@ -4619,6 +4667,43 @@ describe('OrcaRuntimeService', () => {
|
||||
})
|
||||
|
||||
expect(result.worktree.path).toBe(createdWorktree.path)
|
||||
expect(computeWorktreePathMock).toHaveBeenCalledWith(
|
||||
'nautilus',
|
||||
expect.anything(),
|
||||
expect.anything()
|
||||
)
|
||||
expect(getRetiredWorktreeNameRegistry).not.toHaveBeenCalled()
|
||||
expect(addRetiredWorktreeName).toHaveBeenCalledWith(TEST_REPO_ID, 'nautilus')
|
||||
})
|
||||
|
||||
it('never consults the retirement registry for a name outside the creature pool', async () => {
|
||||
// Why: the pool contains ordinary words, so only pool-shaped names may be redirected. A name
|
||||
// the user coined must not even pay for the registry read, which can hit disk.
|
||||
const addRetiredWorktreeName = vi.fn()
|
||||
const getRetiredWorktreeNameRegistry = vi.fn(() => ({ exhaustedTiers: 0, names: ['nautilus'] }))
|
||||
const runtime = new OrcaRuntimeService({
|
||||
...store,
|
||||
addRetiredWorktreeName,
|
||||
getRetiredWorktreeNameRegistry
|
||||
})
|
||||
const createdWorktree = {
|
||||
path: '/tmp/workspaces/fix-login',
|
||||
head: 'def',
|
||||
branch: 'fix-login',
|
||||
isBare: false,
|
||||
isMainWorktree: false
|
||||
}
|
||||
computeWorktreePathMock.mockReturnValue(createdWorktree.path)
|
||||
ensurePathWithinWorkspaceMock.mockReturnValue(createdWorktree.path)
|
||||
vi.mocked(listWorktrees).mockResolvedValue([...MOCK_GIT_WORKTREES, createdWorktree])
|
||||
|
||||
const result = await runtime.createManagedWorktree({
|
||||
repoSelector: 'id:repo-1',
|
||||
name: 'fix-login'
|
||||
})
|
||||
|
||||
expect(result.worktree.path).toBe(createdWorktree.path)
|
||||
expect(getRetiredWorktreeNameRegistry).not.toHaveBeenCalled()
|
||||
expect(addRetiredWorktreeName).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
|
||||
@@ -1081,21 +1081,16 @@ import {
|
||||
} from '../ipc/worktree-remote'
|
||||
import {
|
||||
getBranchNameOverrideCandidate,
|
||||
getGeneratedWorktreeCreateCandidate,
|
||||
getWorktreeCreateCandidate,
|
||||
isGeneratedWorktreeCreateName,
|
||||
WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS
|
||||
} from '../worktree-create-candidates'
|
||||
import { WORKTREE_CREATE_DEDUPE_TTL_MS } from '../../shared/new-workspace/worktree-create-retry-policy'
|
||||
import { planWorktreeCreateNames } from '../worktree-create-name-plan'
|
||||
import {
|
||||
failedWorktreeCreationNeedsRetirement,
|
||||
getRetiredNameRegistryForRepo,
|
||||
retireGeneratedWorktreeName
|
||||
} from '../worktree-name-retirement'
|
||||
import {
|
||||
createRetiredNameLookup,
|
||||
type RetiredNameRegistry
|
||||
} from '../../shared/worktree/retired-name-registry'
|
||||
import type { RetiredNameRegistry } from '../../shared/worktree/retired-name-registry'
|
||||
import { normalizeSparseDirectories } from '../ipc/sparse-checkout-directories'
|
||||
import type { PtyBindingSourceExpectation, Store } from '../persistence'
|
||||
import { collectLayoutLeafIdsInOrder } from '../persistence/restoring-sessions/terminal-layout-normalization'
|
||||
@@ -24576,12 +24571,16 @@ export class OrcaRuntimeService {
|
||||
let branchConflictKind: 'local' | 'remote' | null = null
|
||||
let worktreePath = ''
|
||||
let worktreePathResolved = false
|
||||
const shouldRetireGeneratedName =
|
||||
args.nameWasGenerated === true && isGeneratedWorktreeCreateName(sanitizedName)
|
||||
const retiredNameRegistry = shouldRetireGeneratedName
|
||||
? await getRetiredNameRegistryForRepo(this.store, repo, this.store.getRepos(), settings)
|
||||
: null
|
||||
const isRetiredName = retiredNameRegistry ? createRetiredNameLookup(retiredNameRegistry) : null
|
||||
// Why the alias: `this.store` is nullable, and narrowing does not survive into the closure
|
||||
// below — capturing it once here is what keeps `loadRetiredNames` type-safe.
|
||||
const retirementStore = this.store
|
||||
const namePlan = await planWorktreeCreateNames({
|
||||
sanitizedName,
|
||||
requestedName: args.name,
|
||||
nameWasGenerated: args.nameWasGenerated,
|
||||
loadRetiredNames: () =>
|
||||
getRetiredNameRegistryForRepo(retirementStore, repo, retirementStore.getRepos(), settings)
|
||||
})
|
||||
// Why: runtime/mobile create-from-review callers should get a new workspace
|
||||
// even when the PR branch or review branch name is already in use.
|
||||
for (
|
||||
@@ -24589,21 +24588,12 @@ export class OrcaRuntimeService {
|
||||
attempts < WORKTREE_CREATE_MAX_SUFFIX_ATTEMPTS;
|
||||
suffix += 1
|
||||
) {
|
||||
effectiveSanitizedName = shouldRetireGeneratedName
|
||||
? getGeneratedWorktreeCreateCandidate(
|
||||
sanitizedName,
|
||||
suffix,
|
||||
retiredNameRegistry?.exhaustedTiers
|
||||
)
|
||||
: getWorktreeCreateCandidate(sanitizedName, suffix)
|
||||
effectiveRequestedName = shouldRetireGeneratedName
|
||||
? effectiveSanitizedName
|
||||
: args.name.trim()
|
||||
? getWorktreeCreateCandidate(args.name, suffix)
|
||||
: effectiveSanitizedName
|
||||
if (isRetiredName?.(effectiveSanitizedName)) {
|
||||
const nameCandidate = namePlan.candidateAt(suffix)
|
||||
if (!nameCandidate) {
|
||||
continue
|
||||
}
|
||||
effectiveSanitizedName = nameCandidate.sanitizedName
|
||||
effectiveRequestedName = nameCandidate.requestedName
|
||||
attempts += 1
|
||||
branchName = await resolveCreateBranchName(
|
||||
repo.path,
|
||||
@@ -24906,14 +24896,14 @@ export class OrcaRuntimeService {
|
||||
settings.refreshLocalBaseRefOnWorktreeCreate
|
||||
))) ?? {}
|
||||
} catch (error) {
|
||||
if (shouldRetireGeneratedName && failedWorktreeCreationNeedsRetirement(error)) {
|
||||
if (namePlan.retiresCreatedName && failedWorktreeCreationNeedsRetirement(error)) {
|
||||
await retireGeneratedWorktreeName(this.store, repo, settings, effectiveSanitizedName)
|
||||
}
|
||||
throw error
|
||||
}
|
||||
|
||||
// Why: fallible metadata work after creation must not leave a real workspace name reusable.
|
||||
if (shouldRetireGeneratedName) {
|
||||
if (namePlan.retiresCreatedName) {
|
||||
await retireGeneratedWorktreeName(this.store, repo, settings, effectiveSanitizedName)
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,140 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { planWorktreeCreateNames } from './worktree-create-name-plan'
|
||||
import {
|
||||
EMPTY_RETIRED_NAME_REGISTRY,
|
||||
type RetiredNameRegistry
|
||||
} from '../shared/worktree/retired-name-registry'
|
||||
|
||||
function plan(args: {
|
||||
sanitizedName: string
|
||||
requestedName?: string
|
||||
nameWasGenerated?: boolean
|
||||
retired?: RetiredNameRegistry
|
||||
}): ReturnType<typeof planWorktreeCreateNames> {
|
||||
return planWorktreeCreateNames({
|
||||
sanitizedName: args.sanitizedName,
|
||||
requestedName: args.requestedName ?? args.sanitizedName,
|
||||
nameWasGenerated: args.nameWasGenerated,
|
||||
loadRetiredNames: async () => args.retired ?? EMPTY_RETIRED_NAME_REGISTRY
|
||||
})
|
||||
}
|
||||
|
||||
/** The candidate ladder, with retired rungs dropped the way the create loops drop them. */
|
||||
function walk(
|
||||
namePlan: Awaited<ReturnType<typeof planWorktreeCreateNames>>,
|
||||
suffixes: number
|
||||
): string[] {
|
||||
const taken: string[] = []
|
||||
for (let suffix = 1; suffix <= suffixes; suffix += 1) {
|
||||
const candidate = namePlan.candidateAt(suffix)
|
||||
if (candidate) {
|
||||
taken.push(candidate.sanitizedName)
|
||||
}
|
||||
}
|
||||
return taken
|
||||
}
|
||||
|
||||
describe('planWorktreeCreateNames', () => {
|
||||
it('steps over a retired cwd for a generated name', async () => {
|
||||
const namePlan = await plan({
|
||||
sanitizedName: 'nautilus',
|
||||
nameWasGenerated: true,
|
||||
retired: { exhaustedTiers: 0, names: ['nautilus'] }
|
||||
})
|
||||
|
||||
expect(namePlan.candidateAt(1)).toBeNull()
|
||||
expect(namePlan.candidateAt(2)).toEqual({
|
||||
sanitizedName: 'nautilus-2',
|
||||
requestedName: 'nautilus-2'
|
||||
})
|
||||
})
|
||||
|
||||
it('hands a typed name the retired cwd it asked for', async () => {
|
||||
// Why: the pool is ordinary English, and people type those words on purpose. Redirecting a
|
||||
// deliberate `nautilus` to `nautilus-2` is a worse surprise than inheriting a spent cwd.
|
||||
const namePlan = await plan({
|
||||
sanitizedName: 'nautilus',
|
||||
nameWasGenerated: false,
|
||||
retired: { exhaustedTiers: 0, names: ['nautilus'] }
|
||||
})
|
||||
|
||||
expect(namePlan.candidateAt(1)).toEqual({
|
||||
sanitizedName: 'nautilus',
|
||||
requestedName: 'nautilus'
|
||||
})
|
||||
})
|
||||
|
||||
it('treats a client that omits the provenance bit as having typed the name', async () => {
|
||||
// Why: clients predating `nameWasGenerated` are indistinguishable from a typing user, and
|
||||
// pre-#14350 they kept the cwd. Guessing "generated" would silently redirect real requests.
|
||||
const loadRetiredNames = vi.fn(async () => ({ exhaustedTiers: 0, names: ['nautilus'] }))
|
||||
|
||||
const namePlan = await planWorktreeCreateNames({
|
||||
sanitizedName: 'nautilus',
|
||||
requestedName: 'nautilus',
|
||||
nameWasGenerated: undefined,
|
||||
loadRetiredNames
|
||||
})
|
||||
|
||||
expect(namePlan.candidateAt(1)).toEqual({
|
||||
sanitizedName: 'nautilus',
|
||||
requestedName: 'nautilus'
|
||||
})
|
||||
// The typed path never consults the registry, so it never pays for its backfill scan either.
|
||||
expect(loadRetiredNames).not.toHaveBeenCalled()
|
||||
expect(namePlan.retiresCreatedName).toBe(true)
|
||||
})
|
||||
|
||||
it('never loads the registry for a name the generator could not have produced', async () => {
|
||||
const loadRetiredNames = vi.fn(async () => ({ exhaustedTiers: 0, names: ['fix-login'] }))
|
||||
|
||||
const namePlan = await planWorktreeCreateNames({
|
||||
sanitizedName: 'fix-login',
|
||||
requestedName: 'fix-login',
|
||||
nameWasGenerated: undefined,
|
||||
loadRetiredNames
|
||||
})
|
||||
|
||||
expect(loadRetiredNames).not.toHaveBeenCalled()
|
||||
expect(namePlan.retiresCreatedName).toBe(false)
|
||||
expect(walk(namePlan, 2)).toEqual(['fix-login', 'fix-login-2'])
|
||||
})
|
||||
|
||||
it('keeps a typed name literal instead of canonicalizing it onto the tier ladder', async () => {
|
||||
// `nautilus-2-3` is a legacy repeat-suffixed spelling. The generated ladder folds it to
|
||||
// `nautilus-4`; a name the user typed must keep the directory it asked for.
|
||||
const typed = await plan({ sanitizedName: 'nautilus-2-3' })
|
||||
const generated = await plan({ sanitizedName: 'nautilus-2-3', nameWasGenerated: true })
|
||||
|
||||
expect(walk(typed, 2)).toEqual(['nautilus-2-3', 'nautilus-2-3-2'])
|
||||
expect(walk(generated, 2)).toEqual(['nautilus-4', 'nautilus-5'])
|
||||
})
|
||||
|
||||
it('suffixes the display name in step with a typed name a path collision pushed along', async () => {
|
||||
const namePlan = await plan({ sanitizedName: 'nautilus', requestedName: 'Nautilus' })
|
||||
|
||||
expect(namePlan.candidateAt(2)).toEqual({
|
||||
sanitizedName: 'nautilus-2',
|
||||
requestedName: 'Nautilus-2'
|
||||
})
|
||||
})
|
||||
|
||||
it('starts a generated name past the compaction watermark rather than walking every spent tier', async () => {
|
||||
const namePlan = await plan({
|
||||
sanitizedName: 'nautilus',
|
||||
nameWasGenerated: true,
|
||||
retired: { exhaustedTiers: 3, names: [] }
|
||||
})
|
||||
|
||||
expect(walk(namePlan, 2)).toEqual(['nautilus-4', 'nautilus-5'])
|
||||
})
|
||||
|
||||
it('records a pool-shaped name as spent however the client labelled it', async () => {
|
||||
await expect(plan({ sanitizedName: 'nautilus' })).resolves.toMatchObject({
|
||||
retiresCreatedName: true
|
||||
})
|
||||
await expect(
|
||||
plan({ sanitizedName: 'nautilus', nameWasGenerated: true })
|
||||
).resolves.toMatchObject({ retiresCreatedName: true })
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,81 @@
|
||||
import {
|
||||
getGeneratedWorktreeCreateCandidate,
|
||||
getWorktreeCreateCandidate,
|
||||
isGeneratedWorktreeCreateName
|
||||
} from './worktree-create-candidates'
|
||||
import type { RetiredNameRegistry } from '../shared/worktree/retired-name-registry'
|
||||
import { createRetiredNameLookup } from '../shared/worktree/retired-name-registry'
|
||||
|
||||
export type WorktreeCreateNameCandidate = {
|
||||
/** Filesystem-safe name; drives the workspace path and branch. */
|
||||
sanitizedName: string
|
||||
/** Display name the workspace is labelled with. */
|
||||
requestedName: string
|
||||
}
|
||||
|
||||
export type WorktreeCreateNamePlan = {
|
||||
/** True when the created name must be recorded as spent once creation commits. */
|
||||
readonly retiresCreatedName: boolean
|
||||
/** Null when this suffix lands on a retired cwd; the caller skips it without spending an attempt. */
|
||||
candidateAt(suffix: number): WorktreeCreateNameCandidate | null
|
||||
}
|
||||
|
||||
export type WorktreeCreateNamePlanArgs = {
|
||||
sanitizedName: string
|
||||
/** The raw name the caller asked for; empty falls back to the sanitized candidate. */
|
||||
requestedName: string
|
||||
/** `true` only from clients that know the name came from the creature-name generator. Every
|
||||
* producer strips it when false, so absent means typed — from a current client as much as an
|
||||
* older one — and must be read that way. */
|
||||
nameWasGenerated: boolean | undefined
|
||||
loadRetiredNames: () => Promise<RetiredNameRegistry>
|
||||
}
|
||||
|
||||
/** Resolves what a create attempt may call itself, shared by the three create paths (local IPC, SSH,
|
||||
* runtime) so a retirement rule cannot hold on one and not the others.
|
||||
*
|
||||
* Two decisions, deliberately split — PR #14350 conflated them, and STA-4471 is the fallout:
|
||||
*
|
||||
* - *Which cwds a create may land on* follows the client's `nameWasGenerated` bit. A generated name
|
||||
* is one of ours: nothing was chosen, so folding it onto the canonical tier ladder and stepping
|
||||
* over cwds this host already spent costs the user nothing and keeps the previous occupant's
|
||||
* Claude/Codex history out of the new workspace. A name the user typed is a request — it keeps
|
||||
* its literal spelling, its plain `-2`, `-3` suffixes, and the cwd it asked for even when that
|
||||
* cwd is retired. Asking for `nautilus` and silently getting `nautilus-2` is the worse surprise.
|
||||
* - *Whether the created name is recorded as spent* is decided by the host alone, from the name's
|
||||
* shape. A client that predates the provenance bit still hands back pool-shaped names, and a
|
||||
* registry that never learns about them lets the next generated create collide with one.
|
||||
* `addRetiredWorktreeName` self-gates on pool shape, so a coined name (`fix-login`) is never
|
||||
* recorded — recording is only ever a write, and never redirects anybody.
|
||||
*
|
||||
* Only the generated ladder reads the registry, so a typed name and a name outside the pool alike
|
||||
* skip the load — and with it `ensureRetiredWorktreeNamesBackfilled`'s scan (STA-4473). */
|
||||
export async function planWorktreeCreateNames(
|
||||
args: WorktreeCreateNamePlanArgs
|
||||
): Promise<WorktreeCreateNamePlan> {
|
||||
const isPoolShaped = isGeneratedWorktreeCreateName(args.sanitizedName)
|
||||
const usesGeneratedLadder = args.nameWasGenerated === true && isPoolShaped
|
||||
// Only the generated ladder reads the registry, so a typed name skips the load and its backfill.
|
||||
const registry = usesGeneratedLadder ? await args.loadRetiredNames() : null
|
||||
const isRetiredName = registry ? createRetiredNameLookup(registry) : null
|
||||
const trimmedRequestedName = args.requestedName.trim()
|
||||
return {
|
||||
retiresCreatedName: isPoolShaped,
|
||||
candidateAt(suffix) {
|
||||
// Skipping is pure string math, so a run of retired tiers costs no I/O and no attempt budget.
|
||||
const sanitizedName = usesGeneratedLadder
|
||||
? getGeneratedWorktreeCreateCandidate(args.sanitizedName, suffix, registry?.exhaustedTiers)
|
||||
: getWorktreeCreateCandidate(args.sanitizedName, suffix)
|
||||
if (isRetiredName?.(sanitizedName)) {
|
||||
return null
|
||||
}
|
||||
return {
|
||||
sanitizedName,
|
||||
requestedName:
|
||||
!usesGeneratedLadder && trimmedRequestedName
|
||||
? getWorktreeCreateCandidate(args.requestedName, suffix)
|
||||
: sanitizedName
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user