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) */