fix(chat): harden bridge_save/agentic_resume against empty and corrupt state

BLOCKER 1: use untyped reassignment (let x = ...) for the fallback bindings
in agentic_resume instead of re-declaring typed let bindings (let x: Type = ...)
for the same variable in the same scope. The typed form risks shadowing semantics
that differ from the established pattern used everywhere else in the loop
(e.g. agentic_loop line 720).

BLOCKER 2: add empty-string guards in both bridge_save and agentic_resume.
bridge_save now returns false without writing state if messages or tools_json
is empty — preventing syntactically invalid JSON blobs. agentic_resume now
returns an error envelope after the fallback resolution if either field is
still empty, rather than passing empty strings into agentic_loop which would
silently start a fresh turn with no context.

Also add tests:
- test_bridge_serialization.el: covers bridge_save empty-guard, golden-path
  raw-JSON round-trip, agentic_resume unknown/corrupt/missing-fields paths,
  and legacy string-escaped fallback path
- test_sessions_routes.el: covers DELETE and PATCH /api/sessions/:id routes
  (valid args, unknown id, empty body) and GET /api/sessions regression after
  removal of the duplicate route_sessions() handler

chat.el

@@ -772,6 +772,12 @@ fn agentic_loop(session_id: String, model: String, safe_sys: String, tools_json:
 // stored `messages` already includes the assistant turn that requested the tool, so
 // resume just appends the client's tool_result for `tool_use_id`.
 fn bridge_save(session_id: String, model: String, safe_sys: String, tools_json: String, messages: String, tools_log: String, tool_use_id: String) -> Bool {
+    // Guard: empty messages or tools_json would produce syntactically invalid JSON.
+    // Return false so the caller detects the failure rather than writing a corrupt
+    // blob that agentic_resume would later resume with no context.
+    if str_eq(messages, "") || str_eq(tools_json, "") {
+        return false
+    }
     // messages and tools_json are already well-formed JSON arrays; embed them as raw
     // JSON values (not string-escaped) so the round-trip through state_get/json_get_raw
     // never corrupts nested quotes. Scalar strings (model, safe_sys, tools_log,
@@ -802,9 +808,15 @@ fn agentic_resume(session_id: String, tool_use_id: String, content: String) -> S
     // messages_raw and tools_raw are embedded as raw JSON (not string-escaped);
     // fall back to legacy string-escaped fields for sessions saved before this fix.
     let messages: String = json_get_raw(blob, "messages_raw")
-    let messages: String = if str_eq(messages, "") { json_get(blob, "messages") } else { messages }
+    let messages = if str_eq(messages, "") { json_get(blob, "messages") } else { messages }
     let tools_json: String = json_get_raw(blob, "tools_raw")
-    let tools_json: String = if str_eq(tools_json, "") { json_get(blob, "tools_json") } else { tools_json }
+    let tools_json = if str_eq(tools_json, "") { json_get(blob, "tools_json") } else { tools_json }
+    // Guard: a corrupt or missing bridge blob (e.g. state cleared mid-flight)
+    // yields empty messages/tools. Return an error envelope rather than resuming
+    // with no context, which would cause the model to start a fresh turn.
+    if str_eq(messages, "") || str_eq(tools_json, "") {
+        return "{\"error\":\"corrupt bridge state\",\"reply\":\"\"}"
+    }
     let tools_log: String = json_get(blob, "tools_log")
     let saved_use_id: String = json_get(blob, "tool_use_id")
 

tests/test_bridge_serialization.el

@@ -0,0 +1,257 @@
+// ── test_bridge_serialization.el ──────────────────────────────────────────────
+//
+// Tests for PR #20 fix/bridge-save-serialization:
+//   - bridge_save raw JSON serialization (BLOCKER 1 & 2 regression guards)
+//   - agentic_resume error-path handling
+//   - Legacy fallback: old string-escaped fields still readable
+//   - Corrupt/missing bridge state error envelope
+//   - Empty messages/tools_json guard in bridge_save
+//
+// What CANNOT be tested here without a live Anthropic API:
+//   - agentic_resume golden-path (calls agentic_loop which hits the API)
+//   - Full save/resume round-trip with a real tool_result
+//
+// To run:
+//   elc chat.el && ./soul --test tests/test_bridge_serialization.el
+//
+// ──────────────────────────────────────────────────────────────────────────────
+
+import "../chat.el"
+
+// ── Test harness ──────────────────────────────────────────────────────────────
+
+let pass_count: Int = 0
+let fail_count: Int = 0
+
+fn assert_eq(label: String, got: String, expected: String) -> Void {
+    if str_eq(got, expected) {
+        let pass_count = pass_count + 1
+        println("  PASS: " + label)
+    } else {
+        let fail_count = fail_count + 1
+        println("  FAIL: " + label)
+        println("    got:      " + got)
+        println("    expected: " + expected)
+    }
+}
+
+fn assert_true(label: String, cond: Bool) -> Void {
+    if cond {
+        let pass_count = pass_count + 1
+        println("  PASS: " + label)
+    } else {
+        let fail_count = fail_count + 1
+        println("  FAIL: " + label)
+    }
+}
+
+fn assert_false(label: String, cond: Bool) -> Void {
+    assert_true(label, !cond)
+}
+
+fn assert_contains(label: String, haystack: String, needle: String) -> Void {
+    if str_contains(haystack, needle) {
+        let pass_count = pass_count + 1
+        println("  PASS: " + label)
+    } else {
+        let fail_count = fail_count + 1
+        println("  FAIL: " + label)
+        println("    missing '" + needle + "' in: " + haystack)
+    }
+}
+
+fn assert_not_contains(label: String, haystack: String, needle: String) -> Void {
+    if str_contains(haystack, needle) {
+        let fail_count = fail_count + 1
+        println("  FAIL: " + label)
+        println("    unexpected '" + needle + "' found in: " + haystack)
+    } else {
+        let pass_count = pass_count + 1
+        println("  PASS: " + label)
+    }
+}
+
+fn assert_not_empty(label: String, s: String) -> Void {
+    if str_eq(s, "") {
+        let fail_count = fail_count + 1
+        println("  FAIL: " + label + " (got empty string)")
+    } else {
+        let pass_count = pass_count + 1
+        println("  PASS: " + label)
+    }
+}
+
+// ── Section 1: bridge_save — empty messages guard ─────────────────────────────
+//
+// BLOCKER 2 regression guard: bridge_save must refuse to write a blob when
+// messages or tools_json is empty, as the resulting JSON would be syntactically
+// invalid (bare colon with no value).
+
+println("")
+println("1. bridge_save — empty messages guard")
+
+let sid1: String = "test-session-empty-messages"
+state_set("mcp_bridge:" + sid1, "")
+
+let save1_ok: Bool = bridge_save(sid1, "claude-sonnet-4-5", "sys", "[]", "", "", "call-1")
+assert_false("empty messages -> bridge_save returns false", save1_ok)
+
+let saved1: String = state_get("mcp_bridge:" + sid1)
+assert_eq("empty messages -> no blob written to state", saved1, "")
+
+// ── Section 2: bridge_save — empty tools_json guard ───────────────────────────
+
+println("")
+println("2. bridge_save — empty tools_json guard")
+
+let sid2: String = "test-session-empty-tools"
+state_set("mcp_bridge:" + sid2, "")
+
+let save2_ok: Bool = bridge_save(sid2, "claude-sonnet-4-5", "sys", "", "[{\"role\":\"user\",\"content\":\"hi\"}]", "", "call-2")
+assert_false("empty tools_json -> bridge_save returns false", save2_ok)
+
+let saved2: String = state_get("mcp_bridge:" + sid2)
+assert_eq("empty tools_json -> no blob written to state", saved2, "")
+
+// ── Section 3: bridge_save — golden path writes raw JSON fields ───────────────
+//
+// Verifies that messages_raw and tools_raw are stored as inline JSON (not
+// string-escaped) so that json_get_raw retrieves them without corruption.
+
+println("")
+println("3. bridge_save — golden path writes messages_raw and tools_raw as raw JSON")
+
+let sid3: String = "test-session-golden"
+state_set("mcp_bridge:" + sid3, "")
+
+let msgs3: String = "[{\"role\":\"user\",\"content\":\"hello\"}]"
+let tools3: String = "[{\"name\":\"read_file\"}]"
+let save3_ok: Bool = bridge_save(sid3, "claude-sonnet-4-5", "You are a helper.", tools3, msgs3, "read_file", "toolu_abc")
+assert_true("valid args -> bridge_save returns true", save3_ok)
+
+let blob3: String = state_get("mcp_bridge:" + sid3)
+assert_not_empty("valid args -> blob written to state", blob3)
+
+// messages_raw should be stored as a raw JSON array (not a quoted string)
+// so json_get_raw on the blob returns the array directly
+let raw_msgs3: String = json_get_raw(blob3, "messages_raw")
+assert_contains("messages_raw field present in blob", blob3, "messages_raw")
+assert_eq("messages_raw round-trips without corruption", raw_msgs3, msgs3)
+
+let raw_tools3: String = json_get_raw(blob3, "tools_raw")
+assert_eq("tools_raw round-trips without corruption", raw_tools3, tools3)
+
+// Scalar fields should still be present as normal string-escaped JSON fields
+let model3: String = json_get(blob3, "model")
+assert_eq("model field preserved in blob", model3, "claude-sonnet-4-5")
+
+let tool_use_id3: String = json_get(blob3, "tool_use_id")
+assert_eq("tool_use_id field preserved in blob", tool_use_id3, "toolu_abc")
+
+// Verify the blob does NOT contain old-style double-escaped fields
+assert_not_contains("no legacy 'messages' string field in new-format blob", blob3, "\"messages\":\"")
+assert_not_contains("no legacy 'tools_json' string field in new-format blob", blob3, "\"tools_json\":\"")
+
+// ── Section 4: agentic_resume — unknown session_id returns error envelope ──────
+
+println("")
+println("4. agentic_resume — unknown session_id (empty state)")
+
+let sid4: String = "test-session-unknown-xyzzy"
+state_set("mcp_bridge:" + sid4, "")
+
+let resume4: String = agentic_resume(sid4, "toolu_xyz", "some result")
+assert_contains("unknown session_id -> error field present", resume4, "\"error\"")
+assert_contains("unknown session_id -> reply field present", resume4, "\"reply\"")
+assert_contains("unknown session_id -> 'unknown session_id' message", resume4, "unknown session_id")
+let reply4: String = json_get(resume4, "reply")
+assert_eq("unknown session_id -> reply is empty string", reply4, "")
+
+// ── Section 5: agentic_resume — syntactically invalid JSON in state ───────────
+
+println("")
+println("5. agentic_resume — syntactically invalid JSON blob in state")
+
+let sid5: String = "test-session-corrupt-json"
+// Write a non-JSON value that state_get would return as-is
+state_set("mcp_bridge:" + sid5, "NOT_JSON_AT_ALL")
+
+let resume5: String = agentic_resume(sid5, "toolu_xyz", "some result")
+// The function may take multiple paths here; in all cases it must not crash and
+// must return a JSON envelope with at least an error or empty reply field.
+// When json_get_raw returns "" on unparseable input, the guard catches it.
+assert_contains("corrupt JSON blob -> resume returns JSON", resume5, "\"reply\"")
+
+// ── Section 6: agentic_resume — blob with no messages produces error envelope ─
+
+println("")
+println("6. agentic_resume — blob missing messages_raw and messages fields")
+
+let sid6: String = "test-session-no-messages"
+// Blob with only model/safe_sys — no messages or tools
+state_set("mcp_bridge:" + sid6, "{\"model\":\"claude-sonnet-4-5\",\"safe_sys\":\"sys\",\"tool_use_id\":\"toolu_abc\"}")
+
+let resume6: String = agentic_resume(sid6, "toolu_abc", "result")
+assert_contains("missing messages -> error field present", resume6, "\"error\"")
+assert_contains("missing messages -> error mentions corrupt state", resume6, "corrupt bridge state")
+let reply6: String = json_get(resume6, "reply")
+assert_eq("missing messages -> reply is empty string", reply6, "")
+
+// ── Section 7: Legacy fallback — old-format blob (string-escaped fields) ──────
+//
+// BLOCKER 1 regression guard: sessions saved before the fix used 'messages'
+// and 'tools_json' as string-escaped fields. The fallback path in agentic_resume
+// must read them correctly. We verify the fallback resolves the correct values
+// before the function reaches the api call (which we cannot make in tests).
+//
+// We test the fallback by writing a legacy blob and verifying that
+// agentic_resume does NOT return the "corrupt bridge state" error
+// (which would mean the fallback is broken), instead it gets past the guard
+// and then fails on the API call (outside our test scope).
+//
+// NOTE: We cannot confirm a successful API-dependent round-trip in this test;
+// the goal is only to confirm the state-reading fallback path resolves values.
+
+println("")
+println("7. Legacy fallback — old-format blob with string-escaped 'messages' field")
+
+let sid7: String = "test-session-legacy-format"
+// Simulate an old-format blob: messages and tools_json as json_safe-escaped strings.
+// json_safe escapes " to \" so the stored value is a JSON string containing the array.
+let legacy_msgs: String = "[{\"role\":\"user\",\"content\":\"legacy hello\"}]"
+let legacy_tools: String = "[{\"name\":\"read_file\"}]"
+// Build the blob the OLD way: string-escaped
+let safe_msgs: String = json_safe(legacy_msgs)
+let safe_tools: String = json_safe(legacy_tools)
+let legacy_blob: String = "{\"model\":\"claude-sonnet-4-5\",\"safe_sys\":\"sys\",\"messages\":\"" + safe_msgs + "\",\"tools_json\":\"" + safe_tools + "\",\"tool_use_id\":\"toolu_legacy\"}"
+state_set("mcp_bridge:" + sid7, legacy_blob)
+
+let resume7: String = agentic_resume(sid7, "toolu_legacy", "legacy result")
+// The fallback should successfully read the fields and NOT return "corrupt bridge state"
+assert_not_contains("legacy blob -> no 'corrupt bridge state' error (fallback working)", resume7, "corrupt bridge state")
+// It will fail on API call in test env, but should get past the state-reading guard
+// Accept "unknown session_id" NOT happening - the blob was found, just API fails
+
+// ── Section 8: bridge_save with tool_use_id containing special chars ──────────
+
+println("")
+println("8. bridge_save — tool_use_id with JSON-special characters is escaped")
+
+let sid8: String = "test-session-special-chars"
+state_set("mcp_bridge:" + sid8, "")
+
+let special_id: String = "toolu_test\"quoted\""
+let msgs8: String = "[{\"role\":\"user\",\"content\":\"hi\"}]"
+let tools8: String = "[{\"name\":\"read_file\"}]"
+let save8_ok: Bool = bridge_save(sid8, "claude-sonnet-4-5", "sys", tools8, msgs8, "", special_id)
+assert_true("special chars in tool_use_id -> bridge_save returns true", save8_ok)
+
+let blob8: String = state_get("mcp_bridge:" + sid8)
+// The blob must be parseable (json_get succeeds on it)
+let retrieved_id: String = json_get(blob8, "tool_use_id")
+assert_eq("tool_use_id with quotes round-trips via json_safe", retrieved_id, special_id)
+
+// ── Summary ────────────────────────────────────────────────────────────────────
+
+println("")
+println("test_bridge_serialization.el: " + int_to_str(pass_count) + " passed, " + int_to_str(fail_count) + " failed")

tests/test_sessions_routes.el

@@ -0,0 +1,171 @@
+// ── test_sessions_routes.el ────────────────────────────────────────────────────
+//
+// Tests for PR #20 fix/bridge-save-serialization — sessions and routes layer:
+//
+// Covers:
+//   - DELETE /api/sessions/:id with valid/unknown session_id
+//   - PATCH /api/sessions/:id with title/folder fields
+//   - PATCH /api/sessions/:id with unknown id and missing fields
+//   - GET /api/sessions regression: session_list() returns after removal of
+//     duplicate route_sessions() handler
+//
+// NOTE: These tests call handle_request() which dispatches to sessions.el
+// functions that use engram_search_json. Results for unknown session IDs
+// will yield zero-deletion successes (not 404) per the current implementation.
+//
+// To run:
+//   elc routes.el && ./soul --test tests/test_sessions_routes.el
+//
+// ──────────────────────────────────────────────────────────────────────────────
+
+import "../routes.el"
+
+// ── Test harness ──────────────────────────────────────────────────────────────
+
+let pass_count: Int = 0
+let fail_count: Int = 0
+
+fn assert_eq(label: String, got: String, expected: String) -> Void {
+    if str_eq(got, expected) {
+        let pass_count = pass_count + 1
+        println("  PASS: " + label)
+    } else {
+        let fail_count = fail_count + 1
+        println("  FAIL: " + label)
+        println("    got:      " + got)
+        println("    expected: " + expected)
+    }
+}
+
+fn assert_contains(label: String, haystack: String, needle: String) -> Void {
+    if str_contains(haystack, needle) {
+        let pass_count = pass_count + 1
+        println("  PASS: " + label)
+    } else {
+        let fail_count = fail_count + 1
+        println("  FAIL: " + label)
+        println("    missing '" + needle + "' in: " + haystack)
+    }
+}
+
+fn assert_not_contains(label: String, haystack: String, needle: String) -> Void {
+    if str_contains(haystack, needle) {
+        let fail_count = fail_count + 1
+        println("  FAIL: " + label)
+        println("    unexpected '" + needle + "' found in: " + haystack)
+    } else {
+        let pass_count = pass_count + 1
+        println("  PASS: " + label)
+    }
+}
+
+fn assert_true(label: String, cond: Bool) -> Void {
+    if cond {
+        let pass_count = pass_count + 1
+        println("  PASS: " + label)
+    } else {
+        let fail_count = fail_count + 1
+        println("  FAIL: " + label)
+    }
+}
+
+// ── Section 1: DELETE /api/sessions/:id — unknown id ─────────────────────────
+//
+// session_delete does not return 404 for unknown ids; it returns ok:true with
+// zero-count deletions. This test codifies the current contract so any future
+// change to the behavior is caught.
+
+println("")
+println("1. DELETE /api/sessions/:id — unknown session_id")
+
+let del_unknown: String = handle_request("DELETE", "/api/sessions/nonexistent-session-uuid", "")
+assert_contains("DELETE unknown id -> ok field present", del_unknown, "\"ok\"")
+assert_contains("DELETE unknown id -> ok is true (zero-count success)", del_unknown, "\"ok\":true")
+assert_contains("DELETE unknown id -> deleted_meta count present", del_unknown, "deleted_meta")
+assert_contains("DELETE unknown id -> deleted_msgs count present", del_unknown, "deleted_msgs")
+
+// ── Section 2: DELETE /api/sessions/:id — missing id ─────────────────────────
+
+println("")
+println("2. DELETE /api/sessions (no id in path) -> 404")
+
+let del_no_id: String = handle_request("DELETE", "/api/sessions", "")
+assert_contains("DELETE with no id -> 404 error", del_no_id, "\"error\"")
+
+// ── Section 3: PATCH /api/sessions/:id — update title ────────────────────────
+//
+// PATCH with a known title field should not error on the missing-fields check.
+// For an unknown session_id, session_update_patch will search and find nothing,
+// but it should still return a JSON response (not crash).
+
+println("")
+println("3. PATCH /api/sessions/:id — title field")
+
+let patch_title: String = handle_request("PATCH", "/api/sessions/test-sess-patch-1", "{\"title\":\"My new title\"}")
+// Should return JSON with ok field or error field — must not be empty
+assert_not_contains("PATCH title -> response is not empty", patch_title, "")
+assert_true("PATCH title -> response is non-empty string", str_len(patch_title) > 0)
+// Must not return the missing-fields error (since title IS provided)
+assert_not_contains("PATCH title -> no 'title or folder required' error", patch_title, "title or folder required")
+
+// ── Section 4: PATCH /api/sessions/:id — folder field ────────────────────────
+
+println("")
+println("4. PATCH /api/sessions/:id — folder field")
+
+let patch_folder: String = handle_request("PATCH", "/api/sessions/test-sess-patch-2", "{\"folder\":\"my-folder\"}")
+assert_true("PATCH folder -> response is non-empty", str_len(patch_folder) > 0)
+assert_not_contains("PATCH folder -> no 'title or folder required' error", patch_folder, "title or folder required")
+
+// ── Section 5: PATCH /api/sessions/:id — empty body (missing fields) ──────────
+
+println("")
+println("5. PATCH /api/sessions/:id — empty body returns field-required error")
+
+let patch_empty: String = handle_request("PATCH", "/api/sessions/test-sess-patch-3", "{}")
+assert_contains("PATCH empty body -> error field present", patch_empty, "\"error\"")
+assert_contains("PATCH empty body -> missing fields message", patch_empty, "title or folder required")
+
+// ── Section 6: PATCH /api/sessions (no id in path) -> 404 ────────────────────
+
+println("")
+println("6. PATCH /api/sessions (no id) -> 404")
+
+let patch_no_id: String = handle_request("PATCH", "/api/sessions", "{\"title\":\"x\"}")
+assert_contains("PATCH no id -> 404 error", patch_no_id, "\"error\"")
+
+// ── Section 7: GET /api/sessions — session_list regression ───────────────────
+//
+// After removal of the duplicate route_sessions() GET handler in routes.el,
+// GET /api/sessions must still return a valid JSON array (possibly empty) from
+// session_list(). Verifies the deduplication fix does not break the endpoint.
+
+println("")
+println("7. GET /api/sessions — session_list() returns valid JSON array")
+
+let get_sessions: String = handle_request("GET", "/api/sessions", "")
+assert_true("GET /api/sessions -> response is non-empty", str_len(get_sessions) > 0)
+// Result must be a JSON array (starts with '[')
+let first_char: String = str_slice(get_sessions, 0, 1)
+assert_eq("GET /api/sessions -> response is a JSON array", first_char, "[")
+
+// ── Section 8: DELETE then GET — session_index cache invalidation ─────────────
+//
+// After a DELETE, session_list() must not return the deleted session.
+// Since we don't have a real session to delete in this test environment,
+// we verify the GET still returns an array after the DELETE attempt.
+
+println("")
+println("8. GET /api/sessions after DELETE attempt -> still returns valid array")
+
+let del_first: String = handle_request("DELETE", "/api/sessions/test-cache-inval-sess", "")
+assert_contains("pre-DELETE: ok field present", del_first, "\"ok\"")
+
+let get_after_del: String = handle_request("GET", "/api/sessions", "")
+let first_char2: String = str_slice(get_after_del, 0, 1)
+assert_eq("GET after DELETE -> still returns JSON array", first_char2, "[")
+
+// ── Summary ────────────────────────────────────────────────────────────────────
+
+println("")
+println("test_sessions_routes.el: " + int_to_str(pass_count) + " passed, " + int_to_str(fail_count) + " failed")