From 05e656fc6880ac78aee803e63433d44cc3bc19e5 Mon Sep 17 00:00:00 2001 From: Xan Torres Date: Sat, 22 Aug 2026 15:44:47 +0800 Subject: [PATCH] fix: reject a worktrees baseBranch inside the managed branch namespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit worktrees.baseBranch was validated only as a non-empty string, so it could name any ref — including one RepoKernel itself creates and later deletes. Basing worktrees on an epic branch makes every sprint branch merged into that epic register as merged into the project's base, so branch cleanup removes work that never reached trunk. Validate it the way branchPrefix already is: reject illegal Git branch names, and reject a base that falls inside the namespace an epic or sprint branch pattern generates into. That namespace is the pattern up to its first id token, so a hand-made branch there is caught alongside a rendered one, while a base that merely shares branchPrefix stays accepted. --- docs/internals/config-reference.md | 4 +- docs/internals/specs/config.md | 2 +- packages/cli/test/worktree.test.ts | 60 ++++++++++++++++++++++++++++++ packages/core/src/config/schema.ts | 56 ++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 2 deletions(-) diff --git a/docs/internals/config-reference.md b/docs/internals/config-reference.md index 04d2b48..d65fae3 100644 --- a/docs/internals/config-reference.md +++ b/docs/internals/config-reference.md @@ -72,7 +72,7 @@ All fields are optional. Defaults are applied when omitted. |---|---|---|---| | `root` | string | `../.repokernel-worktrees` | Root directory for managed worktrees. May be absolute or relative to the main checkout. | | `branchPrefix` | string | `rk/` | Prefix for managed branches. Epic branches: `epic/`. Sprint branches: `sprint//`. | -| `baseBranch` | string | `main` | Branch used as the base when creating a new epic worktree. | +| `baseBranch` | string | `main` | Branch used as the base when creating a new epic worktree. Must be a legal Git branch name and must sit outside the epic and sprint branch namespaces. | | `autoAcquire` | boolean | `true` | `rk run` creates or reuses the epic worktree automatically. | | `branchPattern` | string \| omitted | omitted | Compatibility shorthand. Without `{sprintId}`, applies to epic branches only. With `{sprintId}`, applies to sprint branches only. See below. | | `epicBranchPattern` | string \| omitted | omitted | Explicit epic branch template. Cannot contain `{sprintId}`. | @@ -115,6 +115,8 @@ Prefer explicit `epicBranchPattern` + `sprintBranchPattern` for team-specific na RepoKernel also renders representative epic and sprint refs at config load and validates the final Git ref strings. This catches unsafe `branchPrefix` values, dot-prefixed path components, `.lock` components, accidental double slashes after token substitution, and epic/sprint ref collisions such as `feature/E-001` plus `feature/E-001/S-001`. +`baseBranch` is validated against the same ref rules and is additionally rejected when it falls inside the namespace a branch pattern generates into — everything up to the pattern's first `{epicId}` or `{sprintId}` token. With the default patterns that rules out `rk/epic/...` and `rk/sprint/...` while leaving a base that merely shares `branchPrefix`, such as `release/current` under prefix `release/`, accepted. RepoKernel creates and deletes branches in those namespaces, so a base inside one makes work merged only into a worktree branch look merged into the project's trunk. + **Examples:** ```yaml diff --git a/docs/internals/specs/config.md b/docs/internals/specs/config.md index 2a91766..cb9d393 100644 --- a/docs/internals/specs/config.md +++ b/docs/internals/specs/config.md @@ -73,7 +73,7 @@ All configured `paths` values must be repo-relative. Absolute paths, NUL bytes, |---|---|---|---| | `root` | string | `../.repokernel-worktrees` | Root directory for managed worktrees. May be absolute or relative to the control checkout. | | `branchPrefix` | string | `rk/` | Prefix for managed branches. Epic branches use `epic/`; sprint branches use `sprint//`. | -| `baseBranch` | string | `main` | Base branch used when creating a new epic worktree branch. | +| `baseBranch` | string | `main` | Base branch used when creating a new epic worktree branch. Rejected at config load when it is not a legal Git branch name, or when it falls inside the epic or sprint branch namespace. | | `autoAcquire` | boolean | `true` | `rk run` automatically creates/reuses the epic worktree. | | `branchPattern` | string | omitted | Shorthand branch template. Without `{sprintId}`, applies to epic branches; with `{sprintId}`, applies to sprint branches. Rendered refs are validated at config load. | | `epicBranchPattern` | string | omitted | Explicit epic branch template. Cannot contain `{sprintId}` and must render to a valid non-colliding Git ref. | diff --git a/packages/cli/test/worktree.test.ts b/packages/cli/test/worktree.test.ts index b8dd5b8..a2dd69b 100644 --- a/packages/cli/test/worktree.test.ts +++ b/packages/cli/test/worktree.test.ts @@ -218,3 +218,63 @@ describe('worktree naming — branchPattern', () => { expect(worktreeBranch(eid('E-001'), config)).toBe('feat-2026.q2/E-001_a'); }); }); + +describe('worktree config — baseBranch', () => { + it('accepts the default base branch', () => { + expect(CONFIG.worktrees.baseBranch).toBe('main'); + }); + + it('rejects a baseBranch inside the epic worktree branch namespace', () => { + expect(() => configWithWorktrees({ baseBranch: 'rk/epic/E-001' })).toThrow(); + }); + + it('rejects a baseBranch inside the sprint worktree branch namespace', () => { + expect(() => configWithWorktrees({ baseBranch: 'rk/sprint/E-001/S-001' })).toThrow(); + }); + + it('rejects a baseBranch in the generated namespace that is not an id', () => { + expect(() => configWithWorktrees({ baseBranch: 'rk/epic/legacy' })).toThrow(); + }); + + it('rejects a baseBranch inside a custom epic branch namespace', () => { + expect(() => + configWithWorktrees({ + epicBranchPattern: 'feature/epic/{epicId}', + sprintBranchPattern: 'feature/sprint/{epicId}/{sprintId}', + baseBranch: 'feature/epic/E-001', + }), + ).toThrow(); + }); + + it('rejects a baseBranch equal to a pattern that generates one fixed branch', () => { + expect(() => + configWithWorktrees({ epicBranchPattern: 'devel', baseBranch: 'devel' }), + ).toThrow(); + }); + + it('accepts a baseBranch under branchPrefix that no pattern generates', () => { + const config = configWithWorktrees({ branchPrefix: 'release/', baseBranch: 'release/current' }); + expect(config.worktrees.baseBranch).toBe('release/current'); + }); + + it('accepts a baseBranch that only shares a leading segment with the namespace', () => { + const config = configWithWorktrees({ baseBranch: 'rk-mainline' }); + expect(config.worktrees.baseBranch).toBe('rk-mainline'); + }); + + it('rejects a baseBranch that is not a legal git ref — leading dash', () => { + expect(() => configWithWorktrees({ baseBranch: '-x' })).toThrow(); + }); + + it('rejects a baseBranch that is not a legal git ref — whitespace', () => { + expect(() => configWithWorktrees({ baseBranch: 'my branch' })).toThrow(); + }); + + it('rejects a baseBranch that is not a legal git ref — `..` range syntax', () => { + expect(() => configWithWorktrees({ baseBranch: 'main..dev' })).toThrow(); + }); + + it('rejects a baseBranch that is not a legal git ref — trailing .lock', () => { + expect(() => configWithWorktrees({ baseBranch: 'main.lock' })).toThrow(); + }); +}); diff --git a/packages/core/src/config/schema.ts b/packages/core/src/config/schema.ts index fcd2aa2..9586897 100644 --- a/packages/core/src/config/schema.ts +++ b/packages/core/src/config/schema.ts @@ -195,6 +195,33 @@ function refsConflict(a: string, b: string): boolean { return a === b || a.startsWith(`${b}/`) || b.startsWith(`${a}/`); } +const ID_TOKEN_RE = /\{(?:epicId|sprintId)\}/; + +/** + * The branch namespace a worktree pattern generates into: everything up to its + * first id token, with `{branchPrefix}` substituted. + * + * `isPrefix` is false when the pattern carries no id token at all — it then + * names a single fixed branch rather than a namespace, so containment has to be + * tested as a ref conflict instead of a string prefix. + * + * Cutting at the first id token rather than matching rendered ids keeps the + * test on the namespace RepoKernel claims, so a hand-made `rk/epic/legacy` is + * caught alongside `rk/epic/E-001` while a base that merely shares the + * configured `branchPrefix` (`release/current` under prefix `release/`) is not. + */ +function generatedBranchNamespace( + pattern: string, + branchPrefix: string, +): { readonly value: string; readonly isPrefix: boolean } { + const idToken = ID_TOKEN_RE.exec(pattern); + const head = idToken === null ? pattern : pattern.slice(0, idToken.index); + return { + value: head.replace(/\{branchPrefix\}/g, branchPrefix), + isPrefix: idToken !== null, + }; +} + /** * Validate a `worktrees.branchPattern` template string. * @@ -282,6 +309,14 @@ export const WorktreesSchema = z }); } + if (!isValidGitBranchRef(value.baseBranch)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['baseBranch'], + message: `baseBranch \`${value.baseBranch}\` is not a valid git branch name`, + }); + } + const epicPattern = epicBranchPatternFor(value); const sprintPattern = sprintBranchPatternFor(value); if (hasToken(epicPattern, 'sprintId')) { @@ -301,6 +336,27 @@ export const WorktreesSchema = z if (!hasOnlyCurrentTokens(epicPattern) || !hasOnlyCurrentTokens(sprintPattern)) return; + // A base inside the worktree branch namespace makes every worktree branch + // merged into it look merged into trunk, so cleanup deletes work that never + // reached the real base. + for (const [pattern, label] of [ + [epicPattern, 'epic'], + [sprintPattern, 'sprint'], + ] as const) { + const namespace = generatedBranchNamespace(pattern, value.branchPrefix); + const collides = namespace.isPrefix + ? value.baseBranch.startsWith(namespace.value) + : refsConflict(value.baseBranch, namespace.value); + if (collides) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['baseBranch'], + message: `baseBranch \`${value.baseBranch}\` sits inside the ${label} worktree branch namespace \`${namespace.value}\` — RepoKernel creates and deletes branches there, so the base must live outside it`, + }); + break; + } + } + const sampleCtx = { branchPrefix: value.branchPrefix, epicId: 'E-001', sprintId: 'S-001' }; const epicRef = renderBranchPattern(epicPattern, sampleCtx); const sprintRef = renderBranchPattern(sprintPattern, sampleCtx);