From a2946f0a6505fc6c29eed7f726e02cd06aee48db Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 18 Oct 2026 00:51:41 -0700 Subject: [PATCH] chore(worktree): composeWorktreeId plus a ratchet on hand-built worktree ids (#30238) Worktree ids built as `${repo.id}::${path}` by hand are how one worktree got two spellings (#30150). New code composes them through src/shared/worktree/id.ts; the ratchet counts the remaining hand-built ids per file (18) and may only go down. Six byte-identical `${repo.id}::${repo.path}` sites move to getRepoMainWorktreeId. --- .github/workflows/pr.yml | 4 + .../check-worktree-id-composer-ratchet.mjs | 96 +++++++++++++++++++ ...heck-worktree-id-composer-ratchet.test.mjs | 34 +++++++ config/scripts/pr-preflight-gates.test.mjs | 1 + config/worktree-id-composer-baseline.txt | 19 ++++ package.json | 1 + src/main/folder-workspace-meta-presence.ts | 12 ++- .../workspace-cleanup-folder-workspaces.ts | 7 +- .../ipc/worktrees/folder-workspace-model.ts | 7 +- src/main/runtime/runtime-folder-workspace.ts | 7 +- src/main/usage-worktree-metadata.ts | 8 +- .../components/settings/McpConfigSection.tsx | 4 +- src/shared/worktree/id.test.ts | 14 +++ src/shared/worktree/id.ts | 7 +- 14 files changed, 207 insertions(+), 14 deletions(-) create mode 100644 config/scripts/check-worktree-id-composer-ratchet.mjs create mode 100644 config/scripts/check-worktree-id-composer-ratchet.test.mjs create mode 100644 config/worktree-id-composer-baseline.txt diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 117eda638b3..8f0b532c75a 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -400,6 +400,10 @@ jobs: if: needs.code_paths.outputs.static_analysis == 'true' run: node config/scripts/check-path-literal-ratchet.mjs + - name: Enforce worktree-id composer ratchet + if: needs.code_paths.outputs.static_analysis == 'true' + run: node config/scripts/check-worktree-id-composer-ratchet.mjs + - name: Check Node runtime pin if: needs.code_paths.outputs.static_analysis == 'true' run: node config/scripts/check-node-runtime-pin.mjs diff --git a/config/scripts/check-worktree-id-composer-ratchet.mjs b/config/scripts/check-worktree-id-composer-ratchet.mjs new file mode 100644 index 00000000000..14af4a11a37 --- /dev/null +++ b/config/scripts/check-worktree-id-composer-ratchet.mjs @@ -0,0 +1,96 @@ +import fs from 'node:fs' +import path from 'node:path' +import process from 'node:process' +import { pathToFileURL } from 'node:url' +import { + diffCounts, + parseCountBaseline, + pruneCountBaseline, + readTrackedSources, + totalCount +} from './ratchet-baseline.mjs' + +// Ratchet on worktree ids built by hand (`${repo.id}::${path}`) instead of through the composers in +// src/shared/worktree/id.ts. About 22 hand-built ids each pick whichever path spelling is at hand, +// which is how one worktree ends up with two ids (#30150). Counts per file may only go down. + +const BASELINE_PATH = 'config/worktree-id-composer-baseline.txt' +const SCAN_ROOTS = ['src', 'mobile/src'] +const COMPOSER_FILE = 'src/shared/worktree/id.ts' +const HEADER = [ + '# Hand-built worktree ids (`${repoId}::${path}`) per file, outside src/shared/worktree/id.ts.', + '# This is a RATCHET: counts may only go DOWN. Compose ids with composeWorktreeId or', + '# getRepoMainWorktreeId instead. Prune after a fix: pnpm check:worktree-id-composer-ratchet --prune' +] + +// A repo-id interpolation, the `::` separator (literal or constant), then a path interpolation. +const HAND_BUILT_WORKTREE_ID = + /\$\{[^{}]*\b(?:repo\.id|repoId|repo_id)\b[^{}]*\}(?:::|\$\{\s*WORKTREE_ID_SEPARATOR\s*\})\$\{[^{}]*[pP]ath\b[^{}]*\}/g +// Block and line comments; `://` inside a string (a URL) is not a line comment. +const COMMENT = /\/\*[\s\S]*?\*\/|(? rel.startsWith(`${root}/`)) && + /\.(ts|tsx)$/.test(rel) && + !/\.(test|spec)\.tsx?$/.test(rel) && + !/(?:^|[/.-])(?:test-fixture|test-support|test-harness|fixtures?)(?:[/.-]|$)|__fixtures__/.test( + rel + ) && + rel !== COMPOSER_FILE + ) +} + +export function countHandBuiltWorktreeIds(sourceText) { + return sourceText.replace(COMMENT, ' ').match(HAND_BUILT_WORKTREE_ID)?.length ?? 0 +} + +export function collectHandBuiltWorktreeIdCounts(root = process.cwd()) { + const counts = new Map() + for (const [rel, source] of readTrackedSources(root, SCAN_ROOTS, isScannedPath)) { + const count = countHandBuiltWorktreeIds(source) + if (count > 0) { + counts.set(rel, count) + } + } + return counts +} + +export function main(root = process.cwd(), arg = undefined) { + const current = collectHandBuiltWorktreeIdCounts(root) + if (arg === '--prune') { + const total = pruneCountBaseline(root, BASELINE_PATH, HEADER, current) + console.log(`worktree-id-composer baseline pruned to ${total}.`) + return 0 + } + const baselineFile = path.join(root, BASELINE_PATH) + const baseline = parseCountBaseline( + fs.existsSync(baselineFile) ? fs.readFileSync(baselineFile, 'utf8') : '' + ) + const { grown, shrunk } = diffCounts(current, baseline) + for (const { file, now, allowed } of grown) { + console.error(`${file}: ${now} hand-built worktree id(s), baseline allows ${allowed}.`) + } + if (grown.length > 0) { + console.error( + 'Compose worktree ids with composeWorktreeId / getRepoMainWorktreeId (src/shared/worktree/id.ts).' + ) + } + for (const { file, now, allowed } of shrunk) { + console.error(`${file}: now ${now}, baseline ${allowed}.`) + } + if (shrunk.length > 0) { + console.error( + 'The baseline may only shrink. Fix it: pnpm check:worktree-id-composer-ratchet --prune' + ) + } + if (grown.length + shrunk.length > 0) { + return 1 + } + console.log(`worktree-id-composer ratchet OK — ${totalCount(current)} grandfathered id(s).`) + return 0 +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + process.exit(main(process.cwd(), process.argv[2])) +} diff --git a/config/scripts/check-worktree-id-composer-ratchet.test.mjs b/config/scripts/check-worktree-id-composer-ratchet.test.mjs new file mode 100644 index 00000000000..52e74bf7c16 --- /dev/null +++ b/config/scripts/check-worktree-id-composer-ratchet.test.mjs @@ -0,0 +1,34 @@ +import { describe, expect, it } from 'vitest' + +import { countHandBuiltWorktreeIds, isScannedPath } from './check-worktree-id-composer-ratchet.mjs' + +describe('countHandBuiltWorktreeIds', () => { + it('counts a repo id joined to a path by a literal or constant separator', () => { + expect(countHandBuiltWorktreeIds('const id = `${repo.id}::${worktree.path}`')).toBe(1) + expect(countHandBuiltWorktreeIds('f(`${repoId}::${gitWorktree.path}`)')).toBe(1) + expect( + countHandBuiltWorktreeIds('`${args.repoId}${WORKTREE_ID_SEPARATOR}${newPath}${suffix}`') + ).toBe(1) + expect(countHandBuiltWorktreeIds('[`${repo.id}::${a.path}`, `${repoId}::${b.path}`]')).toBe(2) + }) + + it('ignores comments and non-worktree keys', () => { + expect( + countHandBuiltWorktreeIds('// ids are `${repoId}::${path}`\n/* `${repo.id}::${p.path}` */') + ).toBe(0) + expect(countHandBuiltWorktreeIds('`${owner}::${repoName}`')).toBe(0) + expect(countHandBuiltWorktreeIds('composeWorktreeId(repo.id, worktree.path)')).toBe(0) + }) +}) + +describe('isScannedPath', () => { + it('scans production sources outside the composer module', () => { + expect(isScannedPath('src/main/runtime/a.ts')).toBe(true) + expect(isScannedPath('mobile/src/session/a.tsx')).toBe(true) + expect(isScannedPath('src/shared/worktree/id.ts')).toBe(false) + expect(isScannedPath('src/main/a.test.ts')).toBe(false) + expect(isScannedPath('src/main/runtime/__fixtures__/a.ts')).toBe(false) + expect(isScannedPath('src/renderer/src/store/host-twin.test-fixture.ts')).toBe(false) + expect(isScannedPath('config/scripts/a.mjs')).toBe(false) + }) +}) diff --git a/config/scripts/pr-preflight-gates.test.mjs b/config/scripts/pr-preflight-gates.test.mjs index 1132cb19f17..9c0c7a10308 100644 --- a/config/scripts/pr-preflight-gates.test.mjs +++ b/config/scripts/pr-preflight-gates.test.mjs @@ -245,6 +245,7 @@ it('pins every foreground and background step to its selected phase', () => { ['Enforce layout-writer ratchets', staticPhase], ['Enforce deprecated runtime-owner importer ratchet', staticPhase], ['Enforce path-literal ratchet', staticPhase], + ['Enforce worktree-id composer ratchet', staticPhase], ['Check Node runtime pin', staticPhase], ['Decide whether the orcad terminal smoke runs', staticPhase], ['Verify the generated RPC params catalog', staticPhase], diff --git a/config/worktree-id-composer-baseline.txt b/config/worktree-id-composer-baseline.txt new file mode 100644 index 00000000000..e1811efef57 --- /dev/null +++ b/config/worktree-id-composer-baseline.txt @@ -0,0 +1,19 @@ +# Hand-built worktree ids (`${repoId}::${path}`) per file, outside src/shared/worktree/id.ts. +# This is a RATCHET: counts may only go DOWN. Compose ids with composeWorktreeId or +# getRepoMainWorktreeId instead. Prune after a fix: pnpm check:worktree-id-composer-ratchet --prune +1 src/main/ipc/workspace-cleanup-scan.ts +1 src/main/ipc/worktree-folder-rename-target.ts +1 src/main/ipc/worktree-metadata-merge.ts +1 src/main/ipc/worktree-push-target-reconciliation.ts +2 src/main/ipc/worktree-remote.ts +1 src/main/ipc/worktrees/listing/detected-provider-listing.ts +1 src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts +2 src/main/ipc/worktrees/removal/nested-worktree-removal.ts +1 src/main/memory/hydrate-local-pty-registry.ts +1 src/main/runtime/repo-worktree-row-resolution.ts +1 src/main/runtime/runtime-local-worktree-materialization.ts +1 src/main/runtime/runtime-managed-worktree-queries.ts +1 src/main/runtime/runtime-provisioned-root-adoption.ts +1 src/main/workspace-space-repo-scan.ts +1 src/main/worktree-lineage-pruning.ts +1 src/renderer/src/components/sidebar/DeleteNestedWorktreesDialog.tsx diff --git a/package.json b/package.json index 208e228458a..43b45f434cf 100644 --- a/package.json +++ b/package.json @@ -40,6 +40,7 @@ "check:ts-nocheck-ratchet": "node config/scripts/check-ts-nocheck-ratchet.mjs", "check:deprecated-runtime-owner-ratchet": "node config/scripts/check-deprecated-runtime-owner-ratchet.mjs", "check:path-literal-ratchet": "node config/scripts/check-path-literal-ratchet.mjs", + "check:worktree-id-composer-ratchet": "node config/scripts/check-worktree-id-composer-ratchet.mjs", "check:runtime-electron-ratchet": "node config/scripts/check-runtime-electron-ratchet.mjs", "check:focus-read-ratchet": "node config/scripts/check-focus-read-ratchet.mjs", "check:layout-writer-ratchets": "node config/scripts/check-layout-writer-ratchets.mjs", diff --git a/src/main/folder-workspace-meta-presence.ts b/src/main/folder-workspace-meta-presence.ts index a344523350c..8e81b44f279 100644 --- a/src/main/folder-workspace-meta-presence.ts +++ b/src/main/folder-workspace-meta-presence.ts @@ -2,7 +2,11 @@ import type { ExecutionHostId } from '../shared/execution-host' import { isFolderRepo } from '../shared/repo-kind' import type { Repo } from '../shared/repo-types' import type { WorktreeMeta } from '../shared/worktree/meta-types' -import { FOLDER_WORKSPACE_INSTANCE_SEPARATOR, getRepoIdFromWorktreeId } from '../shared/worktree/id' +import { + FOLDER_WORKSPACE_INSTANCE_SEPARATOR, + getRepoIdFromWorktreeId, + getRepoMainWorktreeId +} from '../shared/worktree/id' type FolderWorkspaceMetaReader = { getRepos: () => Repo[] @@ -30,7 +34,9 @@ export function isRemovedFolderWorkspaceInstance( (repo) => repo.id === repoId && isFolderRepo(repo) && - worktreeId.startsWith(`${repo.id}::${repo.path}${FOLDER_WORKSPACE_INSTANCE_SEPARATOR}`) + worktreeId.startsWith( + `${getRepoMainWorktreeId(repo)}${FOLDER_WORKSPACE_INSTANCE_SEPARATOR}` + ) ) if (!isFolderInstance) { return false @@ -51,6 +57,6 @@ export function findExtraFolderWorkspaceIds( repo: Pick, metaById: Record ): string[] { - const prefix = `${repo.id}::${repo.path}${FOLDER_WORKSPACE_INSTANCE_SEPARATOR}` + const prefix = `${getRepoMainWorktreeId(repo)}${FOLDER_WORKSPACE_INSTANCE_SEPARATOR}` return Object.keys(metaById).filter((worktreeId) => worktreeId.startsWith(prefix)) } diff --git a/src/main/ipc/workspace-cleanup-folder-workspaces.ts b/src/main/ipc/workspace-cleanup-folder-workspaces.ts index 7d57db30ec1..37ad3986578 100644 --- a/src/main/ipc/workspace-cleanup-folder-workspaces.ts +++ b/src/main/ipc/workspace-cleanup-folder-workspaces.ts @@ -3,7 +3,10 @@ import { createFolderWorktree } from '../repo-worktrees' import type { Repo } from '../../shared/repo-types' import type { Worktree } from '../../shared/worktree/types' import type { WorktreeMeta } from '../../shared/worktree/meta-types' -import { FOLDER_WORKSPACE_INSTANCE_SEPARATOR } from '../../shared/worktree/id' +import { + FOLDER_WORKSPACE_INSTANCE_SEPARATOR, + getRepoMainWorktreeId +} from '../../shared/worktree/id' import { mergeWorktree } from './worktree-logic' import { getRepoOwnedWorktreeMeta } from '../worktree-metadata-ownership' @@ -12,7 +15,7 @@ export function listWorkspaceCleanupFolderWorkspaces( repo: Repo, repoOwnerCount: number ): Worktree[] { - const rootId = `${repo.id}::${repo.path}` + const rootId = getRepoMainWorktreeId(repo) const instancePrefix = `${rootId}${FOLDER_WORKSPACE_INSTANCE_SEPARATOR}` const allMeta: Readonly> = store.getAllWorktreeMeta() const worktreeIds = [ diff --git a/src/main/ipc/worktrees/folder-workspace-model.ts b/src/main/ipc/worktrees/folder-workspace-model.ts index 83738697bba..fbbfb61f157 100644 --- a/src/main/ipc/worktrees/folder-workspace-model.ts +++ b/src/main/ipc/worktrees/folder-workspace-model.ts @@ -1,12 +1,15 @@ import { randomUUID } from 'node:crypto' import type { Repo } from '../../../shared/repo-types' import { DEFAULT_WORKSPACE_STATUS_ID } from '../../../shared/workspace-statuses' -import { FOLDER_WORKSPACE_INSTANCE_SEPARATOR } from '../../../shared/worktree/id' +import { + FOLDER_WORKSPACE_INSTANCE_SEPARATOR, + getRepoMainWorktreeId +} from '../../../shared/worktree/id' import type { WorktreeMeta } from '../../../shared/worktree/meta-types' import type { Worktree } from '../../../shared/worktree/types' export function getFolderWorkspaceRootId(repo: Repo): string { - return `${repo.id}::${repo.path}` + return getRepoMainWorktreeId(repo) } export function getFolderWorkspaceInstanceId(repo: Repo, instanceId: string): string { diff --git a/src/main/runtime/runtime-folder-workspace.ts b/src/main/runtime/runtime-folder-workspace.ts index cce0d0f73ac..8d9c3677361 100644 --- a/src/main/runtime/runtime-folder-workspace.ts +++ b/src/main/runtime/runtime-folder-workspace.ts @@ -2,11 +2,14 @@ import { DEFAULT_WORKSPACE_STATUS_ID } from '../../shared/workspace-statuses' import type { Repo } from '../../shared/repo-types' import type { WorktreeMeta } from '../../shared/worktree/meta-types' import type { Worktree } from '../../shared/worktree/types' -import { FOLDER_WORKSPACE_INSTANCE_SEPARATOR } from '../../shared/worktree/id' +import { + FOLDER_WORKSPACE_INSTANCE_SEPARATOR, + getRepoMainWorktreeId +} from '../../shared/worktree/id' import { normalizeWorkspaceCreatorProvenance } from '../../shared/workspace-creator-provenance' export function getRuntimeFolderWorkspaceRootId(repo: Repo): string { - return `${repo.id}::${repo.path}` + return getRepoMainWorktreeId(repo) } export function getRuntimeFolderWorkspaceInstanceId(repo: Repo, instanceId: string): string { diff --git a/src/main/usage-worktree-metadata.ts b/src/main/usage-worktree-metadata.ts index 4b571a05708..2060f22a4f7 100644 --- a/src/main/usage-worktree-metadata.ts +++ b/src/main/usage-worktree-metadata.ts @@ -1,6 +1,10 @@ import { basename } from 'node:path' import type { Repo } from '../shared/repo-types' -import { splitWorktreeId, splitWorktreeIdForFilesystem } from '../shared/worktree/id' +import { + getRepoMainWorktreeId, + splitWorktreeId, + splitWorktreeIdForFilesystem +} from '../shared/worktree/id' import { isFolderRepo } from '../shared/repo-kind' import type { Store } from './persistence' @@ -35,7 +39,7 @@ export function loadKnownUsageWorktreesByRepo( for (const repo of localRepos) { worktreesByRepo.set(repo.id, [ { - worktreeId: `${repo.id}::${repo.path}`, + worktreeId: getRepoMainWorktreeId(repo), path: repo.path, displayName: repo.displayName || getDefaultUsageWorktreeLabel(repo.path) } diff --git a/src/renderer/src/components/settings/McpConfigSection.tsx b/src/renderer/src/components/settings/McpConfigSection.tsx index 9ed02a0c3a3..9c7ec7632fb 100644 --- a/src/renderer/src/components/settings/McpConfigSection.tsx +++ b/src/renderer/src/components/settings/McpConfigSection.tsx @@ -4,7 +4,7 @@ import { toast } from 'sonner' import { useMountedRef } from '@/hooks/useMountedRef' import type { Repo } from '../../../../shared/repo-types' import type { Worktree } from '../../../../shared/worktree/types' -import { getRepoIdFromWorktreeId } from '../../../../shared/worktree/id' +import { composeWorktreeId, getRepoIdFromWorktreeId } from '../../../../shared/worktree/id' import { getRepoExecutionHostId, getRepoSshConnectionId, @@ -94,7 +94,7 @@ export function McpConfigSection({ repo }: McpConfigSectionProps): React.JSX.Ele return ( worktreesForRepo.find((worktree) => worktree.isMainWorktree) ?? worktreesForRepo.find((worktree) => worktree.path === repo.path) ?? - worktreesForRepo[0] ?? { id: `${repo.id}::${repo.path}`, path: repo.path } + worktreesForRepo[0] ?? { id: composeWorktreeId(repo.id, repo.path), path: repo.path } ) }, [activeWorktreeId, allWorktreesForRepo, repo.id, repo.path, worktreesForRepo]) const targetWorktreeId = targetWorktree.id diff --git a/src/shared/worktree/id.test.ts b/src/shared/worktree/id.test.ts index c357586def5..b946a5de2dd 100644 --- a/src/shared/worktree/id.test.ts +++ b/src/shared/worktree/id.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest' import { WORKTREE_ID_SEPARATOR, + composeWorktreeId, getRepoIdFromWorktreeId, getRepoMainWorktreeId, getWorktreePathBasenameFromId, @@ -23,6 +24,19 @@ describe('getRepoMainWorktreeId', () => { }) }) +describe('composeWorktreeId', () => { + it('keeps the path spelling exactly as given', () => { + for (const path of [ + '/srv/Repo/', + 'C:\\Users\\me\\repo', + '//wsl$/Ubuntu/home/a', + '/private/tmp/x' + ]) { + expect(composeWorktreeId('repo-1', path)).toBe(`repo-1${WORKTREE_ID_SEPARATOR}${path}`) + } + }) +}) + describe('getRepoIdFromWorktreeId', () => { it('returns the repo id for a canonical worktree id', () => { expect(getRepoIdFromWorktreeId('repo-123::/abs/path')).toBe('repo-123') diff --git a/src/shared/worktree/id.ts b/src/shared/worktree/id.ts index f8ef4f81d4f..b7525cd4003 100644 --- a/src/shared/worktree/id.ts +++ b/src/shared/worktree/id.ts @@ -19,7 +19,12 @@ const FOLDER_WORKSPACE_INSTANCE_SUFFIX = new RegExp( * runtimes reject it with `worktree_id_requires_full_path` (#16447). */ export function getRepoMainWorktreeId(repo: Pick): string { - return `${repo.id}${WORKTREE_ID_SEPARATOR}${repo.path}` + return composeWorktreeId(repo.id, repo.path) +} + +/** Worktree id for a checkout, spelled exactly as given: ids are never reassigned by a fold. */ +export function composeWorktreeId(repoId: string, worktreePath: string): string { + return `${repoId}${WORKTREE_ID_SEPARATOR}${worktreePath}` } export function getRepoIdFromWorktreeId(worktreeId: string): string {