From 805e685ecf60c5c6bbcdc13ba4efbf0eeeedb357 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 11 Sep 2026 22:32:44 +0200 Subject: [PATCH 1/2] fix(tui): keep the plan strip live through transient errors and dead confirms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects froze the plan progress strip at a stale count (the reported plan 0/6 forever). First, any fetch error — a timeout under load, a 5xx, a transport blip — marked the plan endpoint unavailable forever and the tick chain dropped every future poll; only a 404 (old engine, route absent) is permanent now, carried by a new ErrPlanUnavailable sentinel the client wraps (ErrJobsUnavailable pattern). Second, the live tick refused to fetch while planDirty, waiting for the in-flight tool_result confirm — but that reply could already be dead (superseded by a poll that bumped planReqSeq), deadlocking the strip at the optimistic patch; the tick now re-issues an idempotent confirm fetch alongside re-arming. Existing tests that pinned the over-eager classification and the wait-forever behavior are updated to the corrected contract. --- internal/client/plan.go | 10 ++++ internal/client/plan_test.go | 3 ++ internal/tui/model.go | 11 ++-- internal/tui/plan.go | 24 +++++++-- internal/tui/plan_realtime_test.go | 82 ++++++++++++++++++++++++++++++ internal/tui/plan_tab_test.go | 4 +- internal/tui/plan_test.go | 14 +++-- 7 files changed, 132 insertions(+), 16 deletions(-) create mode 100644 internal/tui/plan_realtime_test.go diff --git a/internal/client/plan.go b/internal/client/plan.go index 27376fa..e1ef858 100644 --- a/internal/client/plan.go +++ b/internal/client/plan.go @@ -2,6 +2,7 @@ package client import ( "encoding/json" + "errors" "fmt" "net/http" "net/url" @@ -41,6 +42,12 @@ type PlanSnapshot struct { Steps []PlanStep `json:"steps"` } +// ErrPlanUnavailable marks a 404 from the plan route — the engine is too +// old to serve it. Permanent for this process; every other error (timeout, +// 5xx, transport blip) is transient and must not stop the client's poll +// chain (ErrJobsUnavailable pattern). +var ErrPlanUnavailable = errors.New("session plan route unavailable") + // SessionPlan fetches the structured plan of a session. The token is the // session-scoped auth token (same as cancel/resume); rate limiting and auth // match every sibling session endpoint. @@ -51,6 +58,9 @@ func (c *Client) SessionPlan(sessionID, sessionToken string) (PlanSnapshot, erro return PlanSnapshot{}, err } defer func() { _ = resp.Body.Close() }() + if resp.StatusCode == http.StatusNotFound { + return PlanSnapshot{}, fmt.Errorf("%w: status %s", ErrPlanUnavailable, resp.Status) + } if resp.StatusCode != http.StatusOK { return PlanSnapshot{}, fmt.Errorf("session plan: status %s", resp.Status) } diff --git a/internal/client/plan_test.go b/internal/client/plan_test.go index 32fe5de..391b714 100644 --- a/internal/client/plan_test.go +++ b/internal/client/plan_test.go @@ -1,6 +1,7 @@ package client import ( + "errors" "net/http" "strings" "testing" @@ -70,6 +71,8 @@ func TestSessionPlan_HTTP404(t *testing.T) { t.Fatal("expected error on 404") } else if !strings.Contains(err.Error(), "404") { t.Errorf("error should mention status, got: %v", err) + } else if !errors.Is(err, ErrPlanUnavailable) { + t.Errorf("404 must wrap ErrPlanUnavailable (permanent, stops the poll chain), got: %v", err) } } diff --git a/internal/tui/model.go b/internal/tui/model.go index 32f122f..63d82b9 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -275,11 +275,12 @@ type Model struct { confirm confirmKind // armed destructive action: y fires, any other key disarms stopTarget string // task_id armed by confirmStopAgent - agentsReg []client.SubagentEntry // agents tab: sub-agent registry snapshot - agentsSeq int // agents-tab poll generation; stale ticks drop - eventsTabSeq int // events-tab poll generation; stale ticks drop - kickAgents bool // pending agents-tab refresh (flushKicks) - kickMemory bool // pending memory-tab refresh (flushKicks) + agentsReg []client.SubagentEntry // agents tab: sub-agent registry snapshot + agentsSeq int // agents-tab poll generation; stale ticks drop + eventsTabSeq int // events-tab poll generation; stale ticks drop + kickAgents bool // pending agents-tab refresh (flushKicks) + kickMemory bool // pending memory-tab refresh (flushKicks) + planConfirmArmed bool // newest plan fetch is a tool_result confirm // Background jobs tab + lifecycle watcher (odek v1.38+ /api/jobs — the // engine pushes nothing for job lifecycle, so bodek watches REST). diff --git a/internal/tui/plan.go b/internal/tui/plan.go index 2586e65..f673aca 100644 --- a/internal/tui/plan.go +++ b/internal/tui/plan.go @@ -2,6 +2,7 @@ package tui import ( "encoding/json" + "errors" "fmt" "strings" "time" @@ -96,6 +97,7 @@ func (m *Model) issuePlanFetch(confirm bool) tea.Cmd { return nil } m.planReqSeq++ + m.planConfirmArmed = confirm // test-visible: the newest fetch's kind cl := m.cl want := m.sessionID token := m.authToken @@ -117,8 +119,17 @@ func (m *Model) handlePlanMsg(msg planMsg) (cmd tea.Cmd) { } }() if msg.err != nil { - m.planAvail = planUnavailable - if m.panel == panelPlan { + // Only a 404 (old engine, route absent) is permanent. A transient + // error — timeout under load, a 5xx, a transport blip — must not + // kill the poll chain: the defer below re-arms and the next tick + // retries. Freezing the strip on one slow GET was the "plan 0/6 + // forever" bug. + if errors.Is(msg.err, client.ErrPlanUnavailable) { + m.planAvail = planUnavailable + if m.panel == panelPlan { + m.syncPlanPanelMsg() + } + } else if m.panel == panelPlan { m.syncPlanPanelMsg() } return nil @@ -177,8 +188,13 @@ func (m *Model) handlePlanTick(msg planTickMsg) tea.Cmd { return nil } if m.planDirty { - // Stay armed; do not bump planReqSeq over the in-flight confirm. - return m.armPlanPoll() + // The confirm fetch is the only reply that can clear a dirty + // strip — and it can die in flight (superseded by a poll/kick that + // bumped planReqSeq). Waiting forever for a dead reply froze the + // strip at the optimistic patch. Re-issue instead: the fetch is an + // idempotent GET, confirm=true keeps it eligible to clear the dirty + // flag, and the newest reply wins. + return tea.Batch(m.fetchPlanConfirm(), m.armPlanPoll()) } return m.fetchPlan() } diff --git a/internal/tui/plan_realtime_test.go b/internal/tui/plan_realtime_test.go new file mode 100644 index 0000000..418098c --- /dev/null +++ b/internal/tui/plan_realtime_test.go @@ -0,0 +1,82 @@ +package tui + +import ( + "errors" + "net/http" + "strings" + "testing" + + "github.com/BackendStack21/bodek/internal/client" +) + +// ── plan realtime: transient errors must not kill the poll chain, and a +// superseded confirm must be re-issued instead of deadlocking planDirty. + +// TestPlanTransientErrorKeepsChain: a generic fetch error (timeout, 500, +// network blip) must NOT mark the endpoint unavailable — the next tick +// still polls. Only a 404 (old engine, route absent) is permanent. +func TestPlanTransientErrorKeepsChain(t *testing.T) { + m := newTestModel() + m.cl = &client.Client{} // cmd construction only; the fetch never runs + m.busy = true + m.sessionID = "s1" + m.planAvail = planAvailable + m.handlePlanMsg(planMsg{want: "s1", seq: m.planReqSeq, err: errors.New("timeout")}) + if m.planAvail == planUnavailable { + t.Fatal("transient error killed the poll chain (plan unavailable)") + } + // The chain re-arms and the next tick still fetches. + seq := m.planPollSeq + if cmd := m.handlePlanTick(planTickMsg{seq: seq}); cmd == nil { + t.Fatal("tick after a transient error must keep polling") + } + // A 404-class error IS permanent — old engine, route absent. + m.handlePlanMsg(planMsg{want: "s1", seq: m.planReqSeq, err: client.ErrPlanUnavailable}) + if m.planAvail != planUnavailable { + t.Fatal("404 must mark the endpoint unavailable") + } + if cmd := m.handlePlanTick(planTickMsg{seq: m.planPollSeq}); cmd != nil { + t.Fatal("tick with an unavailable endpoint must stop the chain") + } +} + +// TestPlanDirtyTickReissuesConfirm: when a confirm fetch was superseded (the +// only thing that can clear planDirty died in flight), the live tick must +// re-issue a confirm instead of arming forever — otherwise the strip stays +// frozen at the optimistic patch (plan 0/N). +func TestPlanDirtyTickReissuesConfirm(t *testing.T) { + m := newTestModel() + m.busy = true + m.sessionID = "s1" + m.cl = &client.Client{} // cmd construction only; the fetch never runs + m.planInit = true + m.planAvail = planAvailable + m.plan.Steps = []client.PlanStep{{ID: "a", Status: client.PlanDone}, {ID: "b"}} + m.planDirty = true + cmd := m.handlePlanTick(planTickMsg{seq: m.planPollSeq}) + if cmd == nil { + t.Fatal("dirty tick must re-issue the confirm fetch") + } + // The re-issue is observable: planReqSeq advances (a fresh fetch armed). + if m.planReqSeq == 0 { + t.Fatal("dirty tick did not arm any fetch") + } + // And a landed confirm clears the dirty flag (existing accept path). + m.handlePlanMsg(planMsg{want: "s1", seq: m.planReqSeq, confirm: true, + snap: client.PlanSnapshot{SessionID: "s1", Version: 1, Steps: m.plan.Steps}}) + if m.planDirty { + t.Fatal("confirm reply did not clear planDirty") + } +} + +// TestSessionPlanErrorClassification: the client marks 404 as the permanent +// sentinel; other statuses stay ordinary errors. +func TestSessionPlanErrorClassification(t *testing.T) { + // Covered via the tui-facing sentinel contract in TestPlanTransientErrorKeepsChain; + // the client-side mux test lives in client/plan_test.go. Here we pin the + // sentinel exists and wraps. + if !strings.Contains(client.ErrPlanUnavailable.Error(), "plan") { + t.Fatalf("sentinel missing or misnamed: %v", client.ErrPlanUnavailable) + } + _ = http.StatusNotFound +} diff --git a/internal/tui/plan_tab_test.go b/internal/tui/plan_tab_test.go index 9dae1fd..485fda5 100644 --- a/internal/tui/plan_tab_test.go +++ b/internal/tui/plan_tab_test.go @@ -1,7 +1,7 @@ package tui import ( - "errors" + "fmt" "strings" "testing" @@ -14,7 +14,7 @@ import ( // house grammar (⏎ expand / esc fold), and the silent-degrade empty states. // Snapshots are injected through handlePlanMsg so no server is involved. -var errPlanRoute = errors.New("session plan: status 404 Not Found") +var errPlanRoute = fmt.Errorf("%w: status 404 Not Found", client.ErrPlanUnavailable) func planFixture() client.PlanSnapshot { return client.PlanSnapshot{ diff --git a/internal/tui/plan_test.go b/internal/tui/plan_test.go index 63ad2f0..95fe74b 100644 --- a/internal/tui/plan_test.go +++ b/internal/tui/plan_test.go @@ -1,7 +1,7 @@ package tui import ( - "errors" + "fmt" "strings" "testing" @@ -15,7 +15,7 @@ import ( // closures are never executed — the wire contract lives in internal/client, // and invoking arbitrary batch children (listen…) would block a test. -var errTestPlanRoute = errors.New("session plan: status 404 Not Found") +var errTestPlanRoute = fmt.Errorf("%w: status 404 Not Found", client.ErrPlanUnavailable) func planCallEvent(name string) client.Event { return client.Event{Type: "tool_call", Name: name, Data: "{}"} @@ -383,13 +383,17 @@ func TestPlanPoll_SkipsFetchWhileDirty(t *testing.T) { if c := m.armPlanPoll(); c == nil { t.Fatal("busy run must arm the live poll") } - req := m.planReqSeq c := m.handlePlanTick(planTickMsg{seq: m.planPollSeq}) if c == nil { t.Fatal("dirty tick must re-arm rather than drop the chain") } - if m.planReqSeq != req { - t.Fatal("dirty tick must not issue a fetch (would supersede confirm)") + // A dirty tick re-issues a CONFIRM fetch (not a poll): the in-flight + // confirm may be dead (superseded by a poll that bumped planReqSeq), + // and waiting for it forever froze the strip at the optimistic patch. + // The newest confirm reply is what clears planDirty — see + // TestPlanDirtyTickReissuesConfirm in plan_realtime_test.go. + if !m.planConfirmArmed { + t.Fatal("dirty tick must re-issue the confirm fetch") } } From 1029603bf6c46d00612483397c002ec32fbbe51d Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 11 Sep 2026 22:41:36 +0200 Subject: [PATCH 2/2] =?UTF-8?q?feat(tui):=20simplify=20the=20top=20bar=20?= =?UTF-8?q?=E2=80=94=20plan,=20odek=20version,=20and=20tok/s=20move=20out?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The header drops three indicators whose data already lives elsewhere: the plan x/t count (the /plan tab owns plan progress, and the busy-line strip still shows it while a turn runs), the odek engine version (moved into the cockpit stats sheet as an engine row — rendered once, before the first turn, with the server card's duplicate row removed), and the live tok/s chip (the cockpit's speed row carries last/mean/peak for sealed turns and a live row while a turn streams; the sealed turn footer keeps the per-turn rate). bodek's own version stays beside the logo and the jobs instrument stays — a running or failed job is actionable from any surface. README and AGENTS.md header-contract paragraphs updated to the new shape. Includes adversarial-review hardenings from all three review rounds. --- AGENTS.md | 10 +-- README.md | 11 +-- internal/tui/chrome.go | 23 +----- internal/tui/chrome_test.go | 20 ++--- internal/tui/cockpit.go | 11 +-- internal/tui/commands.go | 14 ++++ internal/tui/header_simplify_test.go | 61 +++++++++++++++ internal/tui/model.go | 14 ++-- internal/tui/plan.go | 28 +++++-- internal/tui/plan_realtime_test.go | 1 + internal/tui/plan_test.go | 21 +++-- internal/tui/review3_closures_test.go | 106 ++++++++++++++++++++++++++ internal/tui/stats_test.go | 15 ++-- internal/tui/update_test.go | 13 ++-- internal/tui/view.go | 7 -- 15 files changed, 269 insertions(+), 86 deletions(-) create mode 100644 internal/tui/header_simplify_test.go create mode 100644 internal/tui/review3_closures_test.go diff --git a/AGENTS.md b/AGENTS.md index 7ef0d51..6b7681f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -141,11 +141,11 @@ feat(tui): compact tool steps with Ctrl+E details toggle Absent/zero `windowTokens` holds the last fill. The bar saturates at full; the percent stays honest when `winCtxTok` exceeds `maxContext` (stale catalog / last-resort max) — never a contradictory `100%` - beside a larger used count. Live tok/s rides the - header from this-call `usage`/`done` fields (`generationTokensPerSecond` - preferred, else `tokensPerSecond`). Absent/zero holds the last rate; a - new turn clears the chip. Never divide cumulative `outputTokens` by - wall latency. + beside a larger used count. Live tok/s is tracked from this-call + `usage`/`done` fields (`generationTokensPerSecond` preferred, else + `tokensPerSecond`) and renders in the cockpit stats sheet — the header + carries no rate chip. Absent/zero holds the last rate; a new turn + clears it. Never divide cumulative `outputTokens` by wall latency. - `internal/tui` is split by responsibility: `model.go` holds the core model, `inspect.go` owns transcript item focus and bounded tool paging, `events.go` event handling, `input.go` key/text input, `input_reassembler.go` the raw terminal input stream, `approval.go` diff --git a/README.md b/README.md index 8b6f64a..a8cabc1 100644 --- a/README.md +++ b/README.md @@ -322,19 +322,20 @@ own front-end settings are separate; see [Configuration](#configuration). session spend or sub-agent tokens. The bar saturates at full; if the observed window exceeds the advertised model limit the percent goes over 100% instead of a contradictory `100%` next to a larger used - count. Live `plan N/M` and `▶ N jobs` / - `✗ job` instruments ride the same bar when a plan or background job is - active. + count. `▶ N jobs` / `✗ job` instruments ride the same bar when a + background job is active; plan progress rides the busy line while a + turn runs and the `/plan` tab otherwise. - **Per-turn footers & `/stats`** — token counts and latency ride every turn head (`⚡` latency, `⌂` context, `↳` output tokens, `↗` tok/s, `⚒` tools); `/stats` opens a sheet that rolls up the session (speed, TTFT, LLM time, cost, cache, context). The `⎇` glyph is reserved for git commits in the transcript. -- **Generation speed** — live `↗ tok/s` in the header from `usage` frames +- **Generation speed** — live `↗ tok/s` in the cockpit stats sheet + (`/server`) from `usage` frames (prefers `generationTokensPerSecond` when the stream measured TTFT; otherwise end-to-end `tokensPerSecond`). The same rate seals onto the turn footer after `done`. Missing/zero rates are held, never invented - from cumulative output ÷ wall latency; a new turn clears the chip. + from cumulative output ÷ wall latency; a new turn clears it. - **Cost tracking** — when odek has token prices configured, the header shows the running session spend, each turn footer its estimated cost, and `/stats` adds the `max_cost_usd` cap when set; hidden entirely otherwise. diff --git a/internal/tui/chrome.go b/internal/tui/chrome.go index 8ab310f..7cdecb5 100644 --- a/internal/tui/chrome.go +++ b/internal/tui/chrome.go @@ -147,23 +147,6 @@ func (m *Model) shelfView() string { // ── header instruments ────────────────────────────────────────────────────── -func (m *Model) headerPlanLabel() string { - if !m.planInit || m.planAvail != planAvailable || !m.plan.Found { - return "" - } - total := len(m.plan.Steps) - if total == 0 { - return "" - } - done := 0 - for _, st := range m.plan.Steps { - if st.Status == client.PlanDone { - done++ - } - } - return fmt.Sprintf("plan %d/%d", done, total) -} - func (m *Model) headerJobsLabel() string { n := 0 failed := false @@ -184,11 +167,11 @@ func (m *Model) headerJobsLabel() string { return "" } +// headerInstruments renders the header's status strip. Plan progress no +// longer rides here — the /plan tab owns it — and jobs remain: a running +// or failed job is actionable from any surface. func (m *Model) headerInstruments() string { var parts []string - if s := m.headerPlanLabel(); s != "" { - parts = append(parts, s) - } if s := m.headerJobsLabel(); s != "" { parts = append(parts, s) } diff --git a/internal/tui/chrome_test.go b/internal/tui/chrome_test.go index cdf2805..4f51795 100644 --- a/internal/tui/chrome_test.go +++ b/internal/tui/chrome_test.go @@ -97,22 +97,24 @@ func TestApprovalKeepsComposer(t *testing.T) { func TestHeaderInstruments(t *testing.T) { m := newTestModel() + m.jobs = []client.Job{{Status: "running"}, {Status: "running"}} + if got := m.headerJobsLabel(); got != "▶ 2 jobs" { + t.Errorf("jobs label = %q, want ▶ 2 jobs", got) + } + // Plan progress no longer rides the header (the /plan tab owns it) — + // even with an accepted, progressing plan on the model. m.planInit = true m.planAvail = planAvailable m.plan = client.PlanSnapshot{Found: true, Steps: []client.PlanStep{ {ID: "a", Status: client.PlanDone}, {ID: "b", Status: client.PlanPending}, }} - m.jobs = []client.Job{{Status: "running"}, {Status: "running"}} - if got := m.headerPlanLabel(); got != "plan 1/2" { - t.Errorf("plan label = %q, want plan 1/2", got) - } - if got := m.headerJobsLabel(); got != "▶ 2 jobs" { - t.Errorf("jobs label = %q, want ▶ 2 jobs", got) - } head := plain(m.header()) - if !strings.Contains(head, "plan 1/2") || !strings.Contains(head, "2 jobs") { - t.Errorf("header missing instruments:\n%s", head) + if strings.Contains(head, "plan 1/2") { + t.Errorf("plan label must not render in the header:\n%s", head) + } + if !strings.Contains(head, "2 jobs") { + t.Errorf("header missing jobs instrument:\n%s", head) } m.jobs = []client.Job{{Status: "failed"}} diff --git a/internal/tui/cockpit.go b/internal/tui/cockpit.go index a5d5e5a..402033b 100644 --- a/internal/tui/cockpit.go +++ b/internal/tui/cockpit.go @@ -108,8 +108,9 @@ func (m *Model) popoverView(w, h int) string { // cockpitServerSection is the server/link card: identity and liveness from // the server_info/pong snapshot plus the heartbeat round-trip. func (m *Model) cockpitServerSection() string { + // The engine version row lives in the session stats sheet below (⬢ engine) + // — rendering it here too duplicated it inside the same cockpit. rows := [][2]string{ - {"version", orDash(prefixVersion("odek ", m.odekVersion))}, {"model", orDash(m.model)}, {"stream", boolDash(m.serverStream, "⚡ live deltas", "buffered")}, {"sandbox", boolDash(m.sandbox, "isolated", "host access")}, @@ -197,14 +198,6 @@ func (m *Model) cockpitRows(title string, rows [][2]string) string { return b.String() } -// prefixVersion prepends a label when v is non-empty. -func prefixVersion(label, v string) string { - if v == "" { - return "" - } - return label + v -} - // boolDash renders a yes/no value with distinct labels per state. func boolDash(v bool, yes, no string) string { if v { diff --git a/internal/tui/commands.go b/internal/tui/commands.go index 0c3b2c4..35fc091 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -539,6 +539,20 @@ func (m *Model) statsBody() string { } } + // The engine version lives here now — the header dropped its odek + // segment — and it renders even before the first turn: knowing the + // engine build is a fresh-attach question, not a session-rollup one. + if m.odekVersion != "" { + rows = append(rows, row{"⬢", th.statsLabel, "engine", th.statsValue.Render(m.odekVersion)}) + } + // Live rate while a turn streams: the sealed rows below only exist + // after done, but the cockpit is the rate's home now — the header + // dropped its chip, so an open sheet must not go blind mid-turn. + if m.busy && m.tokPerSec > 0 { + live := th.statsValue.Render(formatTokPerSec(m.tokPerSec)) + th.statsDim.Render(" · live") + rows = append(rows, row{"↗", th.statTime, "speed", live}) + } + // Align values into a column just past the widest label. gutter := 0 for _, r := range rows { diff --git a/internal/tui/header_simplify_test.go b/internal/tui/header_simplify_test.go new file mode 100644 index 0000000..384f3d8 --- /dev/null +++ b/internal/tui/header_simplify_test.go @@ -0,0 +1,61 @@ +package tui + +import ( + "strings" + "testing" + + "github.com/BackendStack21/bodek/internal/client" +) + +// ── header simplification: plan x/t, odek version, and tok/s leave the top +// bar. Plan progress lives in the /plan tab, tok/s and the context gauge in +// the cockpit; the engine version moves INTO the cockpit so it stays reachable. + +func headerModel(t *testing.T) *Model { + t.Helper() + m := newTestModel() + m.odekVersion = "v1.40.2" + m.bodekVersion = "v1.11.10" + // A plan with progress would previously paint "plan 1/2" in the header. + m.planInit = true + m.planAvail = planAvailable + m.plan.Found = true + m.plan.Steps = []client.PlanStep{ + {ID: "a", Status: client.PlanDone}, + {ID: "b", Status: client.PlanPending}, + } + // A live rate would previously paint "↗ x tok/s". + m.tokPerSec = 42.5 + return m +} + +func TestHeaderDropsPlanVersionRate(t *testing.T) { + m := headerModel(t) + head := plain(m.header()) + for _, banned := range []string{"plan 1/2", "odek v1.40.2", "tok/s", "↗"} { + if strings.Contains(head, banned) { + t.Errorf("header still shows %q:\n%s", banned, head) + } + } + // Kept: bodek's own version rides the logo; instruments strip keeps jobs. + if !strings.Contains(head, "v1.11.10") { + t.Errorf("bodek version missing from header:\n%s", head) + } +} + +func TestCockpitShowsEngineVersion(t *testing.T) { + m := headerModel(t) + m.popover = true // cockpit popover embeds the stats sheet + v := plain(m.View()) + if !strings.Contains(v, "v1.40.2") { + t.Errorf("cockpit missing the engine version row:\n%s", firstLines(v, 22)) + } +} + +func firstLines(s string, n int) string { + parts := strings.Split(s, "\n") + if len(parts) > n { + parts = parts[:n] + } + return strings.Join(parts, "\n") +} diff --git a/internal/tui/model.go b/internal/tui/model.go index 63d82b9..e2feafc 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -251,7 +251,7 @@ type Model struct { thinking string // canonical: "" inherit, or disabled|low|medium|high expandAll bool // Ctrl+E: render every step's full output/logs - odekVersion string // engine version shown in the header ("" hides it) + odekVersion string // engine version, shown in the cockpit stats sheet ("" hides it) bodekVersion string // bodek's own version, for the startup update check panel panelMode @@ -275,12 +275,12 @@ type Model struct { confirm confirmKind // armed destructive action: y fires, any other key disarms stopTarget string // task_id armed by confirmStopAgent - agentsReg []client.SubagentEntry // agents tab: sub-agent registry snapshot - agentsSeq int // agents-tab poll generation; stale ticks drop - eventsTabSeq int // events-tab poll generation; stale ticks drop - kickAgents bool // pending agents-tab refresh (flushKicks) - kickMemory bool // pending memory-tab refresh (flushKicks) - planConfirmArmed bool // newest plan fetch is a tool_result confirm + agentsReg []client.SubagentEntry // agents tab: sub-agent registry snapshot + agentsSeq int // agents-tab poll generation; stale ticks drop + eventsTabSeq int // events-tab poll generation; stale ticks drop + kickAgents bool // pending agents-tab refresh (flushKicks) + kickMemory bool // pending memory-tab refresh (flushKicks) + planConfirmIssued bool // tool_result debounce fired a confirm fetch // Background jobs tab + lifecycle watcher (odek v1.38+ /api/jobs — the // engine pushes nothing for job lifecycle, so bodek watches REST). diff --git a/internal/tui/plan.go b/internal/tui/plan.go index f673aca..3ff9460 100644 --- a/internal/tui/plan.go +++ b/internal/tui/plan.go @@ -97,7 +97,9 @@ func (m *Model) issuePlanFetch(confirm bool) tea.Cmd { return nil } m.planReqSeq++ - m.planConfirmArmed = confirm // test-visible: the newest fetch's kind + if confirm { + m.planConfirmIssued = true // the tool_result debounce fired + } cl := m.cl want := m.sessionID token := m.authToken @@ -156,6 +158,7 @@ func (m *Model) handlePlanMsg(msg planMsg) (cmd tea.Cmd) { m.planVer = msg.snap.Version m.planInit = true m.planDirty = false + m.planConfirmIssued = false if m.panel == panelPlan { m.syncPlanPanelMsg() } @@ -191,10 +194,16 @@ func (m *Model) handlePlanTick(msg planTickMsg) tea.Cmd { // The confirm fetch is the only reply that can clear a dirty // strip — and it can die in flight (superseded by a poll/kick that // bumped planReqSeq). Waiting forever for a dead reply froze the - // strip at the optimistic patch. Re-issue instead: the fetch is an - // idempotent GET, confirm=true keeps it eligible to clear the dirty - // flag, and the newest reply wins. - return tea.Batch(m.fetchPlanConfirm(), m.armPlanPoll()) + // strip at the optimistic patch. BUT a confirm is only legitimate + // AFTER the tool_result debounce fired: re-issuing before that + // would fetch the PRE-write store (create leaves planVer at 0, so + // the monotonic guard cannot reject it) and wipe the optimistic + // steps. So: re-issue only once a confirm was issued; until then + // the debounce owns the fetch and the tick stays armed. + if m.planConfirmIssued { + return tea.Batch(m.fetchPlanConfirm(), m.armPlanPoll()) + } + return m.armPlanPoll() } return m.fetchPlan() } @@ -307,6 +316,7 @@ func (m *Model) resetPlanState() { m.planVer, m.planInit = 0, false m.planAvail = planUnknown m.planDirty = false + m.planConfirmIssued = false m.planLiveKick = false m.planDebSeq++ m.planReqSeq++ @@ -479,6 +489,14 @@ func (m *Model) planFollowup() tea.Cmd { if c := m.kickPlanLive(); c != nil { cmds = append(cmds, c) } + // A dirty strip whose confirm died in flight (run went idle before + // the reply landed) would stay frozen until the first busy tick. + // Re-issue immediately at the turn boundary — only after the + // debounce fired (planConfirmIssued), so a pre-write fetch can + // never wipe the optimistic patch. + if m.planDirty && m.planConfirmIssued { + cmds = append(cmds, m.fetchPlanConfirm()) + } } switch len(cmds) { case 0: diff --git a/internal/tui/plan_realtime_test.go b/internal/tui/plan_realtime_test.go index 418098c..b03ba11 100644 --- a/internal/tui/plan_realtime_test.go +++ b/internal/tui/plan_realtime_test.go @@ -53,6 +53,7 @@ func TestPlanDirtyTickReissuesConfirm(t *testing.T) { m.planAvail = planAvailable m.plan.Steps = []client.PlanStep{{ID: "a", Status: client.PlanDone}, {ID: "b"}} m.planDirty = true + m.planConfirmIssued = true // the tool_result debounce fired; its reply died in flight cmd := m.handlePlanTick(planTickMsg{seq: m.planPollSeq}) if cmd == nil { t.Fatal("dirty tick must re-issue the confirm fetch") diff --git a/internal/tui/plan_test.go b/internal/tui/plan_test.go index 95fe74b..766ccef 100644 --- a/internal/tui/plan_test.go +++ b/internal/tui/plan_test.go @@ -387,13 +387,20 @@ func TestPlanPoll_SkipsFetchWhileDirty(t *testing.T) { if c == nil { t.Fatal("dirty tick must re-arm rather than drop the chain") } - // A dirty tick re-issues a CONFIRM fetch (not a poll): the in-flight - // confirm may be dead (superseded by a poll that bumped planReqSeq), - // and waiting for it forever froze the strip at the optimistic patch. - // The newest confirm reply is what clears planDirty — see - // TestPlanDirtyTickReissuesConfirm in plan_realtime_test.go. - if !m.planConfirmArmed { - t.Fatal("dirty tick must re-issue the confirm fetch") + // A dirty tick re-issues a CONFIRM fetch only after the tool_result + // debounce fired one (planConfirmIssued): before that, a fetch would + // hit the PRE-write store and wipe the optimistic patch (create leaves + // planVer at 0, so the monotonic guard cannot reject it). Until the + // debounce fires, the tick stays armed and the debounce owns the fetch. + req := m.planReqSeq + m.handlePlanTick(planTickMsg{seq: m.planPollSeq}) + if m.planReqSeq != req { + t.Fatal("dirty tick before any confirm was issued must NOT fetch") + } + m.planConfirmIssued = true + m.handlePlanTick(planTickMsg{seq: m.planPollSeq}) + if m.planReqSeq == req { + t.Fatal("dirty tick after a dead confirm must re-issue the fetch") } } diff --git a/internal/tui/review3_closures_test.go b/internal/tui/review3_closures_test.go new file mode 100644 index 0000000..0351975 --- /dev/null +++ b/internal/tui/review3_closures_test.go @@ -0,0 +1,106 @@ +package tui + +import ( + "strings" + "testing" + + "github.com/BackendStack21/bodek/internal/client" +) + +// ── review round-3 closures: cockpit engine row renders exactly once, live +// rate streams in the sheet, and the planConfirmIssued lifecycle is pinned. + +// TestCockpitEngineVersionOnce: the engine version renders exactly once in +// the cockpit (server card dropped its row; the stats sheet owns it). +func TestCockpitEngineVersionOnce(t *testing.T) { + m := headerModel(t) + m.popover = true + v := plain(m.View()) + if n := strings.Count(v, "v1.40.2"); n != 1 { + t.Fatalf("engine version rendered %d times, want 1:\n%s", n, firstLines(v, 20)) + } +} + +// TestCockpitLiveRateWhileStreaming: a busy turn's live rate paints the +// stats sheet mid-stream — the sheet is the rate's home now. +func TestCockpitLiveRateWhileStreaming(t *testing.T) { + m := headerModel(t) + m.busy = true + m.popover = true + v := plain(m.View()) + if !strings.Contains(v, "42.5 tok/s") || !strings.Contains(v, "live") { + t.Fatalf("stats sheet missing the live rate while streaming:\n%s", firstLines(v, 22)) + } + // Idle: no live row (sealed turn rows own the rate after done). + m.busy = false + v = plain(m.View()) + if strings.Contains(v, "· live") { + t.Fatalf("idle sheet must not show the live rate row:\n%s", firstLines(v, 22)) + } +} + +// TestPlanConfirmIssuedLifecycle: the debounce sets it, an accepted reply +// clears it, and reset drops it. +func TestPlanConfirmIssuedLifecycle(t *testing.T) { + m := newTestModel() + m.sessionID = "s1" + m.cl = &client.Client{} + m.planInit = true + m.planAvail = planAvailable + + // issuePlanFetch(confirm=true) sets the flag. + _ = m.fetchPlanConfirm() + if !m.planConfirmIssued { + t.Fatal("confirm fetch must set planConfirmIssued") + } + // A non-confirm fetch must not set it (it is never unset by polls). + m.planConfirmIssued = false + _ = m.fetchPlan() + if m.planConfirmIssued { + t.Fatal("a poll fetch must not set planConfirmIssued") + } + + // An accepted confirm reply clears the flag along with planDirty. + m.planConfirmIssued = true + m.planDirty = true + m.handlePlanMsg(planMsg{want: "s1", seq: m.planReqSeq, confirm: true, + snap: client.PlanSnapshot{SessionID: "s1", Version: 3, Found: true}}) + if m.planConfirmIssued || m.planDirty { + t.Fatalf("accept must clear both: issued=%v dirty=%v", m.planConfirmIssued, m.planDirty) + } + + // resetPlanState drops it too. + m.planConfirmIssued = true + m.resetPlanState() + if m.planConfirmIssued { + t.Fatal("reset must clear planConfirmIssued") + } +} + +// TestPlanFollowupReissuesDeadConfirm: a turn boundary with a dirty strip +// whose confirm died re-issues the fetch instead of waiting for a tick. +func TestPlanFollowupReissuesDeadConfirm(t *testing.T) { + m := newTestModel() + m.sessionID = "s1" + m.cl = &client.Client{} + m.planInit = true + m.planAvail = planAvailable + m.planDirty = true + m.planConfirmIssued = true // debounce fired; the reply died in flight + + m.planLiveKick = true + seq := m.planReqSeq + _ = m.planFollowup() + if m.planReqSeq == seq { + t.Fatal("turn boundary must re-issue the dead confirm") + } + // Without planConfirmIssued, no confirm re-issue (pre-write guard). + m.planConfirmIssued = false + m.planDirty = true + m.planLiveKick = true + seq = m.planReqSeq + _ = m.planFollowup() + if m.planReqSeq != seq { + t.Fatal("turn boundary must not fetch a pre-write confirm") + } +} diff --git a/internal/tui/stats_test.go b/internal/tui/stats_test.go index a34aa83..11b1a9c 100644 --- a/internal/tui/stats_test.go +++ b/internal/tui/stats_test.go @@ -619,8 +619,9 @@ func TestTurnStatLineShowsTokPerSec(t *testing.T) { if !strings.Contains(foot, "↗") || !strings.Contains(foot, "25.2 tok/s") { t.Errorf("turn foot missing generation tok/s: %q", foot) } - if !strings.Contains(plain(m.header()), "25.2 tok/s") { - t.Errorf("header missing live tok/s chip:\n%s", plain(m.header())) + // The header no longer carries tok/s — the cockpit owns the live rate. + if strings.Contains(plain(m.header()), "tok/s") { + t.Errorf("header must not show tok/s:\n%s", plain(m.header())) } } @@ -632,8 +633,10 @@ func TestChromeFooterOmitsTokPerSec(t *testing.T) { }) m.sendPrompt("next") m.handleEvent(client.Event{Type: "usage", TokensPerSecond: 9.6}) - if !strings.Contains(plain(m.header()), "9.6 tok/s") { - t.Errorf("header should show live in-flight rate:\n%s", plain(m.header())) + // The header no longer carries the in-flight rate — the cockpit owns it; + // the sealed turn foot keeps the last sealed rate. + if strings.Contains(plain(m.header()), "tok/s") { + t.Errorf("header must not show tok/s:\n%s", plain(m.header())) } foot := plain(m.footer()) if strings.Contains(foot, "tok/s") { @@ -658,8 +661,8 @@ func TestUsageAppliesLiveSpeed(t *testing.T) { if m.ttftMs != 420 || m.callDurMs != 8100 { t.Fatalf("live timing = ttft %d call %d", m.ttftMs, m.callDurMs) } - if out := plain(m.header()); !strings.Contains(out, "25.2 tok/s") { - t.Errorf("header chip missing mid-run tok/s:\n%s", out) + if out := plain(m.header()); strings.Contains(out, "25.2 tok/s") { + t.Errorf("header must not show tok/s: %q", out) } if m.msgs[0].stats != nil { t.Fatal("usage must not seal turn stats") diff --git a/internal/tui/update_test.go b/internal/tui/update_test.go index 1343a60..70e24d6 100644 --- a/internal/tui/update_test.go +++ b/internal/tui/update_test.go @@ -9,16 +9,17 @@ import ( // TestHeaderShowsOdekVersion verifies the engine version appears in the header // left cluster when known, and leaves no stray separator when it is not. func TestHeaderShowsOdekVersion(t *testing.T) { + // The header dropped its odek segment (simplification): the engine + // version now lives in the cockpit stats sheet. m := newTestModel() m.model = "deepseek-v4-flash" m.odekVersion = "v0.2.0" - if out := plain(m.header()); !strings.Contains(out, "odek v0.2.0") { - t.Errorf("header missing odek version: %q", out) + if out := plain(m.header()); strings.Contains(out, "odek v0.2.0") { + t.Errorf("header must not show the odek version: %q", out) } - - m.odekVersion = "" - if out := plain(m.header()); strings.Contains(out, "odek v") { - t.Errorf("header should hide the odek segment when unknown: %q", out) + m.popover = true + if out := plain(m.View()); !strings.Contains(out, "v0.2.0") { + t.Errorf("cockpit missing engine version: %q", out) } } diff --git a/internal/tui/view.go b/internal/tui/view.go index 19b74b8..2379b66 100644 --- a/internal/tui/view.go +++ b/internal/tui/view.go @@ -100,9 +100,6 @@ func (m *Model) header() string { if inst := m.headerInstruments(); inst != "" { tail += th.headerMeta.Render(" · ") + th.headerKey.Render(truncate(inst, 28)) } - if m.odekVersion != "" { - tail += th.headerMeta.Render(" · odek ") + th.headerKey.Render(m.odekVersion) - } // Sandbox status, prominently colored: green ● when isolated, amber ▲ // when the agent has host access. tail += th.headerMeta.Render(" · ") + m.sandboxBadge() @@ -112,10 +109,6 @@ func (m *Model) header() string { if inPrice, outPrice := m.prices(); inPrice > 0 && outPrice > 0 { tail += th.headerMeta.Render(" · ") + th.headerKey.Render(formatUSD(costUSD(m.sessCtxTok, m.sessOutTok, inPrice, outPrice)+m.subCostTotal())) } - if s := formatTokPerSec(m.tokPerSec); s != "" { - tail += th.headerMeta.Render(" · ") + th.headerKey.Render("↗ "+s) - } - status := m.statusBadge() // The gauge is the header's sole token metric — session totals live in // /stats and the per-turn stat line, so a fresh session never flashes