From 134d3a33ff88e9b1df2f08f4d2d55eb589c048f0 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Wed, 30 Sep 2026 12:27:44 +0200 Subject: [PATCH 1/2] fix(tui): block /copy-session-id placeholder window and /new re-persist - /copy-session-id: ordering-based awaitSessionConfirm gate so the connect-time placeholder id cannot be copied before the post-prompt session frame confirms the real id; resumed sessions keep copying - /new: one-shot suppressRemember stops the reconnect session frame from re-persisting the placeholder id into workspaces.json - heartbeat: skip ping and pingSentAt stamp while disconnected; reset the stamp on reconnect swap so RTT pairs with the live socket - update: Newer parses the latest tag via pseudoBase so prerelease latest releases no longer read as always up-to-date - settings: refuse persistence for the run when the config file fails to parse instead of overwriting it with a near-empty file --- cmd/bodek/main.go | 30 ++++++- cmd/bodek/settings_persist_test.go | 67 ++++++++++++++++ internal/tui/clipboard.go | 9 ++- internal/tui/events.go | 9 ++- internal/tui/heartbeat_test.go | 31 ++++++++ internal/tui/input.go | 10 ++- internal/tui/model.go | 25 ++++-- internal/tui/reconnect.go | 1 + internal/tui/session_placeholder_test.go | 99 ++++++++++++++++++++++++ internal/update/update.go | 12 ++- internal/update/update_test.go | 5 ++ 11 files changed, 278 insertions(+), 20 deletions(-) create mode 100644 cmd/bodek/settings_persist_test.go create mode 100644 internal/tui/heartbeat_test.go create mode 100644 internal/tui/session_placeholder_test.go diff --git a/cmd/bodek/main.go b/cmd/bodek/main.go index 0c08079..f9ff1ec 100644 --- a/cmd/bodek/main.go +++ b/cmd/bodek/main.go @@ -50,6 +50,24 @@ type config struct { extraArgs []string persist settings.Settings // the loaded file, re-saved when /theme switches + // persistDisabled is set when the settings file could not be read; + // a full-replace Save would then overwrite the (unparsed) file with + // a near-empty struct, so all persistence is refused for the run. + persistDisabled bool +} + +// save persists the current preference set. When the settings file was +// unreadable at startup it refuses to write — the user's hand-written +// file must never be clobbered with a partially-populated struct — and +// reports the refusal as an error (a silent no-op success would hide +// that /theme etc. are not being persisted). Startup already printed a +// one-time warning via parseConfig, so callers surface this at most once +// per action. +func (c *config) save() error { + if c.persistDisabled { + return fmt.Errorf("settings file could not be parsed; changes are not saved this run") + } + return settings.Save(c.persist) } func parseConfig(args []string, output io.Writer) (config, error) { @@ -63,6 +81,12 @@ func parseConfig(args []string, output io.Writer) (config, error) { _, _ = fmt.Fprintf(output, "bodek: ignoring settings file: %v\n", err) } st = settings.Settings{} + // The file stays untouched on disk; refusing to save protects + // every unparsed key from the full-replace write. + cfg.persistDisabled = true + } + if output != nil && cfg.persistDisabled { + _, _ = fmt.Fprintln(output, "bodek: settings changes will not be saved this run (file could not be parsed)") } cfg.persist = st fs := flag.NewFlagSet("bodek", flag.ContinueOnError) @@ -275,15 +299,15 @@ func run() error { ResumeSession: cfg.sessionID, OnThemeChange: func(name string) error { cfg.persist.Theme = name - return settings.Save(cfg.persist) + return cfg.save() }, OnVerbosityChange: func(name string) error { cfg.persist.Verbosity = name - return settings.Save(cfg.persist) + return cfg.save() }, OnThinkingChange: func(level string) error { cfg.persist.Thinking = level - return settings.Save(cfg.persist) + return cfg.save() }, Reconnect: func() (*client.Client, error) { return client.Dial(srv.WSURL, srv.Origin, srv.BaseURL, srv.Token) diff --git a/cmd/bodek/settings_persist_test.go b/cmd/bodek/settings_persist_test.go new file mode 100644 index 0000000..b7a18ce --- /dev/null +++ b/cmd/bodek/settings_persist_test.go @@ -0,0 +1,67 @@ +package main + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +// A malformed settings file must never be overwritten by a later +// preference change: persistence is refused for the run instead of +// saving the zeroed struct as a full-replace write. +func TestParseConfigBrokenFileRefusesPersistence(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + broken := "{not json" + if err := os.WriteFile(path, []byte(broken), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("BODEK_CONFIG", path) + + var out bytes.Buffer + cfg, err := parseConfig(nil, &out) + if err != nil { + t.Fatalf("parseConfig returned error: %v", err) + } + if !cfg.persistDisabled { + t.Fatal("parseConfig did not disable persistence for a broken settings file") + } + if warning := out.String(); !strings.Contains(warning, "not be saved") { + t.Fatalf("startup warning %q does not mention persistence being disabled", warning) + } + + // Simulate a theme change: the save path must leave the broken file + // untouched rather than replacing it with a minimal settings file, + // and must report the refusal instead of a silent success. + cfg.persist.Theme = "dracula" + if err := cfg.save(); err == nil { + t.Fatal("save on a disabled persistence run must report an error, not succeed silently") + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(data) != broken { + t.Fatalf("broken settings file was overwritten: %q", data) + } + + // A healthy settings file keeps persistence enabled. + ok := filepath.Join(dir, "ok.json") + t.Setenv("BODEK_CONFIG", ok) + cfg2, err := parseConfig(nil, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if cfg2.persistDisabled { + t.Fatal("persistence disabled for a valid (missing) settings file") + } + cfg2.persist.Theme = "ember-light" + if err := cfg2.save(); err != nil { + t.Fatalf("save returned error: %v", err) + } + if _, err := os.Stat(ok); err != nil { + t.Fatalf("expected saved settings file: %v", err) + } +} diff --git a/internal/tui/clipboard.go b/internal/tui/clipboard.go index c070447..e093907 100644 --- a/internal/tui/clipboard.go +++ b/internal/tui/clipboard.go @@ -244,8 +244,13 @@ func (m *Model) copySessionID() tea.Cmd { // sessionLive, not just a non-empty id: the connect-time session event // stamps an id before any prompt — that placeholder is never the // operator's session, so the command stays dormant until a prompt (or - // a resume) makes the session real. - if !m.sessionLive || !validSessionID(m.sessionID) { + // a resume) makes the session real. Even then the id is still the + // placeholder until the confirming session frame lands: sendPrompt arms + // awaitSessionConfirm and any session frame clears it, so the gate holds + // exactly for the window between the prompt and its confirmation — and + // never blocks a confirmed id again on later prompts (odek does not + // re-emit a session frame per turn). + if !m.sessionLive || !validSessionID(m.sessionID) || m.awaitSessionConfirm { return m.transientNoteCmd("no session yet — the id exists once a session is created; send a prompt first") } return m.copyText(m.sessionID) diff --git a/internal/tui/events.go b/internal/tui/events.go index 4b2af21..590e22f 100644 --- a/internal/tui/events.go +++ b/internal/tui/events.go @@ -88,13 +88,16 @@ func (m *Model) handleEvent(ev client.Event) (tea.Model, tea.Cmd) { case "session": prevSession := m.sessionID m.sessionID = ev.SessionID + m.awaitSessionConfirm = false // any session frame confirms the current id + if m.suppressRemember { + m.suppressRemember = false // /new: the placeholder frame must not re-persist + } else if !m.freshStart { + m.rememberSession(m.homePrompt) + } if ev.AuthToken != "" { m.authToken = ev.AuthToken m.tokens.Set(ev.SessionID, ev.AuthToken) } - if !m.freshStart { - m.rememberSession(m.homePrompt) - } if prevSession != "" && ev.SessionID != prevSession { m.planResetPending = true // switch/attach: drop + refetch at the tail m.resetJobsState() // jobs are session-scoped; re-baseline on the next snapshot diff --git a/internal/tui/heartbeat_test.go b/internal/tui/heartbeat_test.go new file mode 100644 index 0000000..f8dcdd8 --- /dev/null +++ b/internal/tui/heartbeat_test.go @@ -0,0 +1,31 @@ +package tui + +import ( + "testing" + "time" +) + +// A heartbeat fired while the connection is down must not send a ping +// (the dead client would swallow it) and must not stamp the RTT clock — +// a stamp with no possible pong leaves a bogus latency measurement armed. +func TestHeartbeatSkipsWhileDisconnected(t *testing.T) { + m := newTestModel() + m.disconn = true + m.handleHeartbeat() + if !m.pingSentAt.IsZero() { + t.Fatalf("pingSentAt stamped while disconnected: %v", m.pingSentAt) + } +} + +// The reconnect swap must drop any outstanding ping stamp from the dead +// socket, so the first pong on the fresh connection cannot pair with a +// send that left before the swap. +func TestReconnectResetsPingSentAt(t *testing.T) { + m := wired(t) // live stand-in: m.cl is a real connected client + m.disconn = true + m.pingSentAt = time.Now() + m.handleReconnect(reconnectMsg{attempt: 0, cl: m.cl}) + if !m.pingSentAt.IsZero() { + t.Fatalf("pingSentAt survived the reconnect swap: %v", m.pingSentAt) + } +} diff --git a/internal/tui/input.go b/internal/tui/input.go index e6d7ee0..a1fc846 100644 --- a/internal/tui/input.go +++ b/internal/tui/input.go @@ -515,7 +515,15 @@ func (m *Model) sendPrompt(text string) tea.Cmd { m.ta.Reset() m.closeAC() m.busy = true - m.sessionLive = true // the first prompt creates the session + // The id stays a placeholder until a session frame confirms it — but + // odek does not re-emit a session frame per turn, so only a + // still-unconfirmed id (re)arms the gate; a confirmed one must keep + // copying across later prompts. Evaluated before sessionLive flips: + // the first prompt finds it false and arms; later prompts of an + // already-confirmed session do not. + armSessionConfirm := !m.sessionLive || m.awaitSessionConfirm + m.sessionLive = true // the first prompt creates the session + m.awaitSessionConfirm = armSessionConfirm m.cancelAck = false // a fresh run's errors are real errors again m.failBellFired = false // a fresh local turn re-arms the failure BEL m.skillSuggest = nil // the suggestion's window closed with the turn diff --git a/internal/tui/model.go b/internal/tui/model.go index 3e4439b..5806195 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -261,14 +261,16 @@ type Model struct { histIdx int // index into history while navigating histDraft string // input stashed while navigating history - model string - sandbox bool - sessionID string - sessionLive bool // a prompt created/adopted this session — copy-session-id's gate - authToken string // session-scoped token (for cancel / resume) - pendModel string // model to apply on the next prompt - thinking string // canonical: "" inherit, or disabled|low|medium|high - expandAll bool // Ctrl+E: render every step's full output/logs + model string + sandbox bool + sessionID string + sessionLive bool // a prompt created/adopted this session — copy-session-id's gate + awaitSessionConfirm bool // armed per prompt; cleared by the confirming session frame + suppressRemember bool // one-shot: /new's first post-reconnect session frame must not re-persist + authToken string // session-scoped token (for cancel / resume) + pendModel string // model to apply on the next prompt + 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 cockpit stats sheet ("" hides it) bodekVersion string // bodek's own version, for the startup update check @@ -508,6 +510,11 @@ func (m *Model) armHeartbeat() tea.Cmd { } func (m *Model) handleHeartbeat() tea.Cmd { + if m.disconn { + // The socket is down and reconnect is backing off: pinging the dead + // client only stamps an RTT clock no pong can ever answer. Re-arm. + return m.armHeartbeat() + } m.pingSentAt = time.Now() cl := m.cl if cl == nil { @@ -1244,12 +1251,14 @@ func (m *Model) startFreshSession() tea.Cmd { homeFetch := m.clearConversation() m.sessionID = "" m.sessionLive = false // /new: no session until the next prompt creates one + m.awaitSessionConfirm = false m.authToken = "" m.pendModel = m.model // the new session re-asserts the active model m.resetPlanState() m.resetJobsState() clearHome(m) m.freshStart = true + m.suppressRemember = true // the fresh connection's first session frame carries only a placeholder m.pendingResume = "" if m.ws != nil && m.opts.CWD != "" { m.ws.ClearSession(m.opts.CWD) diff --git a/internal/tui/reconnect.go b/internal/tui/reconnect.go index 22b5ebf..9e8719d 100644 --- a/internal/tui/reconnect.go +++ b/internal/tui/reconnect.go @@ -70,6 +70,7 @@ func (m *Model) handleReconnect(msg reconnectMsg) (tea.Model, tea.Cmd) { m.cl = msg.cl m.events = msg.cl.Events m.disconn = false + m.pingSentAt = time.Time{} // a pre-swap send must not pair with a post-swap pong m.status = "ready" // Session continuity survives the drop: session_switch adopts the // session on the fresh connection (restoring the server-side memory diff --git a/internal/tui/session_placeholder_test.go b/internal/tui/session_placeholder_test.go new file mode 100644 index 0000000..d0efa54 --- /dev/null +++ b/internal/tui/session_placeholder_test.go @@ -0,0 +1,99 @@ +package tui + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/BackendStack21/bodek/internal/client" + "github.com/BackendStack21/bodek/internal/workspace" +) + +// Regression tests for the connect-time placeholder session id: between the +// first prompt and the session frame that confirms the minted id, m.sessionID +// may still hold the placeholder the connect-time session event stamped — +// /copy-session-id must never hand it to the clipboard, and /new must not let +// the fresh connection's first session frame re-persist it. + +// BUG 1: a prompt arms sessionLive while the id is still the connect-time +// placeholder; /copy-session-id must wait for the post-prompt session frame. +func TestCopySessionIDPlaceholderNotCopiedBetweenPromptAndConfirm(t *testing.T) { + m := newTestModel() + m.handleEvent(client.Event{Type: "session", SessionID: "sess-placeholder"}) + _ = m.sendPrompt("hi") // arms sessionLive; the confirm frame has not arrived + + runCopySessionID(m) + if m.copyFlashing() { + t.Error("placeholder id copied before the post-prompt session frame confirmed it") + } + if n := len(m.notices); n == 0 || !strings.Contains(m.notices[n-1], "no session") { + t.Errorf("placeholder window must show the no-session note, got %v", m.notices) + } + + m.handleEvent(client.Event{Type: "session", SessionID: "sess-real"}) + runCopySessionID(m) + if !m.copyFlashing() { + t.Error("confirmed session id must copy after the post-prompt session frame") + } +} + +// BUG 3: the old wall-clock gate compared sessionIDAt against +// lastPromptStart, which sendPrompt re-stamps on every dispatch. odek does +// not re-emit a session frame per turn, so after the SECOND prompt the +// confirmed stamp was older than the prompt stamp and /copy-session-id +// permanently reported "no session yet". The ordering-based gate +// (awaitSessionConfirm, armed per prompt, cleared by any session frame) +// must keep the confirmed id copyable across later prompts. +func TestCopySessionIDStillCopiesAfterSecondPrompt(t *testing.T) { + m := newTestModel() + m.handleEvent(client.Event{Type: "session", SessionID: "sess-placeholder"}) + _ = m.sendPrompt("first") + m.handleEvent(client.Event{Type: "session", SessionID: "sess-real"}) // confirms the minted id + _ = m.sendPrompt("second") // no further session frame arrives + + runCopySessionID(m) + if !m.copyFlashing() { + t.Error("the confirmed session id must still copy after a second prompt") + } +} + +// Resume path is unaffected: an adopted session is live and copyable without +// any prompt. +func TestCopySessionIDResumedSessionCopiesWithoutPrompt(t *testing.T) { + m := wired(t) + m.handleSessionDetail(sessionDetailMsg{sess: client.Session{ID: "sess-resumed"}}) + runCopySessionID(m) + if !m.copyFlashing() { + t.Error("an adopted (resumed) session id must copy without a prompt") + } +} + +// BUG 2: the reconnect consumes freshStart before the fresh connection's +// session frame is ingested, so rememberSession re-persists the placeholder. +func TestNewReconnectDoesNotRepersistPlaceholder(t *testing.T) { + t.Setenv("BODEK_WORKSPACE", filepath.Join(t.TempDir(), "workspaces.json")) + m := wired(t) + m.ws = workspace.Open() + m.opts.CWD = "/tmp/bodek-test-fresh-remember" + _ = m.ws.Save(m.opts.CWD, workspace.State{SessionID: "s1"}) + seedSessionState(m) + + runNew(m) + if got := m.ws.Load(m.opts.CWD).SessionID; got != "" { + t.Fatalf("/new must clear the mapping, got %q", got) + } + + m.disconn = true + m.handleReconnect(reconnectMsg{cl: m.cl}) + m.handleEvent(client.Event{Type: "session", SessionID: "sess-placeholder"}) + + if got := m.ws.Load(m.opts.CWD).SessionID; got != "" { + t.Errorf("fresh connection's placeholder frame re-persisted session %q", got) + } + + // The suppression is one-shot: the real id from a later frame persists. + m.handleEvent(client.Event{Type: "session", SessionID: "sess-real"}) + if got := m.ws.Load(m.opts.CWD).SessionID; got != "sess-real" { + t.Errorf("real session id must persist after the fresh session starts, got %q", got) + } +} diff --git a/internal/update/update.go b/internal/update/update.go index 2e6a45c..3b9ea01 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -189,14 +189,20 @@ func statusRetryable(err error) bool { } // Newer reports whether latest is a higher version than current. Both may -// carry a "v" prefix; comparison is numeric over up to 3 dot-separated -// components, with missing components treated as 0. A Go pseudo-version +// carry a "v" prefix; both sides are reduced to their release core by +// pseudoBase before comparison — including the latest side, so a +// prerelease tag like "v1.2.0-rc1" no longer fails the numeric parse and +// reads as "already up to date" forever. Consequence of the release-core +// rule: "v1.2.0-rc1" against current "v1.2.0" compares equal and reports +// false — this simple comparator ignores prerelease ordering. Comparison +// is numeric over up to 3 dot-separated components, with missing +// components treated as 0. A Go pseudo-version // current (v0.1.3-0.20260901abcdef12-abc1234) compares by its release // prefix, so a commit-installed build still sees newer releases. Anything // unparsable — including a dev build's "dev" or an empty string — reports // false, so a failed or skipped check never nags. func Newer(latest, current string) bool { - l, lok := parseSemver(latest) + l, lok := parseSemver(pseudoBase(latest)) c, cok := parseSemver(pseudoBase(current)) if !lok || !cok { return false diff --git a/internal/update/update_test.go b/internal/update/update_test.go index 5074af6..c791fbd 100644 --- a/internal/update/update_test.go +++ b/internal/update/update_test.go @@ -27,6 +27,11 @@ func TestNewer(t *testing.T) { {"v0.0.12", "0.0", true}, // two-component current pads to 0 {"v0.0.1.2", "0.0.1", false}, // four components: unparsable {"v0.0.x", "0.0.1", false}, // non-numeric component + // Prerelease latest: both sides compare by release core, so + // "v1.2.0-rc1" must not fail the numeric parse of the latest side. + {"v1.2.0-rc1", "v1.1.9", true}, + {"v1.2.0-rc1", "v1.2.0", false}, // release core equal → not newer + {"1.3.0-rc1", "v1.2.0", true}, } for _, tc := range cases { if got := Newer(tc.latest, tc.current); got != tc.want { From 5cb606e115d5e4f46313f30b0fa38e92c92e3b69 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Wed, 30 Sep 2026 12:35:03 +0200 Subject: [PATCH 2/2] fix(server): derace TestConnectSpawnTokenFromStderr banner token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fake banner carried ?token=cafef00d, so waitSpawned returned as soon as line 1 was scanned — Stop() then closed the pipe while lines 2-3 were still in flight, failing the passthrough assertions on fast runners. The banner now omits the token so the wait ends on the WS token line, which also makes the token assertion prove the WS-token path instead of the banner fallback. --- internal/server/server_internal_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/server/server_internal_test.go b/internal/server/server_internal_test.go index 0983ef3..dffe0cc 100644 --- a/internal/server/server_internal_test.go +++ b/internal/server/server_internal_test.go @@ -179,8 +179,12 @@ func fakeOdekScript(t *testing.T, stderrLines ...string) string { func TestConnectSpawnTokenFromStderr(t *testing.T) { // A current odek serve prints its token to stderr; Connect must pick it up // from the "WS token:" line while passing stderr through verbatim. + // No ?token= in the banner: the wait must end on the "WS token:" line, + // which also guarantees every earlier stderr line has been copied + // through before Stop closes the pipe (a token-bearing banner let + // Connect return after line 1 and race the copier on lines 2-3). bin := fakeOdekScript(t, - "odek serve ⚡ http://127.0.0.1:9999/?token=cafef00d", + "odek serve ⚡ http://127.0.0.1:9999", " WebSocket: ws://127.0.0.1:9999/ws", " WS token: cafef00d", )