From 8db3c8c7f7381641dd5c083444d154e53b4cb44a Mon Sep 17 00:00:00 2001 From: Will Anderson Date: Wed, 17 Jun 2026 12:59:13 -0500 Subject: [PATCH] fix(chat): harden bridge_save/agentic_resume against empty and corrupt state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 16 +- tests/test_bridge_serialization.el | 257 +++++++++++++++++++++++++++++ tests/test_sessions_routes.el | 171 +++++++++++++++++++ 3 files changed, 442 insertions(+), 2 deletions(-) create mode 100644 tests/test_bridge_serialization.el create mode 100644 tests/test_sessions_routes.el diff --git a/chat.el b/chat.el index f53957f..83972f6 100644 --- a/chat.el +++ b/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") diff --git a/tests/test_bridge_serialization.el b/tests/test_bridge_serialization.el new file mode 100644 index 0000000..290b9f1 --- /dev/null +++ b/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") diff --git a/tests/test_sessions_routes.el b/tests/test_sessions_routes.el new file mode 100644 index 0000000..8810cf6 --- /dev/null +++ b/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")