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/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", ) 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 {