Conversation
…y 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 `<container>/pr` when read from `<container>/main`. Resolved against the container those escape it entirely, and the new worktree was silently created beside the container with the container segment dropped: <container>/main + "../pr" -> /home/chris/workspace/pr/pr2897.<slug> expected -> /home/chris/workspace/syrf/pr/pr2897.<slug> 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
|
Claude finished @chrissena's task in 4m 15s —— View job Review: fix(config): keep a relative worktreeParent inside its bare-repository container
Overall verdict: ✅ Approve (3 nits, no blocking issues)The fix is correct. The algorithm ( Three things to address (all minor):
One observation (not blocking): The integration test titled |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| function isBareRepositoryContainer(dir: string): boolean { | ||
| try { | ||
| const bare = path.join(dir, '.bare'); | ||
| return fs.statSync(bare).isDirectory() && fs.existsSync(path.join(bare, 'HEAD')); |
There was a problem hiding this comment.
Minor inconsistency: statSync (throws on missing path, caught by the outer try/catch) is mixed with existsSync (never throws). Keeping both in the caught pattern is cleaner and avoids a potential TOCTOU if .bare disappears between the two calls:
| return fs.statSync(bare).isDirectory() && fs.existsSync(path.join(bare, 'HEAD')); | |
| return ( | |
| fs.statSync(bare).isDirectory() && fs.statSync(path.join(bare, 'HEAD')).isFile() | |
| ); |
This follows the existing statSync-in-try/catch pattern from git.ts.
| */ | ||
| function stripLeadingParentSegments(relativePath: string): string { | ||
| const segments = relativePath.split(/[/\\]+/).filter((segment) => segment.length > 0); | ||
| while (segments.length > 0 && (segments[0] === '..' || segments[0] === '.')) { |
There was a problem hiding this comment.
The . branch of this condition is dead code for the clamping trigger. A "./pr" input resolves to <container>/pr, which is within the container, so isWithin returns true and stripLeadingParentSegments is never called with a leading . segment in normal use. The stripping is harmless but adds noise; the JSDoc examples ("../pr" -> "pr", .." -> "") don't mention it either.
If keeping it for defensive completeness, add "./pr" -> "pr" to the JSDoc. Otherwise just remove the || segments[0] === '.' branch since a contained "./foo" is returned untouched above.
| 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 ` + |
There was a problem hiding this comment.
The literal "../" in this string is POSIX-style. On Windows the user's config value could use \ ("..\pr"), and the warning text would be slightly off ("leading '../' segments" vs the actual backslash form). Since worktreeParent is echoed directly in the earlier part of the message (which is correct), consider referencing the value there rather than hard-coding the separator:
| `the container, so leading "../" segments were dropped and the worktree will be created ` + | |
| `the container, so leading parent-directory segments were dropped and the worktree will be created ` + |
Cross-platform cosmetic only — no logic impact.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
+ Coverage 81.76% 81.79% +0.02%
==========================================
Files 97 97
Lines 20839 20873 +34
Branches 4546 4558 +12
==========================================
+ Hits 17040 17074 +34
Misses 3799 3799 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
The bug
Running
wt new … --jsonfrom/home/chris/workspace/syrf/main— a.bare/containerlayout — created the worktree at
instead of
The
syrfcontainer segment was silently dropped. A second stranded worktree from anearlier run (
pr2864.…) shows the same misplacement, so it is reproducible rather thana one-off.
Root cause
Two correct-in-isolation pieces combine into a silent misplacement:
getMainWorktreeRoot()—src/lib/git.ts:694-722. For a.bare/container itreturns
path.dirname(<common-dir>), i.e. the container (…/syrf), not the mainworktree (
…/syrf/main). That is deliberate (feat(config): anchor worktree placement to the main worktree root (spec Part 3) #25, v1.15.0) and is whatsrc/integration/worktree-layout.integration.test.tsandwt init's bare-layoutscaffold (
worktreeParent: "pr",src/cli/wt/init.ts:260) both assume.generateWorktreePath()—src/lib/config.ts:800-808. It didpath.resolve(anchor, config.worktreeParent)with no containment check.#25moved the anchor from the invoking worktree root to the container withoutmigrating existing configs.
syrf's.worktreerc.localstill carries the pre-#25value
"worktreeParent": "../pr"— correct when read from…/syrf/main, and onedirectory too high when read from
…/syrf. Soresolve("…/syrf", "../pr")escapes thecontainer to
/home/chris/workspace/pr.Reproduced against the real layout with the installed 1.15.0 build before touching
anything:
— byte-for-byte the reported wrong path.
The fix
New exported
resolveRelativeWorktreeParent(anchor, worktreeParent)insrc/lib/config.ts. When the anchor is a bare-repository container (a.bare/directory containing a
HEAD) and a relative parent resolves outside it, the escapingleading
../segments are dropped so the parent stays inside the container, and awarning names the offending value, the path that would have been used, and the
replacement to write:
Deliberately not clamped, so putting worktrees outside a container stays possible —
it just has to be unambiguous:
worktreeParentworktreeParentAnchor: "repo-root"Non-container anchors are untouched. The trigger is the
.bare/+HEADfilesystemcheck, not "the path escapes the anchor" — the default
worktreeParent: ".."escapesthe anchor by design and must keep resolving to the repository's sibling directory.
Also documents
worktreeParentAnchor(previously absent from the README options table)and adds a Bare-repository containers section.
Test evidence
Baseline before any change — green
Regression tests written first — red
Real bare-container fixture (
git clone --bareinto<container>/.bare, a container-level.gitfile pointing at it, amain/worktree and apr/worktree — matching the layoutthat triggered this):
After the fix — green
Full suite after the fix — green, +19 tests, no regressions
pnpm run buildclean ·pnpm run lint0 errors (66 pre-existing warnings) ·prettier --check .clean.End-to-end against the real layout, with the fixed build
(Read-only: the pure resolver was called against the live config; nothing under
/home/chris/workspace/syrfwas created or modified.)New coverage
19 tests — 5 asserting non-container anchors are never clamped (default
"..","../worktrees",".worktrees", a.barefile, a.baredirectory with noHEAD),6 on container clamping and the warning text, 3 on
generateWorktreePathincluding therepo-rootanchor and absolute-parent escape hatches, and 5 integration tests against areal bare container.
Config finding (no change made here)
syrf's.worktreerc.local—"worktreeParent": "../pr"— is a stale pre-#25 value.It now works either way, but the tidy fix on that side is
"worktreeParent": "pr",which silences the warning. Left alone deliberately; that repo is out of scope for this PR.
https://claude.ai/code/session_01R84soQ82VQsGznotpWTvHo