From 5e76e4aa90e70245404c10085e809880635572a0 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 23 Jun 2026 11:20:45 -0600 Subject: Fix tool override swaps to stay differential Treat 1:1 component swaps and pure insertions as differential TUI updates instead of structural changes that force full redraws. Keep full redraws for net removals and edits above the viewport, and add tests covering the streaming-tool flicker regression and scrollback behavior. Also remove the unfinished Go ToolSource batch-registration bridge. --- libpanto-go/panto.go | 71 ++++++---------------------------------------------- src/tui_engine.zig | 71 ++++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 71 insertions(+), 71 deletions(-) diff --git a/libpanto-go/panto.go b/libpanto-go/panto.go index 89e4317..543d4b6 100644 --- a/libpanto-go/panto.go +++ b/libpanto-go/panto.go @@ -16,7 +16,6 @@ static PantoStatus pantoGoStoreAppendMessagesConst(void *ctx, PantoSlice session return pantoGoStoreAppendMessages(ctx, session_id, (PantoPersistentMessage*)messages, len); } extern PantoStatus pantoGoToolInvoke(void *ctx, PantoSlice input, PantoResultParts *out); -extern PantoStatus pantoGoToolSourceInvokeBatch(void *ctx, PantoToolCall *calls, PantoToolCallResult *results, size_t len); extern void pantoGoCallbackDestroy(void *ctx); static PantoSessionStoreVTable panto_go_store_vtable = { pantoGoStoreCreate, @@ -1264,16 +1263,6 @@ type ( } ) -type ( - ToolCall struct{ ToolName, Input string } - ToolSourceFunc func(calls []ToolCall) ([]ResultParts, []error, error) - ToolSource struct { - Name string - Tools []ToolDecl - InvokeBatch ToolSourceFunc - } -) - var ( callbackHandles = map[uintptr]any{} nextCallbackHandle uintptr = 1 @@ -1298,35 +1287,16 @@ func pantoGoToolInvoke(ctx unsafe.Pointer, input C.PantoSlice, out *C.PantoResul return C.PANTO_OK } -//export pantoGoToolSourceInvokeBatch -func pantoGoToolSourceInvokeBatch(ctx unsafe.Pointer, calls *C.PantoToolCall, results *C.PantoToolCallResult, n C.size_t) C.PantoStatus { - src := callbackHandles[uintptr(ctx)].(ToolSource) - cc := unsafe.Slice(calls, int(n)) - goCalls := make([]ToolCall, int(n)) - for i, c := range cc { - goCalls[i] = ToolCall{ToolName: goSlice(c.tool_name), Input: goSlice(c.input)} - } - parts, errs, err := src.InvokeBatch(goCalls) - if err != nil { - return C.PANTO_ERROR - } - rr := unsafe.Slice(results, int(n)) - for i := range rr { - if i < len(errs) && errs[i] != nil { - rr[i].ok = false - continue - } - rr[i].ok = true - if i < len(parts) { - rr[i].parts = makeCResultParts(parts[i]) - } - } - return C.PANTO_OK -} - //export pantoGoCallbackDestroy func pantoGoCallbackDestroy(ctx unsafe.Pointer) { dropCallback(ctx) } +// RegisterTool registers a single tool. Note: libpanto invokes each tool call +// on its own thread, so Invoke may run concurrently — guard any shared state. +// +// TODO: back all Go tool registrations with one process-global Zig ToolSource so +// libpanto delivers the whole turn's batch on a single thread and we fan out onto +// goroutines instead of libpanto threads. Pure efficiency (cheaper than N pthreads +// per turn); the public API stays exactly this. Punted as not worth the work yet. func (a *Agent) RegisterTool(t Tool) error { ctx := saveCallback(t) d, free := cToolDecl(t.Decl) @@ -1338,33 +1308,6 @@ func (a *Agent) RegisterTool(t Tool) error { return nil } -func (a *Agent) RegisterToolSource(src ToolSource) error { - ctx := saveCallback(src) - cname := cSlice(src.Name) - defer C.free(unsafe.Pointer(cname.ptr)) - decls := make([]C.PantoToolDecl, len(src.Tools)) - frees := make([]func(), 0, len(src.Tools)) - for i, d := range src.Tools { - cd, fr := cToolDecl(d) - decls[i] = cd - frees = append(frees, fr) - } - defer func() { - for _, f := range frees { - f() - } - }() - var ptr *C.PantoToolDecl - if len(decls) > 0 { - ptr = &decls[0] - } - if C.panto_agent_register_tool_source(a.ptr, cname, ptr, C.size_t(len(decls)), ctx, (C.PantoToolSourceInvokeBatchFn)(C.pantoGoToolSourceInvokeBatch), (C.PantoToolDestroyFn)(C.pantoGoCallbackDestroy)) != C.PANTO_OK { - dropCallback(ctx) - return lastError() - } - return nil -} - func cToolDecl(d ToolDecl) (C.PantoToolDecl, func()) { n := cSlice(d.Name) desc := cSlice(d.Description) diff --git a/src/tui_engine.zig b/src/tui_engine.zig index f63e269..4924347 100644 --- a/src/tui_engine.zig +++ b/src/tui_engine.zig @@ -398,10 +398,14 @@ pub const Engine = struct { old[old.len - 1 - suf].comp.ptr == comps[comps.len - 1 - suf].ptr) suf += 1; // OLD slots in [pre, old.len - suf) are replaced/removed; NEW comps in - // [pre, comps.len - suf) are inserted. A pure insertion replaces zero - // old slots; anything else is a structural change. + // [pre, comps.len - suf) are inserted. const old_changed = old.len - suf - pre; // old slots dropped - const structural_change = old_changed != 0; + const new_inserted = comps.len - suf - pre; // new slots added + // Only a NET REMOVAL forces a full redraw. A pure insertion or a 1:1 + // in-place replacement (override swap) stays differential: the + // replaced/inserted slot first-renders from an empty baseline, so the + // cut lands at THAT slot's offset (clear_to_end clears any orphaned tail). + const structural_change = new_inserted < old_changed; // Detach the preserved suffix slots (with their baselines) before we // mutate the middle, then re-attach them after the inserted slots. @@ -1499,7 +1503,15 @@ test "height change forces a full redraw" { try testing.expect(std.mem.indexOf(u8, out, terminal.seq.full_clear) != null); } -test "change above viewport_top forces a full redraw" { +test "change above viewport_top forces a full redraw (keeps scrollback honest)" { + // A change to a line that has scrolled into native scrollback (above + // viewport_top) cannot be patched in place — the cursor can't reach it. + // The ONLY way to make scrollback reflect the change is a clearing full + // redraw (\x1b[3J wipes history) + reprint from the top. We accept that + // cost to keep scrollback honest; the differential path can't address + // those lines. (Tool INPUT streaming does not hit this: the tool is small + // and in-view while args stream — it has no output yet — so the changing + // header stays at/below viewport_top.) var buf = std.Io.Writer.Allocating.init(testing.allocator); defer buf.deinit(); // Short viewport so most content scrolls into native scrollback. @@ -1595,7 +1607,13 @@ test "syncComponents: removal in the middle forces a full redraw (structural cha try testing.expect(std.mem.indexOf(u8, out, "b") == null); } -test "syncComponents: replacing a slot (override swap) resets only that baseline" { +test "syncComponents: replacing a slot (override swap) stays differential, no reprint-from-top" { + // A 1:1 in-place swap (e.g. a tool-renderer override installed on a + // `tool_delta`) must NOT force a full reprint. The replaced slot + // first-renders from an empty baseline, so the differential cut lands at + // THAT slot's offset and only the slot (and anything below it) repaints. + // Forcing a full redraw here reprinted the whole transcript from line 0 on + // every delta — the streaming-tool flicker. var buf = std.Io.Writer.Allocating.init(testing.allocator); defer buf.deinit(); var eng = makeEngine(&buf, 80, 100); @@ -1607,15 +1625,54 @@ test "syncComponents: replacing a slot (override swap) resets only that baseline try eng.syncComponents(&.{ a.comp(), b.comp() }); try eng.render(); - // Swap b -> b2 at the same position: structural change (pointer differs). + // Swap b -> b2 at the same position (pointer differs, count unchanged). buf.clearRetainingCapacity(); try eng.syncComponents(&.{ a.comp(), b2.comp() }); - try testing.expect(eng.force_full); + try testing.expect(!eng.force_full); // NOT a full redraw try eng.render(); const out = buf.written(); try testing.expect(std.mem.indexOf(u8, out, "b2") != null); } +test "syncComponents: override swap with scrolled content does not reprint from the top (tool-stream flicker)" { + // The real streaming-tool flicker. A tool-renderer override is re-installed + // on every `tool_delta`; the Lua bridge mints a fresh component per + // `set_component`, so the App swaps the slot every delta. With earlier + // content scrolled into native scrollback (viewport_top > 0), forcing a + // full redraw reprinted the ENTIRE conversation from line 0 each delta — + // the visible flash. The swap must stay differential and never re-emit the + // scrolled-away prefix. + var buf = std.Io.Writer.Allocating.init(testing.allocator); + defer buf.deinit(); + // Short viewport so the header scrolls off: total 9 lines, height 8 => + // viewport_top 1. The swapped tool slot stays in view (offset 6 >= 1). + var eng = makeEngine(&buf, 80, 8); + defer eng.deinit(); + + // [tall header h0..h5][tool][input][footer]; header h0 scrolls off the top. + var header = FakeComponent{ .scripts = &.{&.{ "h0", "h1", "h2", "h3", "h4", "h5" }}, .first_changed = &.{0} }; + var tool = FakeComponent{ .scripts = &.{&.{"tool_v0"}}, .first_changed = &.{0} }; + var tool2 = FakeComponent{ .scripts = &.{&.{"tool_v1"}}, .first_changed = &.{0} }; + var input = FakeComponent{ .scripts = &.{&.{"input"}}, .first_changed = &.{0} }; + var footer = FakeComponent{ .scripts = &.{&.{"f0"}}, .first_changed = &.{0} }; + + try eng.syncComponents(&.{ header.comp(), tool.comp(), input.comp(), footer.comp() }); + try eng.render(); + try testing.expect(eng.viewport_top > 0); + + // Swap the tool component (the per-delta override re-install). + buf.clearRetainingCapacity(); + try eng.syncComponents(&.{ header.comp(), tool2.comp(), input.comp(), footer.comp() }); + try eng.render(); + + const out = buf.written(); + // No screen clear and — the flicker signature — no reprint from line 0: + // the scrolled-away header top is never re-emitted. + try testing.expect(std.mem.indexOf(u8, out, terminal.seq.full_clear) == null); + try testing.expect(std.mem.indexOf(u8, out, "h0") == null); + try testing.expect(std.mem.indexOf(u8, out, "tool_v1") != null); // the swap shows +} + test "width overflow is a hard error" { var buf = std.Io.Writer.Allocating.init(testing.allocator); defer buf.deinit(); -- cgit v1.3