feat(agent-mode): show agent file-edit diffs — chip, read-only pane, permission preview#2656
feat(agent-mode): show agent file-edit diffs — chip, read-only pane, permission preview#2656zeroliu wants to merge 6 commits into
Conversation
…tats Phase 1: add deriveEditDiff / rawEditPath / diffStats (backend-agnostic) and replace the naive every-line-counts stat with a real jsdiff line count, so a one-line change in a large file reports +1/-1 rather than the file size. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 2: guard-parse the Claude SDK's tool_use_result (FileEditOutput /
FileWriteOutput) and emit a {type:"diff"} so SDK Edit/Write calls carry a real
before/after (previously discarded as text-only). Edit after-text is rebuilt
with a literal first-occurrence replace so newString $ sequences aren't mangled.
Parser is shape-pinned to the documented SDK types as a drift tripwire.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 3: extract ApplyView's split/side-by-side render primitives into @/components/diff/DiffBlocks (behavior-preserving, byte-identical) and add a dedicated read-only AgentDiffView editor pane with openAgentDiffView — no accept/reject, no file writes, reuses the pane for a given path. Registered via the agentMode barrel, desktop-gated like PlanPreviewView. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 4: completed agent edits show an always-visible +N/-M chip in the trail row, and the whole row opens the read-only AgentDiffView (filename included) rather than the file. Edit rows are pane-only (no inline diff pre, no dead chevron); non-edit cards are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 5: the permission card synthesizes an edit preview from the pending tool input (Edit old/new strings; a Write reads the current file for its before), shows the +N/-M chip + a capped preview, and opens the same read-only AgentDiffView on click. Paths are normalized vault-relative so the preview pane reuses the tab the completed action opens. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 6: document the read-only AgentDiffView and where edit diffs are sourced in agentMode/AGENTS.md, and apply prettier formatting. Full jest suite (4384 tests), eslint (incl. boundaries), and the production build are all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daab29af46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (diffs.length > 0) { | ||
| const first = diffs[0]; |
There was a problem hiding this comment.
When a permission request includes multiple {type: "diff"} blocks (for example ACP/MultiEdit or any batch edit), this collapses the request to diffs[0]. Because ToolPermissionCard renders the preview branch whenever this returns non-null, the raw input/all-diff fallback is suppressed and the user can approve additional file changes without seeing them; the previous implementation rendered every entry from diffContents.map(...). Please keep all diff entries visible/openable before resolving the permission.
Useful? React with 👍 / 👎.
| const file = app.vault.getAbstractFileByPath(path); | ||
| const oldText = file instanceof TFile ? await app.vault.read(file) : ""; |
There was a problem hiding this comment.
For an absolute Write target outside the vault, toVaultRelative leaves path absolute and Obsidian's vault lookup cannot resolve it, so an existing external file falls through to oldText = "". PermissionBridge does route non-plan absolute writes through the prompter, so the approval card can show an overwrite outside the vault as a create/all-additions diff, hiding the content that will be replaced; use a desktop file read for absolute external paths or make the fallback explicit.
Useful? React with 👍 / 👎.
| !b.is_error && editDiff | ||
| ? [ | ||
| ...(outputs ?? []), |
There was a problem hiding this comment.
If an SDK user message contains more than one tool_result block, this single message-level editDiff is appended inside the loop to every non-error result. That can make unrelated tools in the same batched result message display the edit chip and open the edited file's diff; guard this to the single-result case or associate the parsed diff with the matching tool_use_id before attaching it.
Useful? React with 👍 / 👎.
| // The snapshot is serializable, so a workspace reload re-renders the same | ||
| // static before/after — acceptable for a read-only preview. | ||
| getState(): Record<string, unknown> { | ||
| return this.state ? { ...this.state } : {}; |
There was a problem hiding this comment.
Returning the whole state here serializes oldText and newText into Obsidian's workspace layout whenever a diff tab is open. For large notes or files containing secrets, opening the read-only diff can bloat .obsidian/workspace*.json and leak full before/after contents into vault sync/backups; persist only a lightweight identifier such as path (or return {} like PlanPreviewView) and recreate the view from the chat state when needed.
Useful? React with 👍 / 👎.
| if (typeof input.old_string === "string" && typeof input.new_string === "string") { | ||
| return { path, oldText: input.old_string, newText: input.new_string }; | ||
| } | ||
| if (typeof input.content === "string") { | ||
| return { path, oldText: "", newText: input.content }; |
There was a problem hiding this comment.
Because ActionCard calls deriveEditDiff for every completed tool call, this raw-input fallback treats any non-edit tool whose input happens to contain path/content or old_string/new_string as an edit. That is plausible for MCP/custom tools and will suppress the normal output, show an edit chip, and open a fake diff pane; the permission preview has the same shape-based fallback, so the fix should gate synthesis on known edit tool kinds/vendor names or explicit diff content.
Useful? React with 👍 / 👎.
| if (typeof input.old_string === "string" && typeof input.new_string === "string") { | ||
| return { path, oldText: input.old_string, newText: input.new_string }; |
There was a problem hiding this comment.
For an Edit request that replaces all occurrences, this preview still uses only the literal old_string and new_string as the entire before/after. The approval card therefore shows one hunk and small stats even though the tool can change every occurrence in the file (the completed-result parser already handles replaceAll), so users can approve a much broader edit than the preview indicates; read the target file and apply the replace-all semantics or explicitly show that all matches will change.
Useful? React with 👍 / 👎.
| const changes = diffTrimmedLines(state.oldText, state.newText, { | ||
| newlineIsToken: true, | ||
| }); |
There was a problem hiding this comment.
diffTrimmedLines ignores leading/trailing whitespace when building the read-only agent diff. If the agent changes indentation, list nesting, code-block whitespace, or trailing spaces, the pane can render those lines as unchanged (or omit the change entirely) even though the file content changed; use a non-trimming line diff for this trust surface so whitespace-only edits remain visible.
Useful? React with 👍 / 👎.
1 participant
Why
When the agent edits a file, it was hard to see what actually changed — the chat showed a raw JSON input blob (for the default Claude SDK agent) or a naive full-file dump, and there was no way to open a real diff. That erodes trust in the agent. This makes every agent edit legible.
Root cause found during planning: the in-process Claude Agent SDK already ships each edit's result on
tool_use_result— withoriginalFile(full before) and a real minimalstructuredPatch— for both Edit and Write, but the SDK translator discarded it (extracted text only). The olddiffStatsalso counted every line (ACP-only), so a 1-line change reported the whole file size.What
+N / −Mchip, and the whole row opens a read-only diff pane in the editor area.+N / −Mchip + a capped preview, and clicking opens the same pane (a Write reads the current file for its true "before").EditDiff {path, oldText, newText}, sourced from the SDK'stool_use_result(guard-parsed against the documented, byte-stableFileEditOutput/FileWriteOutput) for completed edits, from ACP{type:"diff"}content otherwise, and synthesized from tool input for permission previews.Design decisions (reviewed plan)
AgentDiffView(not the composerApplyViewaccept/reject flow); its split/side-by-side primitives are shared withApplyView.tool_use_result(hybrid: guard-parse + input fallback, no SDK bump — verified 0.3.197 doesn't improve theunknowntyping). A shape-pin test trips loudly if the SDK reshapes the result.Phases (one commit each)
9d2354b6— canonicalderiveEditDiff+ real jsdiff line stats.66bb1db6— surface real SDK edit diffs fromtool_use_result.068f1c11— read-onlyAgentDiffView+ shared diff primitives (behavior-preservingApplyViewrefactor).8bb1491e— edit-row chip + whole-row opens the pane.e4b4e084— permission-card preview + open.daab29af— docs + formatting.Notable review catches (fixed before commit)
$-pattern corruption: the Edit after-text reconstruction usedString.replace, which mangles anewStringcontaining$&/$1— switched to a literal first-occurrence replace (+ proof test).[text, diff]output, so edit rows were both pane-opening and expandable — made edit rows pane-only.Testing
Full suite 4384 tests / 0 failures,
eslint(incl. agent-mode boundaries) clean, and the production build all green. New unit tests cover the derivation, the guard-parse (shape-pinned to the SDK types), and the permission synthesis.🤖 Generated with Claude Code