fix(lazy-patch): address review feedback on PR #44

- LazyObject.__newindex: raise on non-string keys instead of silently
  recording an unreadable patch (Copilot C3).
- Remove unused has_patches / has_deletions helpers (Copilot C2).
- encode_with_patches: classify each patch in a single qjson_cursor_field_bytes
  pass (was doing two) and propagate unexpected return codes via check()
  (Copilot C4 + CodeRabbit R6).
- encode_lazy_object_walking_with_patches: propagate unexpected
  qjson_cursor_field_bytes return codes (CodeRabbit R7).
- lazy_object_iter: same error-propagation fix (CodeRabbit R5).
- tests/lua/lazy_patch_spec.lua: extend special-key test to verify encoded
  output, swap the unicode test to multi-byte UTF-8, add "delete non-existent
  field", "all originals deleted + new fields added", and "non-string key
  raises" cases (CodeRabbit R8/R9/R10 + nitpick N2).
- docs/lazy-patch-spec.md: add upfront note that the listing is design
  pseudocode (pointing helper names at qjson_cursor_field_bytes), and
  correct the patch-record example to include lua_value alongside
  encoded_value (CodeRabbit R1 + nitpick N3).

Author: membphis

docs/lazy-patch-spec.md

@@ -1,5 +1,7 @@
 # Lazy Patch: Structural Patching for qjson.decode
 
+> **Note:** This document is the original *design* memo. The Lua snippets below are high-level pseudocode showing intent — they are not the shipped code. The actual implementation lives in `lua/qjson/table.lua` and `src/ffi.rs`; the splice path is used only for the "all patches target existing fields, no deletions" case, while deletions and new fields fall through to a walking-based encoder (`encode_lazy_object_walking_with_patches`) that avoids the comma-rewriting issues that the splice pseudocode below has. Helper names like `find_field_value_span` correspond to the FFI symbol `qjson_cursor_field_bytes` (`src/ffi.rs:821`).
+
 ## Overview
 
 This spec describes an optimization for the "decode → modify few fields → encode" workflow. Instead of materializing the entire root container on first write, we record patches and apply them during encode by splicing the original buffer.
@@ -53,12 +55,13 @@ local out = qjson.encode(tab)       -- Splices: buf[0..model_start] + "gpt4o" +
 #### Patch Record (Lua side)
 
 ```lua
--- Stored in the lazy view's internal table
+-- Stored in the lazy view's internal table.
+-- Each patch records key, encoded JSON value, and the original Lua value
+-- (so __index can hand back the user's value without re-decoding).
+-- Byte offsets are resolved lazily during encode via qjson_cursor_field_bytes.
 _patches = {
-    -- Each patch records the key and new encoded value
-    -- Byte offsets are resolved lazily during encode
-    { key = "model", encoded_value = '"gpt4o"' },
-    { key = "temperature", encoded_value = "0.9" },
+    { key = "model",       encoded_value = '"gpt4o"', lua_value = "gpt4o" },
+    { key = "temperature", encoded_value = "0.9",      lua_value = 0.9 },
 }
 ```
 
@@ -105,23 +108,23 @@ LazyObject.__newindex = function(t, k, v)
             end
         end
     else
-        -- Encode the new value
+        -- Encode the new value (we keep both encoded and the original Lua
+        -- value so __index can return v without re-decoding).
         local encoded = encode_value(v)
-        
-        -- Update or add patch
+
         local found = false
         for _, p in ipairs(patches) do
             if p.key == k then
                 p.encoded_value = encoded
+                p.lua_value = v
                 found = true
                 break
             end
         end
         if not found then
-            patches[#patches + 1] = { key = k, encoded_value = encoded }
+            patches[#patches + 1] = { key = k, encoded_value = encoded, lua_value = v }
         end
-        
-        -- Remove from deleted if previously deleted
+
         deleted[k] = nil
     end
 end

lua/qjson/table.lua

@@ -210,6 +210,7 @@ local function lazy_object_iter(state, _prev_key)
             if rc == QJSON_NOT_FOUND then
                 return p.key, p.lua_value
             end
+            check(rc)  -- propagate unexpected errors
         end
     end
 
@@ -324,6 +325,9 @@ local encode
 -- On write, record a patch instead of materializing the entire object.
 -- This allows encode to splice the original buffer with patched values.
 LazyObject.__newindex = function(t, k, v)
+    if type(k) ~= "string" then
+        error("qjson: LazyObject key must be a string, got " .. type(k))
+    end
     -- Initialize patches table if needed
     if not rawget(t, "_patches") then
         rawset(t, "_patches", {})
@@ -515,20 +519,6 @@ local function is_dirty(v)
     return false
 end
 
--- Check if a lazy view has patches (but not necessarily dirty children)
-local function has_patches(v)
-    local patches = rawget(v, "_patches")
-    return patches and #patches > 0
-end
-
--- Check if a lazy view has deletions
-local function has_deletions(v)
-    local deleted = rawget(v, "_deleted")
-    if not deleted then return false end
-    for _ in pairs(deleted) do return true end
-    return false
-end
-
 -- Walk a dirty LazyObject and emit JSON, preferring cached children (which
 -- may be materialized) over freshly resolved cursors. Non-cached children
 -- emit through a fresh proxy and naturally fast-path their unmodified subtree.
@@ -637,6 +627,8 @@ local function encode_lazy_object_walking_with_patches(t, patches, deleted)
         local rc = C.qjson_cursor_field_bytes(t._cur, p.key, #p.key, sz_a, sz_b)
         if rc == QJSON_NOT_FOUND then
             parts[#parts + 1] = encode_string(p.key) .. ":" .. p.encoded_value
+        elseif rc ~= QJSON_OK then
+            check(rc)  -- propagate unexpected errors
         end
     end
 
@@ -650,39 +642,31 @@ local function encode_with_patches(t)
     local patches = rawget(t, "_patches") or {}
     local deleted = rawget(t, "_deleted") or {}
 
-    -- Collect replacements: { {start, end_, value}, ... }
+    -- Classify each patch into a replacement (existing field) or a new field
+    -- in a single FFI pass. Replacements carry the byte span; presence of any
+    -- new field forces the walking fallback.
     local replacements = {}
-
-    -- For each patch, find the field's byte range in the original buffer
+    local has_new_field = false
     for _, p in ipairs(patches) do
         local rc = C.qjson_cursor_field_bytes(t._cur, p.key, #p.key, sz_a, sz_b)
         if rc == QJSON_OK then
-            -- Existing field: replace value
             replacements[#replacements + 1] = {
                 start = tonumber(sz_a[0]),
                 end_ = tonumber(sz_b[0]),
                 value = p.encoded_value,
             }
+        elseif rc == QJSON_NOT_FOUND then
+            has_new_field = true
+        else
+            check(rc)  -- propagate unexpected errors
         end
-        -- If NOT_FOUND, it's a new field - handled separately below
     end
 
-    -- Collect deleted field spans (we need to find the full field span including key)
-    -- For simplicity, we'll handle deletions by walking and skipping deleted keys
+    -- Deletions and new fields are handled by the walking fallback,
+    -- which avoids the comma-rewriting headache the splice path would have.
     local has_deleted = false
     for _ in pairs(deleted) do has_deleted = true; break end
 
-    -- If we have deletions or new fields, fall back to walking
-    -- (splicing deletions is complex due to comma handling)
-    local has_new_field = false
-    for _, p in ipairs(patches) do
-        local rc = C.qjson_cursor_field_bytes(t._cur, p.key, #p.key, sz_a, sz_b)
-        if rc == QJSON_NOT_FOUND then
-            has_new_field = true
-            break
-        end
-    end
-
     if has_deleted or has_new_field then
         return encode_lazy_object_walking_with_patches(t, patches, deleted)
     end

tests/lua/lazy_patch_spec.lua

@@ -242,16 +242,21 @@ describe("Lazy Patch - edge cases", function()
         local t = qjson.decode('{"a.b":1}')
         t["a.b"] = 10
         assert.are.equal(10, t["a.b"])
+        local out = qjson.encode(t)
+        local cjson = require("cjson")
+        local parsed = cjson.decode(out)
+        assert.are.equal(10, parsed["a.b"])
     end)
 
     it("handles unicode in values", function()
         local t = qjson.decode('{"a":"hello"}')
-        t.a = "hello world"
-        assert.are.equal("hello world", t.a)
+        local unicode = "你好 🌏 café"
+        t.a = unicode
+        assert.are.equal(unicode, t.a)
         local out = qjson.encode(t)
         local cjson = require("cjson")
         local parsed = cjson.decode(out)
-        assert.are.equal("hello world", parsed.a)
+        assert.are.equal(unicode, parsed.a)
     end)
 
     it("handles boolean values", function()
@@ -273,6 +278,38 @@ describe("Lazy Patch - edge cases", function()
         local parsed = cjson.decode(out)
         assert.are.equal(cjson.null, parsed.a)
     end)
+
+    it("deletes a non-existent field without error", function()
+        local t = qjson.decode('{"a":1}')
+        t.b = nil
+        assert.is_nil(t.b)
+        assert.are.equal(1, t.a)
+        local out = qjson.encode(t)
+        local cjson = require("cjson")
+        local parsed = cjson.decode(out)
+        assert.is_nil(parsed.b)
+        assert.are.equal(1, parsed.a)
+    end)
+
+    it("handles all original fields deleted plus new fields added", function()
+        local t = qjson.decode('{"a":1,"b":2}')
+        t.a = nil
+        t.b = nil
+        t.c = 3
+        t.d = 4
+        local out = qjson.encode(t)
+        local cjson = require("cjson")
+        local parsed = cjson.decode(out)
+        assert.is_nil(parsed.a)
+        assert.is_nil(parsed.b)
+        assert.are.equal(3, parsed.c)
+        assert.are.equal(4, parsed.d)
+    end)
+
+    it("rejects non-string keys on LazyObject", function()
+        local t = qjson.decode('{"a":1}')
+        assert.has_error(function() t[1] = "x" end)
+    end)
 end)
 
 describe("Lazy Patch - metatable preservation", function()