feat(git): widen worktree symlink candidates - #232
Conversation
johannesjo
left a comment
There was a problem hiding this comment.
Thanks for this — the split is exactly what we agreed on #231, keeping isDefault classification in the backend is the right call, and the real-git integration tests are a genuinely valuable addition (first coverage these functions have had).
I ran the enumeration against a few repo shapes before merging and found one thing I think blocks.
--directory also collapses directories that aren't themselves ignored
--directory collapses any untracked directory whose contents are all ignored — the directory itself doesn't have to match a rule. So with content-pattern gitignores, which are very common:
.gitignore |
enumerated | git check-ignore on it |
|---|---|---|
*.log + a logs/ dir |
logs |
not ignored |
coverage/* |
coverage |
not ignored |
*.tmp + a tmp/ dir |
tmp |
not ignored |
This repo's own .gitignore uses bare directory names (node_modules, dist, coverage), so it doesn't reproduce here — which is probably why it got through review and CI.
The consequence is the one-way door from the issue. If a user ticks logs, createWorktree symlinks it and ensureSymlinkExcludes appends /logs to the main repo's .git/info/exclude — permanently hiding a directory the user never ignored from git status, in the main checkout and in every worktree, with no removal path.
Verified end-to-end against this branch:
picker offers -> [{"name":"logs","isDefault":false}]
MAIN .git/info/exclude now contains:
...
# parallel-code: worktree symlinks
/logs
Smaller: core.quotePath mangles non-ASCII names
Paths are parsed without -z, so git C-quotes them. Verified on this branch:
- ignored dir
dàta/→ git emits"d\303\240ta/". The trailing"defeatsendsWith('/'), so the slash is never stripped and!name.includes('/')drops the entry entirely →[], the directory is silently unselectable. - ignored file
nöte.txt→ survives as"n\303\266te.txt", renders with literal quotes and escapes in the picker, then silently no-ops increateWorktreebecausefs.existsSyncis false.
Both fall out of one change
export async function getGitIgnoredDirs(projectRoot: string): Promise<GitIgnoredEntry[]> {
// `-z` so non-ASCII names aren't C-quoted by core.quotePath.
const { stdout } = await exec(
'git',
['ls-files', '-z', '--others', '--ignored', '--exclude-standard', '--directory'],
{ cwd: projectRoot },
);
const candidates = stdout
.split('\0')
.filter(Boolean)
.map((entry) => (entry.endsWith('/') ? entry.slice(0, -1) : entry))
.filter((name) => !name.includes('/') && !INTERNAL_SYMLINK_EXCLUSIONS.has(name));
// `--directory` also collapses untracked directories whose *contents* are all
// ignored (e.g. `logs/` under a bare `*.log` rule). Such a directory is not
// itself ignored, and symlinking it would append a permanent `/logs` line to
// the main repo's .git/info/exclude. Keep only self-ignored entries.
const checked = await Promise.all(
candidates.map(async (name) => {
try {
await exec('git', ['check-ignore', '-q', '--', name], { cwd: projectRoot });
return name;
} catch {
return null;
}
}),
);
return checked
.filter((name): name is string => name !== null)
.map((name) => ({ name, isDefault: DEFAULT_SYMLINK_CANDIDATES.has(name) }));
}check-ignore -z only works with --stdin, and promisify(execFile) can't write stdin — hence per-candidate calls rather than one batched one. Candidates are top-level only (8 in this repo) and the old implementation already did up to 16 execs, so it isn't a cost regression.
I applied that on top of your branch locally: your 9 tests still pass, plus 5 new ones covering *.log + logs/, coverage/*, both non-ASCII cases, and an end-to-end assertion that /logs never reaches .git/info/exclude. Full suite 841 passed, typecheck clean.
Worth adding a regression test for the collapsed-but-not-ignored case — the current suite covers ignored files under a tracked directory, but not an untracked directory whose contents are all ignored, which is the inverse.
Two non-blocking notes
src/store/remoteTaskHandler.ts:58—getGitIgnoredDirscan now reject, where the old per-candidate try/catch always resolved[].NewTaskDialogcatches and falls back, but here the throw fails the entire remote task creation, where it previously degraded to "no symlinks". Narrow trigger, sinceGetMainBranchusually throws first — though it's skipped whendefaultBaseBranchis set, which leaves this as the first git call. Probably worth catching and returning[].src/components/SymlinkDirPicker.tsx— nothing distinguishes the pre-checked defaults from newly discovered entries, and git's ordering interleaves them. With the list growing from 8 to potentially dozens, amax-height+ scroll would help;NewTaskDialog.tsx:262already uses that pattern. Longer term it'd also be good to surface that ticking a box writes a permanent.git/info/excludeline.
The .claude / .worktrees / sandbox-artifact exclusion reasoning is all correct, and the .worktree vs .worktrees test is a nice catch. Just the enumeration predicate to tighten.
- Use git ls-files -z to receive unquoted, NUL-terminated paths so core.quotePath cannot corrupt non-ASCII candidate names. - Filter each candidate through git check-ignore -q so only paths that are themselves ignored are returned, preventing collapsed content-ignored directories from being written to .git/info/exclude. - Rename getGitIgnoredDirs to getSymlinkCandidates. - Add regression tests for collapsed-but-not-ignored dirs and non-ASCII paths. Co-Authored-By: Claude <noreply@anthropic.com>
getSymlinkCandidates now swallows probe errors (missing git, broken repo) with a console.warn and returns [], so remote task creation proceeds without symlinks — matching the pre-verification behavior. Also cap the symlink picker at 160px with internal scroll, and add a muted disclosure line that checked entries are written to .git/info/exclude and apply to all worktrees. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the follow-up commits — the original I also did a fresh adversarial end-to-end pass and found these remaining issues: Blocking
Smaller mismatch
The backend-owned defaults and real-Git tests are good additions, but the issues above make my current verdict Needs changes. |
… and stale UI state Single git ls-files call with :(glob) pathspec magic replaces the unbounded enumeration + per-candidate check-ignore fan-out, bounding output by root entry count and eliminating the --stdin hang. - ensureSymlinkExcludes: escape names to gitignore literals, reject CR/LF, dedup per exact line instead of substring - Reserved-name checks (.claude, .worktrees, sandbox artifacts) fold case per the repo's core.ignorecase, enforced defensively in createWorktree so the backend doesn't trust the UI's list - New Task dialog: candidate state clears before every load, stale responses are dropped via a request id, and submit is blocked while candidates for the current project are unresolved - Single isValidSymlinkName predicate shared by producer and consumer; names like foo..bar are legal again, only full-component ".." is rejected Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the detailed adversarial pass — all six points are addressed in 1. Arbitrary basenames written as gitignore patterns — 2. Option-like filenames blocking discovery — Eliminated with the code itself: the per-candidate 3. Unbounded probe / process fan-out — Same refactor: output size is bounded by the root entry count (nested ignored files below tracked directories can no longer match the root-anchored globs), the call uses the module's existing 10 MB 4. Case-sensitive managed-name comparisons — Reserved-name and default-candidate comparisons now fold case per the repo's 5. Dialog opt-in leakage — The picker state is extracted to 6. Producer/consumer validation mismatch — A single exported Full suite is green: 1604 tests passing, including all 29 pre-existing candidate/worktree contract tests unchanged. |
|
Thanks for the thorough follow-up. I rechecked the current head ( I still found two blockers: Blocking
Smaller regressions / test gap
Both CI checks and all existing tests are green, but they do not cover the repros above. My verdict remains Needs changes. |
…orecase bool, probe gating - Match exclude markers with gitignore semantics: strip trailing whitespace only, so a hand-written line with leading spaces no longer suppresses appends - Escape trailing spaces in exclude patterns (`foo ` → `foo\ `) so they match the exact name instead of the trimmed one - Parse core.ignorecase via `git config --bool` so yes/on/1/case variants enable case folding; both consumers (candidate discovery and the createWorktree reserved-name guard) covered by a synonym matrix - Gate the symlink-candidate probe and submit blocking on worktree mode: direct-mode tasks never send symlinkDirs, so no probe IPC fires and a pending probe can't block submit - Make the buffer regression test actually exceed the old 1 MiB execFile default (5000 × 240-char names ≈ 1.2 MB) Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the detailed re-review — all four points are addressed in 1a. Trailing spaces — 1b. Marker semantics — the append helper now compares with trailing-whitespace-only stripping ( 2. core.ignorecase — switched to 3. Direct-mode gating — both gates now key off the isolation mode: the load effect reads 4. Buffer regression — replaced with 5,000 files of 240-char basenames (just under the 255 fs limit), which would produce ~1.2 MB of output, ~17% over the old 1 MiB Typecheck is clean and the full suite stays green (1,618 passed, 25 skipped). |
johannesjo
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I re-reviewed the full current head (a5c842d) from scratch. The earlier trailing-space filename, core.ignorecase, buffer-test, and direct-mode issues are addressed, and the full suite/checks are green. One blocking marker issue remains.
Blocking: the block marker can suppress unrelated missing exclusions
ensureSymlinkExcludes first computes toAdd using exact line equality, then passes only toAdd[0] as the marker for the entire multi-line block. appendGitInfoExcludeBlockAtPath applies different normalization to that marker and returns present without writing any part of the block.
Current-head repro:
fs.writeFileSync(excludePath, '/foo \n');
ensureSymlinkExcludes(root, ['foo', 'bar']);
expect(fs.readFileSync(excludePath, 'utf8')).toContain('/bar\n');This fails: the exact prefilter leaves both /foo and /bar in toAdd, the helper strips the trailing space from the existing /foo line, treats the first marker as present, and suppresses the whole append. /bar is never written, so a selected bar/ directory that becomes a symlink can remain visible to Git and be staged accidentally.
There is a second manifestation in the same comparison: \s+$ also strips tabs and Unicode whitespace, but Git only ignores unescaped trailing ASCII spaces. An existing /foo<TAB> pattern is distinct to Git, yet the helper incorrectly treats it as /foo and suppresses the append. I reproduced both cases against a5c842d with failing regression tests.
Please avoid using one marker to gate the entire toAdd block (or bypass the helper's marker recheck after the caller has computed the missing lines), and use one consistent comparison that preserves tabs/escaped spaces while handling CRLF and unescaped trailing ASCII spaces. A regression with existing /foo \n plus requested foo, bar, and one with /foo\t\n, would cover both failure modes.
Verification: clean-head full suite 1,618 passed / 25 skipped; compile, typecheck, lint, formatting, and both CI checks pass. Verdict remains Needs changes for the correctness issue above.
…ison ensureSymlinkExcludes computed the missing lines with exact equality, then passed only toAdd[0] as the marker for the whole multi-line block. The helper re-checked that marker with different normalization (\s+$ strip), so a hand-written `/foo ` or `/foo<TAB>` line could mark the block present and silently drop genuinely missing entries like /bar. - Add normalizeExcludeLine (CR + unescaped trailing ASCII spaces only; tabs/escaped spaces stay significant) and use it on both sides of the comparison. - Add skipMarkerCheck so callers that already computed the missing lines bypass the single-marker recheck. - Regressions: `/foo \n` + foo,bar and `/foo\t\n` at both mock level and with real-git check-ignore, plus CRLF and escaped-space locking tests. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the detailed repro — fixed in b2b57aa. Root cause: as you described, the caller computed Fix:
Regressions added (mock-level in
Full suite: 1,624 passed / 25 skipped; compile, typecheck, and lint clean. One disclosed simplification: |
|
Thank you very much for this! <3 |
…ol, probe gating - Match exclude markers with gitignore semantics: strip trailing whitespace only, so a hand-written line with leading spaces no longer suppresses appends - Escape trailing spaces in exclude patterns (`foo ` → `foo\ `) so they match the exact name instead of the trimmed one - Parse core.ignorecase via `git config --bool` so yes/on/1/case variants enable case folding; both consumers (candidate discovery and the createWorktree reserved-name guard) covered by a synonym matrix - Gate the symlink-candidate probe and submit blocking on worktree mode: direct-mode tasks never send symlinkDirs, so no probe IPC fires and a pending probe can't block submit - Make the buffer regression test actually exceed the old 1 MiB execFile default (5000 × 240-char names ≈ 1.2 MB) Co-Authored-By: Claude <noreply@anthropic.com>
Description
Related to #231. This is PR 1 of 2 and only widens candidate enumeration; persisted per-project selection and symlink-vs-copy behavior remain follow-up work.
Summary
git ls-files --others --ignored --exclude-standard --directorycall.Implementation details
GetGitignoredDirsnow returns{ name, isDefault }[]. The backend owns the default-candidate classification, so the desktop and remote consumers do not duplicate the hardcoded list.Enumeration preserves Git's path order and filters Parallel Code-managed entries that should never be symlink choices:
.claude, because it is seeded by copying for Claude's bwrap sandbox rather than symlinked..worktrees, because it is Parallel Code's managed worktree container. The similarly named.worktreeremains a valid user entry..mcp.jsonand.ripgreprc. Parallel Code writes these patterns to.git/info/exclude; without filtering, its own bookkeeping would reappear as phantom picker options.The New Task dialog renders all returned entries but initially selects only those marked
isDefault. Remote task creation also uses only the default subset because it has no confirmation UI for newly discovered entries.Tests
Added integration coverage against real temporary Git repositories for:
.claude,.worktrees, and all nine sandbox artifact exclusions;.worktreename;createWorktreesymlink creation and nested-name rejection.Notes and follow-ups
.git/info/excludegrowth remains intentionally one-way and has no new marker-block machinery. Newly discovered entries are unchecked, so new root-anchored exclusions are appended only after explicit desktop opt-in and remain idempotent.