fix(opencode): prevent /undo from using unsafe root pathspecs#25403
fix(opencode): prevent /undo from using unsafe root pathspecs#25403himax12 wants to merge 1 commit intoanomalyco:devfrom
Conversation
Skip unsafe revert targets (worktree root and outside paths) and use normalized relative pathspecs for checkout operations. Adds regression coverage to ensure worktree-root targets do not mutate tracked file mtimes.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
There was a problem hiding this comment.
Pull request overview
Hardens snapshot revert (used by /undo) to prevent unsafe git checkout pathspecs (e.g., worktree root) from expanding the checkout scope and touching unrelated files/mtimes.
Changes:
- Normalize revert targets against the active worktree and skip unsafe targets (worktree root / outside worktree).
- Use normalized relative pathspecs for
git checkoutduring revert (single + batched paths). - Add a regression test asserting that worktree-root targets are ignored (no mtime churn).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/opencode/src/snapshot/index.ts | Adds worktree-based normalization/filtering and switches revert checkout to relative pathspecs. |
| packages/opencode/test/snapshot/snapshot.test.ts | Adds regression coverage for ignoring worktree-root revert targets. |
Comments suppressed due to low confidence (1)
packages/opencode/src/snapshot/index.ts:415
- Switching
git checkoutto useop.relintroduces Git pathspec parsing (e.g. pathspec magic like:(exclude)/:(icase)and globbing) for any revert target that starts with:or contains pattern characters. That can broaden the checkout scope again despite the root/outside filtering. To ensure the argument is treated as a literal path, consider enabling literal pathspecs for these invocations (e.g. pass--literal-pathspecsto git, or prefix each pathspec with:(literal)), and apply the same treatment to thels-treeprobe below.
log.info("reverting", { file: op.file, hash: op.hash })
const result = yield* git([...core, ...args(["checkout", op.hash, "--", op.rel])], {
cwd: state.worktree,
})
if (result.code === 0) return
const tree = yield* git([...core, ...args(["ls-tree", op.hash, "--", op.rel])], {
cwd: state.worktree,
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (list.length) { | ||
| log.info("reverting", { hash: first.hash, files: list.length }) | ||
| const result = yield* git( | ||
| [...core, ...args(["checkout", first.hash, "--", ...list.map((item) => item.file)])], | ||
| [...core, ...args(["checkout", first.hash, "--", ...list.map((item) => item.rel)])], | ||
| { |
| const worktree = path.resolve(state.worktree) | ||
|
|
||
| const normalize = (file: string) => { | ||
| const resolved = path.resolve(file) | ||
| const rel = path.relative(worktree, resolved).replaceAll("\\", "/") | ||
| const outside = rel === ".." || rel.startsWith("../") | ||
| const root = !rel || rel === "." | ||
| const absoluteRel = path.isAbsolute(rel) || /^[A-Za-z]:[\\/]/.test(rel) | ||
| if (outside || root || absoluteRel) return | ||
| return { file: resolved, rel } |
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
|
please reopenand review this fix! |
Fixes #25400
/undocan receive snapshot patch entries that resolve to an unsafe target (for example worktree root).When that reaches git checkout pathspec handling, it can broaden the checkout scope and touch unrelated files.
What changed:
Verification:
bun --cwd packages/opencode test test/snapshot/snapshot.test.ts -t revert --timeout 120000bun --cwd packages/opencode test test/session/revert-compact.test.ts --timeout 120000