feat(render): pre-execution diff preview for edit_file - #1366
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: 22486ffa90
ℹ️ 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".
| // No-op edit: mirrors computeLineDiff null-on-identical behavior | ||
| if (oldStr === newStr) return {}; | ||
|
|
||
| const diff = computeLineDiff(oldStr, newStr); |
There was a problem hiding this comment.
When replace_all: true and old_string occurs multiple times, this computes a diff for only one isolated old_string → new_string replacement, while edit-file.ts actually applies the replacement to every occurrence with content.split(old_string).join(new_string). The preview can therefore report a single-line change with snippet-relative hunk coordinates even though the tool is about to modify many locations; construct the preview by applying the edit semantics to the current file contents, or avoid presenting this incomplete diff for replace-all calls.
Useful? React with 👍 / 👎.
| const diff = computeLineDiff(oldStr, newStr); | ||
| if (diff === null) return {}; | ||
|
|
||
| opts.addPreviewDiffRef.current(context.toolUseId, diff); |
There was a problem hiding this comment.
For sessions whose canUseTool callback returns allow with updatedInput for edit_file, runPreDispatchGates dispatches this hook first and then runCanUseTool replaces call.input, so the displayed preview describes the original arguments while the handler executes different ones. Generate or refresh the preview only after the final input rewrite so it represents the edit that will actually run.
Useful? React with 👍 / 👎.
| if (entry.previewDiff) { | ||
| // Pre-execution diff preview: rendered under the in-flight " …" line, | ||
| // indented to visually attach to this entry. formatDiffBlock already | ||
| // returns [] when AFK_SHOW_DIFFS=0, so no extra guard needed. | ||
| for (const line of formatDiffBlock(entry.previewDiff, 'overlay', ' ')) { |
There was a problem hiding this comment.
The live overlay calls the ordinary formatDiffBlock renderer here, so it never displays the newly introduced ⟳ Proposed label or previewDiff framing. A repo-wide search shows previewDiff() is otherwise used only by its own tests, meaning those tests pass without exercising the production path and users cannot visually distinguish this pre-execution block as proposed; route this branch through the preview-specific renderer or add the proposed label here.
Useful? React with 👍 / 👎.
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (scheduled sweep), generated by the /review tool — a maintainer will follow up.
PR: feat(render): pre-execution diff preview for edit_file
Reviewed ref: 22486ffa907b91df6b91fd81a0efa514f85d898f | 17 files | ~800 lines (incl. .filesize-baseline.json reformat) | regime: full
Stated intent: PR title+body — spec-compliance assessed.
Prior automated rounds
No prior automated rounds on this PR — no dispositions to carry forward.
Decision: MERGE (with follow-ups)
0 critical, 0 high → MERGE (with follow-ups); 3 medium advisory (non-blocking), 1 low advisory, 2 nit.
The core feature is structurally sound: the mutable-ref pattern correctly bridges the agent/CLI layer without coupling them, the hook guards are complete (non-interactive surfaces never arm the ref, subagents skip via parentSessionId, AFK_SHOW_DIFFS=0 parity is delegated to formatDiffBlock's diffsDisabled() gate), and the overlay-only guarantee holds — previewDiff is rendered exclusively in the in-flight else branch (no entry.result) in getOverlay(), and all flush/scrollback paths (renderGroupedRootTools, tool-lane-render-children.ts flush path) only render entry.diff, never entry.previewDiff. CI is green across all platforms. 17 new tests cover the main paths.
Advisory findings (non-blocking)
1. medium · blocking:false · correctness · src/cli/commands/interactive/tool-lane-render-children.ts:285–290 · ref:22486ffa · file-state
renderOverlayChildren (the path for edit_file children of NESTING roots — skill, Agent, compose) renders the in-flight else branch with only the elapsed counter and thinkingTail. There is no child.previewDiff render block here, unlike the flat-root path in tool-lane.ts:577–582. An edit_file call issued from inside a skill or multi-agent dispatch shows no pre-execution diff preview — silently, with no error. This is a partial feature gap against the stated intent (which says "when the agent calls edit_file" without qualification).
Suggestion: Add a previewDiff block to the in-flight else branch in renderOverlayChildren after line 290, guarded by !child.result && child.previewDiff, mirroring the pattern at tool-lane.ts:577.
· waived: bounded display-only gap, no safety/data impact; feature works correctly for flat-root edit_file calls (the most common case).
2. medium · blocking:false · correctness · src/cli/_lib/stream-renderer.ts:326–331 (arm) and 543–586 (dispose) · ref:22486ffa · file-state
dispose() does not reset addPreviewDiffRef.current back to () => {} before tearing down the turn. The session-lifetime ref retains a closure over the disposed turn's toolLane and nulled overlayComposer until the next turn's arm() overwrites it. In the narrow inter-turn gap, a delayed PreToolUse dispatch (theoretically possible if hook execution races with turn teardown) writes into the dead toolLane silently — overlayComposer?.flush() is a no-op on null, so no throw, but the write is lost.
Suggestion: At the top of dispose(), before building DisposeCtx, add: if (this.addPreviewDiffRef) this.addPreviewDiffRef.current = () => {};
· waived: bounded non-data-affecting defect — PreToolUse hooks in practice only fire during active tool dispatch (within a turn), not in the inter-turn gap; the blast radius is a silently-dropped display update.
3. medium · blocking:false · spec-compliance · src/agent/tools/hooks/edit-preview-hook.ts:55–64 · ref:22486ffa · diff-context
The hook calls computeLineDiff(oldStr, newStr) unconditionally, even when AFK_SHOW_DIFFS=0 is set. formatDiffBlock (called by the overlay renderer) correctly returns [] on suppression, so display is correctly suppressed — but the O(m×n) LCS computation runs on every edit_file call regardless of the display opt-out. For a 500-line file edit, this is O(250,000) synchronous work per call on surfaces where the result is never shown.
Suggestion: Import diffsDisabled (from tool-lane-format-diff.ts) and return {} early in the hook if diffs are suppressed, before calling computeLineDiff.
· waived: bounded non-behavioral defect — wasted CPU on suppressed-display deployments only; the compute cap at MAX_DIFF_CELLS = 4_000_000 limits worst-case allocation.
4. low · blocking:false · test-coverage · src/agent/tools/hooks/edit-preview-hook.test.ts and src/agent/hooks/config-bridge.test.ts · ref:22486ffa · diff-context
No test exercises the behavior when addPreviewDiffRef is constructed but the StreamRenderer never calls arm() (non-TTY path). In production this is always safe — the ref starts as { current: () => {} } and the hook calls current(...) which is a no-op. But the hook tests inject a spy callback directly, so coverage of the no-op path is implicit rather than explicit.
Suggestion: Add one hook test: construct createEditPreviewHook({ addPreviewDiffRef: { current: () => {} } }), fire it with a valid edit_file context, and assert it does not throw and returns {}.
5. nit · blocking:false · correctness · src/agent/tools/hooks/edit-preview-hook.ts:21 · ref:22486ffa · diff-context
const diff = computeLineDiff(oldStr, newStr);
if (diff === null) return {}; // line 21This guard is unreachable in practice: the preceding if (oldStr === newStr) return {} (line 19) excludes the only documented null-return case for computeLineDiff. The 0-hunks case (the other null source) cannot arise for two distinct strings under the current implementation. The guard is harmless but creates a false impression that null is reachable post-equality-check.
Suggestion: Replace with a comment: // computeLineDiff returns non-null for distinct strings (no-hunk case requires identical input).
6. nit · blocking:false · correctness · src/cli/commands/interactive/tool-lane-render.ts:92–104 · ref:22486ffa · diff-context
previewDiff is set on ToolEntryFields but never cleared in addResult. It persists on the entry after the result arrives. All current rendering paths correctly gate on !entry.result before rendering previewDiff, so there is no current leakage. This is a defensive note for future maintainers extending the render paths.
Suggestion: In addResult (tool-lane.ts:~181), add entry.previewDiff = undefined; after setting entry.result = chunk as a belt-and-suspenders guard.
What was not checked
- Citations verified against branch HEAD
22486ffa907b91df6b91fd81a0efa514f85d898f. - Stated intent: PR #1366 title+body — spec-compliance assessed (both major items met; partial gap in NESTING branch noted as Finding 1).
- Did not run the test suite; CI reports green on all platforms.
- Did not review
compactDiffViewinternals beyond confirmingsanitizeDiffTextstrips escape sequences (ANSI injection mitigation confirmed). - Did not review Telegram or daemon surfaces (ref threading is gated by
if (ctx.addPreviewDiffRef ?)so non-REPL surfaces are correctly excluded).
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
PR: feat(render): pre-execution diff preview for edit_file
Reviewed ref: 22486ffa | 20 files | ~480 lines | regime: full
Stated intent: PR title+body — spec-compliance assessed.
Decision: DO NOT MERGE — 3 medium blocking (2 correctness/spec-compliance, 1 security); 4 low, 2 nit
Findings
1. medium · blocking · security · src/cli/_lib/stream-renderer.ts:326–331, 543–584 · ref:22486ffa
Use-after-dispose on TTY compositor. arm() installs a live closure over this.toolLane into addPreviewDiffRef.current. dispose() never resets addPreviewDiffRef.current to the no-op. If a hook fires between dispose() and the next arm(), it calls into a torn-down compositor via the stale closure.
- fix: In
dispose(), addthis.addPreviewDiffRef?.current = () => {};
2. medium · blocking · correctness/spec-compliance · src/cli/render/preview-diff.ts:54 / src/cli/commands/interactive/tool-lane.ts:577–582 · ref:22486ffa
preview-diff.ts exports previewDiff() which prepends the '⟳ Proposed' label and delegates to compactDiffView. However, tool-lane.ts never imports or calls previewDiff() — it calls formatDiffBlock(entry.previewDiff, 'overlay', ' ') directly. The '⟳ Proposed' label stated in the PR body (behavior #4) never reaches the terminal. previewDiff() is dead production code.
- fix: Replace the
formatDiffBlockcall attool-lane.ts:577–582withpreviewDiff(), or removepreview-diff.tsif the label is intentionally omitted.
3. medium · blocking · correctness/spec-compliance · src/cli/commands/interactive/tool-lane.ts:393, 577–583 · ref:22486ffa
Preview renders only for flat-root entries (no agentContext). Any edit_file call issued from within a skill or tool group (addStartWithAgentContext) has previewDiff set by the hook, but renderOverlayChildren has no awareness of previewDiff — the preview is silently discarded. Stated behavior #1 ("fires on every edit_file call") is partially unmet: the hook fires correctly but the render does not follow for nested entries.
- fix: Surface
previewDiffinrenderOverlayChildrenfor child entries, or explicitly document root-only as intentional.
4. low · security · src/agent/tools/hooks/edit-preview-hook.ts:51–55 · ref:22486ffa
No size bound on old_string/new_string before passing to computeLineDiff. The MAX_DIFF_CELLS guard bails cleanly, but string splitting happens first. Transient main-thread latency on very large edits; bounded by existing write-handler limits, no data loss.
- suggestion: Early-out if
oldStr.length + newStr.length > 512KB.
5. low · correctness/perf · src/cli/commands/interactive/tool-lane.ts:179–184, 209–212 · ref:22486ffa
addResult() sets entry.result but never deletes entry.previewDiff. The DiffPayload accumulates in the entry map until flush(). Minor GC pressure in high-frequency refactor turns.
- suggestion: Add
delete entry.previewDiff;inaddResult().
6. low · test-coverage · src/cli/commands/interactive/tool-lane.test.ts:3057–3065 · ref:22486ffa
Test (b) asserts absence of hunk header after result but does not positively assert the replacement outcome line. Weak negative-only assertion.
- suggestion: Add
expect(overlay).toContain('Replaced 1 occurrence').
7. low · test-coverage · src/agent/hooks/config-bridge.test.ts · ref:22486ffa · confidence:medium
Count assertions (3→4, 4→5) are correct but fragile — a property rename or removal passes if the count happens to be preserved.
- suggestion: Supplement with
expect(bundle).toHaveProperty('addPreviewDiffRef').
8. nit · api-compat · src/agent/hooks.ts:225–227 · ref:22486ffa
toolUseId added only to PreToolUseContext, not PostToolUseContext. No breaking change; design gap for future correlation.
9. nit · api-compat · src/cli/commands/interactive/shared.ts:534,774 / src/cli/_lib/stream-renderer-options.ts · ref:22486ffa
Inline ref type { current: (toolUseId: string, diff: DiffPayload) => void } repeated 3× with no named export. Signature change requires three independent updates.
- suggestion: Export a named
PreviewDiffReftype fromedit-preview-hook.ts.
Spec-Compliance
| # | Stated behavior | Status |
|---|---|---|
| 1 | PreToolUse hook fires on every edit_file call |
|
| 2 | Computes diff from old_string/new_string (no file read) |
✅ Met |
| 3 | Delivers via mutable callback ref bridging agent/CLI layers | ✅ Met |
| 4 | Overlay renders diff with Proposed label |
❌ Unmet — previewDiff() never called [2] |
| 5 | Preview replaced by normal outcome on result | ✅ Met |
What was not checked
- Citations verified inline against branch HEAD
22486ffa907b91df6b91fd81a0efa514f85d898f. - Stated intent: PR #1366 title+body — spec-compliance assessed.
- Did not run the test suite; CI reports green on all platforms.
- Runtime overlay rendering was assessed via static analysis only — no headless terminal session exercised.
.filesize-baseline.json160-line mechanical compaction was not audited for correctness.- E2E turn lifecycle (
arm → hook → lane → dispose) not traced under live I/O; finding [1] is a static-analysis inference.
Activates CompactDiffView in the live overlay at PreToolUse time so users see a diff before the edit executes. New files: - src/cli/render/preview-diff.ts: pure render component (⟳ Proposed header) - src/cli/render/preview-diff.test.ts: 5 tests covering label, content, truncation - src/agent/tools/hooks/edit-preview-hook.ts: PreToolUse hook computing diff payload - src/agent/tools/hooks/edit-preview-hook.test.ts: 7 tests (skip cases + normal path) Modified files: - src/cli/render/index.ts: barrel export for previewDiff - src/cli/commands/interactive/tool-lane-render.ts: previewDiff? field on ToolEntryFields - src/cli/commands/interactive/tool-lane.ts: addPreviewDiff() method + overlay render - src/cli/commands/interactive/tool-lane.test.ts: 5 new describe-block tests - src/agent/default-hook-registry.ts: register edit-preview hook, expose addPreviewDiffRef - src/cli/commands/interactive/bootstrap-hooks.ts: thread addPreviewDiffRef to caller - src/cli/commands/interactive/bootstrap.ts: propagate ref into InteractiveCtx - src/cli/commands/interactive/shared.ts: addPreviewDiffRef on InteractiveCtx + TurnHandles - src/cli/commands/interactive/loop-iteration.ts: forward ref into runTurn handles - src/cli/commands/interactive/turn-handler.ts: spread ref into buildRenderer() options - src/cli/_lib/stream-renderer-options.ts: addPreviewDiffRef? option field - src/cli/_lib/stream-renderer.ts: arm() wires callback → toolLane.addPreviewDiff() - src/agent/hooks.ts: toolUseId field on HookContext - src/agent/hooks/config-bridge.test.ts: update PreToolUse count assertions (3→4, 4→5) tsc --noEmit: 0 errors; lint: 0 errors; 152 tests pass
095b593 to
b5c7f5c
Compare
1 participant
Summary
Pre-execution diff preview for
edit_file-- show users what's about to change before it happens.When the agent calls
edit_file, the live overlay now shows a diff preview before the edit is applied, using the existingCompactDiffViewcomponent. This is the #1 UX gap between Claude Code and agent-afk -- CC renders aStructuredDiffin its permission dialogs before any write occurs.How it works
PreToolUsehook (edit-preview-hook.ts) fires on everyedit_filecallold_string/new_string(no file read needed)ProposedlabelWhat's new
src/cli/render/preview-diff.tspreviewDiff()component wrappingcompactDiffViewwith "Proposed" framingsrc/agent/tools/hooks/edit-preview-hook.tsPreToolUsehook: guards foredit_file, computes diff, delivers to lanesrc/agent/hooks.tstoolUseId?toPreToolUseContext(needed for lane correlation)src/agent/tools/dispatcher.tscall.idintopreCtxsrc/cli/commands/interactive/tool-lane.tsaddPreviewDiff()method + overlay renderingsrc/cli/commands/interactive/tool-lane-render.tspreviewDiff?field toToolEntryFieldssrc/agent/default-hook-registry.tsaddPreviewDiffRefsrc/cli/commands/interactive/bootstrap-hooks.tssrc/cli/commands/interactive/bootstrap.tsInteractiveCtxsrc/cli/commands/interactive/shared.tsInteractiveCtx+TurnHandlessrc/cli/commands/interactive/turn-handler.tsStreamRendererOptionssrc/cli/_lib/stream-renderer-options.tsaddPreviewDiffRef?optionsrc/cli/_lib/stream-renderer.tsarm()each turnDesign decisions
{ current: () => {} }ref is created at registry time, armed each turn by the StreamRenderer, and called synchronously by the hook. Non-interactive surfaces never arm the ref (remains no-op).{}for subagents (parentSessionIdset) since they have no overlay.AFK_SHOW_DIFFS=0parity: The preview respects the existing diff opt-out viaformatDiffBlock's internaldiffsDisabled()gate.write_filesupport (different input shape -- needs a file read for preview; separate scope).Tests
preview-diff.test.ts-- 5 tests (label, hunk content, maxLines, filePath, ANSI safety)edit-preview-hook.test.ts-- 7 tests (normal fire, no-op, subagent skip, wrong tool, wrong event, no injectContext, missing toolUseId)tool-lane.test.ts-- 5 new tests inaddPreviewDiffdescribe block (overlay render, result replacement, unknown id, AFK_SHOW_DIFFS suppression, flush exclusion)Part of the component library initiative
This activates
CompactDiffView-- one of two components built in the Aug 27-28 render sprint (#1321-#1361) that had zero live callers. The component library goal is surpassing Claude Code's terminal UX by building composable components on the existing compositor.