diff --git a/CLAUDE.md b/CLAUDE.md index 06bfa97d..f540fed1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,6 +1,6 @@ # Claude Code Working Guide -_Last synced: v2.8.41 (2026-10-30) — a session outside every workspace is billed to the user of the Linux account it ran as._ A change that makes any statement in this file, `AGENTS.md`, or a README wrong — a moved path, a new or changed extension point, a retired rule — updates that file in the same change-set and bumps this marker. **Bump = replace the version/date and a ≤ 1-line summary; never append a changelog here** — details belong in the commit message and `docs/`, because this whole file is loaded into every session. +_Last synced: v2.8.44 (2026-10-30) — `discard` takes the Changes panel's ticked files and blocks (`hunks`), never every file._ A change that makes any statement in this file, `AGENTS.md`, or a README wrong — a moved path, a new or changed extension point, a retired rule — updates that file in the same change-set and bumps this marker. **Bump = replace the version/date and a ≤ 1-line summary; never append a changelog here** — details belong in the commit message and `docs/`, because this whole file is loaded into every session. ## Project mission - Maintain a reliable local-first dashboard for Claude Code session monitoring. @@ -45,7 +45,7 @@ A **workspace** — called a **lane** in the API, CLI, database and code, which - **The native picker opens only for a browser on this machine, and nothing from the request but a start folder reaches its argv.** `POST /api/lanes/pick-path` refuses a non-loopback socket and any request a proxy forwarded (`EREMOTE`) — behind nginx every request comes from loopback, and the dialog would open on a screen nobody watches. Do not relax that check, and do not let the request name the dialog program, a title or any other argument. - **Never build a git command as a shell string.** Lane git operations use `execFile("git", [...args])` with an explicit argument array (see `server/lib/worktree.js`'s `git()` helper), never a concatenated/interpolated string passed to a shell. - **An AI Proxy (called AI Router before v2.5.5) is a SESSION's choice, and only its name comes from the request.** A new session names a profile (`proxy` on `POST /api/lanes/:id/start` / `POST /api/run` — the pre-rename `router` is still accepted —, the **AI Proxy** select beside the model picker); `resolveProxy` (`server/lib/proxy-apply.js`) checks it (`EBADPROXY`, `EUNSUPPORTED` off Claude/Codex) and the key and endpoint come from the store. A profile's `baseUrl` is the proxy's ROOT (a trailing `/v1` is dropped on save and on read): a **claude** pane gets `--settings=/proxy-settings/.json` with `ANTHROPIC_BASE_URL` = the root (0700/0600; plus `modelPicker` rows only when the profile sets a user-chosen `behavesAs` — never guess one, it sets the context window Claude Code compacts at; the command-line layer outranks project-local and user settings, so only the PATH is in argv), a **codex** pane gets `-p ` and `$CODEX_HOME/.config.toml` with `base_url` = root + `/v1` (its base `config.toml` is never touched); both are rewritten at each spawn, `PATCH /api/ai-proxies/:name` rewrites the existing ones and `DELETE` removes them. `dashboard_runs.proxy` (renamed in place from `router`) records it, and a resume naming none goes back to it. **Never write a proxy into the user's project again** — `.claude/settings.local.json` is shared by every session in the folder and is not in a worktree; `clearLaneProxies` takes the old keys out at start and `lanes.router_profile` stays NULL. Key material never leaves the server: profiles live at `/agenthub-proxies/` (0700/0600; the old `agenthub-routers/` is moved there on first read and left as a symlink for an older build), `GET /api/ai-proxies` reports only `hasKey`, and there is deliberately no route that returns one profile in full. `/api/ai-routers` is the same router under its pre-rename path. **`POST /api/ai-proxies/test` sends a stored key ONLY to the endpoint it was stored against** — a blank `apiKey` falls back to the profile named by `name`, but only while the submitted `baseUrl` still equals the saved one, else `EBADKEY`; it probes `/v1/messages` (Claude) and `/v1/responses` (Codex) and answers `{claude, codex}`. Relax that and the route becomes a way to make the server POST a saved key to any URL a browser names. **Codex's key reaches the pane through tmux's `-e` argv (`AGENTHUB_PROXY_KEY`) and is never typed into it** — `tmux.sendCommand` takes over an idle pane with `respawn-pane` for exactly this reason; a `KEY='sk-…' codex …` line typed with `send-keys` would be rendered to every attached browser, kept in the scrollback and appended to the user's shell history. Do not reintroduce a typed command line there, and do not put a Claude key in argv either. See `server/lib/ai-proxies.js`, `server/lib/proxy-apply.js`. -- **The dashboard's git writes are these twenty-two, and the guards are IN `git-ops.js`, not in the UI.** `POST /api/lanes/:id/git/:action` offers `fetch`, `pull`, `push`, `stage`, `unstage`, `discard`, `stage-hunk`, `discard-hunk`, `commit`, `switch`, `branch-create`, `branch-delete`, `publish`, `merge`, `resolve`, `merge-commit`, `merge-abort`, `cherry-pick`, `revert`, `undo-commit`, `amend`, `ignore` — nothing else. `commit`/`amend` with `paths` (the Changes panel's ticked files, JetBrains-style) run `git commit --only -- ` over `resolvePaths`-checked paths, never a conflicted one, and with `hunks` (ticked change blocks) commit from a TEMPORARY `GIT_INDEX_FILE` built from git's own fresh diff (`selectionPatch`, never request text), then reset only those paths' real index entries; `ignore` appends an anchored `/path` line to the lane's OWN `.gitignore` for an untracked path only (`ETRACKED`), never through a symlink. **`publish` is the ONE write that takes a destination from the request** (`{url}`), so it can only give a remote to a repo that has NONE: it refuses before the first commit (`ENOCOMMITS`) and whenever `git remote` prints anything (`EREMOTEEXISTS` — it never re-points a remote, so it cannot redirect a repo that already publishes somewhere), the name is always `origin`, the URL passes `validateRemoteUrl`'s allowlist (`https://` with NO userinfo, `ssh://`, scp-like `user@host:path` — never a local path, `file://`, `ext::`, `fd::`, `git://`, `http://` or anything starting with `-`), it runs `remote add origin -- ` then the same `push -u origin HEAD` as `push`, and a failed push removes `origin` again. That is still weaker than the AI-proxy "a key goes only to its stored endpoint" rule — a browser on the dashboard's origin can publish a remote-less repo to any allowlisted host — so do not widen the allowlist or let it touch a repo that already has a remote. **A path in the body is never a path**: `resolvePaths` accepts only what `git status` is reporting as changed right now, which is the whole authorization story for the per-file actions (no `..`, no absolute path, no untouched file). `discard`, `discard-hunk` and `merge-abort` are the irreversible actions and all refuse to run without `confirm: true`; do not add another destructive action without the same gate. `branch-create` never switches (the reason to create one from a lane is a sibling worktree, not moving a working agent's HEAD) and `branch-delete` is `-d`, never `-D`, refusing `main`/`master`/`development`/`develop`, the current branch and one another worktree holds. `pull` is ALWAYS `--ff-only` and `switch` refuses a dirty tree, so neither can invent a merge commit or a conflict underneath a running agent (a `switch` to a branch only a remote has creates its tracking branch from a `refs/remotes` ref it LOOKED UP — `listRemoteBranches` — never one from the request); `push` pushes `HEAD`, never a branch name from the request; `commit` passes its message as one argv element. **The lane is the boundary, not the repo**: a subdir lane's reads and writes stay under its own cwd, and an extra folder (`?source=`, resolved by `sourceFolder` from a `folder` row of THIS lane's `lane_sources`, never a path — `404 ENOSOURCE` otherwise) and a repo inside the lane's folder (`?repo=`, accepted only when `server/lib/nested-repos.js`'s own disk walk found it — `404 ENOREPO` otherwise) are each their own boundary under every guard here; no read may return a `settings.local.json` (`ESECRET`) — do not add a git read that skips `lanePath`/`isSecret`. **A merge is a history-joining action allowed only because its recovery is a button too** (`merge-abort`) — and `cherry-pick`/`revert` ride the same rule (a non-merge commit by sha, `EMERGECOMMIT`/`EBADSHA`; same clean-tree, repo-root and conflict flow; `merge-commit`/`merge-abort` finish or abort whichever of the three `operationInProgress` finds open; a no-op is aborted again as `EEMPTY`, never left open): it starts only by name, on a clean tree, from a lane at the repo ROOT (`ESUBDIR` — a merge is repo-wide and a subdir lane could not resolve a conflict outside itself), one at a time (`pull`/`switch`/`merge`/hunk actions answer `EMERGING` meanwhile); the ref is looked up in `refs/heads`+`refs/remotes`, never passed through; a conflicted file is resolved only through `resolve` (ours/theirs, or `done` once no conflict marker is left — `stage` refuses a conflicted file that still has markers, `EMARKERS`), and `merge-commit` refuses while anything is unmerged (`ECONFLICTS`). Do not add rebase, a hard/mixed reset or stash here: a dashboard click is the wrong place to start an operation whose recovery needs a terminal, and a hard `reset` already lives behind `worktree.js`'s three-check destroy guard. The ONE reset is `undo-commit` = `reset --soft HEAD~1` (`confirm: true`, repo root, never the root or a merge commit), and it and `amend` act only on the HEAD the request names (`sha`, `ESTALE` once an agent committed past it) and refuse any HEAD a remote-tracking branch already holds (`EPUSHED`); a terminal-started rebase blocks every history action (`EMERGING`) — rewriting a pushed commit is a terminal decision; do not relax that check. A button that looks disabled in `LaneGitBar` is a courtesy; the refusal that matters is the server's. Reads (`/git`, `/git/changes`, `/git/log`, `/git/file`, `/git/raw` — an image's bytes for the viewer, extension-allowlisted and served under a `sandbox` CSP + `nosniff` so an SVG runs no script; keep both headers —, `/git/ls`, `/git/tree`, `/git/branches`) stay unguarded like `/git` always was, and resolve their repo from the LANE ROW (or its session / `lane_sources` row) — `GET /api/lanes/branches` is the only one that takes a path from the request, because its lane does not exist yet. +- **The dashboard's git writes are these twenty-two, and the guards are IN `git-ops.js`, not in the UI.** `POST /api/lanes/:id/git/:action` offers `fetch`, `pull`, `push`, `stage`, `unstage`, `discard`, `stage-hunk`, `discard-hunk`, `commit`, `switch`, `branch-create`, `branch-delete`, `publish`, `merge`, `resolve`, `merge-commit`, `merge-abort`, `cherry-pick`, `revert`, `undo-commit`, `amend`, `ignore` — nothing else. `commit`/`amend` with `paths` (the Changes panel's ticked files, JetBrains-style) run `git commit --only -- ` over `resolvePaths`-checked paths, never a conflicted one, and with `hunks` (ticked change blocks) commit from a TEMPORARY `GIT_INDEX_FILE` built from git's own fresh diff (`selectionPatch`, never request text), then reset only those paths' real index entries; `discard` takes the same `paths` + `hunks` selection (the Changes panel's **Discard (n)**, #87 — never every file) and undoes only the ticked blocks off the working tree; `ignore` appends an anchored `/path` line to the lane's OWN `.gitignore` for an untracked path only (`ETRACKED`), never through a symlink. **`publish` is the ONE write that takes a destination from the request** (`{url}`), so it can only give a remote to a repo that has NONE: it refuses before the first commit (`ENOCOMMITS`) and whenever `git remote` prints anything (`EREMOTEEXISTS` — it never re-points a remote, so it cannot redirect a repo that already publishes somewhere), the name is always `origin`, the URL passes `validateRemoteUrl`'s allowlist (`https://` with NO userinfo, `ssh://`, scp-like `user@host:path` — never a local path, `file://`, `ext::`, `fd::`, `git://`, `http://` or anything starting with `-`), it runs `remote add origin -- ` then the same `push -u origin HEAD` as `push`, and a failed push removes `origin` again. That is still weaker than the AI-proxy "a key goes only to its stored endpoint" rule — a browser on the dashboard's origin can publish a remote-less repo to any allowlisted host — so do not widen the allowlist or let it touch a repo that already has a remote. **A path in the body is never a path**: `resolvePaths` accepts only what `git status` is reporting as changed right now, which is the whole authorization story for the per-file actions (no `..`, no absolute path, no untouched file). `discard`, `discard-hunk` and `merge-abort` are the irreversible actions and all refuse to run without `confirm: true`; do not add another destructive action without the same gate. `branch-create` never switches (the reason to create one from a lane is a sibling worktree, not moving a working agent's HEAD) and `branch-delete` is `-d`, never `-D`, refusing `main`/`master`/`development`/`develop`, the current branch and one another worktree holds. `pull` is ALWAYS `--ff-only` and `switch` refuses a dirty tree, so neither can invent a merge commit or a conflict underneath a running agent (a `switch` to a branch only a remote has creates its tracking branch from a `refs/remotes` ref it LOOKED UP — `listRemoteBranches` — never one from the request); `push` pushes `HEAD`, never a branch name from the request; `commit` passes its message as one argv element. **The lane is the boundary, not the repo**: a subdir lane's reads and writes stay under its own cwd, and an extra folder (`?source=`, resolved by `sourceFolder` from a `folder` row of THIS lane's `lane_sources`, never a path — `404 ENOSOURCE` otherwise) and a repo inside the lane's folder (`?repo=`, accepted only when `server/lib/nested-repos.js`'s own disk walk found it — `404 ENOREPO` otherwise) are each their own boundary under every guard here; no read may return a `settings.local.json` (`ESECRET`) — do not add a git read that skips `lanePath`/`isSecret`. **A merge is a history-joining action allowed only because its recovery is a button too** (`merge-abort`) — and `cherry-pick`/`revert` ride the same rule (a non-merge commit by sha, `EMERGECOMMIT`/`EBADSHA`; same clean-tree, repo-root and conflict flow; `merge-commit`/`merge-abort` finish or abort whichever of the three `operationInProgress` finds open; a no-op is aborted again as `EEMPTY`, never left open): it starts only by name, on a clean tree, from a lane at the repo ROOT (`ESUBDIR` — a merge is repo-wide and a subdir lane could not resolve a conflict outside itself), one at a time (`pull`/`switch`/`merge`/hunk actions answer `EMERGING` meanwhile); the ref is looked up in `refs/heads`+`refs/remotes`, never passed through; a conflicted file is resolved only through `resolve` (ours/theirs, or `done` once no conflict marker is left — `stage` refuses a conflicted file that still has markers, `EMARKERS`), and `merge-commit` refuses while anything is unmerged (`ECONFLICTS`). Do not add rebase, a hard/mixed reset or stash here: a dashboard click is the wrong place to start an operation whose recovery needs a terminal, and a hard `reset` already lives behind `worktree.js`'s three-check destroy guard. The ONE reset is `undo-commit` = `reset --soft HEAD~1` (`confirm: true`, repo root, never the root or a merge commit), and it and `amend` act only on the HEAD the request names (`sha`, `ESTALE` once an agent committed past it) and refuse any HEAD a remote-tracking branch already holds (`EPUSHED`); a terminal-started rebase blocks every history action (`EMERGING`) — rewriting a pushed commit is a terminal decision; do not relax that check. A button that looks disabled in `LaneGitBar` is a courtesy; the refusal that matters is the server's. Reads (`/git`, `/git/changes`, `/git/log`, `/git/file`, `/git/raw` — an image's bytes for the viewer, extension-allowlisted and served under a `sandbox` CSP + `nosniff` so an SVG runs no script; keep both headers —, `/git/ls`, `/git/tree`, `/git/branches`) stay unguarded like `/git` always was, and resolve their repo from the LANE ROW (or its session / `lane_sources` row) — `GET /api/lanes/branches` is the only one that takes a path from the request, because its lane does not exist yet. - **A clone runs as its folder's account with THAT account's own identity, and only the public key ever leaves the server.** `POST /api/lanes/clone` takes a URL only through `validateRemoteUrl` (the `publish` allowlist) and a parent held to the user's own folder like New folder; `server/lib/git-identity.js` runs `git clone` as the folder's account (S3: the member) and writes the identity into the CLONE's config (`core.sshCommand` / `credential.helper=store --file=`), never the user's global config. A member's SSH key and HTTPS store live in their home under `~/.config/agenthub/`, written only through `write-as` (secrets on stdin, never sudo argv/env), the host's under `/agenthub-ssh/` + the shared store below; `/api/me/git` returns only `publicKey`/`hasToken`. The git bar's fetch/pull/push in a member's folder uses that same store (`memberHttpsConfig`; a typed login goes in through `write-as` stdin for the one call), and a store that cannot be read is never rewritten (`ESTOREUNREADABLE`); the private key is only `lstat`ed, never read. The commit author is the ACCOUNT's `git config --global` (`PUT /api/me/git/author`). Do not add a route that returns a private key or a token, or one that writes a git identity outside the clone's own config or the account's own `~/.gitconfig`. - **AgentHub's git credentials are its OWN store, shared by every user of the install, and git is given ONLY that store when it holds a login for the host.** `server/lib/git-credentials.js` keeps one record per `protocol + host` under `/agenthub-git-credentials/` (0700/0600) and no route ever returns a password (`hasToken` only). `gitConfigFor` runs a network action with `-c credential.helper= -c credential.helper=` whenever AgentHub holds a login for a repo remote's host or the request carries one — the machine's helpers are left out on purpose: a stale token there would win over the typed one, and git's post-success `store` would copy a use-once token into the OS user's helper. The secret travels in the helper's env or the store file, never argv. A typed credential is accepted only for one of the repo's OWN remote hosts (`EBADHOST`), so a login is never sent to a server it was not typed for, and it is saved only after git SUCCEEDED and `remember` is set. Do not add a route that returns a stored password or a path that forwards one to a host other than its own. - **A lane's `tool` is its DEFAULT, immutable; a session's tool is the session's choice — and only an AI Tools catalog id ever comes from a request.** `lanes.tool` (default `claude`) is any id `server/lib/ai-tools.js` knows; `createLane` refuses an unknown one (`EBADTOOL`) and one the user disabled (`EDISABLED` — only CREATION asks, an existing lane on a tool disabled since keeps working), and it is deliberately absent from `PATCHABLE`. A NEW session names its own with `tool` on `POST /api/lanes/:id/start` (the home's **AI** select, sent only when it differs from the lane's): `validateTool` again (`EBADTOOL`/`EDISABLED`, 400), and the id only picks a catalog file — the command, args and env still come from that file. A resume keeps its session's own tool (`sessions.tool`), never the request's. `POST /api/run` still takes the lane's tool. A lane whose tool left the catalog answers `ENOTOOL` at start. The sidebar's tool icon is per SESSION (`WorkspaceSessionList`), not per workspace row. A pane has no run form: `RunSetup` is gone from `LaneConsolePane`, and the workspace home's prompt box (`WorkspaceHome.tsx`: AI and ONE Model chip — `ModelMenu` in `RunSetup.tsx`, the model list plus Effort, a proxy's own models on a proxy — inside the box; AI Proxy, Permission and Local / New worktree in a row under it, like Claude Desktop) is the one place a session starts with a prompt (the session view's tab bar `+`, `ConsoleTabs.tsx`, starts a bare CLI or a plain shell — `shell: true`, nothing from the request but the slot; the home's AI menu has the same **Open terminal**). `claude`/`codex` keep their hand-written argv builders (`launcher` in their catalog file — a `DASHBOARD_AI_TOOLS_DIR` file may NOT claim one); every other tool is started by its detected absolute path as `[path, ...args, ...resume?, ...bypass?, ...prompt]` (`...resumeById, ` in place of `resume` for a named session, only when its catalog file declares `resumeById`), every piece but the prompt from its catalog file, the prompt ONE argv element or, for an `after-start` tool, a tmux paste buffer (`tmux.pasteText`) after `delayMs`. Codex-only fields (`sandbox`, `approval`) are validated against the fixed allowlists in `server/lib/pty-run.js`. **Bypass args are emitted ONLY when the run request carries `bypass: true`** — the home's red per-run switch, which starts from the account's own default (Settings → AI tools → **Session defaults**, `client/src/lib/sessionDefaults.ts`: Claude's default mode may be `bypassPermissions`, Codex's bypass may default on — set only behind a confirm, never by the first-run wizard, never for the pet chat; the server never remembers it) — and they come from the tool's catalog file (Codex: `--dangerously-bypass-approvals-and-sandbox`, which then replaces `-s`/`-a`); Claude has none (it has the `bypassPermissions` mode), and `--dangerously-bypass-hook-trust` is never emitted. Every pane gets the dashboard's hydrated `PATH` through tmux `-e`. Only `claude`/`codex` are `proxyCapable`; `resolveProxy` accepts only `native` for any other tool, else `EUNSUPPORTED`. `GET /api/run/binary?tool=` resolves through the catalog — the query string never names a command. Do not add a route that takes a command, args or env for a tool from a request. diff --git a/client/src/components/lanes/LaneChangesPanel.tsx b/client/src/components/lanes/LaneChangesPanel.tsx index ff07aa7e..c7923a9f 100644 --- a/client/src/components/lanes/LaneChangesPanel.tsx +++ b/client/src/components/lanes/LaneChangesPanel.tsx @@ -1470,25 +1470,40 @@ export function LaneChangesPanel({ } }; - // Discard all: every non-conflicted file in one `discard`, asked once in a - // dialog that names how many untracked files it will delete. - const discardAll = async () => { - const rows = files.filter((f) => !f.conflicted); - if (!rows.length) return; + // Discard selected: exactly what Commit would take — the ticked files, and + // only the ticked blocks of a partly ticked one (#87: it used to discard + // every file, ticked or not) — in one `discard`, asked once in a dialog + // that names how many untracked files it will delete. + const discardTicked = async () => { + if (!tickedPaths.length) return; const ok = await confirmDialog({ - title: t("changes.discardAllTitle"), - message: t("changes.discardAllConfirm", { - count: rows.length, - untracked: rows.filter((f) => f.untracked).length, + title: t("changes.discardTickedTitle"), + message: t("changes.discardTickedConfirm", { + count: tickedPaths.length, + untracked: files.filter((f) => f.untracked && tickedPaths.includes(f.path)).length, }), - confirmLabel: t("changes.discardAll"), + confirmLabel: t("changes.discardTicked"), }); - if (ok) - await act( - "discard", - rows.map((f) => f.path), - true - ); + if (!ok) return; + const lane = laneId; + setBusy(true); + setError(null); + try { + const { paths, hunks } = await selection(); + await api.lanes.gitAction(target, "discard", { + paths, + ...(hunks ? { hunks, context: CONTEXT_STEPS[0] } : {}), + confirm: true, + }); + if (laneNow.current !== lane) return; + setConfirmDiscard(null); + refreshGit(); + await load(); + } catch (err) { + if (laneNow.current === lane) setError(err instanceof Error ? err.message : String(err)); + } finally { + if (laneNow.current === lane) setBusy(false); + } }; // resolve / merge-commit / merge-abort: same shape as `act`, other body. @@ -2067,8 +2082,7 @@ export function LaneChangesPanel({ // An HTML page's relative CSS/scripts/images, read like a download — only // from the working tree: a commit's page would get today's files. const htmlFetch = useMemo( - () => - range === "commit" ? null : fetchVia((p) => api.lanes.gitDownloadUrl(target, p)), + () => (range === "commit" ? null : fetchVia((p) => api.lanes.gitDownloadUrl(target, p))), // eslint-disable-next-line react-hooks/exhaustive-deps [range, JSON.stringify(target)] ); @@ -2863,13 +2877,13 @@ export function LaneChangesPanel({ diff --git a/client/src/components/lanes/__tests__/LaneChangesPanel.test.tsx b/client/src/components/lanes/__tests__/LaneChangesPanel.test.tsx index 3e7727b0..2423098a 100644 --- a/client/src/components/lanes/__tests__/LaneChangesPanel.test.tsx +++ b/client/src/components/lanes/__tests__/LaneChangesPanel.test.tsx @@ -837,10 +837,12 @@ describe("LaneChangesPanel", () => { }); }); - it("discards every file at once only after the dialog is confirmed", async () => { + it("discards only the ticked files, and only after the dialog is confirmed (#87)", async () => { const user = userEvent.setup(); render(); await screen.findByTestId("lane-changes-overview"); + // notes.txt (unversioned) starts unticked; untick docs/API.md too. + await user.click(tickOf("docs/API.md")); vi.mocked(confirmDialog).mockResolvedValueOnce(false); await user.click(screen.getByTestId("lane-changes-discard-all")); @@ -849,10 +851,14 @@ describe("LaneChangesPanel", () => { await user.click(screen.getByTestId("lane-changes-discard-all")); await waitFor(() => expect(api.lanes.gitAction).toHaveBeenCalledWith(7, "discard", { - paths: ["server/lib/git-ops.js", "notes.txt", "docs/API.md"], + paths: ["server/lib/git-ops.js"], confirm: true, }) ); + + // Nothing ticked: nothing to discard. + await user.click(tickOf("server/lib/git-ops.js")); + expect(screen.getByTestId("lane-changes-discard-all")).toBeDisabled(); }); it("right-click on a changed file offers Commit File, a two-click discard, copy path and patch", async () => { @@ -1673,6 +1679,25 @@ describe("LaneChangesPanel", () => { expect(api.lanes.gitAction).not.toHaveBeenCalled(); }); + it("discards only the ticked change blocks of a partly ticked file (#87)", async () => { + const user = userEvent.setup(); + await openReview(user); + await user.click( + screen.getAllByRole("checkbox", { name: "Include this change in the commit" })[1]! + ); + await user.click(screen.getByRole("button", { name: "Changes" })); + await screen.findByTestId("lane-changes-commit"); + await user.click(screen.getByTestId("lane-changes-discard-all")); + await waitFor(() => + expect(api.lanes.gitAction).toHaveBeenCalledWith(7, "discard", { + paths: [], + hunks: { "server/lib/git-ops.js": [{ hunk: "@@ -1,2 +1,3 @@", blocks: [0] }] }, + context: 3, + confirm: true, + }) + ); + }); + it("commits only the ticked change blocks of a partly ticked file (hunks)", async () => { const user = userEvent.setup(); await openReview(user); diff --git a/client/src/i18n/locales/en/lanes.json b/client/src/i18n/locales/en/lanes.json index 86709b9d..2a4ec824 100644 --- a/client/src/i18n/locales/en/lanes.json +++ b/client/src/i18n/locales/en/lanes.json @@ -532,9 +532,9 @@ "changes.unversionedGroup": "Unversioned Files", "changes.commitTicked": "Commit ({{count}})", "changes.commitAndPush": "Commit and Push", - "changes.discardAll": "Discard all", - "changes.discardAllTitle": "Discard all changes?", - "changes.discardAllConfirm": "Every change in {{count}} file(s) goes back to the last commit, and {{untracked}} new file(s) git does not track yet are deleted. This cannot be undone.", + "changes.discardTicked": "Discard ({{count}})", + "changes.discardTickedTitle": "Discard the ticked changes?", + "changes.discardTickedConfirm": "The ticked changes in {{count}} file(s) go back to the last commit, and {{untracked}} ticked new file(s) git does not track yet are deleted. Unticked files and blocks are kept. This cannot be undone.", "changes.amendAndPush": "Amend and Push", "changes.pushFailed": "Committed, but the push failed: {{error}}", "changes.tickFile": "Include {{path}} in the commit", diff --git a/client/src/i18n/locales/vi/lanes.json b/client/src/i18n/locales/vi/lanes.json index 8ce0cc57..778cb74f 100644 --- a/client/src/i18n/locales/vi/lanes.json +++ b/client/src/i18n/locales/vi/lanes.json @@ -532,9 +532,9 @@ "changes.unversionedGroup": "File chưa theo dõi", "changes.commitTicked": "Commit ({{count}})", "changes.commitAndPush": "Commit và Push", - "changes.discardAll": "Bỏ tất cả", - "changes.discardAllTitle": "Bỏ tất cả thay đổi?", - "changes.discardAllConfirm": "Mọi thay đổi trong {{count}} file sẽ trở về commit cuối, và {{untracked}} file mới chưa được git theo dõi sẽ bị xoá. Không thể hoàn tác.", + "changes.discardTicked": "Bỏ ({{count}})", + "changes.discardTickedTitle": "Bỏ các thay đổi đã chọn?", + "changes.discardTickedConfirm": "Các thay đổi đã chọn trong {{count}} file sẽ trở về commit cuối, và {{untracked}} file mới đã chọn chưa được git theo dõi sẽ bị xoá. File và đoạn không chọn được giữ nguyên. Không thể hoàn tác.", "changes.amendAndPush": "Amend và Push", "changes.pushFailed": "Đã commit, nhưng push thất bại: {{error}}", "changes.tickFile": "Đưa {{path}} vào commit", diff --git a/docs/API.md b/docs/API.md index 88433096..ac97dd0c 100644 --- a/docs/API.md +++ b/docs/API.md @@ -1063,7 +1063,7 @@ cannot collide on `index.lock`. | `push` | `git push -u HEAD` — the branch's upstream remote, else the only remote, else `origin`, else `400 ENOREMOTE` | — | | `stage` | `git add -- ` | `{"paths": ["..."]}` | | `unstage` | `git restore --staged -- ` (a staged rename also passes its original path, so both halves unstage) | `{"paths": ["..."]}` | -| `discard` | `git restore --source=HEAD --staged --worktree` (tracked) / `git clean -fd` (untracked) | `{"paths": ["..."], "confirm": true}` | +| `discard` | `git restore --source=HEAD --staged --worktree` (tracked) / `git clean -fd` (untracked); with `hunks` (a partly ticked file's ticked change blocks, named as in `commit`), `git apply -R` of only those blocks off the working tree first — every patch built before any is applied, so a stale one is `409 EHUNKGONE` with nothing undone | `{"paths": ["..."], "confirm": true}` / `{"paths": [], "hunks": {"": [{"hunk": "@@ … @@", "blocks": [0]}]}, "context": 3, "confirm": true}` | | `stage-hunk` | `git apply --cached` of ONE hunk of `git diff -U HEAD -- ` — or, with `block`, of only that hunk's `block`-th run of +/- lines (the other runs folded into context, as an edited `git add -p` hunk) | `{"path": "...", "hunk": "@@ -1,2 +1,3 @@", "block": 0, "context": 3}` | | `discard-hunk` | `git apply -R` of that one hunk (or that one `block` of it), off the working tree | `{"path": "...", "hunk": "@@ ... @@", "block": 0, "context": 3, "confirm": true}` | | `commit` | `git add -A` then `git commit -m `, or the index as it stands with `only: "staged"`, or — with `paths` — `git commit --only -- `: exactly those files' working-tree content (an untracked one is added first, and taken back out if the commit fails), every other change left as it was | `{"message": "...", "only": "staged"}` / `{"message": "...", "paths": ["..."]}` | diff --git a/server/__tests__/git-diff-ops.test.js b/server/__tests__/git-diff-ops.test.js index 6fb033f9..0b28fab0 100644 --- a/server/__tests__/git-diff-ops.test.js +++ b/server/__tests__/git-diff-ops.test.js @@ -851,6 +851,51 @@ describe("runGitAction — one change block inside a hunk (issue #50)", () => { assert.equal(fs.readFileSync(path.join(LANE, "tail.txt"), "utf8"), "l1\nl2\nl3\nl4\nl5\nl6"); }); + it("discards only the ticked blocks and whole files, nothing unticked (issue #87)", async () => { + const edited = threeBlocks(); + write(LANE, "keep.txt", "untouched by discard\n"); + write(LANE, "drop.txt", "gone\n"); + const [hunk] = await hunkOf(); + await assert.rejects( + () => + runGitAction(LANE, "discard", { + paths: ["drop.txt"], + hunks: { "near.txt": [{ hunk: hunk.header, blocks: [0, 2] }] }, + }), + { code: "ECONFIRM" } + ); + await runGitAction(LANE, "discard", { + paths: ["drop.txt"], + hunks: { "near.txt": [{ hunk: hunk.header, blocks: [0, 2] }] }, + confirm: true, + }); + const want = [...edited]; + want.splice(11, 0, "line 11"); + want.splice(3, 2, "line 4", "line 5"); + assert.deepEqual(read(), [...want, ""]); + assert.equal(fs.existsSync(path.join(LANE, "drop.txt")), false); + assert.equal(fs.readFileSync(path.join(LANE, "keep.txt"), "utf8"), "untouched by discard\n"); + }); + + it("discards ticked blocks alone, and refuses a stale one with nothing undone", async () => { + const edited = threeBlocks(); + const [hunk] = await hunkOf(); + await assert.rejects( + () => + runGitAction(LANE, "discard", { + hunks: { "near.txt": [{ hunk: "@@ -1,2 +1,2 @@", blocks: [0] }] }, + confirm: true, + }), + { code: "EHUNKGONE" } + ); + assert.deepEqual(read(), [...edited, ""]); + await runGitAction(LANE, "discard", { + hunks: { "near.txt": [{ hunk: hunk.header, blocks: [1] }] }, + confirm: true, + }); + assert.deepEqual(read(), [...edited.filter((l) => l !== "INSERTED"), ""]); + }); + it("refuses a block the hunk does not have", async () => { threeBlocks(); const [hunk] = await hunkOf(); diff --git a/server/lib/git-diff.js b/server/lib/git-diff.js index c391d036..9224af08 100644 --- a/server/lib/git-diff.js +++ b/server/lib/git-diff.js @@ -1241,16 +1241,18 @@ async function diffHunks(dir, file, context) { * in file order, to be applied FORWARD onto an index holding HEAD. Every * header is matched against a fresh diff like `hunkPatch`'s; null when one is * gone or names a block the hunk lacks. Each hunk's new-side start is left as - * git wrote it — `git apply` locates a hunk by its old side. + * git wrote it — `git apply` locates a hunk by its old side. `reverse` cuts + * it for `apply -R` off the working tree instead (the partial discard, #87), + * where the NEW side is the one checked and every start stays git's own. */ -async function selectionPatch(dir, file, context, picks) { +async function selectionPatch(dir, file, context, picks, reverse = false) { const diff = await diffHunks(dir, file, context); if (!diff || !picks.length) return null; const at = (header) => diff.hunks.findIndex((h) => h.slice(0, h.indexOf("\n")) === header); if (picks.some((p) => at(p.hunk) < 0)) return null; const bodies = []; for (const p of [...picks].sort((x, y) => at(x.hunk) - at(y.hunk))) { - const body = onlyBlock(diff.hunks[at(p.hunk)], p.blocks, false); + const body = onlyBlock(diff.hunks[at(p.hunk)], p.blocks, reverse); if (!body) return null; bodies.push(body); } diff --git a/server/lib/git-ops.js b/server/lib/git-ops.js index 88148527..2ce6d5cd 100644 --- a/server/lib/git-ops.js +++ b/server/lib/git-ops.js @@ -424,6 +424,13 @@ async function runGitAction(dir, action, body = {}) { if (action === "discard") { if (body.confirm !== true) throw bad("ECONFIRM", "discard requires confirm: true"); + // Ticked change blocks of partly ticked files (#87) go first: each patch + // is built before any is applied, so a stale one refuses before the + // irreversible whole-file part below has run. + const partial = hasHunks(body) ? await discardSelection(dir, body) : null; + if (partial && !(Array.isArray(body.paths) && body.paths.length)) { + return done(action, partial); + } const paths = await resolvePaths(dir, body); const untracked = paths.filter((p) => p.code === "??").map((p) => p.path); // A rename's ORIGINAL path rides along: restoring only the new one would @@ -431,7 +438,7 @@ async function runGitAction(dir, action, body = {}) { const tracked = paths .filter((p) => p.code !== "??") .flatMap((p) => (p.from && p.code.includes("R") ? [p.path, p.from] : [p.path])); - const out = []; + const out = partial ? [partial] : []; // Untracked files are deleted (git has no copy to restore); tracked ones // go back to HEAD in both gutters, so a staged-then-edited file lands in // one state rather than half-reverted. @@ -825,18 +832,11 @@ function hasHunks(body) { const MAX_PARTIAL_FILES = 300; /** - * JetBrains' partial commit: whole files (`paths`) plus, per file in `hunks`, - * only the ticked change blocks — `{"": [{hunk: "@@ … @@", blocks: [0]}]}`, - * the hunks named as `stage-hunk` names them, at `context`. Built in a - * TEMPORARY index (HEAD + the whole files + one patch per partial file from - * `selectionPatch`, git's own fresh text, never the request's) and committed - * from it, so the real index and the working tree are never in the way; after - * the commit the real index entries of exactly those paths are reset to the - * new HEAD, the rest of each partial file left as an unstaged change. Same - * path rules as `commitPaths`; a partial file must be tracked (EUNTRACKED) - * and not a rename, and a hunk the tree no longer has is EHUNKGONE. + * `body.hunks` checked for shape, and its files resolved as changed paths + * that may be taken in part: tracked (EUNTRACKED), not a rename, not in + * conflict, and not also named whole in `body.paths`. */ -async function commitSelection(dir, args, body) { +async function partialPicks(dir, body, verb) { const picks = Object.entries(body.hunks); if (picks.length > MAX_PARTIAL_FILES) throw bad("EBADPATH", "too many files"); for (const [, list] of picks) { @@ -858,12 +858,54 @@ async function commitSelection(dir, args, body) { const conflicted = [...whole, ...part].find((p) => CONFLICT_CODES.has(p.code)); if (conflicted) throw bad("ECONFLICTS", `${conflicted.path} is in conflict: resolve it first`); for (const p of part) { - if (p.code === "??") throw bad("EUNTRACKED", `${p.path} is untracked: commit it whole`); - if (p.code.includes("R")) throw bad("EBADPATH", `${p.path} is a rename: commit it whole`); + if (p.code === "??") throw bad("EUNTRACKED", `${p.path} is untracked: ${verb} it whole`); + if (p.code.includes("R")) throw bad("EBADPATH", `${p.path} is a rename: ${verb} it whole`); if (whole.some((w) => w.path === p.path)) { throw bad("EBADPATH", `${p.path} is named both whole and in part`); } } + return { picks, whole, part }; +} + +/** + * The partial discard (#87): per file in `hunks`, only the ticked change + * blocks are undone off the working tree — `selectionPatch` in reverse, git's + * own fresh text, never the request's — exactly what one `discard-hunk` per + * block would do, in one step. Every patch is built before the first is + * applied, so a stale selection refuses (EHUNKGONE) with nothing undone. + */ +async function discardSelection(dir, body) { + const { picks } = await partialPicks(dir, body, "discard"); + const patches = []; + for (const [p, list] of picks) { + const patch = await selectionPatch(dir, p, body.context, list, true); + if (!patch) + throw bad("EHUNKGONE", `${p}: that change is no longer in the diff; refresh and retry`); + patches.push(patch); + } + // The patch names paths from the repo root, so it is applied there. + const top = (await git(dir, ["rev-parse", "--show-toplevel"])).stdout.trim(); + const zero = Number(body.context) === 0 ? ["--unidiff-zero"] : []; + const out = []; + for (const patch of patches) + out.push(await git(top, ["apply", "-R", ...zero, "-"], { input: patch })); + return mergeResults(out); +} + +/** + * JetBrains' partial commit: whole files (`paths`) plus, per file in `hunks`, + * only the ticked change blocks — `{"": [{hunk: "@@ … @@", blocks: [0]}]}`, + * the hunks named as `stage-hunk` names them, at `context`. Built in a + * TEMPORARY index (HEAD + the whole files + one patch per partial file from + * `selectionPatch`, git's own fresh text, never the request's) and committed + * from it, so the real index and the working tree are never in the way; after + * the commit the real index entries of exactly those paths are reset to the + * new HEAD, the rest of each partial file left as an unstaged change. Same + * path rules as `commitPaths`; a partial file must be tracked (EUNTRACKED) + * and not a rename, and a hunk the tree no longer has is EHUNKGONE. + */ +async function commitSelection(dir, args, body) { + const { picks, whole, part } = await partialPicks(dir, body, "commit"); const wholePaths = whole.flatMap((p) => p.from && p.code.includes("R") ? [p.path, p.from] : [p.path] );