(changes): edit a changed file in place, with the list beside it and the diff live - #302
Conversation
The panel holds a repo-relative path and nothing else; editing a changed file needs the original/working-tree pair behind it and a way to write it back, without ever handing an absolute path to the renderer. git-changes-file reads the side git diff itself compares against — the index blob for the unstaged view, HEAD for the staged one — so the panel and the editor cannot disagree about what changed. The blob is read with `git cat-file blob`, not `git show`: `git show` resolves `:/<text>` to a commit and a directory path to a tree listing, both with exit 0, and would hand either to the editor as file content; `cat-file blob` refuses anything that is not a blob. `<rev>:<path>` is a revision operand, not a pathspec — `--literal-pathspecs` does not reach it and `--` cannot separate it — so it gets its own guard instead of the pathspec one, including the `:<n>:<path>` conflict-stage syntax the pathspec guard has no reason to know about. git-changes-save re-resolves the session's cwd on every call, resolves the repository root and the target on disk, requires the target to stay inside the root, applies the sensitive-path denylist, and writes the path the guard returned. Both handlers refuse a remote session: there is no file-write path to a remote host in this app.
… as you type A changed file of a local session opens in a CodeMirror merge view over the content pair instead of an inert block of diff text: the original on the left, the working tree on the right, editable, with the diff recomputed on every keystroke by construction. One button cycles side-by-side, inline and plain; the choice persists under its own localStorage key. Ctrl/Cmd+S and a Save button write through git-changes-save and refresh the status so the row's counts follow. The render path stops being a teardown. The diff view's chrome is built once and the editor instance lives on the tab, keyed by path, staged flag and mode. The tab re-renders on every busy→idle edge, so rebuilding it would have destroyed the editor under the user's cursor once per turn the session finishes. An idle refresh arriving on a dirty buffer refreshes the file list only: the buffer is left alone and the panel says the view may be out of date. A remote session, a binary file and a file over the panel's size cap keep the read-only unified diff, with a line naming which of those it is. The merge view CSS now names both hosts in one rule list rather than being duplicated for the new one.
The panel sits beside a session writing the same files, so a save was an unconditional overwrite of whatever had appeared on disk since the file was opened, and the session's uncommitted work was unrecoverable. git-changes-file now issues a version token — the hash of the bytes it read — and git-changes-save requires it back, re-reads the file and refuses when it no longer matches. A save with no token is refused too, so a caller that forgets it cannot clobber anything, and every successful write returns the token the next one must carry. git-changes-watch adds the second layer: fs.watch on the same guarded path, reported to the renderer as (sessionId, repo-relative path) so no absolute path crosses the boundary. Four further ways the round trip lost or corrupted content: - CodeMirror normalises CRLF to LF, so a CRLF file came back LF and rewrote every line. The read hands out LF-only text and the write re-applies the file's own dominant ending, measured from the bytes the token was taken from. - Latin-1 and friends carry no NUL, so they passed the binary gate, decoded to U+FFFD and were written back as replacement bytes. Both sides now decode strictly and a file that is not UTF-8 is refused, saying so. - .git is inside the repository root, so .git/config — core.pager, [alias] — and the hooks were writable. No path with a .git segment is accepted. - A symlink was followed: the pair was the link text against the target's content, and the save landed on a file the row did not name. A symlink at the target is refused before anything follows it; an escape through a symlinked directory is still the containment check's job. Also: only exit 128 means "absent from this tree" — a timeout or an unreadable object is an error, not an empty original side; `..` and `.git` are matched as path segments, so a file legitimately named `a..b` is editable; and the work-files signature invalidation splits on both separators.
…fers The editor watches its file and carries the version token, so a session writing it reloads a clean buffer as it happens and a dirty one is reported instead of silently clobbered on the next save. A refused stale save keeps the buffer and offers Reload, which asks before discarding unsaved edits. The staleness notice used to fire on any refresh that found a dirty buffer, whether or not anything had happened, and said the session had changed files either way. It now fires only when the version token says the file moved, and a file that stops being readable or a status refresh that fails are visible while a file is open instead of leaving the panel looking healthy. Three more ways work went missing: - Back, closing the tab and closing the panel destroyed the buffer with no confirmation, which docs/changes-view.md promised they would not. - Two saves in flight at once left the write order to the filesystem. - Two sessions' editors stacked in the one shared host, so after switching away and back the panel showed another session's file above the current one, and typing into the visible-but-wrong editor discarded the keystrokes. A clean selection is also re-pointed at its own refreshed row, so a file the session stages mid-turn is compared against HEAD from then on; inline mode asks for a merge view with no accept/reject chunk buttons, which would revert a working-tree change this panel says it does not touch; and the editor key is built with JSON.stringify rather than an in-band separator, which had put three raw NUL bytes in the source.
… you can type in The side-by-side merge view is the Changes panel's default writing surface, and its editable side carried none of the extensions writing needs: Ctrl/Cmd+S raised nothing at all, and there was no undo, no default keymap and no indent-on-input. It now has history, the default keymap, indentWithTab, drawSelection and the save keymap — the DOM-level save handler stays with the read-only viewers, since an editable view with both raises two saves per keystroke. The inline view gains the same editing extensions and a mergeControls option: per-chunk Accept/Reject restores the original side into the document, which is a working-tree revert. The MCP diff tab keeps the buttons; the Changes panel turns them off. The tests drive the real thing — codemirror-setup.js imported as the ES module it is, under jsdom, with a real keydown — because a stub that dispatches the save event itself proves only that the stub works.
…e panel now holds to The context docs and the user-facing page describe the version token and the watcher, the line-ending and encoding round trip, the .git and symlink refusals, what the notice line can and cannot claim, and the confirmations before unsaved edits are discarded. docs/changes-view.md keeps its "not a git client" line and now names the inline view's missing revert buttons under it.
…t cover Each of these fixed the case that was reported and missed the adjacent one. The git directory was reachable through a symlinked directory component: `gitlink/config`, where `gitlink` links to `.git`, carries no `.git` segment in the string the renderer sends, and the symlink check only ever looked at the final component. The literal check stays as a cheap pre-filter, and the guarantee moves to where it belongs: every check now runs on the disk-resolved path — the `.git` segment rule, containment in the repository root, and containment in the directories `git rev-parse --absolute-git-dir --git-common-dir` reports, which also covers a git directory that is not called `.git` at all. That is the third time a guard reading the renderer's literal string has been walked past by a symlinked directory; resolve first, check the resolved path, use the resolved value. A byte-order mark was being deleted on save. `TextDecoder` consumes a leading U+FEFF unless told not to, so a Windows-authored file lost three bytes to a round trip that reported success. The decoder now keeps it, the editor never sees it, and the write restores it. Line endings were only preserved for uniform CRLF. CodeMirror folds a lone CR as well, so a CR-only file was rewritten to LF and — because the comparison side folded only CRLF — was also treated as dirty forever, never re-reading from disk. Both forms fold now. A file that genuinely mixes endings is refused like a binary one rather than normalised to the majority: no editor whose document carries a single separator can preserve them line by line, and rewriting the minority is the manufactured diff this rule exists to prevent. The watcher moves into its own module so the half that detects the change is testable at all — main.js cannot be required from a test, and stubbing `fs.watch` in it left the whole suite green. It also re-arms on a rename: `fs.watch` follows the inode, so an atomic replacement delivered one event and then silence.
An MCP-driven open replaces whatever the panel is showing, and it fires when the session acts — which is routine precisely while someone is editing. It destroyed unsaved edits with no prompt, and a prompt is not the answer either: the session is waiting on the diff it just opened. The buffer is stashed instead, with the pair it was based on and its version token, and reopening Changes restores it and says so. A restored buffer that has gone stale meanwhile is still refused at save time, so the recovery cannot become the clobber the token exists to prevent. Reload re-arms the watch as well as re-reading: a file replaced on disk is both the usual reason to press it and the way a watch goes deaf. A save whose IPC rejects outright — the channel is gone, or the handler threw outside its own try/catch — is now reported in the notice line like any other failure, instead of escaping as an unhandled rejection and leaving the Save button disabled until something else re-rendered the panel. An untracked file's count derived from the content pair goes through the same status-identity check as the one derived from a diff, so a count computed against one status result is never written onto a later one.
The inline and plain modes were asserted with "at least one" in exactly the two places a double-fire could reappear — a keymap and a DOM handler on the same editable view raise two saves per keystroke, which is why the merge pane carries only the keymap. The measured count is one everywhere, including the MCP diff tab's default, so the tests say one. The harness also stopped swallowing every jsdom error: anything that is not a missing-layout measurement now fails the test, so a view that failed to construct cannot pass as noise.
…d the stash The context docs carry the containment rule as it now stands — resolve first, check the resolved path, use the resolved value — the git-directory refusals, what a byte-order mark and each kind of line ending do on a round trip, the watcher's re-arm, and the buffer stash behind an MCP-driven open. docs/changes-view.md no longer promises that line endings are preserved without saying which files it will not open.
… out of three
Closing the Changes tab asked whether to discard unsaved edits, and then
stashed them anyway: the toggle path tears down through destroyCurrentTab,
which stashes unconditionally. Reopening the panel restored the buffer the
user had just chosen to throw away, under a notice blaming a takeover that
never happened, and the next save would have written it.
The answer to that question is now authoritative for the session rather than
for the editor in front of it. confirmDiscardChangesEdits takes the panel
state and clears the stash whenever it returns true — which also drops a
buffer stashed by an earlier takeover and since restored — and the toggle
passes {stash: false} so nothing re-stashes behind the answer. Back and the
panel close button already tore down inline; all three now leave the same
state.
The restore notice names the only cause that can still produce it: the
session opening something else in this panel.
…down resolveRepoRoot had no callers left once resolveRepoDirs took over both the read and the write path, and an exported helper nothing calls reads as an unfinished intention. closeAll was the opposite case: written and tested, never connected, while the registry's watches outlived the window that asked for them. It now runs from the same mainWindow 'closed' handler that releases the PTYs and the subagent watchers, and a source assertion pins the wiring the way the fs.watch arming is pinned. Two defensive paths that no test reached are now covered: the watch IPC returning the guard's own refusal rather than letting fs.watch fail on an undefined path, and a debounced notification whose entry has since been dropped or replaced — a timer that loses the race with clearTimeout must not report a file the panel no longer has open.
…t it
Closing the panel while the session's own file or diff tab was showing threw
away a buffer stashed from the Changes tab, without a prompt: the clear was
tied to confirmDiscardChangesEdits returning true, and that function returns
true whenever there is nothing to ask about. The panel promised those edits
would come back on the next open, and they did not.
confirmDiscardChangesEdits now asks and does nothing else. What happens to the
buffer is decided at the exit, which is where the user's action is known.
The clearing half of the earlier fix is gone rather than narrowed. Measured
across the suite, it never cleared a live stash: a stash exists only while a
non-Changes tab is showing, since it is created when a Changes tab is replaced
and consumed the moment one is opened, so an exit reached from the Changes tab
always sees a null stash. `{stash: false}` on the one exit that tears down
through destroyCurrentTab is the whole of what a confirmed discard needs, and
the tests still fail without it.
Each of the three exits that ask is now pinned separately, with a live stash
of its own, so none of them passes on the back of another having cleared it.
…tion The wiring between the watch registry and main.js is asserted against main.js source, because main.js cannot be required from a test. A call commented out in place still matched, so the assertion that it is made was satisfied by its own corpse. Whole-line comments are stripped before matching. Only whole-line comments: `/*` also occurs inside string literals in main.js, and a block-comment stripper eats them. The assertions catch deletion and commenting out, which are the regressions that happen; a call left in place but made unreachable still passes, and the test says so.
devsuitup
left a comment
devsuitup
left a comment
There was a problem hiding this comment.
Reviewed 75bb4c0 against main 65d8aca by exercising readChangesFile / writeChangesFile / resolveTargetInsideRepo with main.js's exact argument shape against real scratch repos — Windows junctions, a 2-hop chain, a git worktree add linked worktree, a hardlink — plus EOL/BOM/encoding round trips checked with Buffer.compare and git status, four of the 46 mutations replayed on a scratch copy, CI read on the head.
Holds, confirmed: junction out of the repo → outside; junction into .git and into .git/hooks (existing hook file too) → git-dir; 2-hop chain → outside; in a linked worktree, the gitlink wt/.git, ../repo/.git/config, .GIT/HEAD and .git/worktrees/wt/HEAD → invalid-path before any git or fs call (plain fs.realpathSync does resolve junctions here). <rev>:<path> operand guard blocks leading -/:, .. segments and the n:path stage syntax; argv pinned to cat-file blob (the git show mutation is red). LF / CRLF / CR / no trailing newline / BOM+LF / BOM+CRLF / empty round-trip byte-identical and git-clean; mixed → mixed-eol; latin-1, overlong UTF-8 and on-disk CESU-8 surrogates → encoding; maxBytes accepted, maxBytes+1 → too-large. Remote refused in all three handlers via requireLocalTarget; watcher validated main-side, closeAll on window close, renderer unwatches before re-arming. Mutations: containment check, .git segment rule, token requirement, cat-file→show — all four red. 15 commits, no trailers; CI green on 75bb4c0 including windows-2022.
Two things on the write path itself:
-
Hardlink (
git-changes-file.js:96-138). A file inside the repo hard-linked to a file outside passes containment (its realpath is itself) andwriteChangesFilesucceeds — the outside file changes with it. Every other cheap vector is closed and named in the "Containment" section; this one is neither.fs.statSync(real)is already there (line 130): refuse whenstat.nlink !== 1, or state the accepted gap in.ai/contexts/changes-view.md. The threat model here is a session with shell access on the same tree, so a planted link is not theoretical. -
Unpaired surrogate in
content(git-changes-file.js:265).Buffer.from(prefix + applyEol(stripBom(content), eol), 'utf8')takes the renderer string as-is:'line1\n\uD800line2\n'with a valid token returns{ ok: true }and writesef bf bd— the U+FFFD substitution the read side refuses by design. Whether the editor can produce such a string is unverified without Electron; the main-process function has no defence if it arrives (paste, IPC). Re-decode with a fatalTextDecoder(or a surrogate regex) and refuse withreason: 'encoding', symmetric with the read side.
Smaller:
test/git-changes-file-real-git.test.js:112-122pins/not in the index/; git 2.24 here saysNot a valid object name :new.txt, so the file fails locally (CI's git is fine). The exit code 128 is already asserted; drop the wording match or list it as a second environmental gap — the body only lists one.- Comment ceiling:
git-changes-file.js:117-119, 165-166, 180-181,git-changes-watch.js:11-15, 44-45,public/file-panel.js:401-402, 583-584, 1335-1336carry two/three-line rationale without a doc pointer.
Unverified here, as in the body: the three product questions (mixed-EOL refusal, notice visibility, scroll across refresh), real fs.watch rename ordering on Windows/macOS, and anything on screen.
75bb4c0 to
fea54fb
Compare
|
fea54fb is the rebase onto main after #300 plus a |
A terminal file link hands the renderer an absolute path, and the Changes panel speaks repo-relative pathspecs. git-changes-locate does the mapping main-side, against the repository root resolved from the session's own cwd — the same root the read and the write already use — so the renderer never learns where the repository is. It answers whether the file is one of this session's changed rows, and which side to open it against. An untracked file counts: it is a legitimate row and the editor handles an empty original. An unmodified file is not an error, just a file with no diff to show. A path outside the repository, and everything the row guard already refuses — the git directory, a sensitive file, a directory — come back refused rather than as a row a later read would reject. The answer costs one `git status` scoped to that one path. Measured on a 20 000-file repository: 14-17 ms against 117-126 ms for the unscoped status the panel runs when it opens, which is cheap enough per click that the renderer needs no cache and stays correct when no Changes tab is open.
Reviewing a set of changed files meant list, click, editor, Back, click, editor. The list now stays: the selected file opens below it, the current row is highlighted, and clicking another row swaps the file without leaving the list. The editor's first button closes the file and keeps the list, so it is Close rather than Back — there is no navigation step left to undo. The divider between the two is the splitter the panel's shell region already uses, with the height model that region settled on: the drag's own value is what is stored, and it is clamped only for display, so a short panel or an open shell cannot ratchet the list down. The list has a floor of its own (96px, about four rows) and the editor keeps 120px; below that the list scrolls rather than either one collapsing. Switching rows is an exit like the others, and asks the same question through the same function when the buffer is dirty; a refusal stays on the file it was already showing. Clicking the row that is already open is not a switch and asks nothing. A terminal file link pointing at one of the session's changed files now opens there too, on that row, instead of the plain viewer. Anything else — an unmodified file, a path outside the repository, a remote session, or a main process that cannot answer — keeps the plain viewer it has always had.
…k goes The user-facing page no longer describes a Back round trip that does not exist, and says what the divider does and which links open in the editor. The context doc carries the height model, the list and editor floors, the fact that switching rows is an exit like the others, and where the absolute-to- relative mapping lives and what it costs.
The commit that added the routing claims a link still opens the plain viewer when the main process cannot answer; the catch arm that does it had no test.
…en there is one Saving an untracked file made its counts vanish and the header total drop back, while git still reported the file as changed. The counts are derived from the content when the row is opened, and the status refresh a save triggers has none of its own — so the save now re-applies them from the bytes it just wrote. The original side is untouched by any of this: a save writes the working tree, not the index or HEAD, so the diff after a save is the same diff. The Save button was always active, including with nothing to save. It now follows the buffer rather than the last render: the three editor factories take an onChange and install a CodeMirror updateListener, so typing, pasting, undo and a programmatic edit all reach it — a DOM input listener would miss the last two. The keyboard path does not consult the disabled attribute, so the handler keeps the clean and in-flight guards itself. Inline becomes the default for this panel. At the panel's 450px default a side-by-side merge view gives each side about 225px and clips code mid-token; one column gets the full width. The MCP diff tab keeps side-by-side under its own key, since it is not confined to this panel. Refresh is the icon the search bar already uses, through the toolbar's existing icon-button class.
The two carried the same surfaces, hairlines, accents and control borders as separate literals — 188 occurrences of eleven values — so the panel's chrome could drift from the sidebar's a rule at a time. They are tokens on :root now, and every substitution holds the literal it replaced, so nothing moves except the one deliberate change: the file panel sits on the sidebar's surface rather than a darker one of its own. The busy-spinner tint assertions matched the accent by spelling; they accept either form now, which is the property they were written to protect — that those rules reuse the shared violet rather than picking a colour of their own.
…first The page described a save as a refresh that resets an untracked row's counts, which is what made the disappearing diff read as by design. It now names the exception and keeps the rule for the Refresh button and the idle refresh. It also records inline as the panel's default and why, and that Save is inactive until there is something to save.
…sked to git-changes-locate was the one Changes handler whose local-only guard had no source assertion; all four are asserted together now, so a fifth handler added without one is visible. Plain mode's change listener was wired but unpinned, where inline and side-by-side were — the point of the listener is that the three modes behave alike, so the third is worth the same test. And the rev-operand guard is now pinned on the one input only it can refuse: a file literally named `1:f.txt`, which containment has no objection to and which `:<path>` would read as git's conflict-stage syntax.
…the status measurement Opening the row `innerlink` is refused; a link to it opens the row of the file it points at. Both are deliberate and they are not the same operation, so the doc says which is which rather than leaving the next reader to file the difference as a bug. The scoped-status figure is a measurement on one repository shape, not a property of scoping: with untracked files present a scoped run can be slower than the unscoped one, worst case around 60 ms. The conclusion is unchanged.
jbr-sekoia
changed the title
…sent A hard link is the one escape resolving on disk cannot see: a second name for the same inode, whose real path is the in-repo name, so containment has nothing to object to while a write through it changes the file outside as well. The shared guard refuses any target whose link count is not one, on the read as well as the write, so the panel says so when the file is opened rather than when the save fails. Measured before choosing, because refusing a legitimate file would be its own defect: 0 of 38561 git-tracked files across 12 real repositories have a link count above one, and the hard links package managers create live in node_modules, which is ignored and therefore never a row. The rule is on the link rather than on where the other name is, so a link between two files inside the repository is refused too — "which of these two names did the user mean" has no answer this panel can defend. The write also took a JavaScript string on trust. An unpaired surrogate — reachable by paste or by any future caller of the IPC — would have been written as U+FFFD, the exact substitution the read side exists to prevent. It is refused now with the same reason, before any bytes are produced.
… dying Windows CI failed to remove the temp repo for the one case that trips the maxBuffer cap: EBUSY on rmdir. The cap SIGTERMs the overrunning git child, and execFile's callback runs before that child is reaped — measured, exitCode null and killed true at callback time. On Windows a live process holds a handle on its working directory, so removing the scratch repo races a process that is still exiting. Linux unlinks by name and never noticed. The product behaviour is right: an overrunning child must be killed, and production must not block waiting for it to die. So the cleanup tolerates the window instead, with the retries rmSync provides for exactly this. Both real-git suites get it — only one case trips the cap today, but the race belongs to the shape, not to that test.
… encoding check The Containment section named every cheap vector except this one. It now says what a hard link defeats, what refusing it costs (measured), and why the rule is on the link rather than on where its other name lives. The encoding paragraph gains the write side, and both user-facing lists gain the case.
The rename re-arm carried its reasoning in the source; it lives in the context doc, which is where the rule about atomic replacement already is.
|
Head is now 1. Hard link. Refused, and on the read as well as the write ( Refusing a legitimate file would be its own defect, so the cost was measured before choosing: 0 of 38 561 git-tracked regular files across 12 real repositories have a link count above one, and the hard links package managers create live in 2. Unpaired surrogate on save. Refused with 3. 4. Comment ceiling. All eight sites are one-line pointers to 5. Windows EBUSY. Diagnosed rather than retried ( The product behaviour is right — an overrunning child must be killed, and production must not block waiting for it to die — so the test cleanup tolerates the window, in both real-git suites rather than only the one that trips it today. Also since CI is green on |
devsuitup
left a comment
devsuitup
left a comment
There was a problem hiding this comment.
Re-reviewed afca6c0 (base now the #300 merge, 0018635; git diff origin/main HEAD is 19 files / +4940 −277, exactly the PR's numbers — the panel-shell files are rebase noise, not content). Both write-path items are closed and mutation-tested:
- Hardlink:
git-changes-file.js:137refusesnlink !== 1;fs.linkSyncon NTFS here reportsnlink === 2so the guard is live on Windows, not only in CI; read and write both refuse (reason:'hardlink'), including a link to a file inside the repo. Guard removed → the test is red. - Unpaired surrogate:
hasLoneSurrogateat 177-180, applied at 308; lone high, lone low and pair+lone all refused (reason:'encoding'); a real astral pair still round-trips byte-identical. Guard removed → red. /not in the index/dropped, exit 128 kept plusstdout === '', with the reason stated.- The six comment sites collapsed to pointers.
New layout (list kept mounted, editor below it behind a splitter): 500-row cap and note intact; a MutationObserver on the diff host records zero mutations across an idle rebuild and the editor survives; confirmDiscardChangesEdits gates all five exits (panel close, toggle, row switch, back, reload) and each has a test — the body's "the question lives in one place" undercounts, harmless. hidePanel() keeping the panel open for the shell runs after the discard gate in handleClose, so no bypass. sidebar-busy-agents-tint now accepts var(--accent) = #8088ff, same hue via the token refactor, no weakening. CSS source tests 11/11; 180/180 on the five suites; eslint 0 errors, the 16 warnings identical at 75bb4c0; CI green on afca6c0 including windows-2022.
Two small things, non-blocking: public/codemirror-setup.js:419-421 carries three lines of rationale above docChangeListener — same ceiling as last round, a pointer to .ai/contexts/viewer-panel.md and the prose there; and the combination dirty buffer × open shell × panel close is traced safe but has no test of its own (panel-terminal.test.js:646-664 covers the clean-buffer case).
Human at the keyboard: the master-detail stack (list, splitter, editor, optional shell) at the panel's minimum width and on a short screen; the three product questions from the body stand.
The reason an updateListener is used instead of a DOM input listener was spelled out in the source; it belongs in the viewer-panel context doc, beside the rest of the editable-viewer contract.
…ll open The close path asks before discarding unsaved edits, but only the clean-buffer case was covered, so removing that question from handleClose stayed green.
2 participants
Why
The Changes panel shows you what a session did to your working tree, and then stops. Every correction — a typo in a generated comment, a stray debug line, a line to delete before committing — meant leaving the panel, finding the file elsewhere, editing it there, coming back and refreshing. The panel is where you review a session's work; reviewing and fixing are the same activity.
What
A changed file is edited in place, with the diff recomputed as you type, and the file list stays on screen while you do it.
Click a row and the editor opens below the list; click another and it swaps. There is no navigation step to undo, so there is no Back — the editor has a Close. Three modes, persisted: inline (the default here), side-by-side and plain.
@codemirror/mergewas already bundled and already driving the MCP diff tab, so the editor and the live diff are assembly, not invention —mergeControls: falsekeeps the Accept/Reject chunk buttons out, because this panel is still not a git client.A file link in the terminal opens the diff rather than the plain viewer when the file is one of the session's changed files.
The work was everywhere CodeMirror does not reach: the filesystem, git, and the panel's own state machine.
Keeping the resolution boundary
The
'changes'tab was the only tab type that never carried an absolute path —git-changes-target.jsresolves the session's cwd inside the main process and neither IPC returned it. That boundary is kept rather than worked around.git-changes-filereturns the content pair. The original side is whatgit diffitself compares against —git cat-file blob :<path>unstaged,HEAD:<path>staged.cat-file blobovergit showbecausegit showis content-type-polymorphic: measured,git show ':/SECRETWORD'exits 0 printing a commit andgit show 'HEAD:'exits 0 printing a tree, whilecat-file blobrefuses all three. A guard bug then degrades to a refusal instead of putting a commit object in the editor as "the original".git-changes-savewrites the working-tree file.git-changes-watch/unwatchcarry no absolute path.git-changes-locateis the only one that takes an absolute path — a terminal link hands the renderer one — and it maps it to a repo-relative row main-side, against the root the session's own cwd resolves to. The renderer never sees that root.requireLocalTarget(resolveGitChangesTarget(...)), pinned by a test that loops over the four rather than asserting each once, so a fifth handler added without a guard fails by construction.<rev>:<path>is a revision, not a pathspec:--literal-pathspecsdoes not reach it and--cannot separate it, so it carries its own guard.Not overwriting what the session wrote
The panel sits next to a session actively writing those same files, so an unconditional
writeFileSyncis a data-loss device. Two layers:reason: 'stale'. A save with no token is refused, so it fails closed.rename, which is howgit checkout,sed -iand atomic saves replace a file —fs.watchotherwise goes deaf after the first one.An idle refresh never replaces an open editor's content, and the render path stops being a teardown:
renderChangesDiff()used toinnerHTML = ''on every call, which would have destroyed the editor under the cursor every time the session finished a turn.Five exits ask the same question and all five honour the answer: switching rows, the tab's own close button, the panel toggle, Back and Reload. Each carries the gate independently rather than funnelling through one place, and each has its own test — independence is what protects the buffer, since a single funnel is one refactor away from being bypassed. When the session takes the panel over (an MCP-driven open, which fires while you edit), the dirty buffer, its pair and its token are stashed and restored with a notice; the token travels, so a restore can never become a clobber.
Content fidelity
Reading bytes and writing a string back is where a file editor quietly corrupts things:
Containment
The guard resolves on disk first and checks the resolved path, because a literal renderer-supplied string is defeated by a symlinked directory component. Root and git directories come from one
git rev-parse --show-toplevel --absolute-git-dir --git-common-dir; the target must stay inside the root;isSensitivePathapplies on top; the write runs on the path the guard returned.Nothing under
.git/—.git/configis acore.pager/alias command-execution primitive. Holds againstgitlink/config, a 2-hop symlink chain, a link into.git/hooks, and the git directories of separate-git-dir, linked-worktree and submodule layouts, without over-blocking.gitignore,.github/workflows/ci.yml,a.git/x.txtordotgit.md...and.gitare matched as path segments, soschema..v2.sqlis editable.A row for an in-repo symlink is refused: a symlink's content is its target string, and a save would land on a file the row does not name. A link to that symlink resolves to the target's row, where title, save, watch and token all name the same file. The asymmetry is between editing a link and following one.
A hard link is refused too, on read as well as write.
realpathresolves a symlink and a hard link is not one, so a file inside the repo hard-linked to a file outside passes containment and a save would change the outside file with it. The rule is on the link itself, not on where the other name sits — a link between two in-repo files is refused as well, because "which of these two names did the user mean" has no answer the panel can defend. Measured before choosing: 12 repositories, 38 561 tracked regular files, none with a link count above 1, and package-manager hard links live innode_modules, which is ignored and never a row. Refusing at open rather than only at save means the user is told before typing, not after.An unpaired surrogate in the content to be written is refused with
reason: 'encoding', symmetric with the read side — otherwiseBuffer.from(…, 'utf8')would write the U+FFFD substitution the read side exists to refuse.Found by running it
The panel was driven in a live instance — a real session editing real files — and four things came out of it that no unit test could see:
side-by-sideunderfilePanelDiffMode, and a stored preference is untouched.onChangebacked by a CodeMirrorupdateListener, which catches typing, paste, undo and programmatic edits where a DOMinputlistener misses the last two. The clean and in-flight guards stay in the handler, becauseCtrl+Sbypassesdisabled.cat-file blob, verified before anything changed. What vanished were the untracked row's counts and the header total, which for an untracked row are the visible diff: those counts are click-derived and the status refresh a save triggers carries none. The save now re-applies them from the bytes it wrote. Fixing it surfaced a second bug: the post-save guard compared the selected file by identity whilerepointSelectedFilehad just swapped that object; it compares by path there, and the five row-switch guards stay on identity.The panel also moved onto the sidebar's surface. Eleven values both carried as literals are
:roottokens now — 188 substitutions, every one value-preserving, verified by resolving everyvar()back to its literal and diffing: five structural lines and exactly one value change,#file-panel's background. Two assertions intest/sidebar-busy-agents-tint.test.jsmatched the accent by spelling and now accept either form; they still go red on a different colour, a one-digit near-miss, a different token and removal.Not in scope
Verification
Six adversarial review rounds, 28 findings, all closed by execution. 72 mutations, all red, including: the containment check, the
.gitrule under a resolved segment and under containment alone, the version token and its requirement, both watcher halves, the EOL and BOM round trips, the remote refusal in main and in the renderer, editor reuse, the non-teardown render, each of the four panel exits pinned independently, andonChangein each of the three modes.The
cat-file blob→git showmutation is red, so that decision is pinned by the module's own argv rather than by a test asserting git's behaviour.npx eslint .→ 0 errors, 331 warnings (themainbaseline).npm test→ stage 1tests 1806 / pass 1803 / fail 1 / skipped 2, stage 2120 / 119 / 0 / 1. The single failure istest/ipc-path-validator.test.js"allows files under ~/.claude/", pre-existing and environmental: that machine's~/.claude/CLAUDE.mdis a symlink out of~/.claudeand the validator resolves on disk. This branch touches neither that test noripc-path-validator.js.Patch coverage 98.7 %.
One Windows-only test failure was fixed here rather than retried blindly: the
maxBuffercap SIGTERMs the overrunninggit cat-file, andexecFile's callback runs before that child is reaped (measured:exitCode === null,signalCode === 'SIGTERM',killed === true). On Windows a live process holds a handle on its working directory, so removing the scratch repo raced a process still exiting — which is why only the over-the-cap case tripped it. The product behaviour is right (an overrunning child must be killed, and production must not block waiting for it), so the test cleanup tolerates the window, in both real-git suites rather than only the one that trips it today.What a human still has to judge, as opposed to measure: whether the panel reads as an editor in use, whether its new surface and accent read as one system with the sidebar, whether inline-by-default is right at your width, and whether the icon-only Refresh is self-evident without its tooltip.
Follow-ups
#301 (three
fs.watchregistries, two released on window close — this branch wires the one it adds), #303 (a session's panel ignores the worktrees its subagents work in), #304 (keep the journeys that only a running app can check).