From 021de1a556fff293f21f3796ed272e9731e442e4 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 16 Aug 2026 18:32:18 -0700 Subject: [PATCH] docs(worktrees): pin why the host identity separator is safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The store index recovers a workspace id from its own identity key rather than re-reading `worktree.id` (retained selectors assert that getter is read exactly once per snapshot), so the split has to be exact, not best-effort. It is: an execution host id is `local` or a fixed `ssh:`/`runtime:` prefix plus an encodeURIComponent-escaped part, which encodes `|` as %7C — so the host half can never contain the separator while the workspace id, being a repo id plus a filesystem path, may. Replaces a hand-wave in the comment with that reasoning and adds a test pinning it, including a workspace id that does contain `|`. --- .../worktree/host-qualified-identity.test.ts | 61 +++++++++++++++++++ .../worktree/host-qualified-identity.ts | 15 +++-- 2 files changed, 72 insertions(+), 4 deletions(-) create mode 100644 src/shared/worktree/host-qualified-identity.test.ts diff --git a/src/shared/worktree/host-qualified-identity.test.ts b/src/shared/worktree/host-qualified-identity.test.ts new file mode 100644 index 00000000000..e0495aa19be --- /dev/null +++ b/src/shared/worktree/host-qualified-identity.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, it } from 'vitest' +import { toRuntimeExecutionHostId, toSshExecutionHostId } from '../execution-host' +import { + composeWorktreeHostIdentity, + getWorktreeHostIdentity, + getWorktreeIdFromHostIdentity +} from './host-qualified-identity' + +const WORKTREE_ID = 'repo-1::/work/orca' + +describe('worktree host identity', () => { + it('separates two hosts that publish the same id', () => { + expect(getWorktreeHostIdentity({ id: WORKTREE_ID, hostId: 'local' })).not.toBe( + getWorktreeHostIdentity({ id: WORKTREE_ID, hostId: 'ssh:build-box' }) + ) + }) + + it('gives an unqualified row its own bucket', () => { + expect(getWorktreeHostIdentity({ id: WORKTREE_ID, hostId: undefined })).not.toBe( + getWorktreeHostIdentity({ id: WORKTREE_ID, hostId: 'local' }) + ) + }) + + // Why this matters: the store index recovers the workspace id from its own key + // rather than re-reading `worktree.id`, so a host id containing the separator + // would silently truncate every id in the index. + it('round-trips the id back out for every host shape', () => { + const hostIds = [ + undefined, + 'local', + toSshExecutionHostId('build-box'), + // Real SSH aliases carry '@' and ':'; runtime ids are uuids. + toSshExecutionHostId('deploy@10.0.0.4:2222'), + toRuntimeExecutionHostId('03ef704c-b180-4b10-998d-e28fbd5de9a3') + ] as const + + for (const hostId of hostIds) { + expect(getWorktreeIdFromHostIdentity(composeWorktreeHostIdentity(hostId, WORKTREE_ID))).toBe( + WORKTREE_ID + ) + } + }) + + it('round-trips a workspace id that itself contains the separator', () => { + // A workspace id is a repo id plus a filesystem path, so it may contain + // anything; only the host half has to be separator-free. + const awkwardId = 'repo-1::/work/a|b/orca' + + expect( + getWorktreeIdFromHostIdentity( + composeWorktreeHostIdentity(toSshExecutionHostId('build-box'), awkwardId) + ) + ).toBe(awkwardId) + }) + + it('keeps a host id free of the separator by construction', () => { + // encodeURIComponent escapes '|' as %7C, which is what makes the split exact. + expect(toSshExecutionHostId('we|rd')).not.toContain('|') + expect(toRuntimeExecutionHostId('we|rd')).not.toContain('|') + }) +}) diff --git a/src/shared/worktree/host-qualified-identity.ts b/src/shared/worktree/host-qualified-identity.ts index 434bacbae10..7e144c6336d 100644 --- a/src/shared/worktree/host-qualified-identity.ts +++ b/src/shared/worktree/host-qualified-identity.ts @@ -7,7 +7,13 @@ import type { Worktree } from './types' * Printable on purpose: these identities become React keys and DOM attribute * values, and HTML attribute parsing replaces U+0000 with U+FFFD — a NUL * separator would not survive the round trip an anchor comparison depends on. - * The host comes first so the variable-length id absorbs any `|` of its own. + * + * `|` is safe as a delimiter because an execution host id cannot contain one: + * it is `local`, or a fixed `ssh:`/`runtime:` prefix followed by an + * `encodeURIComponent`-escaped part (see `toSshExecutionHostId`), which encodes + * `|` as `%7C`. Putting the host first therefore makes the split unambiguous + * even though the workspace id — a repo id and a filesystem path — may contain + * anything. */ const HOST_SEPARATOR = '|' @@ -35,9 +41,10 @@ export function composeWorktreeHostIdentity( /** * The workspace id back out of an identity. * - * Host-first ordering makes this exact: the host cannot contain the separator, - * so everything after the first one is the id — which lets an index recover the - * id without re-reading it off the row. + * Exact, not best-effort: the host cannot contain the separator (see above), so + * everything after the first one is the id. That lets an index recover the id + * from its own key instead of re-reading `worktree.id`, which matters because + * retained selectors assert that getter is read exactly once per snapshot. */ export function getWorktreeIdFromHostIdentity(identity: string): string { return identity.slice(identity.indexOf(HOST_SEPARATOR) + 1)