From 583a310a40fb441b623415935725aa71a904663f Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 18 Sep 2026 12:23:39 +0200 Subject: [PATCH 1/3] fix(client): silence errcheck on deferred socket close --- internal/client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/client/client.go b/internal/client/client.go index 03c6ee8..81413d6 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -315,7 +315,7 @@ var readIdleTimeout = 45 * time.Second func (c *Client) readLoop() { defer close(c.Events) - defer c.conn.Close() // release the fd even when the sender never closes (reconnect swap) + defer func() { _ = c.conn.Close() }() // release the fd even when the sender never closes (reconnect swap) var pending *Event n := 0 flush := func() { From a17914d223a6ed27ba60d33913967b41de5a8cbe Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 18 Sep 2026 13:18:07 +0200 Subject: [PATCH 2/3] fix: close 8 defects from bug-hunt sweep - workspace: per-call staged temp file fixes torn concurrent persist; corrupt workspaces.json now quarantined as .corrupt instead of a silent reset (mirrors tokens.go) - server: background reaper detects exited-but-unreaped children so a crashed odek serve fails fast instead of burning the ready timeout; waitSpawned errors carry a bounded stderr tail - client: bounded WS dial + handshake deadline (wsDialTimeout) - tui: ctrl+enter inserts a newline in the approval composer; approval expiry bell latches per queue entry; queue-strip click controls bounded to their own cells - update: typed status error replaces substring 401/403/429 matching so transport errors no longer trigger the HTML asset fallback --- internal/client/client.go | 43 +++++++++- internal/client/dial_timeout_test.go | 47 +++++++++++ internal/server/proc_liveness_test.go | 57 ++++++++++++++ internal/server/server.go | 89 +++++++++++++++++---- internal/tui/approval.go | 5 +- internal/tui/approval_expiry.go | 12 ++- internal/tui/events.go | 3 + internal/tui/fix_regress_test.go | 95 +++++++++++++++++++++++ internal/tui/model.go | 4 +- internal/tui/queue.go | 10 +-- internal/tui/transcript_additions_test.go | 10 +-- internal/update/transport_error_test.go | 63 +++++++++++++++ internal/update/update.go | 27 +++++-- internal/workspace/persist_fix_test.go | 59 ++++++++++++++ internal/workspace/workspace.go | 32 +++++++- 15 files changed, 512 insertions(+), 44 deletions(-) create mode 100644 internal/client/dial_timeout_test.go create mode 100644 internal/server/proc_liveness_test.go create mode 100644 internal/tui/fix_regress_test.go create mode 100644 internal/update/transport_error_test.go create mode 100644 internal/workspace/persist_fix_test.go diff --git a/internal/client/client.go b/internal/client/client.go index 81413d6..a33514b 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -10,8 +10,10 @@ package client import ( "encoding/json" "fmt" + "net" "net/http" "net/url" + "strings" "sync" "time" @@ -260,7 +262,7 @@ func Dial(wsURL, origin, baseURL, token string) (*Client, error) { } cfg.Header.Set("X-Odek-Ws-Token", token) - conn, err := ws.DialConfig(cfg) + conn, err := dialWS(cfg) if err != nil { return nil, fmt.Errorf("ws dial: %w", err) } @@ -284,6 +286,45 @@ func Dial(wsURL, origin, baseURL, token string) (*Client, error) { return c, nil } +// wsDialTimeout bounds the TCP dial and the WS handshake: ws.DialConfig has +// no timeout of its own, so a black-holed remote blocked Dial until the OS +// TCP timeout. +var wsDialTimeout = 10 * time.Second + +// dialWS dials the server under an explicit deadline and runs the WS +// handshake over the same (deadline-carrying) connection, then clears the +// deadline so the live stream is unbounded. +func dialWS(cfg *ws.Config) (*ws.Conn, error) { + raw, err := net.DialTimeout("tcp", hostPortAddr(cfg.Location), wsDialTimeout) + if err != nil { + return nil, err + } + if err := raw.SetDeadline(time.Now().Add(wsDialTimeout)); err != nil { + _ = raw.Close() + return nil, err + } + conn, err := ws.NewClient(cfg, raw) + if err != nil { + _ = raw.Close() + return nil, err + } + _ = raw.SetDeadline(time.Time{}) // live stream: no deadline + return conn, nil +} + +// hostPortAddr extracts host:port from a ws/wss URL, defaulting to the +// scheme's standard port when absent (odek serve always prints one, but a +// hand-typed ws://host URL should still dial). +func hostPortAddr(u *url.URL) string { + if u.Host != "" && !strings.Contains(u.Host, ":") { + if u.Scheme == "wss" || u.Scheme == "https" { + return u.Host + ":443" + } + return u.Host + ":80" + } + return u.Host +} + // Resources queries the server's @-reference completion endpoint. func (c *Client) Resources(query string, limit int) ([]Resource, error) { u := fmt.Sprintf("%s/api/resources?q=%s&limit=%d", diff --git a/internal/client/dial_timeout_test.go b/internal/client/dial_timeout_test.go new file mode 100644 index 0000000..dcde98c --- /dev/null +++ b/internal/client/dial_timeout_test.go @@ -0,0 +1,47 @@ +package client + +import ( + "net" + "testing" + "time" +) + +// Regression: Dial had no dial timeout — a black-holed (or silently +// non-responsive) remote blocked until the OS TCP timeout. The dial (and +// the WS handshake over the same connection) must be bounded. +func TestDialIsBoundedAgainstSilentPeer(t *testing.T) { + old := wsDialTimeout + wsDialTimeout = 200 * time.Millisecond + defer func() { wsDialTimeout = old }() + + // A peer that accepts TCP but never reads or writes: the WS handshake + // can never complete, so only an explicit deadline can unblock Dial. + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer func() { _ = ln.Close() }() + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + // Hold the connection open, silently — never respond. + time.Sleep(10 * time.Second) + _ = c.Close() + } + }() + + addr := ln.Addr().String() + done := make(chan error, 1) + go func() { + _, err := Dial("ws://"+addr+"/ws", "http://127.0.0.1", "http://"+addr, "tok") + done <- err + }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("Dial blocked past the dial timeout against a silent peer") + } +} diff --git a/internal/server/proc_liveness_test.go b/internal/server/proc_liveness_test.go new file mode 100644 index 0000000..6c78d4f --- /dev/null +++ b/internal/server/proc_liveness_test.go @@ -0,0 +1,57 @@ +//go:build !windows + +package server + +import ( + "os/exec" + "strings" + "testing" + "time" +) + +// Regression: procAlive probed with Signal(0), which succeeds on an +// exited-but-unreaped child — the zombie still counts as alive, so a crashed +// `odek serve` burned the full ready timeout instead of failing fast. +func TestProcAliveDetectsExitedUnreapedChild(t *testing.T) { + bin, err := exec.LookPath("true") + if err != nil { + t.Skip("no 'true' binary") + } + c := &Conn{proc: exec.Command(bin)} + if err := c.proc.Start(); err != nil { + t.Fatalf("start: %v", err) + } + c.startReaper() + + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if !c.procAlive() { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatal("exited child still reported alive (Signal(0) zombie probe)") +} + +// Regression: waitSpawned's timeout error carried no server output, so a +// bind failure (port taken, config error) was undiagnosable — the stderr +// tail must be included in the timeout error. +func TestWaitSpawnedTimeoutIncludesStderrTail(t *testing.T) { + // A port nothing listens on: every probe refuses fast. + baseURL := "http://127.0.0.1:1" + scan := &tokenScanWriter{w: nilDiscard{}} + scan.Write([]byte("listen tcp: bind: address already in use\n")) + + err := waitSpawned(baseURL, scan, func() bool { return true }, 50*time.Millisecond) + if err == nil { + t.Fatal("waitSpawned unexpectedly succeeded against a dead port") + } + if !strings.Contains(err.Error(), "bind: address already in use") { + t.Fatalf("timeout error missing stderr tail:\n%v", err) + } +} + +// nilDiscard is a zero io.Writer (io.Discard import stays out of this file). +type nilDiscard struct{} + +func (nilDiscard) Write(p []byte) (int, error) { return len(p), nil } diff --git a/internal/server/server.go b/internal/server/server.go index 15b5512..fb9c556 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -16,6 +16,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "syscall" "time" @@ -61,10 +62,12 @@ type Conn struct { Token string // per-instance CSRF token Version string // engine version as printed by ` version` (e.g. "v0.2.0"); spawn mode only - proc *exec.Cmd // non-nil when bodek spawned the server - scan *tokenScanWriter // non-nil when bodek spawned the server - watch func() // cancels the orphan watchdog (nil when none) - watchMu sync.Mutex + proc *exec.Cmd // non-nil when bodek spawned the server + scan *tokenScanWriter // non-nil when bodek spawned the server + reaped atomic.Bool // the reaper observed the child exit + reapDone chan struct{} // closed when the reaper's Wait returns + watch func() // cancels the orphan watchdog (nil when none) + watchMu sync.Mutex // OnStopEvent, when set, receives shutdown progress from Stop. OnStopEvent func(StopEvent) @@ -195,6 +198,7 @@ func (c *Conn) spawn(opts Options, addr string) error { return fmt.Errorf("start odek serve: %w", err) } c.proc = cmd + c.startReaper() c.startWatchdog() return nil } @@ -273,8 +277,13 @@ func (c *Conn) Stop() { c.OnStopEvent(StopStopping) } c.signalServer(syscall.SIGINT) - done := make(chan struct{}) - go func() { _ = c.proc.Wait(); close(done) }() + // The reaper owns Wait (started at spawn); select on its completion + // instead of a second Wait, which exec.Cmd forbids. + done := c.reapDone + if done == nil { + done = make(chan struct{}) + go func() { _ = c.proc.Wait(); close(done) }() + } select { case <-done: case <-time.After(stopTimeout): @@ -306,12 +315,16 @@ func splitTokenURL(raw string) (base, token string) { // WebSocket: ws://127.0.0.1:8080/ws // WS token: type tokenScanWriter struct { - w io.Writer - mu sync.Mutex - buf []byte // partial line not yet terminated by '\n' - tok string + w io.Writer + mu sync.Mutex + buf []byte // partial line not yet terminated by '\n' + tok string + tail []string // last complete lines, bounded, for failure diagnostics } +// maxTailLines bounds the stderr tail kept for error reporting. +const maxTailLines = 4 + func (s *tokenScanWriter) Write(p []byte) (int, error) { s.scan(p) return s.w.Write(p) @@ -324,6 +337,16 @@ func (s *tokenScanWriter) Token() string { return s.tok } +// Tail returns the last n complete stderr lines, joined for error text. +func (s *tokenScanWriter) Tail(n int) string { + s.mu.Lock() + defer s.mu.Unlock() + if len(s.tail) > n { + s.tail = s.tail[len(s.tail)-n:] + } + return strings.Join(s.tail, "; ") +} + func (s *tokenScanWriter) scan(p []byte) { s.mu.Lock() defer s.mu.Unlock() @@ -338,6 +361,10 @@ func (s *tokenScanWriter) scan(p []byte) { } line := string(s.buf[:i]) s.buf = s.buf[i+1:] + s.tail = append(s.tail, line) + if len(s.tail) > maxTailLines { + s.tail = s.tail[len(s.tail)-maxTailLines:] + } if tok := parseTokenLine(line); tok != "" { s.tok = tok return @@ -384,14 +411,34 @@ func waitReady(baseURL string, timeout time.Duration) error { } // procAlive reports whether the spawned child is still running. Signal(0) -// probes liveness without waiting: ProcessState stays nil until Wait is -// called (Stop's job, after Connect returns), so it can never report death -// here. +// alone cannot tell an exited-but-unreaped child (zombie) from a live one — +// the ProcessState stays nil until Wait is reaped — so startReaper reaps in +// the background and procAlive consults that result first. func (c *Conn) procAlive() bool { + if c.reaped.Load() { + return false + } return c.proc != nil && c.proc.Process != nil && c.proc.Process.Signal(syscall.Signal(0)) == nil } +// startReaper waits for the spawned child in the background so its exit is +// observed immediately (no zombie) and procAlive can report death. The +// result feeds Stop's Wait — Stop must never Wait the same Cmd twice. +func (c *Conn) startReaper() { + if c.proc == nil { + return + } + proc := c.proc + done := make(chan struct{}) + c.reapDone = done + go func() { + _ = proc.Wait() + c.reaped.Store(true) + close(done) + }() +} + // waitSpawned waits until a spawned server answers HTTP or prints its token // line. Old odek versions print no token, so readiness alone eventually ends // the wait (the legacy token path handles those) — but only after a short @@ -416,11 +463,23 @@ func waitSpawned(baseURL string, scan *tokenScanWriter, alive func() bool, timeo readyAt = time.Time{} // flapping: restart the grace clock } if alive != nil && !alive() { - return fmt.Errorf("odek serve exited before becoming ready") + return fmt.Errorf("odek serve exited before becoming ready%w", stderrTail(scan)) } time.Sleep(150 * time.Millisecond) } - return fmt.Errorf("timed out after %s", timeout) + return fmt.Errorf("timed out after %s%w", timeout, stderrTail(scan)) +} + +// stderrTail returns the captured server stderr tail for inclusion in a +// waitSpawned error (bind failures, config errors), or nil when empty. +func stderrTail(scan *tokenScanWriter) error { + if scan == nil { + return nil + } + if tail := scan.Tail(maxTailLines); tail != "" { + return fmt.Errorf(": %s", tail) + } + return nil } // spawnedTokenGrace is how long a ready-but-tokenless spawned server is diff --git a/internal/tui/approval.go b/internal/tui/approval.go index 06ea899..2a8b338 100644 --- a/internal/tui/approval.go +++ b/internal/tui/approval.go @@ -105,7 +105,7 @@ func (m *Model) handleApprovalKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, m.armConfirm(confirmQuit, "bodek") case "enter": return m, m.submit() - case "shift+enter", "alt+enter", "ctrl+j": + case "shift+enter", "ctrl+enter", "alt+enter", "ctrl+j": return m, m.insertNewline() } return m, m.updateApprovalComposer(msg) @@ -329,6 +329,9 @@ func (m *Model) answer(action string) tea.Cmd { dl = m.apprDeadlines[0] m.apprDeadlines = m.apprDeadlines[1:] // keep the parallel expiry queue in lockstep } + if len(m.apprBells) > 0 { + m.apprBells = m.apprBells[1:] + } m.approvals = m.approvals[1:] m.resetApprovalInput() if len(m.approvals) > 0 { diff --git a/internal/tui/approval_expiry.go b/internal/tui/approval_expiry.go index da619f0..da13aad 100644 --- a/internal/tui/approval_expiry.go +++ b/internal/tui/approval_expiry.go @@ -39,7 +39,7 @@ func approvalTTL(ev client.Event) time.Duration { // the closest client-side approximation (single-digit ms skew locally). func (m *Model) stampApprovalDeadline(ev client.Event) { m.apprDeadlines = append(m.apprDeadlines, time.Now().Add(approvalTTL(ev))) - m.apprBellFired = false // a fresh request re-arms the urgent-window BEL + m.apprBells = append(m.apprBells, false) // a fresh request re-arms its own urgent-window BEL } // apprSecondsLeft is the queue head's remaining lifetime in whole seconds @@ -85,6 +85,7 @@ func (m *Model) handleApprovalExpiry(now time.Time) tea.Cmd { kept := m.approvals[:0] keptDL := m.apprDeadlines[:0] + keptBell := m.apprBells[:0] dropped := 0 for i, a := range m.approvals { var dl time.Time @@ -97,17 +98,20 @@ func (m *Model) handleApprovalExpiry(now time.Time) tea.Cmd { } kept = append(kept, a) keptDL = append(keptDL, dl) + keptBell = append(keptBell, i < len(m.apprBells) && m.apprBells[i]) } m.approvals = kept m.apprDeadlines = keptDL + m.apprBells = keptBell // (A3) the countdown entering its urgent window (< 10s left) rings the // bell exactly once per request — a tick inside the window must not - // re-fire, and expiry pruning below stays silent. + // re-fire, and expiry pruning below stays silent. The latch is + // per-entry: a mid-queue expiry must not silence the survivor. var cmds []tea.Cmd - if !m.apprBellFired && len(m.approvals) > 0 { + if len(m.apprBells) > 0 && !m.apprBells[0] && len(m.approvals) > 0 { if secs := m.apprSecondsLeft(); secs > 0 && secs <= approvalUrgentSecs { - m.apprBellFired = true + m.apprBells[0] = true if a := m.attentionFor(attentionApproval); !a.empty() { a.title, a.notify = "", "" // the card is already on screen — bell only cmds = append(cmds, m.attentionCmd(a)) diff --git a/internal/tui/events.go b/internal/tui/events.go index e7b5edf..2ac0c95 100644 --- a/internal/tui/events.go +++ b/internal/tui/events.go @@ -302,6 +302,7 @@ func (m *Model) handleEvent(ev client.Event) (tea.Model, tea.Cmd) { // engine has moved on, so ⏎ never sends the next prompt. m.approvals = nil m.apprDeadlines = nil + m.apprBells = nil m.resetApprovalInput() m.clearClarify() m.status = "ready" @@ -405,6 +406,7 @@ func (m *Model) handleEvent(ev client.Event) (tea.Model, tea.Cmd) { m.lastArg = "" m.approvals = nil m.apprDeadlines = nil + m.apprBells = nil m.resetApprovalInput() m.clearClarify() if cancelled { @@ -548,6 +550,7 @@ func (m *Model) handleEvent(ev client.Event) (tea.Model, tea.Cmd) { // footer) so ⏎ retry never runs. m.approvals = nil m.apprDeadlines = nil + m.apprBells = nil m.resetApprovalInput() m.clearClarify() if m.shutdownReq { diff --git a/internal/tui/fix_regress_test.go b/internal/tui/fix_regress_test.go new file mode 100644 index 0000000..43b5de1 --- /dev/null +++ b/internal/tui/fix_regress_test.go @@ -0,0 +1,95 @@ +package tui + +import ( + "strings" + "testing" + "time" + + "github.com/BackendStack21/bodek/internal/client" + tea "github.com/charmbracelet/bubbletea" +) + +// Regression tests for three TUI defects: the ctrl+enter newline chord in +// the approval composer, the per-approval urgent-window bell, and the +// queue-strip click hit test bleeding into row-body text. + +func TestApprovalCtrlEnterInsertsNewline(t *testing.T) { + m := newTestModel() + feedApproval(t, m, client.Event{Type: "approval_request", ID: "apr", Risk: "shell_exec", Command: "rm x"}) + m.ta.SetValue("draft") + msg := tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("ctrl+enter")} + m.handleApprovalKey(msg) + if v := m.ta.Value(); !strings.Contains(v, "\n") { + t.Fatalf("ctrl+enter must insert a newline, draft = %q", v) + } + if strings.Contains(m.ta.Value(), "ctrl+enter") { + t.Fatalf("ctrl+enter sentinel leaked into the draft as text: %q", m.ta.Value()) + } +} + +func TestApprovalUrgentBellFiresPerEntry(t *testing.T) { + m := newTestModel() + m.bell = true + // Two approvals in flight; the head is already in the urgent window. + feedApproval(t, m, client.Event{Type: "approval_request", ID: "a1", Risk: "shell_exec", Command: "rm x"}) + feedApproval(t, m, client.Event{Type: "approval_request", ID: "a2", Risk: "shell_exec", Command: "rm y"}) + m.apprDeadlines[0] = time.Now().Add(9 * time.Second) + m.apprDeadlines[1] = time.Now().Add(90 * time.Second) + + m.handleApprovalExpiry(time.Now()) + if len(m.apprBells) != 2 || !m.apprBells[0] { + t.Fatalf("head's urgent-window bell must latch per entry: %v", m.apprBells) + } + // Same window again: no re-fire for the same entry. + m.handleApprovalExpiry(time.Now()) + if len(m.apprBells) != 2 || !m.apprBells[0] || m.apprBells[1] { + t.Fatalf("bell guard must stay latched per entry: %v", m.apprBells) + } + // The head expires; the surviving second entry later enters its own + // urgent window and must ring on its own — a mid-queue expiry bell + // fires once PER ENTRY, not once total. + m.apprDeadlines[0] = time.Now().Add(-time.Second) + m.handleApprovalExpiry(time.Now()) + if len(m.approvals) != 1 { + t.Fatalf("expired head must drop: %d left", len(m.approvals)) + } + m.apprDeadlines[0] = time.Now().Add(9 * time.Second) + m.handleApprovalExpiry(time.Now()) + if !m.apprBells[0] { + t.Fatal("the surviving entry must fire its own urgent-window bell") + } +} + +func TestQueueStripClickBoundsControlsToTheirCells(t *testing.T) { + m := newTestModel() + m.queue = []string{"hello world"} + m.qfocus = true + // Locate the controls on the unstyled first row. + row := unstyle(strings.Split(m.queueStripView(), "\n")[0]) + delC := -1 + cell := 0 + for _, r := range row { + if r == '✕' && delC < 0 { + delC = cell + } + cell += 1 + } + if delC < 0 { + t.Fatal("✕ control not found on the strip row") + } + top := m.queueStripTop() + // A click past the ✕ glyph (row-body / trailing region) must select the + // row, not arm the delete confirm. + m.queueStripClick(top, delC+2) + if m.qarm != -1 { + t.Fatalf("click past the ✕ glyph must not arm delete (qarm = %d)", m.qarm) + } + if m.qsel != 0 { + t.Fatalf("click past the controls must select the row (qsel = %d)", m.qsel) + } + // A click on the ✕ cell itself still arms. + m.queueStripClick(top, delC) + if m.qarm != 0 { + t.Fatalf("click on the ✕ cell must arm delete (qarm = %d)", m.qarm) + } +} diff --git a/internal/tui/model.go b/internal/tui/model.go index 84a4f35..d2423a4 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -215,8 +215,8 @@ type Model struct { lastTool string lastArg string - failBellFired bool // (A3) the failure BEL rang for this turn — guard against double-fire - apprBellFired bool // (A3) the urgent-window BEL rang for this approval head + failBellFired bool // (A3) the failure BEL rang for this turn — guard against double-fire + apprBells []bool // per-approval urgent-window BEL latch, parallel to apprDeadlines approvals []client.Event // pending approval queue — odek runs parallel tools, so requests FIFO apprDeadlines []time.Time // per-approval expiry, stamped on arrival (parallel to approvals) diff --git a/internal/tui/queue.go b/internal/tui/queue.go index a7dad39..15baaeb 100644 --- a/internal/tui/queue.go +++ b/internal/tui/queue.go @@ -205,9 +205,9 @@ func (m *Model) queueStripClick(y, x int) bool { } cell += lipgloss.Width(string(r)) } - // Check right-to-left: the controls trail the row, so a hit test claims - // the rightmost control whose column the click reached. - if delC >= 0 && x >= delC { + // Check right-to-left: each control owns only its own cell — a click + // on the row body or the gaps between controls selects instead. + if delC >= 0 && x >= delC && x < delC+lipgloss.Width("✕") { if m.qarm == rel { // second ✕ on the same row confirms m.queueDeleteAt(rel) return true @@ -216,11 +216,11 @@ func (m *Model) queueStripClick(y, x int) bool { m.refresh() return true } - if downC >= 0 && x >= downC { + if downC >= 0 && x >= downC && x < downC+lipgloss.Width("▼") { m.queueMove(rel, 1) return true } - if upC >= 0 && x >= upC { + if upC >= 0 && x >= upC && x < upC+lipgloss.Width("▲") { m.queueMove(rel, -1) return true } diff --git a/internal/tui/transcript_additions_test.go b/internal/tui/transcript_additions_test.go index 5eab19a..3940fc9 100644 --- a/internal/tui/transcript_additions_test.go +++ b/internal/tui/transcript_additions_test.go @@ -139,19 +139,19 @@ func TestApprovalUrgentBellOnce(t *testing.T) { m.apprDeadlines[0] = time.Now().Add(9 * time.Second) m.handleApprovalExpiry(time.Now()) - if !m.apprBellFired { + if len(m.apprBells) != 1 || !m.apprBells[0] { t.Fatal("urgent countdown must latch the bell guard (fired once)") } // A further tick in the same window does not re-fire. m.handleApprovalExpiry(time.Now()) - if !m.apprBellFired { + if len(m.apprBells) != 1 || !m.apprBells[0] { t.Error("bell guard must stay latched inside the window") } // A fresh approval re-arms the transition. m.approvals = append(m.approvals, client.Event{Type: "approval_request", ID: "a2"}) m.apprDeadlines = append(m.apprDeadlines, time.Now().Add(9*time.Second)) - m.stampApprovalDeadline(client.Event{ID: "a2", TimeoutSeconds: 60}) - if m.apprBellFired { - t.Error("a new approval must reset the urgent-bell guard") + m.apprBells = append(m.apprBells, false) + if len(m.apprBells) != 2 || m.apprBells[1] { + t.Error("a new approval must start with its own unset urgent-bell guard") } } diff --git a/internal/update/transport_error_test.go b/internal/update/transport_error_test.go new file mode 100644 index 0000000..ea7ba80 --- /dev/null +++ b/internal/update/transport_error_test.go @@ -0,0 +1,63 @@ +package update + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// roundTripperFunc stubs the transport so a test can inject an arbitrary +// transport error without hitting the network. +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) { + return f(r) +} + +// RED: a transport error whose message merely contains "403" (or "401"/"429") +// is not an HTTP status — it must NOT trigger the HTML fallback, which would +// fabricate conventionalAssets URLs for a tag we never verified. +func TestTransportErrorWithStatusDigitsDoesNotFallback(t *testing.T) { + fallback := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", "https://github.com/BackendStack21/bodek/releases/tag/v9.9.9") + w.WriteHeader(http.StatusFound) + })) + t.Cleanup(fallback.Close) + client := &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + return nil, errors.New("proxy connect: refused 10.0.0.3:403") + })} + + _, _, err := fetchLatest(context.Background(), client, LatestURL, fallback.URL) + if err == nil { + t.Fatal("transport error must propagate — the HTML fallback fired on a non-status error") + } + if !strings.Contains(err.Error(), "proxy connect") { + t.Fatalf("expected the original transport error, got %v", err) + } +} + +// Typed status errors must still classify as retryable. +func TestStatusRetryableTypedStatuses(t *testing.T) { + for _, code := range []int{401, 403, 429} { + err := statusError(code, "403 Forbidden") + var se *statusErr + if !errors.As(err, &se) || se.code != code { + t.Fatalf("statusError(%d) should wrap a typed statusErr, got %v", code, err) + } + if !statusRetryable(err) { + t.Errorf("status %d should be retryable", code) + } + } + for _, code := range []int{400, 404, 500} { + if statusRetryable(statusError(code, "oops")) { + t.Errorf("status %d should not be retryable", code) + } + } + // Wrapped transport errors are never retryable. + if statusRetryable(errors.New("dial tcp: lookup api.github.com: 429 no such host")) { + t.Error("transport error containing 429 must not be retryable") + } +} diff --git a/internal/update/update.go b/internal/update/update.go index b761492..fb165f3 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -5,6 +5,7 @@ package update import ( "context" "encoding/json" + "errors" "fmt" "net/http" "net/url" @@ -160,19 +161,31 @@ func githubToken() string { return os.Getenv("GH_TOKEN") } -func statusError(code int, status string) error { - if code == http.StatusUnauthorized || code == http.StatusForbidden || code == http.StatusTooManyRequests { - return fmt.Errorf("query latest release: unexpected status %s (GitHub rate limit or anonymous block — set GITHUB_TOKEN)", status) +// statusErr is the typed carrier of a non-200 API response. Classification +// goes through errors.As — never substring matching on error text, which any +// transport error message can accidentally satisfy. +type statusErr struct { + code int + status string +} + +func (e *statusErr) Error() string { + if e.code == http.StatusUnauthorized || e.code == http.StatusForbidden || e.code == http.StatusTooManyRequests { + return fmt.Sprintf("query latest release: unexpected status %s (GitHub rate limit or anonymous block — set GITHUB_TOKEN)", e.status) } - return fmt.Errorf("query latest release: unexpected status %s", status) + return fmt.Sprintf("query latest release: unexpected status %s", e.status) +} + +func statusError(code int, status string) error { + return &statusErr{code: code, status: status} } func statusRetryable(err error) bool { - if err == nil { + var se *statusErr + if !errors.As(err, &se) { return false } - s := err.Error() - return strings.Contains(s, "401") || strings.Contains(s, "403") || strings.Contains(s, "429") + return se.code == http.StatusUnauthorized || se.code == http.StatusForbidden || se.code == http.StatusTooManyRequests } // Newer reports whether latest is a higher version than current. Both may diff --git a/internal/workspace/persist_fix_test.go b/internal/workspace/persist_fix_test.go new file mode 100644 index 0000000..fe1eb3d --- /dev/null +++ b/internal/workspace/persist_fix_test.go @@ -0,0 +1,59 @@ +package workspace + +import ( + "encoding/json" + "os" + "path/filepath" + "sync" + "testing" +) + +// Regression: persist staged at a FIXED path+".tmp" while Save/Patch +// persisted after releasing s.mu — two interleaved persists could publish +// a torn file (A.Write → B.Write truncates → A.Rename) that wipes every +// workspace's draft/queue/session on next Open. +func TestPersistSerializesAcrossConcurrentSaves(t *testing.T) { + path := filepath.Join(t.TempDir(), "workspaces.json") + t.Setenv("BODEK_WORKSPACE", path) + + s := Open() + + var wg sync.WaitGroup + for i := range 64 { + wg.Add(2) + go func(i int) { defer wg.Done(); _ = s.Save("/proj/a", State{Draft: "d", History: []string{"h"}}) }(i) + go func() { defer wg.Done(); s.Patch("/proj/b", func(st *State) { st.Queue = append(st.Queue, "q") }) }() + } + wg.Wait() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("workspace unreadable after concurrent Save/Patch: %v", err) + } + var f fileFormat + if err := json.Unmarshal(data, &f); err != nil { + t.Fatalf("published workspace is not valid JSON (torn write): %v\n%s", err, data) + } + if len(f.Workspaces) == 0 { + t.Fatal("workspace empty after 64 saves — a torn rename dropped the entries") + } +} + +// Regression: a corrupt workspaces.json was silently reset — every saved +// draft, queue, and session id was dropped with no diagnostic. The corrupt +// file must be quarantined as .corrupt instead. +func TestOpenQuarantinesCorruptWorkspace(t *testing.T) { + path := filepath.Join(t.TempDir(), "workspaces.json") + if err := os.WriteFile(path, []byte("{not json"), 0o600); err != nil { + t.Fatalf("seed corrupt file: %v", err) + } + t.Setenv("BODEK_WORKSPACE", path) + + s := Open() + if s == nil { + t.Fatal("Open returned nil") + } + if _, err := os.Stat(path + ".corrupt"); err != nil { + t.Fatalf("corrupt workspace was not quarantined as %s.corrupt: %v", path, err) + } +} diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index bbbe49d..74d1e7e 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -59,6 +59,12 @@ func Open() *Store { } var f fileFormat if json.Unmarshal(data, &f) != nil || f.Workspaces == nil { + // Corrupt on disk: quarantine instead of silently resetting, so a + // torn write never destroys every draft/queue/session undiagnosably + // (mirrors tokens.go). + if qerr := os.Rename(s.path, s.path+".corrupt"); qerr == nil { + fmt.Fprintf(os.Stderr, "bodek: warning: corrupt %s quarantined as %s.corrupt\n", s.path, s.path) + } return s } s.all = f.Workspaces @@ -158,12 +164,30 @@ func persist(path string, all map[string]State) error { if err != nil { return fmt.Errorf("encode workspace: %w", err) } - tmp := path + ".tmp" - if err := os.WriteFile(tmp, data, 0o600); err != nil { + // A per-call staged name: callers persist after releasing the store + // mutex, so a shared path+'.tmp' let two interleaved writes tear the + // file (A.Write → B.Write truncates → A.Rename) — the same tear + // tokens.go already fixed. + tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*.tmp") + if err != nil { + return fmt.Errorf("stage workspace: %w", err) + } + tmpName := tmp.Name() + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) return fmt.Errorf("write workspace: %w", err) } - if err := os.Rename(tmp, path); err != nil { - _ = os.Remove(tmp) + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpName) + return fmt.Errorf("write workspace: %w", err) + } + if err := os.Chmod(tmpName, 0o600); err != nil { + _ = os.Remove(tmpName) + return fmt.Errorf("protect workspace: %w", err) + } + if err := os.Rename(tmpName, path); err != nil { + _ = os.Remove(tmpName) return fmt.Errorf("replace workspace: %w", err) } return nil From 5dc51272964f58d4fdc501d650b26fe14f69fc72 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 18 Sep 2026 13:24:11 +0200 Subject: [PATCH 3/3] fix(server): re-check token before waitSpawned declares child death A token-print-and-exit server can flush its banner after the loop's top-of-loop token check; the reaper's fast liveness signal then failed Connect even though the token was captured. The token wins. --- internal/server/server.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/server/server.go b/internal/server/server.go index fb9c556..723299b 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -462,8 +462,14 @@ func waitSpawned(baseURL string, scan *tokenScanWriter, alive func() bool, timeo } else if !readyAt.IsZero() { readyAt = time.Time{} // flapping: restart the grace clock } + // Re-check the token before declaring death: a fast-exiting + // (or token-print-and-exit) server can flush its banner after + // this iteration's top-of-loop check — the token wins. if alive != nil && !alive() { - return fmt.Errorf("odek serve exited before becoming ready%w", stderrTail(scan)) + if scan == nil || scan.Token() == "" { + return fmt.Errorf("odek serve exited before becoming ready%w", stderrTail(scan)) + } + return nil } time.Sleep(150 * time.Millisecond) }