From d65a926810f63de05aba58fd8ac1209bb6fa3ea3 Mon Sep 17 00:00:00 2001 From: tdwd Date: Mon, 7 Sep 2026 21:10:32 +0200 Subject: [PATCH] diff: render a supplied unified diff, and wire codex file changes to it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codex reports an edit as a ready-made unified hunk and never sends the whole file, so there was no before/after pair to build a fileDiff from. That looked like a design problem and was not one: both renderers already worked FROM unified text. Each computed it on its first line and spent the rest of the function parsing it back. So they now take the unified text and style it, and fileDiff.unifiedText is the one place that decides whether to use a supplied diff or compute one. No reconstruction, no second renderer, and both diff styles work on both backends. Behaviour-preserving for claude: the same string reaches the same styling loop, and the existing renderer tests pass unchanged. The three codex change kinds put different things in the same `diff` field, all confirmed against the live CLI: add and delete carry the file's content, update carries the hunk. Reading that wrong renders a deletion as an addition. Display paths are trimmed against the working root the agent reported on thread/started, not os.Getwd. Those are equal by convention only, and a live test rooted in a temp dir rendered the full absolute path until this was fixed — the kind of assumption that stays invisible in production. --- README.md | 7 ++-- codexengine_test.go | 67 ++++++++++++++++++++++++++++++++++++++ codexitems.go | 79 +++++++++++++++++++++++++++++++++++++++++++++ codexlive_test.go | 62 +++++++++++++++++++++++++++++++++++ codexstream.go | 2 ++ diff.go | 38 +++++++++++++++++----- diff_split.go | 12 +++---- diff_split_test.go | 9 +++--- model.go | 5 +++ render.go | 2 +- render_test.go | 2 +- 11 files changed, 263 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 4c66c86..0a73584 100644 --- a/README.md +++ b/README.md @@ -98,8 +98,11 @@ The codex backend is newer and narrower than the claude one: - `build` and `bypass` work fully. Tools run, and codex asks for nothing. - `ask` and `plan` refuse gated actions rather than granting them, because the approval pane is not wired to codex yet. -- Tool calls and file changes render as cards. Side-by-side diffs, session - replay and the slash-command palette are claude-only so far. +- File changes render as real diff cards, in both the unified and side-by-side + styles. Other tool calls render as plain cards rather than the typed ones + claude gets. +- Session replay, `/compact` and the slash-command palette are claude-only so + far. `CATHODE_CODEX_LIVE=1 go test -run TestCodexLive ./...` exercises the backend against the real CLI. It spends a turn on your subscription, so it is off by diff --git a/codexengine_test.go b/codexengine_test.go index 816ccbc..a75ac03 100644 --- a/codexengine_test.go +++ b/codexengine_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "time" ) @@ -214,3 +215,69 @@ func TestCodexAdapterMapsFramesToEntries(t *testing.T) { t.Error("the echoed user message must not be added again") } } + +// The three file-change kinds, each carrying a different thing in `diff`. All +// three shapes were taken from the live CLI; getting the kind wrong renders a +// deletion as an addition, or a hunk as a wall of new text. +func TestCodexFileDiffsReadEachKindCorrectly(t *testing.T) { + const hunk = "@@ -1,3 +1,3 @@\n line one\n-line two\n+line TWO\n line three\n" + raw := json.RawMessage(`{"changes":[ + {"path":"/tmp/a.txt","kind":{"type":"add"},"diff":"hello\n"}, + {"path":"/tmp/b.txt","kind":{"type":"delete"},"diff":"gone\n"}, + {"path":"/tmp/c.txt","kind":{"type":"update"},"diff":` + mustJSON(hunk) + `}, + {"path":"/tmp/d.txt","kind":{"type":"martian"},"diff":"?"} + ]}`) + + ds := codexFileDiffs(raw, "/tmp") + if len(ds) != 3 { + t.Fatalf("got %d diffs, want 3 (the unknown kind is skipped)", len(ds)) + } + if ds[0].file != "a.txt" { + t.Errorf("path = %q, want it trimmed against the session root", ds[0].file) + } + if ds[0].new != "hello\n" || ds[0].old != "" { + t.Errorf("add: got old=%q new=%q, want the content as the new side", ds[0].old, ds[0].new) + } + if ds[1].old != "gone\n" || ds[1].new != "" { + t.Errorf("delete: got old=%q new=%q, want the content as the old side", ds[1].old, ds[1].new) + } + if ds[2].unified != hunk { + t.Errorf("update: got unified=%q, want the supplied hunk verbatim", ds[2].unified) + } +} + +// A supplied hunk must reach the renderer untouched, and a computed one must +// still be computed. This is the pairing the refactor exists to make safe. +func TestUnifiedTextPrefersASuppliedDiff(t *testing.T) { + const hunk = "@@ -1,1 +1,1 @@\n-a\n+b\n" + if got := (fileDiff{file: "f", unified: hunk}).unifiedText(); got != hunk { + t.Errorf("supplied diff was not used verbatim: %q", got) + } + computed := (fileDiff{file: "f", old: "a\n", new: "b\n"}).unifiedText() + if computed == "" || !strings.Contains(computed, "+b") { + t.Errorf("a before/after pair should still be diffed, got %q", computed) + } +} + +// A codex update renders as a real diff card, not a raw tool card. Without the +// unified path this fell through to addTool and showed the hunk as JSON. +func TestCodexUpdateRendersAsADiffCard(t *testing.T) { + m, _ := newTestModel(t, "") + m.handleCodexEvent(codexFrame{ + Method: "item/started", + Params: json.RawMessage(`{"item":{"type":"fileChange","id":"fc1","changes":[ + {"path":"/tmp/x.go","kind":{"type":"update"},"diff":"@@ -1,1 +1,1 @@\n-a\n+b\n"} + ]}}`), + }) + last := m.entries[len(m.entries)-1] + if last.kind != entDiff { + t.Fatalf("entry kind = %v, want entDiff", last.kind) + } + if len(last.diffs) != 1 || last.diffs[0].unified == "" { + t.Errorf("diff entry = %+v, want the supplied hunk carried through", last.diffs) + } + out := stripANSI(renderDiffFor(diffUnified, last.diffs[0], 80)) + if !strings.Contains(out, "+ b") || !strings.Contains(out, "- a") { + t.Errorf("the card should show the change, got:\n%s", out) + } +} diff --git a/codexitems.go b/codexitems.go index aa18533..f814fa0 100644 --- a/codexitems.go +++ b/codexitems.go @@ -5,6 +5,7 @@ package main import ( "encoding/json" + "path/filepath" "strings" ) @@ -45,6 +46,19 @@ func (m *model) codexItem(f codexFrame, started bool) { if t := codexReasoningText(p.Item); t != "" { m.add(entThinking, t) } + case "fileChange": + if !started { + return + } + if head.ID != "" && !m.noteToolCard(head.ID) { + return + } + if ds := codexFileDiffs(p.Item, m.agentCwd); len(ds) > 0 { + m.addDiffs(ds) + return + } + m.addTool(head.Type, p.Item) + default: // A tool item. Its content is on the opening event, and the id pairs it // with the approval request that may follow, so noteToolCard keeps the @@ -59,6 +73,71 @@ func (m *model) codexItem(f codexFrame, started bool) { } } +// codexFileDiffs turns an item/fileChange into diff cards. +// +// The `diff` field means two different things depending on the kind, which is +// the whole reason this function exists rather than one assignment: +// +// add the new file's CONTENT +// delete the removed file's CONTENT +// update an already-computed unified hunk ("@@ -1,3 +1,3 @@ …") +// +// All three were confirmed against the live CLI. An update carries no copy of +// the whole file, so there is no before/after pair to build — it goes through +// fileDiff.unified, which both renderers accept because they parse unified +// text anyway. +func codexFileDiffs(raw json.RawMessage, root string) []fileDiff { + var it struct { + Changes []struct { + Path string `json:"path"` + Kind struct { + Type string `json:"type"` + } `json:"kind"` + Diff string `json:"diff"` + } `json:"changes"` + } + if json.Unmarshal(raw, &it) != nil { + return nil + } + var out []fileDiff + for _, c := range it.Changes { + if c.Path == "" { + continue + } + d := fileDiff{file: codexShortPath(c.Path, root)} + switch c.Kind.Type { + case "add": + d.new = c.Diff + case "delete": + d.old = c.Diff + case "update": + d.unified = c.Diff + default: + continue // an unknown kind: a plain card says more than a blank diff + } + out = append(out, d) + } + return out +} + +// codexShortPath trims the session's working root off a change path. +// +// codex reports absolute paths. The card title is more readable relative, and +// a screenshot of a session then carries no home directory. root is what the +// agent reported for the thread, NOT os.Getwd: those are equal by convention +// only, and a session rooted elsewhere would render every path in full. +// Falls back to the original when root is unknown or the path sits outside it. +func codexShortPath(p, root string) string { + if root == "" { + return p + } + rel, err := filepath.Rel(root, p) + if err != nil || rel == "" || strings.HasPrefix(rel, "..") { + return p + } + return rel +} + // codexReasoningText flattens a reasoning item. The summary is what the // interactive UI shows, so prefer it and fall back to the full content. func codexReasoningText(raw json.RawMessage) string { diff --git a/codexlive_test.go b/codexlive_test.go index 137cd73..aad286a 100644 --- a/codexlive_test.go +++ b/codexlive_test.go @@ -5,6 +5,8 @@ package main import ( "os" + "path/filepath" + "strings" "testing" "time" ) @@ -137,3 +139,63 @@ func TestCodexLiveGatedActionsAlwaysEndTheTurn(t *testing.T) { }) } } + +// A real edit by the real CLI must reach the screen as a diff card. +// +// The unit tests use a hunk copied from a recorded session, which proves +// cathode parses what it was told to expect. This proves codex still sends it. +func TestCodexLiveEditRendersAsADiffCard(t *testing.T) { + if os.Getenv("CATHODE_CODEX_LIVE") == "" { + t.Skip("set CATHODE_CODEX_LIVE=1 to run against the real codex CLI") + } + dir := t.TempDir() + target := filepath.Join(dir, "target.txt") + if err := os.WriteFile(target, []byte("line one\nline two\nline three\n"), 0o644); err != nil { + t.Fatal(err) + } + + // build mode: codex runs the edit without asking, so the turn completes. + e, err := newCodexEngine(codexEngineConfig{Mode: "build", Cwd: dir}) + if err != nil { + t.Fatalf("spawn: %v", err) + } + defer e.Close() + + frames := make(chan codexFrame, 256) + e.mu.Lock() + e.sink = func(f codexFrame) { frames <- f } + e.mu.Unlock() + + if err := e.Initialize(); err != nil { + t.Fatalf("Initialize: %v", err) + } + if err := e.Send("In target.txt, change the word two to TWO. Edit the file, nothing else."); err != nil { + t.Fatalf("Send: %v", err) + } + + m, _ := newTestModel(t, "") + deadline := time.After(3 * time.Minute) + for { + select { + case f := <-frames: + m.handleCodexEvent(f) + if f.Method == "turn/completed" || f.Method == "turn/failed" { + for _, en := range m.entries { + if en.kind != entDiff { + continue + } + card := stripANSI(renderDiffFor(diffUnified, en.diffs[0], 80)) + t.Logf("diff card:\n%s", card) + if !strings.Contains(card, "TWO") { + t.Errorf("the card should show the edit, got:\n%s", card) + } + return + } + t.Error("the edit never produced a diff entry") + return + } + case <-deadline: + t.Fatal("the turn never ended") + } + } +} diff --git a/codexstream.go b/codexstream.go index 18e217f..c5ca604 100644 --- a/codexstream.go +++ b/codexstream.go @@ -62,6 +62,7 @@ func (m *model) noteCodexThread(f codexFrame) { Thread struct { ID string `json:"id"` Model string `json:"model"` + Cwd string `json:"cwd"` } `json:"thread"` } if json.Unmarshal(f.Params, &p) != nil || p.Thread.ID == "" { @@ -74,6 +75,7 @@ func (m *model) noteCodexThread(f codexFrame) { if p.Thread.Model != "" { m.modelID = p.Thread.Model } + m.agentCwd = p.Thread.Cwd m.add(entInfo, fmt.Sprintf("— thread %s · %s —", short(p.Thread.ID), m.modelID)) } diff --git a/diff.go b/diff.go index 8737b40..758aacd 100644 --- a/diff.go +++ b/diff.go @@ -13,8 +13,27 @@ import ( udiff "github.com/aymanbagabas/go-udiff" ) -// fileDiff is one before/after pair to visualise. A MultiEdit produces several. -type fileDiff struct{ file, old, new string } +// fileDiff is one file change to visualise. A MultiEdit produces several. +// +// Usually it holds the before and after text and the diff is computed here. +// Some backends have already computed it: codex reports an edit as a ready-made +// unified hunk and never sends the whole file, so there is no before/after pair +// to hold. `unified` carries that case, and both renderers work from unified +// text anyway — computing it was only ever the first step. +type fileDiff struct { + file, old, new string + unified string // already-computed diff; old/new are unused when set +} + +// unifiedText is the single answer to "what should this card render". Keeping +// it in one place is what stops the two renderers disagreeing about when a +// supplied diff wins over a computed one. +func (d fileDiff) unifiedText() string { + if d.unified != "" { + return d.unified + } + return udiff.Unified("a/"+d.file, "b/"+d.file, d.old, d.new) +} // edit-tool input shapes from Claude Code. type editInput struct { @@ -43,20 +62,20 @@ func diffsForTool(name string, input json.RawMessage) ([]fileDiff, bool) { case "Edit": var in editInput if json.Unmarshal(input, &in) == nil && in.FilePath != "" { - return []fileDiff{{in.FilePath, in.OldString, in.NewString}}, true + return []fileDiff{{file: in.FilePath, old: in.OldString, new: in.NewString}}, true } case "Write": var in writeInput if json.Unmarshal(input, &in) == nil && in.FilePath != "" { old, _ := os.ReadFile(in.FilePath) // empty if new file - return []fileDiff{{in.FilePath, string(old), in.Content}}, true + return []fileDiff{{file: in.FilePath, old: string(old), new: in.Content}}, true } case "MultiEdit": var in multiEditInput if json.Unmarshal(input, &in) == nil && in.FilePath != "" { var ds []fileDiff for _, e := range in.Edits { - ds = append(ds, fileDiff{in.FilePath, e.OldString, e.NewString}) + ds = append(ds, fileDiff{file: in.FilePath, old: e.OldString, new: e.NewString}) } if len(ds) > 0 { return ds, true @@ -66,12 +85,15 @@ func diffsForTool(name string, input json.RawMessage) ([]fileDiff, bool) { return nil, false } -// renderDiff builds one styled, line-numbered diff card. -func renderDiff(filename, oldText, newText string, width int) string { +// renderDiff builds one styled, line-numbered diff card from unified diff text. +// +// It takes the unified text rather than a before/after pair because that is +// what it always worked on: the old signature computed the unified form on the +// first line and spent the rest of the function parsing it back. +func renderDiff(filename, u string, width int) string { if width < 24 { width = 24 } - u := udiff.Unified("a/"+filename, "b/"+filename, oldText, newText) if strings.TrimSpace(u) == "" { return dBox.Width(width - 2).Render(dTitle.Render(" "+filename+" ") + "\n" + dCtx.Render("(no changes)")) } diff --git a/diff_split.go b/diff_split.go index 0b427d0..b8187c8 100644 --- a/diff_split.go +++ b/diff_split.go @@ -8,7 +8,6 @@ import ( "strconv" "strings" - udiff "github.com/aymanbagabas/go-udiff" "github.com/charmbracelet/lipgloss" ) @@ -57,19 +56,20 @@ func (m *model) commitDiff(id string) { // renderDiffFor dispatches to the configured diff style, falling back to the // unified card when split is selected but the terminal is too narrow for it. -func renderDiffFor(style, filename, oldText, newText string, width int) string { +// The unified text is produced once, here, so both styles render the same diff. +func renderDiffFor(style string, d fileDiff, width int) string { + u := d.unifiedText() if style == diffSplit && width >= splitMinWidth { - return renderDiffSplit(filename, oldText, newText, width) + return renderDiffSplit(d.file, u, width) } - return renderDiff(filename, oldText, newText, width) + return renderDiff(d.file, u, width) } // renderDiffSplit builds a side-by-side diff card: deletions (with old line // numbers) on the left, additions (new line numbers) on the right, context on // both. A change is shown as a removed line beside its added line; unpaired // adds/dels leave the opposite column blank. -func renderDiffSplit(filename, oldText, newText string, width int) string { - u := udiff.Unified("a/"+filename, "b/"+filename, oldText, newText) +func renderDiffSplit(filename, u string, width int) string { if strings.TrimSpace(u) == "" { return dBox.Width(width - 2).Render(dTitle.Render(" "+filename+" ") + "\n" + dCtx.Render("(no changes)")) } diff --git a/diff_split_test.go b/diff_split_test.go index c9440ec..8f70285 100644 --- a/diff_split_test.go +++ b/diff_split_test.go @@ -18,7 +18,7 @@ func stripANSI(s string) string { return ansiRe.ReplaceAllString(s, "") } func TestRenderDiffSplit(t *testing.T) { old := "func add(a, b int) int {\n\treturn a + b\n}\n" neu := "func add(a, b, c int) int {\n\treturn a + b + c\n}\n" - out := stripANSI(renderDiffSplit("math.go", old, neu, 100)) + out := stripANSI(renderDiffSplit("math.go", fileDiff{file: "math.go", old: old, new: neu}.unifiedText(), 100)) if !strings.Contains(out, "math.go") || !strings.Contains(out, "+2") || !strings.Contains(out, "-2") { t.Fatalf("missing title / counts:\n%s", out) @@ -57,13 +57,14 @@ func TestDiffCommand(t *testing.T) { // renderDiffFor honors the style and falls back to unified when too narrow. func TestRenderDiffForFallback(t *testing.T) { old, neu := "a\nb\n", "a\nc\n" - if renderDiffFor(diffSplit, "f", old, neu, 60) != renderDiff("f", old, neu, 60) { + d := fileDiff{file: "f", old: old, new: neu} + if renderDiffFor(diffSplit, d, 60) != renderDiff("f", d.unifiedText(), 60) { t.Error("split below splitMinWidth should fall back to unified") } - if renderDiffFor(diffSplit, "f", old, neu, 120) != renderDiffSplit("f", old, neu, 120) { + if renderDiffFor(diffSplit, d, 120) != renderDiffSplit("f", d.unifiedText(), 120) { t.Error("split at a wide width should render side-by-side") } - if renderDiffFor(diffUnified, "f", old, neu, 120) != renderDiff("f", old, neu, 120) { + if renderDiffFor(diffUnified, d, 120) != renderDiff("f", d.unifiedText(), 120) { t.Error("unified style should always render the single-column card") } } diff --git a/model.go b/model.go index bdd55af..317044a 100644 --- a/model.go +++ b/model.go @@ -154,6 +154,11 @@ type model struct { frameBody string bodyKey bodyKey contentVer int + // agentCwd is the working root the agent reported for this session. Used to + // shorten paths for display. Read from the protocol rather than os.Getwd + // because the two are only equal by convention: the agent is told its root + // explicitly, and nothing stops it differing. + agentCwd string toolUses map[string]string // tool_use_id -> tool name, so tool_result events can show what they're answering shownTools map[string]bool // tool_use_ids already drawn as a card/diff, so the stream and approval paths don't both draw one (toolcard.go) busy bool diff --git a/render.go b/render.go index fc84344..557f15f 100644 --- a/render.go +++ b/render.go @@ -117,7 +117,7 @@ func (m *model) renderEntry(e entry) string { case entDiff: parts := make([]string, 0, len(e.diffs)) for _, d := range e.diffs { - parts = append(parts, renderDiffFor(m.settings.Diff, d.file, d.old, d.new, m.vp.Width)) + parts = append(parts, renderDiffFor(m.settings.Diff, d, m.vp.Width)) } return strings.Join(parts, "\n") case entInfo: diff --git a/render_test.go b/render_test.go index 0346089..e0af205 100644 --- a/render_test.go +++ b/render_test.go @@ -36,7 +36,7 @@ func TestDiffRendering(t *testing.T) { if !ok || len(ds) != 1 { t.Fatalf("Edit not detected: ok=%v n=%d", ok, len(ds)) } - out := renderDiff(ds[0].file, ds[0].old, ds[0].new, 80) + out := renderDiff(ds[0].file, ds[0].unifiedText(), 80) for _, want := range []string{"main.go", "+", "-", "│"} { if !strings.Contains(out, want) { t.Fatalf("diff missing %q in:\n%s", want, out)