From cccc1e31ca9c1208af81033be8468b4be617bb00 Mon Sep 17 00:00:00 2001 From: Chris <6528986+chrissena@users.noreply.github.com> Date: Wed, 26 Aug 2026 10:54:54 +0100 Subject: [PATCH] fix(config): keep a relative worktreeParent inside its bare-repository container A relative `worktreeParent` is resolved against `getMainWorktreeRoot()`, which for a `.bare/` container layout is the container directory itself. Configs written before that anchor moved (v1.15.0, #25) hold worktree-relative values such as `"../pr"`, meaning `/pr` when read from `/main`. Resolved against the container those escape it entirely, and the new worktree was silently created beside the container with the container segment dropped: /main + "../pr" -> /home/chris/workspace/pr/pr2897. expected -> /home/chris/workspace/syrf/pr/pr2897. Add `resolveRelativeWorktreeParent()`: when the anchor is a bare-repository container and a relative parent resolves outside it, drop the escaping leading segments so the parent stays inside the container, and warn with the offending value, the path it would have used and the suggested replacement. Absolute `worktreeParent` values and `worktreeParentAnchor: "repo-root"` are never clamped, so placing worktrees outside a container stays possible - it just has to be said unambiguously. Non-container anchors are untouched: the default `".."` still resolves to the repository's sibling directory. Claude-Session: https://claude.ai/code/session_01R84soQ82VQsGznotpWTvHo --- README.md | 55 +++-- .../worktree-layout.integration.test.ts | 89 ++++++++ src/lib/config.test.ts | 199 ++++++++++++++++++ src/lib/config.ts | 72 ++++++- 4 files changed, 400 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 750fb70..ccaf0c0 100644 --- a/README.md +++ b/README.md @@ -411,20 +411,47 @@ wt init --local # Personal repo overrides ### Configuration Options -| Option | Type | Default | Description | -| ----------------- | -------- | --------------------- | -------------------------------------------------------------------------------------------------------------------------------- | -| `baseBranch` | string | `"main"` | Base branch for new PRs | -| `draftPr` | boolean | `false` | Create PRs as drafts by default | -| `worktreePattern` | string | `"{repo}.pr{number}"` | Naming pattern. Placeholders: `{repo}`, `{number}`, `{branch}`, `{slug}`. Doubled/trailing separators are cleaned automatically. | -| `worktreeParent` | string | `".."` | Parent directory for worktrees. If inside the repo, the directory is auto-created and added to `.gitignore`. | -| `branchPrefix` | string | `"feat"` | Prefix for auto-generated branch names | -| `sharedRepos` | string[] | `[]` | Sibling repos to also create worktrees for | -| `preferredEditor` | string | `"vscode"` | Editor: `"vscode"`, `"cursor"`, or `"auto"` | -| `syncPatterns` | string[] | `[]` | Patterns to sync between worktrees | -| `previewLabel` | string | `"preview"` | Label to highlight in PR browser | -| `ai` | object | `{}` | AI content generation settings | -| `hooks` | object | `{}` | Lifecycle hook commands | -| `logging` | object | `{}` | Logging configuration | +| Option | Type | Default | Description | +| ---------------------- | -------- | --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `baseBranch` | string | `"main"` | Base branch for new PRs | +| `draftPr` | boolean | `false` | Create PRs as drafts by default | +| `worktreePattern` | string | `"{repo}.pr{number}"` | Naming pattern. Placeholders: `{repo}`, `{number}`, `{branch}`, `{slug}`. Doubled/trailing separators are cleaned automatically. | +| `worktreeParent` | string | `".."` | Parent directory for worktrees. If inside the repo, the directory is auto-created and added to `.gitignore`. See [Bare-repository containers](#bare-repository-containers). | +| `worktreeParentAnchor` | string | `"main-worktree"` | What a relative `worktreeParent` is resolved against: `"main-worktree"` (the main worktree root, or the container root for a `.bare/` layout) or `"repo-root"` (the worktree the command was run from). | +| `branchPrefix` | string | `"feat"` | Prefix for auto-generated branch names | +| `sharedRepos` | string[] | `[]` | Sibling repos to also create worktrees for | +| `preferredEditor` | string | `"vscode"` | Editor: `"vscode"`, `"cursor"`, or `"auto"` | +| `syncPatterns` | string[] | `[]` | Patterns to sync between worktrees | +| `previewLabel` | string | `"preview"` | Label to highlight in PR browser | +| `ai` | object | `{}` | AI content generation settings | +| `hooks` | object | `{}` | Lifecycle hook commands | +| `logging` | object | `{}` | Logging configuration | + +### Bare-repository containers + +A common layout keeps the object database in `.bare/` and every checkout beside it: + +``` +myrepo/ # container - not a checkout itself +├── .bare/ # shared git object database +├── main/ # main-branch worktree +└── pr/ # PR worktrees: pr/pr. +``` + +Here the container is the anchor: a relative `worktreeParent` is resolved against +`myrepo/`, so `"pr"` puts new worktrees in `myrepo/pr/pr.` no matter which +worktree the command was run from. + +Because the container is the whole point of that layout, a relative `worktreeParent` +may not escape it. A value such as `"../pr"` — correct before the anchor moved to the +container, when it was read from `myrepo/main/` — would otherwise silently create the +worktree in the container's _parent_ directory, dropping the container segment. Leading +`../` segments are dropped instead (`"../pr"` behaves as `"pr"`) and a warning tells you +to update the config. + +To place worktrees outside a container deliberately, say so unambiguously: use an +absolute `worktreeParent`, or set `"worktreeParentAnchor": "repo-root"`. Neither is +clamped. ### AI Content Generation diff --git a/src/integration/worktree-layout.integration.test.ts b/src/integration/worktree-layout.integration.test.ts index dcb4a5d..c322169 100644 --- a/src/integration/worktree-layout.integration.test.ts +++ b/src/integration/worktree-layout.integration.test.ts @@ -123,3 +123,92 @@ describe('worktree layout anchoring integration', () => { expect(result).not.toBe(path.join(container, 'pr', 'pr2600.new-feature')); }); }); + +describe('bare container: relative worktreeParent must not escape the container', () => { + let tempDir: string; + let container: string; + let mainWorktree: string; + let prWorktree: string; + + beforeAll(() => { + tempDir = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'gwt-bare-escape-'))); + + const seed = path.join(tempDir, 'seed'); + fs.mkdirSync(seed); + execSync('git init -q -b main', { cwd: seed }); + execSync('git config user.email test@test.com', { cwd: seed }); + execSync('git config user.name Test', { cwd: seed }); + fs.writeFileSync(path.join(seed, 'README.md'), 'seed\n'); + execSync('git add README.md', { cwd: seed }); + execSync('git commit -q -m initial', { cwd: seed }); + + // Container layout: /.bare + /main + /pr/* + container = path.join(tempDir, 'container'); + fs.mkdirSync(container); + execSync(`git clone --bare -q "${seed}" "${path.join(container, '.bare')}"`); + // The container itself carries a .git FILE pointing into .bare, exactly like + // the real-world layout this regression came from. + fs.writeFileSync(path.join(container, '.git'), `gitdir: ${path.join(container, '.bare')}\n`); + + mainWorktree = path.join(container, 'main'); + execSync(`git worktree add -q "${mainWorktree}" main`, { cwd: path.join(container, '.bare') }); + + fs.mkdirSync(path.join(container, 'pr')); + prWorktree = path.join(container, 'pr', 'pr1.existing-feature'); + execSync(`git worktree add -q -b feat/existing-feature "${prWorktree}" main`, { + cwd: path.join(container, '.bare'), + }); + }); + + afterAll(() => { + try { + execSync('git worktree prune', { cwd: path.join(container, '.bare'), stdio: 'ignore' }); + } catch { + // ignore + } + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + function pathFor(worktreeParent: string, invokedFrom: string): string { + const config = { + ...getDefaultConfig(), + worktreeParent, + worktreePattern: 'pr{number}.{slug}', + }; + return generateWorktreePath( + config, + invokedFrom, + 'container', + 2897, + 'feat/shared-local-sqlserver-for-quartz', + git.getMainWorktreeRoot(invokedFrom) + ); + } + + const expected = () => + normalizePath(path.join(container, 'pr', 'pr2897.shared-local-sqlserver-for-quartz')); + + it('keeps a legacy "../pr" parent inside the container when invoked from main/', () => { + expect(normalizePath(pathFor('../pr', mainWorktree))).toBe(expected()); + }); + + it('keeps a legacy "../pr" parent inside the container when invoked from a pr worktree', () => { + expect(normalizePath(pathFor('../pr', prWorktree))).toBe(expected()); + }); + + it('clamps multiple escaping segments back into the container', () => { + expect(normalizePath(pathFor('../../pr', mainWorktree))).toBe(expected()); + }); + + it('leaves a contained relative parent untouched', () => { + expect(normalizePath(pathFor('pr', mainWorktree))).toBe(expected()); + expect(normalizePath(pathFor('./pr', prWorktree))).toBe(expected()); + }); + + it('never places the worktree outside the container', () => { + for (const parent of ['../pr', '../../pr', '../../../pr', 'pr']) { + const result = pathFor(parent, mainWorktree); + expect(path.relative(container, result).startsWith('..')).toBe(false); + } + }); +}); diff --git a/src/lib/config.test.ts b/src/lib/config.test.ts index f3cc4f5..bdbccfc 100644 --- a/src/lib/config.test.ts +++ b/src/lib/config.test.ts @@ -6,7 +6,9 @@ import { loadConfig, generateBranchNameAsync, generatePRContentAsync, + resolveRelativeWorktreeParent, } from './config.js'; +import { logger } from './logger.js'; import * as path from 'path'; import * as fs from 'fs'; import * as os from 'os'; @@ -1080,3 +1082,200 @@ describe('config', () => { }); }); }); + +describe('resolveRelativeWorktreeParent', () => { + let tempDir: string; + + beforeEach(() => { + tempDir = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'gwt-parent-'))); + }); + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + + function makeContainer(name: string): string { + const container = path.join(tempDir, name); + fs.mkdirSync(path.join(container, '.bare'), { recursive: true }); + fs.writeFileSync(path.join(container, '.bare', 'HEAD'), 'ref: refs/heads/main\n'); + return container; + } + + function makePlainDir(name: string): string { + const dir = path.join(tempDir, name); + fs.mkdirSync(dir, { recursive: true }); + return dir; + } + + describe('non-container anchors are never clamped', () => { + it('resolves the default ".." to the sibling of the anchor', () => { + const anchor = makePlainDir('repo'); + expect(normalizePath(resolveRelativeWorktreeParent(anchor, '..'))).toBe( + normalizePath(tempDir) + ); + }); + + it('resolves "../worktrees" outside the anchor', () => { + const anchor = makePlainDir('repo'); + expect(normalizePath(resolveRelativeWorktreeParent(anchor, '../worktrees'))).toBe( + normalizePath(path.join(tempDir, 'worktrees')) + ); + }); + + it('resolves ".worktrees" inside the anchor', () => { + const anchor = makePlainDir('repo'); + expect(normalizePath(resolveRelativeWorktreeParent(anchor, '.worktrees'))).toBe( + normalizePath(path.join(anchor, '.worktrees')) + ); + }); + + it('does not treat a ".bare" FILE as a container', () => { + const anchor = makePlainDir('repo'); + fs.writeFileSync(path.join(anchor, '.bare'), 'not a directory\n'); + expect(normalizePath(resolveRelativeWorktreeParent(anchor, '../pr'))).toBe( + normalizePath(path.join(tempDir, 'pr')) + ); + }); + + it('does not treat a ".bare" directory without a HEAD as a container', () => { + const anchor = makePlainDir('repo'); + fs.mkdirSync(path.join(anchor, '.bare')); + expect(normalizePath(resolveRelativeWorktreeParent(anchor, '../pr'))).toBe( + normalizePath(path.join(tempDir, 'pr')) + ); + }); + }); + + describe('bare-repository container anchors', () => { + it('leaves a contained relative parent untouched', () => { + const container = makeContainer('container'); + expect(normalizePath(resolveRelativeWorktreeParent(container, 'pr'))).toBe( + normalizePath(path.join(container, 'pr')) + ); + expect(normalizePath(resolveRelativeWorktreeParent(container, './pr'))).toBe( + normalizePath(path.join(container, 'pr')) + ); + }); + + it('clamps a legacy "../pr" back into the container', () => { + const container = makeContainer('container'); + expect(normalizePath(resolveRelativeWorktreeParent(container, '../pr'))).toBe( + normalizePath(path.join(container, 'pr')) + ); + }); + + it('clamps deeply escaping parents back into the container', () => { + const container = makeContainer('container'); + expect(normalizePath(resolveRelativeWorktreeParent(container, '../../../pr'))).toBe( + normalizePath(path.join(container, 'pr')) + ); + }); + + it('clamps a bare ".." to the container itself', () => { + const container = makeContainer('container'); + expect(normalizePath(resolveRelativeWorktreeParent(container, '..'))).toBe( + normalizePath(container) + ); + }); + + it('warns with the offending value, the escaped path and the suggested fix', () => { + const container = makeContainer('container'); + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => {}); + + resolveRelativeWorktreeParent(container, '../pr'); + + expect(warn).toHaveBeenCalledTimes(1); + const message = String(warn.mock.calls[0][0]); + expect(message).toContain('../pr'); + expect(message).toContain(container); + expect(message).toContain(path.join(container, 'pr')); + expect(message).toContain('worktreeParentAnchor'); + }); + + it('does not warn when the parent already stays inside the container', () => { + const container = makeContainer('container'); + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => {}); + + resolveRelativeWorktreeParent(container, 'pr'); + + expect(warn).not.toHaveBeenCalled(); + }); + }); + + describe('generateWorktreePath integration', () => { + it('anchors a legacy "../pr" to the container, not to the container\'s parent', () => { + const container = makeContainer('container'); + const mainWorktree = path.join(container, 'main'); + fs.mkdirSync(mainWorktree); + vi.spyOn(logger, 'warn').mockImplementation(() => {}); + + const config = { + ...getDefaultConfig(), + worktreeParent: '../pr', + worktreePattern: 'pr{number}.{slug}', + }; + + const result = generateWorktreePath( + config, + mainWorktree, + 'container', + 2897, + 'feat/shared-local-sqlserver-for-quartz', + container + ); + + expect(normalizePath(result)).toBe( + normalizePath(path.join(container, 'pr', 'pr2897.shared-local-sqlserver-for-quartz')) + ); + }); + + it('still honours worktreeParentAnchor: "repo-root" without clamping', () => { + const container = makeContainer('container'); + const mainWorktree = path.join(container, 'main'); + fs.mkdirSync(mainWorktree); + + const config = { + ...getDefaultConfig(), + worktreeParent: '../pr', + worktreePattern: 'pr{number}.{slug}', + worktreeParentAnchor: 'repo-root' as const, + }; + + const result = generateWorktreePath( + config, + mainWorktree, + 'container', + 2897, + 'feat/shared-local-sqlserver-for-quartz', + container + ); + + expect(normalizePath(result)).toBe( + normalizePath(path.join(container, 'pr', 'pr2897.shared-local-sqlserver-for-quartz')) + ); + }); + + it('leaves an absolute worktreeParent untouched inside a container', () => { + const container = makeContainer('container'); + const outside = path.join(tempDir, 'elsewhere'); + + const config = { + ...getDefaultConfig(), + worktreeParent: outside, + worktreePattern: 'pr{number}.{slug}', + }; + + const result = generateWorktreePath( + config, + path.join(container, 'main'), + 'container', + 2897, + 'feat/thing', + container + ); + + expect(normalizePath(result)).toBe(normalizePath(path.join(outside, 'pr2897.thing'))); + }); + }); +}); diff --git a/src/lib/config.ts b/src/lib/config.ts index 7a20a0d..baa158f 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -804,12 +804,82 @@ export function generateWorktreePath( } else { const anchor = config.worktreeParentAnchor === 'repo-root' ? repoRoot : (mainWorktreeRoot ?? repoRoot); - parentDir = path.resolve(anchor, config.worktreeParent); + parentDir = resolveRelativeWorktreeParent(anchor, config.worktreeParent); } return path.join(parentDir, pattern); } +/** + * True when `dir` is a bare-repository container root: a directory that is not a + * checkout itself but holds the shared object database in `.bare/` alongside its + * worktrees (`main/`, `pr/pr.`, ...). Never throws — a failed lookup is + * reported as "not a container" so callers keep their previous behaviour. + */ +function isBareRepositoryContainer(dir: string): boolean { + try { + const bare = path.join(dir, '.bare'); + return fs.statSync(bare).isDirectory() && fs.existsSync(path.join(bare, 'HEAD')); + } catch { + return false; + } +} + +/** + * True when `candidate` is `root` itself or lives beneath it. + */ +function isWithin(root: string, candidate: string): boolean { + const rel = path.relative(root, candidate); + return rel === '' || (!rel.startsWith('..') && !path.isAbsolute(rel)); +} + +/** + * Drop leading `.` / `..` segments from a relative path so it can only descend. + * "../pr" -> "pr", "../../pr" -> "pr", ".." -> "". + */ +function stripLeadingParentSegments(relativePath: string): string { + const segments = relativePath.split(/[/\\]+/).filter((segment) => segment.length > 0); + while (segments.length > 0 && (segments[0] === '..' || segments[0] === '.')) { + segments.shift(); + } + return segments.join(path.sep); +} + +/** + * Resolve a relative `worktreeParent` against its anchor. + * + * For a bare-repository container the anchor IS the container, and the container is + * the whole point of the layout: every worktree belongs inside it. A relative parent + * that resolves outside the container is therefore never intentional — it is a config + * written before the anchor moved from the invoking worktree root to the container + * (see `worktreeParentAnchor`). Such a value (e.g. `"../pr"`, meaning `/pr` + * when read from `/main`) would otherwise silently place the worktree + * *beside* the container, dropping the container segment entirely. + * + * Rather than misplace the worktree, clamp the escaping leading segments so the parent + * stays inside the container, and warn so the config gets fixed. Callers that genuinely + * want worktrees outside a container can say so unambiguously with an absolute + * `worktreeParent` or `worktreeParentAnchor: "repo-root"`, neither of which is clamped. + */ +export function resolveRelativeWorktreeParent(anchor: string, worktreeParent: string): string { + const resolved = path.resolve(anchor, worktreeParent); + + if (isWithin(anchor, resolved) || !isBareRepositoryContainer(anchor)) { + return resolved; + } + + const contained = path.resolve(anchor, stripLeadingParentSegments(worktreeParent)); + logger.warn( + `worktreeParent "${worktreeParent}" resolves outside the bare-repository container ` + + `"${anchor}" (would be "${resolved}"). Relative worktreeParent values are anchored to ` + + `the container, so leading "../" segments were dropped and the worktree will be created ` + + `under "${contained}" instead. Update worktreeParent to "${ + path.relative(anchor, contained) || '.' + }" (or set an absolute path / worktreeParentAnchor: "repo-root") to silence this warning.` + ); + return contained; +} + /** * Generate branch name from description (synchronous, rule-based) */