diff --git a/COMMANDS.md b/COMMANDS.md index 893b0cd..c1cf306 100644 --- a/COMMANDS.md +++ b/COMMANDS.md @@ -359,7 +359,7 @@ Each starts an agent loop in the REPL. | `/snapshot list` | List saved snapshots. | | `/limit ` | Cap UVT spend for the session (`/limit off` to remove). | | `/audit-receipt [n]` | Verified log of tool calls + UVT (local custody + server). | -| `/rollback [n]` | Revert the last n uncommitted filesystem changes (git-backed). | +| `/rollback` | Discard uncommitted changes to tracked files (git-backed). Restores from the index, so files with staged changes come back to their staged state, not to the last commit. Untracked files are never touched. | | `/logs-view`, `/logs` | Interactive session log browser. | ### Goals & workflows diff --git a/src/commands/slash_git_tools.ts b/src/commands/slash_git_tools.ts index c1466a2..cb8508f 100644 --- a/src/commands/slash_git_tools.ts +++ b/src/commands/slash_git_tools.ts @@ -1,64 +1,136 @@ // In-REPL git-backed rollback slash commands: /rollback /revert /stage-diff. // Split out of slash.ts (was 1807 lines) to keep each command group under // the repo's ~800-line file convention. +// +// Two commands here discard the user's uncommitted work, so they follow three +// rules. Every git call is an argv array with literal pathspecs and a `--` +// before any user-supplied path. Nothing is destroyed that the preview did not +// name. And the completion message states what was actually restored, rather +// than the most reassuring thing that could be said. import type { Writable } from "node:stream"; import type { AppContext } from "../core/context.js"; -import { execFileSync } from "node:child_process"; -import { existsSync } from "node:fs"; -import { join } from "node:path"; +import { SpawnGitRunner, type GitRunner } from "../core/git_commit_guard.js"; import { theme } from "../ui/theme.js"; import { generateDiff } from "../core/stage_diff.js"; +/** Injected so the destructive paths are testable without a real repository. */ +export interface GitToolDeps { + cwd: string; + git: GitRunner; +} + +export function defaultGitToolDeps(): GitToolDeps { + const cwd = process.cwd(); + return { cwd, git: new SpawnGitRunner(cwd) }; +} + +/** + * Ask git whether we are in a repository, rather than looking for a `.git` + * directory. The directory probe reports "not a git repository" for every + * subdirectory of one, which is where a REPL usually sits. + */ +function repoRoot(deps: GitToolDeps): string | null { + const result = deps.git.run(["rev-parse", "--show-toplevel"]); + return result.ok ? result.stdout.trim() || null : null; +} + +function lines(raw: string): string[] { + return raw.trim().split("\n").filter(Boolean); +} + +/** + * `git checkout -- ` and `git restore -- ` both restore from the + * INDEX, not from HEAD. When something is staged, the content that comes back + * is the staged content — so a message promising "last commit" is false exactly + * when the user has staged work they might be counting on. + */ +function restoreTargetLabel(deps: GitToolDeps, pathspec: readonly string[]): string { + const staged = deps.git.run(["diff", "--cached", "--name-only", "--", ...pathspec]); + return staged.ok && staged.stdout.trim() ? "their staged state" : "last commit"; +} + // ── /rollback ───────────────────────────────── -export async function rollbackSlash(ctx: AppContext, out: Writable, arg: string): Promise { - const n = parseInt(arg.trim()) || 1; - if (n < 1 || n > 50) { - out.write("usage: /rollback [n] revert last n filesystem changes (1-50, default 1)\n"); +const ROLLBACK_USAGE = + "usage: /rollback discard uncommitted changes to tracked files\n" + + " restores tracked files from the index; untracked files are left alone\n"; + +export async function rollbackSlash( + ctx: AppContext, + out: Writable, + arg: string, + deps: GitToolDeps = defaultGitToolDeps(), +): Promise { + const given = arg.trim(); + if (given) { + // `/rollback [n]` parsed a count, range-checked it, then never referenced it + // again — every invocation reverted everything. There is no checkpoint + // journal behind it, so the honest fix is to refuse the argument rather than + // keep accepting one that does nothing. + out.write( + theme.muted( + "/rollback takes no argument. The old [n] count never did anything —\n" + + " every invocation reverted the whole working tree, whatever number you passed.\n", + ), + ); + out.write(ROLLBACK_USAGE); return; } - const cwd = process.cwd(); - const gitDir = join(cwd, ".git"); - if (!existsSync(gitDir)) { + if (repoRoot(deps) === null) { out.write(theme.muted("Not in a git repository. /rollback requires git for safe undo.\n")); return; } - try { - const status = execFileSync("git", ["-c", "core.literalPathspecs=true", "diff", "--name-only"], { cwd, encoding: "utf8", timeout: 5000 }); - const dirty = status.trim().split("\n").filter(Boolean); - if (dirty.length === 0) { - out.write("(working tree clean — nothing to rollback)\n"); - return; - } + const status = deps.git.run(["diff", "--name-only"]); + if (!status.ok) { + out.write(`✗ ${status.stderr.trim() || "could not read the working tree"}\n`); + return; + } + const dirty = lines(status.stdout); + if (dirty.length === 0) { + out.write("(working tree clean — nothing to rollback)\n"); + return; + } - out.write(`${theme.cyan("↩ Ready to rollback")} ${dirty.length} files changed\n`); - out.write(theme.dim("──────────────────────────────────────────────────────────────\n")); - const show = dirty.slice(0, 20); - for (const f of show) { - out.write(` ${theme.muted(f)}\n`); - } - if (dirty.length > 20) { - out.write(` ${theme.dim(`... and ${dirty.length - 20} more`)}\n`); - } + const target = restoreTargetLabel(deps, []); - const ok = ctx.flags.yes || (await ctx.confirm(`\nRevert all ${dirty.length} uncommitted changes? [y/N] `)); - if (!ok) { - out.write("cancelled.\n"); - return; - } + out.write(`${theme.cyan("↩ Ready to rollback")} ${dirty.length} files changed\n`); + out.write(theme.dim("──────────────────────────────────────────────────────────────\n")); + for (const file of dirty.slice(0, 20)) out.write(` ${theme.muted(file)}\n`); + if (dirty.length > 20) out.write(` ${theme.dim(`... and ${dirty.length - 20} more`)}\n`); - execFileSync("git", ["-c", "core.literalPathspecs=true", "checkout", "--", "."], { cwd, encoding: "utf8", timeout: 10000 }); - out.write(`${theme.cyan("↩ rolled back")} ${dirty.length} files restored to last commit.\n`); - out.write(theme.dim(" Git reflog untouched — all commits preserved.\n")); - } catch (err) { - out.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); + if (target !== "last commit") { + out.write( + theme.muted( + "\n ! you have staged changes, so these files restore to their STAGED state,\n" + + " not to the last commit. To go all the way back to HEAD:\n" + + " git restore --source=HEAD --staged --worktree -- .\n", + ), + ); + } + out.write(theme.dim("\n untracked files are not touched.\n")); + + const ok = + ctx.flags.yes || (await ctx.confirm(`\nDiscard uncommitted changes to ${dirty.length} files? [y/N] `)); + if (!ok) { + out.write("cancelled.\n"); + return; + } + + // `git restore` over `git checkout --`: restore only ever touches files, + // so it cannot be talked into switching branches by a crafted pathspec. + const done = deps.git.run(["restore", "--", "."]); + if (!done.ok) { + out.write(`✗ ${done.stderr.trim() || "restore failed"}\n`); + return; } + out.write(`${theme.cyan("↩ rolled back")} ${dirty.length} files restored to ${target}.\n`); + out.write(theme.dim(" Git reflog untouched — all commits preserved.\n")); } -// ── /stage-diff ──────────────────────────────── +// ── /stage-diff ─────────────────────────────── export async function stageDiffSlash(_ctx: AppContext, out: Writable): Promise { try { @@ -106,75 +178,79 @@ export async function stageDiffSlash(_ctx: AppContext, out: Writable): Promise { +export async function revertSlash( + ctx: AppContext, + out: Writable, + arg: string, + deps: GitToolDeps = defaultGitToolDeps(), +): Promise { const target = arg.trim(); if (!target) { - out.write("usage: /revert surgical rollback\n"); - out.write(" /revert src/core/old.ts revert single file\n"); - out.write(" /revert step-3 revert to checkpoint (coming soon)\n"); + out.write("usage: /revert discard uncommitted changes to one tracked file\n"); + out.write(" /revert src/core/old.ts\n"); return; } - const cwd = process.cwd(); - const gitDir = join(cwd, ".git"); - if (!existsSync(gitDir)) { + if (repoRoot(deps) === null) { out.write(theme.muted("Not in a git repository.\n")); return; } - if (target.startsWith("step-") || target.match(/^\d+$/)) { - out.write(theme.muted("Step-based revert not yet available. Use /rollback to revert all, or /revert for a single file.\n")); - out.write(theme.dim(" Tracked step checkpoints planned for future release.\n")); + if (target.startsWith("step-") || /^\d+$/.test(target)) { + // No checkpoint journal exists, so this cannot work and is not "coming soon". + out.write( + theme.muted( + "/revert takes a file path. There are no step checkpoints to revert to —\n" + + " nothing records per-step filesystem state. Use /revert , or\n" + + " /rollback to discard every uncommitted change.\n", + ), + ); return; } - try { - const isTracked = (() => { - try { - execFileSync("git", ["-c", "core.literalPathspecs=true", "ls-files", "--error-unmatch", "--", target], { cwd, encoding: "utf8", timeout: 3000 }); - return true; - } catch { return false; } - })(); - - if (!isTracked) { - out.write(`${theme.muted(target)} is not tracked by git.\n`); - return; - } - - const diffOut = execFileSync("git", ["-c", "core.literalPathspecs=true", "diff", "--name-only", "--", target], { cwd, encoding: "utf8", timeout: 3000 }); - if (!diffOut.trim()) { - out.write(`(no uncommitted changes in ${target})\n`); - return; - } + if (!deps.git.run(["ls-files", "--error-unmatch", "--", target]).ok) { + out.write(`${theme.muted(target)} is not tracked by git.\n`); + return; + } - const fileDiff = execFileSync("git", ["-c", "core.literalPathspecs=true", "diff", "--", target], { cwd, encoding: "utf8", timeout: 5000 }); - const changes = fileDiff.trim().split("\n").length; + const changed = deps.git.run(["diff", "--name-only", "--", target]); + if (!changed.ok) { + out.write(`✗ ${changed.stderr.trim() || "could not read the working tree"}\n`); + return; + } + if (!changed.stdout.trim()) { + out.write(`(no uncommitted changes in ${target})\n`); + return; + } - out.write(`${theme.cyan("↩ Reverting")} ${theme.bold(target)} (${changes} line changes)\n`); - out.write(theme.dim("──────────────────────────────────────────────────────────────\n")); + const fileDiff = deps.git.run(["diff", "--", target]).stdout; + const restoresTo = restoreTargetLabel(deps, [target]); - for (const line of fileDiff.split("\n").slice(0, 10)) { - if (line.startsWith("+")) out.write(theme.dim(line) + "\n"); - else if (line.startsWith("-")) out.write(theme.muted(line) + "\n"); - else out.write(theme.dim(line) + "\n"); - } + out.write(`${theme.cyan("↩ Reverting")} ${theme.bold(target)} (${lines(fileDiff).length} line changes)\n`); + out.write(theme.dim("──────────────────────────────────────────────────────────────\n")); + for (const line of fileDiff.split("\n").slice(0, 10)) { + out.write((line.startsWith("-") ? theme.muted(line) : theme.dim(line)) + "\n"); + } + if (restoresTo !== "last commit") { + out.write( + theme.muted( + `\n ! ${target} has staged changes, so it restores to its STAGED state,\n` + + " not to the last commit.\n", + ), + ); + } - const ok = ctx.flags.yes || (await ctx.confirm(`\nRevert ${target} to last commit? [y/N] `)); - if (!ok) { - out.write("cancelled.\n"); - return; - } + const ok = ctx.flags.yes || (await ctx.confirm(`\nRestore ${target} to ${restoresTo}? [y/N] `)); + if (!ok) { + out.write("cancelled.\n"); + return; + } - execFileSync("git", ["-c", "core.literalPathspecs=true", "checkout", "--", target], { cwd, encoding: "utf8", timeout: 10000 }); - out.write(`${theme.cyan("↩ reverted")} ${target} restored to last commit.\n`); - } catch (err: unknown) { - const stderr = err && typeof err === "object" && "stderr" in err - ? String((err as { stderr?: unknown }).stderr ?? "") - : ""; - if (stderr.includes("did not match any file")) { - out.write(`not found: ${target}\n`); - } else { - out.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); - } + const done = deps.git.run(["restore", "--", target]); + if (!done.ok) { + const why = done.stderr.trim(); + out.write(why.includes("did not match any file") ? `not found: ${target}\n` : `✗ ${why || "restore failed"}\n`); + return; } + out.write(`${theme.cyan("↩ reverted")} ${target} restored to ${restoresTo}.\n`); } diff --git a/src/commands/slash_registry.ts b/src/commands/slash_registry.ts index 8aa3361..1c1aa54 100644 --- a/src/commands/slash_registry.ts +++ b/src/commands/slash_registry.ts @@ -78,7 +78,7 @@ export const SLASH_COMMANDS: SlashCommand[] = [ { name: "limit", args: "", summary: "cap UVT spend for this session", section: "Context & Limits" }, { name: "token-budget", args: "", summary: "alias for /limit", section: "Context & Limits", hidden: true }, { name: "audit-receipt", args: "[n]", summary: "verified log of tool calls + UVT", section: "Context & Limits" }, - { name: "rollback", args: "[n]", summary: "revert last n filesystem changes", section: "Context & Limits" }, + { name: "rollback", summary: "discard uncommitted changes to tracked files", section: "Context & Limits" }, { name: "logs-view", aliases: ["logs"], summary: "interactive session log browser", section: "Context & Limits" }, // ── Goals & Workflows ── @@ -111,7 +111,7 @@ export const SLASH_COMMANDS: SlashCommand[] = [ { name: "bench", args: "", summary: "profile & optimize code", section: "UVT Tools" }, { name: "purge", summary: "flush transient context & temp files", section: "UVT Tools" }, { name: "stage-diff", summary: "unified diff + commit message", section: "UVT Tools" }, - { name: "revert", args: "", summary: "surgical rollback", section: "UVT Tools" }, + { name: "revert", args: "", summary: "discard uncommitted changes to one file", section: "UVT Tools" }, // ── Media ── { name: "photogen", args: " [--model --aspect]", summary: "generate images", section: "Media" }, diff --git a/test/slash_git_tools.test.ts b/test/slash_git_tools.test.ts new file mode 100644 index 0000000..b6abcfc --- /dev/null +++ b/test/slash_git_tools.test.ts @@ -0,0 +1,190 @@ +// First behavioural coverage for /rollback and /revert. Both mutate the user's +// working tree, and both shipped with no tests at all. +// +// Every test drives a scripted GitRunner, so nothing here touches a real +// repository. The argv assertions are the point: these commands are destructive, +// and what matters is exactly which git verbs they issue and which they never do. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { PassThrough } from "node:stream"; +import type { AppContext } from "../src/core/context.js"; +import type { GitRunner, GitRunResult } from "../src/core/git_commit_guard.js"; +import { rollbackSlash, revertSlash, type GitToolDeps } from "../src/commands/slash_git_tools.js"; + +const OK = (stdout = ""): GitRunResult => ({ ok: true, stdout, stderr: "", exitCode: 0 }); +const FAIL = (stderr = ""): GitRunResult => ({ ok: false, stdout: "", stderr, exitCode: 1 }); + +/** Records every argv and answers from a longest-prefix table. */ +function fakeGit(table: Record): { git: GitRunner; calls: string[][] } { + const calls: string[][] = []; + const git: GitRunner = { + run(args: string[]): GitRunResult { + calls.push(args); + const key = args.join(" "); + let best: { length: number; result: GitRunResult } | null = null; + for (const [pattern, result] of Object.entries(table)) { + if (key.startsWith(pattern) && (best === null || pattern.length > best.length)) { + best = { length: pattern.length, result }; + } + } + return best?.result ?? OK(); + }, + }; + return { git, calls }; +} + +function deps(table: Record): { deps: GitToolDeps; calls: string[][] } { + const { git, calls } = fakeGit({ "rev-parse --show-toplevel": OK("/repo\n"), ...table }); + return { deps: { cwd: "/repo", git }, calls }; +} + +function ctxWith(answer: boolean, yes = false): AppContext { + return { + flags: { yes }, + confirm: async () => answer, + } as unknown as AppContext; +} + +function sink(): { out: PassThrough; text: () => string } { + const chunks: string[] = []; + const out = new PassThrough(); + out.on("data", (chunk) => chunks.push(String(chunk))); + return { out, text: () => chunks.join("") }; +} + +const ran = (calls: string[][], verb: string): boolean => calls.some((call) => call.includes(verb)); + +// ── the count that never worked ───────────────────────────────────────────── + +test("/rollback rejects a count instead of silently ignoring it", async () => { + const { deps: d, calls } = deps({ "diff --name-only": OK("a.ts\nb.ts\n") }); + const { out, text } = sink(); + await rollbackSlash(ctxWith(true, true), out, "5", d); + assert.match(text(), /never/i, "must say the count never did anything"); + assert.equal(ran(calls, "checkout"), false, "a rejected invocation must not mutate the tree"); + assert.equal(ran(calls, "restore"), false); +}); + +test("/rollback usage text does not promise per-change undo", async () => { + const { deps: d } = deps({ "diff --name-only": OK("") }); + const { out, text } = sink(); + await rollbackSlash(ctxWith(false), out, "not-a-number", d); + assert.equal(/revert last n/i.test(text()), false, "the old usage line claimed a capability that does not exist"); +}); + +// ── truthfulness about what was restored ──────────────────────────────────── + +test("/rollback does not claim 'last commit' while changes are staged", async () => { + const { deps: d, calls } = deps({ + "diff --name-only": OK("a.ts\n"), + "diff --cached --name-only": OK("a.ts\n"), + }); + const { out, text } = sink(); + await rollbackSlash(ctxWith(true, true), out, "", d); + assert.equal(ran(calls, "checkout") || ran(calls, "restore"), true, "it should still restore"); + assert.equal( + /restored to last commit/i.test(text()), + false, + "with a populated index the restore target is the staged content, not HEAD", + ); + assert.match(text(), /staged/i, "it must name what it actually restored to"); +}); + +test("/rollback may say 'last commit' only when nothing is staged", async () => { + const { deps: d } = deps({ + "diff --name-only": OK("a.ts\n"), + "diff --cached --name-only": OK(""), + }); + const { out, text } = sink(); + await rollbackSlash(ctxWith(true, true), out, "", d); + assert.match(text(), /last commit/i); +}); + +// ── destructive-scope guards ──────────────────────────────────────────────── + +test("/rollback never removes untracked files", async () => { + const { deps: d, calls } = deps({ "diff --name-only": OK("a.ts\n") }); + const { out } = sink(); + await rollbackSlash(ctxWith(true, true), out, "", d); + assert.equal(ran(calls, "clean"), false, "reverting tracked files must not delete untracked ones"); +}); + +test("/rollback declining the prompt mutates nothing", async () => { + const { deps: d, calls } = deps({ "diff --name-only": OK("a.ts\nb.ts\n") }); + const { out, text } = sink(); + await rollbackSlash(ctxWith(false), out, "", d); + assert.match(text(), /cancel/i); + assert.equal(ran(calls, "checkout"), false); + assert.equal(ran(calls, "restore"), false); +}); + +test("/rollback on a clean tree issues no mutation", async () => { + const { deps: d, calls } = deps({ "diff --name-only": OK("") }); + const { out, text } = sink(); + await rollbackSlash(ctxWith(true, true), out, "", d); + assert.match(text(), /clean/i); + assert.equal(ran(calls, "checkout"), false); + assert.equal(ran(calls, "restore"), false); +}); + +// ── repository detection ──────────────────────────────────────────────────── + +test("/rollback works from a subdirectory, not only a repo root", async () => { + const { git, calls } = fakeGit({ + "rev-parse --show-toplevel": OK("/repo\n"), + "diff --name-only": OK("a.ts\n"), + }); + const { out, text } = sink(); + await rollbackSlash(ctxWith(true, true), out, "", { cwd: "/repo/src/deep", git }); + assert.equal(/not in a git repository/i.test(text()), false, "a subdirectory of a repo is still in the repo"); + assert.equal(ran(calls, "rev-parse"), true, "detection must ask git, not look for a .git directory"); +}); + +test("/rollback outside a repository refuses and mutates nothing", async () => { + const { git, calls } = fakeGit({ "rev-parse --show-toplevel": FAIL("not a git repository") }); + const { out, text } = sink(); + await rollbackSlash(ctxWith(true, true), out, "", { cwd: "/tmp", git }); + assert.match(text(), /not in a git repository/i); + assert.equal(ran(calls, "checkout"), false); +}); + +// ── /revert ───────────────────────────────────────────────────────────────── + +test("/revert does not claim 'last commit' while the file is staged", async () => { + const { deps: d } = deps({ + "ls-files --error-unmatch": OK("a.ts\n"), + "diff --name-only -- a.ts": OK("a.ts\n"), + "diff -- a.ts": OK("--- a\n+++ b\n+x\n"), + "diff --cached --name-only -- a.ts": OK("a.ts\n"), + }); + const { out, text } = sink(); + await revertSlash(ctxWith(true, true), out, "a.ts", d); + assert.equal(/restored to last commit/i.test(text()), false); + assert.match(text(), /staged/i); +}); + +test("/revert passes the path after -- and never through a shell", async () => { + const { deps: d, calls } = deps({ + "ls-files --error-unmatch": OK("-weird file.ts\n"), + "diff --name-only": OK("-weird file.ts\n"), + "diff -- ": OK("+x\n"), + "diff --cached --name-only": OK(""), + }); + const { out } = sink(); + await revertSlash(ctxWith(true, true), out, "-weird file.ts", d); + for (const call of calls) { + const target = call.indexOf("-weird file.ts"); + if (target === -1) continue; + assert.equal(call[target - 1], "--", `path must follow a -- separator: ${call.join(" ")}`); + } +}); + +test("/revert on an untracked file refuses and mutates nothing", async () => { + const { deps: d, calls } = deps({ "ls-files --error-unmatch": FAIL("did not match any file") }); + const { out, text } = sink(); + await revertSlash(ctxWith(true, true), out, "new.ts", d); + assert.match(text(), /not tracked/i); + assert.equal(ran(calls, "checkout"), false); + assert.equal(ran(calls, "restore"), false); +});