feat: add worktree operations and project_dir hot-swap - #68
Conversation
There was a problem hiding this comment.
Post-merge review: worktree operations + has_git() fix
Overall this is a clean, well-structured PR. The has_git() fix is correct, the porcelain parser is solid, and the code follows existing patterns. A few things worth addressing as follow-ups:
🟡 Medium — worktree_switch doesn't validate the target is a git repo
handlers.py — handle_git_worktree_switch
The handler checks os.path.isdir(path) but not has_git(path). Switching to a non-git directory will silently break every subsequent git operation in that session. Suggest:
if not os.path.isdir(path):
abort(404, f"Worktree directory not found: {path}")
if not git_ops.has_git(path):
abort(400, f"Not a git repository or worktree: {path}")🟡 Medium — worktree_switch silently no-ops in multi-project mode
When config["mode"] != "single", the function skips the assignment but still returns {"success": True}. This is misleading — the caller thinks the switch succeeded. Should either handle multi-project mode or abort:
if config["mode"] != "single":
abort(400, "Worktree switch is only supported in single-project mode")🟡 Medium — No -- separator before positional args in git subprocess calls
In worktree_add and worktree_remove, paths and branch names are passed as positional args without a -- separator. A branch named --detach or -f would be interpreted as a git flag. While _run_git uses list-form subprocess (no shell injection), git's own argument parser still sees these.
worktree_add example fix:
rc, out, err = _run_git(project_dir, "worktree", "add", "--", path, branch)🟢 Low — --force placement in worktree_remove
args = ["worktree", "remove", path]
if force:
args.append("--force")This produces git worktree remove <path> --force. Git handles it, but conventional ordering puts flags before positional args:
args = ["worktree", "remove"]
if force:
args.append("--force")
args.extend(["--", path])This also adds the -- separator mentioned above.
🟢 Low — Porcelain parser may drop the last entry
In worktree_list, entries are committed to the list when an empty line is encountered. If git's output doesn't end with a trailing blank line (unlikely but possible in edge cases), the last current dict is silently dropped. Adding after the loop:
if current:
current.setdefault("head", "")
current.setdefault("branch", "")
current["is_main"] = len(worktrees) == 0
worktrees.append(current)🟢 Low — worktree_switch config change is ephemeral
The hot-swap only modifies the in-memory config dict. On server restart, project_path reverts to the original value. This is probably intentional for a "session switch" use case, but worth a docstring note so users aren't surprised.
✅ has_git() fix is correct
The change from os.path.isdir(.git) to os.path.isdir(.git) or os.path.isfile(.git) correctly handles worktree directories (where .git is a file pointing to the main repo) and also fixes detection for submodules (same .git-as-file pattern). Clean and minimal.
Verdict: No showstoppers. The medium items (switch validation, multi-mode no-op, -- separators) are worth a small follow-up PR for robustness.
There was a problem hiding this comment.
Post-Merge Review
Reviewed the full diff. The has_git() fix is clean and correct, and the worktree list/add/remove operations are well-structured. A few things worth addressing as follow-ups.
1. Path traversal in handle_git_worktree_add — name is unsanitized ⚠️
handlers.py, worktree add handler
The name parameter from the request body is .strip()-ped but otherwise passed straight into a path:
wt_name = body.get("name", "").strip() or branch.replace("/", "-")
wt_path = os.path.join(parent_dir, f"{project_basename}--{wt_name}")If name is "../../../tmp/evil", the resulting path escapes the parent directory. Compare with handle_create_project which already validates "/" in name or name.startswith(".") — the worktree handler should do the same on wt_name.
Suggested fix:
if not wt_name or "/" in wt_name or wt_name.startswith("."):
abort(400, "Invalid worktree name")2. handle_git_worktree_switch accepts any directory ⚠️
handlers.py, worktree switch handler
The only validation is os.path.isdir(path). Any existing directory on the filesystem can become the active project path — it doesn't need to be a worktree of the current project (or even a git repo at all).
Suggestion: call has_git(path) at minimum, and ideally verify the path is a registered worktree of the same repository (e.g., check it appears in worktree_list output or shares the same git rev-parse --git-common-dir).
3. worktree_switch silently no-ops in multi mode
handlers.py, ~line 1237
if config["mode"] == "single":
config["project_path"] = path
return {"success": True, "path": path}In multi mode, the function returns {"success": True} without changing anything. Should either abort(400) with a message explaining the limitation, or implement the equivalent for multi-mode. Returning success for a no-op is misleading.
4. --force flag ordering in worktree_remove
git_ops.py, worktree_remove
args = ["worktree", "remove", path]
if force:
args.append("--force")The flag ends up after the positional argument. Modern git usually handles this, but the conventional order is flags before positional args. Safer as:
args = ["worktree", "remove"]
if force:
args.append("--force")
args.append(path)5. DELETE /git/worktrees with request body — portability note
Some HTTP proxies and client libraries strip the body from DELETE requests. If tinyleaf might sit behind a reverse proxy, consider using POST /git/worktrees/remove instead (same pattern as the /switch endpoint). Low priority but worth keeping in mind.
6. No test coverage
The test plan checkboxes are all unchecked and no test files are in the diff. Given the path-handling and config-mutation logic, tests for at least the handler-level validation (name sanitization, switch validation, multi-mode behavior) would be valuable.
What looks good
has_git()fix — correct and minimal. Worktrees use a.gitfile pointing to the main repo's gitdir; checkingisfilein addition toisdiris exactly right.worktree_listporcelain parsing — solid. The--porcelainformat is stable and the parser handles detached HEAD, missing branches, and entry boundaries correctly.- Subprocess safety —
_run_gituses list args throughout, no shell injection surface. - Main worktree protection —
git worktree removeitself refuses to remove the main worktree, so the lack of an explicit guard in the handler is fine. - API route structure — clean, consistent with existing patterns.
Overall this is a solid addition. Items 1 and 2 are the ones I'd prioritize for a follow-up.
— Milo 👻
Addressing review feedbackThanks @elena-oaklight for the post-merge review. All items will be fixed in a follow-up PR:
|
There was a problem hiding this comment.
PR #68 — feat: add worktree operations and project_dir hot-swap → 1 security issue + 1 design gap
has_git() 修复正确——worktree 目录下 .git 是个 file 不是 dir,isfile 检查加得好。worktree_list 的 porcelain 格式解析也干净。
🔴 Security: handle_git_worktree_switch 无路径校验
def handle_git_worktree_switch(body, config, name):
path = body.get("path", "").strip()
if not path:
abort(400, "Worktree path required")
if not os.path.isdir(path):
abort(404, f"Worktree directory not found: {path}")
if config["mode"] == "single":
config["project_path"] = path
return {"success": True, "path": path}请求体里的 path 只检查了 isdir,没有验证它是不是当前 repo 的 worktree。攻击者(或前端 bug)可以把 project_path 指向文件系统上任何目录,后续所有文件操作都会在那个目录执行。
修法:调 worktree_list(project_dir) 拿到合法 worktree 路径列表,校验 os.path.abspath(path) 在列表中。
🐛 Bug: multi-project mode 静默成功
if config["mode"] == "single":
config["project_path"] = path
return {"success": True, "path": path}mode != "single" 时,函数返回 success: True 但什么都没做。前端会以为切换成功,用户看到的还是旧 worktree 内容。应该要么实现 multi-project 的 switch(更新对应 project entry 的 path),要么返回 success: False + 明确消息。
Non-blocking
worktree_remove里--force放在path后面:git worktree remove <path> --force。git 能接受,但惯例 flags 在 positional args 前面。- DELETE
/git/worktrees用request.json()而不是_json_body(request)helper——其他 DELETE route(如 #69 的 delete branch)用的是_json_body,不一致。
Summary
worktree_list(),worktree_add(),worktree_remove()togit_ops.pywith porcelain output parsingworktree_switchhandler for project_dir hot-swap inhandlers.pyGET/POST/DELETE /git/worktrees,POST /git/worktrees/switch) inserver.pyhas_git()to recognize worktree directories where.gitis a file (not a directory)Closes #64
Closes #65
Parent: #60
Test plan
worktree_listreturns correct entries with path, branch, head (8 chars), and is_main flagworktree_addcreates a new worktree and returns success with absolute pathworktree_removeremoves a worktree and respects the force flagworktree_switchupdatesconfig["project_path"]in single modehas_git()returns True for worktree directories (where.gitis a file)