feat(map-ai): repair round for rejected ops, and accept dominant-baseline (PPT-2832)

OPENAPI_DOC.yml

@@ -47306,6 +47306,10 @@ components:
           anyOf:
           - type: object
           - type: "null"
+        repair:
+          anyOf:
+          - type: object
+          - type: "null"
         resolution:
           anyOf:
           - type: object

spec/controllers/map_ai_spec.cr

@@ -372,6 +372,88 @@ module PlaceOS::Api
         ops[0].as_h.has_key?("server_issue").should be_false
         final["server_issues"].as_h.keys.should eq ["op-1"]
         final["server_issues"]["op-1"].as_s.should eq "anchor e404 is not in the index"
+        # the repair round answered the same, so the first answer stands
+        final["repair"].should eq JSON.parse(%({"rejected":1,"remaining":1,"used":false}))
+        final["usage"]["attempts"].as_i.should eq 2
+      end
+
+      it "asks once more with the rejected ops and keeps the corrected answer" do
+        setup_signage_ai
+        _, headers = Spec::Authentication.authentication
+        level = map_ai_level
+        requests = [] of JSON::Any
+
+        first = {
+          summary: "s",
+          ops:     [
+            {op: "set_attr", why: "w", target: {a: "e404", id: nil, h: nil}, name: "fill", value: "#fff"},
+            {op: "set_attr", why: "w", target: {a: "e5", id: nil, h: nil}, name: "fill", value: "#fff"},
+          ],
+          questions: [] of String,
+        }.to_json
+        second = {
+          summary: "s2",
+          ops:     [
+            {op: "set_attr", why: "w", target: {a: "e4", id: nil, h: nil}, name: "fill", value: "#fff"},
+            {op: "set_attr", why: "w", target: {a: "e5", id: nil, h: nil}, name: "fill", value: "#fff"},
+          ],
+          questions: [] of String,
+        }.to_json
+        HttpMocks.map_ai_vendor_sequence([first, second], requests)
+
+        result = client.post(File.join(base, "edit"), headers: headers, body: {zone_id: level.id, instruction: "x", context: map_ai_context}.to_json)
+        final = await_signage_ai_job(base, JSON.parse(result.body)["id"].as_s, headers)
+        final["state"].as_s.should eq "done"
+        final["summary"].as_s.should eq "s2"
+        final["ops"].as_a.map(&.["target"]["a"].as_s).should eq ["e4", "e5"]
+        final["server_issues"].as_h.should be_empty
+        final["repair"].should eq JSON.parse(%({"rejected":1,"remaining":0,"used":true}))
+        final["usage"]["attempts"].as_i.should eq 2
+        final["cost_units"].as_f.should eq 2560.0
+
+        requests.size.should eq 2
+        asked_again = requests[1].to_json
+        asked_again.should contain "REJECTED"
+        asked_again.should contain "op-1: anchor e404 is not in the index"
+      end
+
+      it "keeps the first answer when the repair would drop ops the server accepted" do
+        setup_signage_ai
+        _, headers = Spec::Authentication.authentication
+        level = map_ai_level
+        requests = [] of JSON::Any
+
+        first = {
+          summary: "s",
+          ops:     [
+            {op: "set_attr", why: "w", target: {a: "e404", id: nil, h: nil}, name: "fill", value: "#fff"},
+            {op: "set_attr", why: "w", target: {a: "e5", id: nil, h: nil}, name: "fill", value: "#fff"},
+          ],
+          questions: [] of String,
+        }.to_json
+        HttpMocks.map_ai_vendor_sequence([first, {summary: "gave up", ops: [] of String, questions: [] of String}.to_json], requests)
+
+        result = client.post(File.join(base, "edit"), headers: headers, body: {zone_id: level.id, instruction: "x", context: map_ai_context}.to_json)
+        final = await_signage_ai_job(base, JSON.parse(result.body)["id"].as_s, headers)
+        final["state"].as_s.should eq "done"
+        final["summary"].as_s.should eq "s"
+        final["ops"].as_a.size.should eq 2
+        final["repair"].should eq JSON.parse(%({"rejected":1,"remaining":0,"used":false}))
+        requests.size.should eq 2
+      end
+
+      it "does not ask again when every op passes" do
+        setup_signage_ai
+        _, headers = Spec::Authentication.authentication
+        level = map_ai_level
+        requests = [] of JSON::Any
+        HttpMocks.map_ai_vendor(MAP_AI_ANSWER, requests: requests)
+
+        result = client.post(File.join(base, "edit"), headers: headers, body: {zone_id: level.id, instruction: "x", context: map_ai_context}.to_json)
+        final = await_signage_ai_job(base, JSON.parse(result.body)["id"].as_s, headers)
+        final["state"].as_s.should eq "done"
+        final.as_h.has_key?("repair").should be_false
+        requests.size.should eq 1
       end
 
       it "records a refusal as a moderation failure" do

spec/llm_prompt_spec.cr

@@ -110,6 +110,36 @@ module PlaceOS::Api
       text.should_not contain "recolours Meeting 1.01"
     end
 
+    it "asks again with the rejected operations and why, after the instruction" do
+      request = Llm::Prompt.build(Llm::Prompt::Options.new(
+        context: context,
+        instruction: "Label Meeting 1.01.",
+        model: "gpt-6.1-sol",
+      ))
+      envelope = Llm::Ops::Envelope.new(
+        summary: "Labelled Meeting 1.01.",
+        ops: [
+          JSON.parse(%({"op":"set_attr","target":{"a":"e2","id":null,"h":null},"name":"fill","value":"#90ee90","why":"w"})),
+          JSON.parse(%({"op":"insert_shape","parent":null,"after":{"a":"e2","id":null,"h":null},"kind":"text","attrs":[],"new_id":"ai-label","text":"1.01","why":"w"})),
+        ],
+        questions: [] of String,
+      )
+
+      repair = Llm::Prompt.repair(request, envelope, {"op-2" => "after must be a child of the parent"})
+
+      repair.parts.size.should eq request.parts.size + 1
+      repair.parts[0...request.parts.size].should eq request.parts
+      repair.system.should eq request.system
+      repair.max_output_tokens.should eq request.max_output_tokens
+      text = repair.text
+      text.index!("REPAIR\n").should be > text.index!("INSTRUCTION\n")
+      text.should contain "PREVIOUS ANSWER summary: Labelled Meeting 1.01."
+      text.should contain %("op_id":"op-1","op":"set_attr")
+      text.should contain %("op_id":"op-2","op":"insert_shape")
+      text.should_not contain %("parent":null)
+      text.should contain "REJECTED\nop-2: after must be a child of the parent\n"
+    end
+
     it "caps the index, the subtrees and the history" do
       big = JSON.parse(context.to_json)
       index = big["index"].as_a
@@ -230,6 +260,8 @@ module PlaceOS::Api
       Llm::Values.attribute_value_issue("font-weight", "700").should be_nil
       Llm::Values.attribute_value_issue("font-weight", "heavy").should eq %("heavy" is not a font weight)
       Llm::Values.attribute_value_issue("text-anchor", "middle").should be_nil
+      Llm::Values.attribute_value_issue("dominant-baseline", "middle").should be_nil
+      Llm::Values.attribute_value_issue("dominant-baseline", "centre").should eq %("centre" is not a dominant baseline)
       Llm::Values.attribute_value_issue("data-name", "Boardroom").should be_nil
       Llm::Values.attribute_value_issue("data-name", "<b>x</b>").should eq "data-name may not contain markup"
     end
@@ -337,6 +369,9 @@ module PlaceOS::Api
         { %({"op":"insert_shape","parent":{"a":"e1","id":null,"h":null},"after":null,"kind":"path","attrs":[{"name":"d","value":"M0 0 L10 10 Z"}],"new_id":null,"text":null,"why":"w"}), nil },
         { %({"op":"insert_shape","parent":{"a":"e1","id":null,"h":null},"after":null,"kind":"polygon","attrs":[{"name":"points","value":"0,0 10,0 10"}],"new_id":null,"text":null,"why":"w"}), %("0,0 10,0 10" is not a points list) },
         { %({"op":"insert_shape","parent":{"a":"e99","id":null,"h":null},"after":null,"kind":"g","attrs":[],"new_id":null,"text":null,"why":"w"}), "parent: anchor e99 is not in the index" },
+        { %({"op":"insert_shape","parent":{"a":"e1","id":null,"h":null},"after":{"a":"e2","id":null,"h":null},"kind":"rect","attrs":[],"new_id":null,"text":null,"why":"w"}), nil },
+        { %({"op":"insert_shape","parent":null,"after":{"a":"e2","id":null,"h":null},"kind":"rect","attrs":[],"new_id":null,"text":null,"why":"w"}), "after must be a child of the parent" },
+        { %({"op":"insert_shape","parent":{"a":"e5","id":null,"h":null},"after":null,"kind":"rect","attrs":[],"new_id":null,"text":null,"why":"w"}), "parent must be the root or a <g>, not <rect>" },
         { %({"op":"set_style_rule","selector":".cls-12","declarations":[{"name":"fill","value":"#eee"},{"name":"stroke","value":"#333"}],"why":"w"}), nil },
         { %({"op":"set_style_rule","selector":"*","declarations":[{"name":"fill","value":"#eee"}],"why":"w"}), %(the selector "*" is not allowed) },
         { %({"op":"set_style_rule","selector":".a{} @import url(http://evil); .b","declarations":[{"name":"fill","value":"#eee"}],"why":"w"}), %(the selector ".a{} @import url(http://evil); .b" is not allowed) },
@@ -466,6 +501,64 @@ module PlaceOS::Api
       checked.issues["op-1"].should eq "ai-chair-2 is already an id in the map"
     end
 
+    it "places an insert after a child of its parent, the root when none is named" do
+      tree = JSON.parse(%({"index":[
+        {"a":"e0","id":"Layer_1","tag":"svg","p":null},
+        {"a":"e1","id":"room_meeting","tag":"g","p":null},
+        {"a":"e2","id":"room_border_8","tag":"g","p":"e1"},
+        {"a":"e3","id":"space_room_7","tag":"rect","p":"e2"},
+        {"a":"e4","id":"table","tag":"rect","p":"e1"}
+      ]}))
+      insert = ->(parent : String?, after : String?) do
+        JSON.parse({
+          "op" => "insert_shape", "kind" => "text", "text" => "Sydney Room 1", "attrs" => [] of String, "new_id" => nil,
+          "parent" => parent.try { |anchor| {"a" => anchor} }, "after" => after.try { |anchor| {"a" => anchor} },
+        }.to_json)
+      end
+      ops = [
+        insert.call("e2", "e3"), # after a child of the parent
+        insert.call("e1", "e3"), # after a grandchild: the gpt-5.4-mini mistake
+        insert.call(nil, "e1"),  # after a child of the root, no parent named
+        insert.call("e0", "e1"), # the root named as the parent
+        insert.call(nil, "e3"),  # not a child of the root
+        insert.call("e3", nil),  # a rect is no parent
+        insert.call("e1", nil),
+      ]
+      checked = Llm::Ops.check(ops, tree)
+      checked.issues.should eq({
+        "op-2" => "after must be a child of the parent",
+        "op-5" => "after must be a child of the parent",
+        "op-6" => "parent must be the root or a <g>, not <rect>",
+      })
+    end
+
+    it "leaves the after check to the editor once the batch deletes after or moves elements" do
+      tree = JSON.parse(%({"index":[
+        {"a":"e1","id":"room","tag":"g","p":null},
+        {"a":"e2","id":"border","tag":"g","p":"e1"},
+        {"a":"e3","id":"space","tag":"rect","p":"e2"},
+        {"a":"e4","id":"label","tag":"text","p":"e1"}
+      ]}))
+      deleted = [
+        %({"op":"delete","target":{"a":"e3"}}),
+        %({"op":"insert_shape","kind":"text","text":"x","attrs":[],"parent":{"a":"e1"},"after":{"a":"e3"}}),
+        %({"op":"insert_shape","kind":"text","text":"x","attrs":[],"parent":{"a":"e2"},"after":{"a":"e4"}}),
+      ].map { |op| JSON.parse(op) }
+      Llm::Ops.check(deleted, tree).issues.keys.should eq ["op-3"]
+
+      moved = [
+        %({"op":"group","targets":[{"a":"e3"},{"a":"e4"}],"new_id":"ai-group"}),
+        %({"op":"insert_shape","kind":"text","text":"x","attrs":[],"parent":{"a":"e2"},"after":{"a":"e4"}}),
+      ].map { |op| JSON.parse(op) }
+      Llm::Ops.check(moved, tree).issues.should be_empty
+
+      created = [
+        %({"op":"insert_shape","kind":"g","attrs":[],"new_id":"ai-wrap","parent":{"a":"e1"}}),
+        %({"op":"insert_shape","kind":"text","text":"x","attrs":[],"parent":{"id":"ai-wrap"},"after":{"a":"e4"}}),
+      ].map { |op| JSON.parse(op) }
+      Llm::Ops.check(created, tree).issues.should be_empty
+    end
+
     it "renumbers rooms sequentially and reuses the freed id for a new room" do
       rename_context = JSON.parse(%({"index":[{"a":"e1","id":"room-04","tag":"rect"},{"a":"e2","id":"room-05","tag":"rect"}]}))
       ops = [

spec/spec_helpers/http_mocks.cr

@@ -106,6 +106,22 @@ module PlaceOS::Api::HttpMocks
       end
   end
 
+  # `contents` answered in turn, the last one again once they run out.
+  def self.map_ai_vendor_sequence(contents : Array(String), requests : Array(JSON::Any))
+    WebMock
+      .stub(:post, CHAT_COMPLETIONS)
+      .to_return do |request|
+        requests << JSON.parse(WebMock.body(request).to_s)
+        content = contents[Math.min(requests.size, contents.size) - 1]
+        body = {
+          id:      "chatcmpl-spec",
+          choices: [{message: {role: "assistant", content: content, refusal: nil}, finish_reason: "stop"}],
+          usage:   {prompt_tokens: 1200, completion_tokens: 80, total_tokens: 1280},
+        }.to_json
+        HTTP::Client::Response.new(200, body, HTTP::Headers{"Content-Type" => "application/json"})
+      end
+  end
+
   # The model declining to answer, which the adapter maps to `Moderated`.
   def self.map_ai_vendor_refused
     body = {

src/constants.cr

@@ -78,6 +78,9 @@ module PlaceOS::Api
   # the second attempt's output budget after an answer cut off at MAP_AI_MAX_OUTPUT_TOKENS
   MAP_AI_RETRY_OUTPUT_TOKENS = (ENV["MAP_AI_RETRY_OUTPUT_TOKENS"]? || "24000").to_i
 
+  # one more vendor call, inside the same job, when the server rejects some of an answer's ops
+  MAP_AI_REPAIR = ENV["MAP_AI_REPAIR"]?.try(&.downcase) != "false"
+
   # default quotas in calls, overridden per provider row by map_user_per_day
   # and map_domain_per_month
   MAP_AI_USER_PER_DAY     = (ENV["MAP_AI_USER_PER_DAY"]? || "40").to_i

src/placeos-rest-api/controllers/map_ai.cr

@@ -115,6 +115,10 @@ module PlaceOS::Api
       getter server_issues : Hash(String, String)
       getter questions : Array(JSON::Any)
       getter usage : JSON::Any?
+      # when the server asked again about rejected ops: `rejected` before,
+      # `remaining` in the second answer (null when it was unusable), `used`
+      # when that answer replaced the first
+      getter repair : JSON::Any?
       getter resolution : JSON::Any?
       getter error_kind : String?
       getter error_message : String?
@@ -145,6 +149,7 @@ module PlaceOS::Api
         end
         @questions = result["questions"]?.try(&.as_a?) || [] of JSON::Any
         @usage = result["usage"]?
+        @repair = result["repair"]?
         @resolution = result["resolution"]?
         @error_kind = job.error_kind
         @error_message = job.error_message

src/placeos-rest-api/utilities/llm/ops-schema.json

@@ -33,7 +33,7 @@
                             },
                             "name": {
                                 "type": "string",
-                                "description": "fill, stroke, stroke-width, stroke-dasharray, fill-opacity, stroke-opacity, opacity, class, visibility, rx, ry, font-size, font-family, font-weight, text-anchor or data-*"
+                                "description": "fill, stroke, stroke-width, stroke-dasharray, fill-opacity, stroke-opacity, opacity, class, visibility, rx, ry, font-size, font-family, font-weight, text-anchor, dominant-baseline or data-*"
                             },
                             "value": {
                                 "type": ["string", "null"],

src/placeos-rest-api/utilities/llm/ops.cr

@@ -105,6 +105,7 @@ module PlaceOS::Api::Llm
         elsif (hash = op.as_h?)
           index.register_rename(hash, claimed)
           index.register_created(hash)
+          index.register_structure(hash)
         end
       end
 
@@ -118,6 +119,10 @@ module PlaceOS::Api::Llm
       getter protected_ids : Set(String)
       # The map's viewBox as x, y, width, height; nil when the context has none.
       getter view_box : Array(Float64)?
+      # anchors an earlier op in the batch deletes; an insert placed after one goes where it was
+      getter deleted = Set(String).new
+      # set once an earlier op in the batch can move elements to another parent
+      getter? restructured = false
 
       def initialize(context : JSON::Any)
         @anchors = {} of String => Hash(String, JSON::Any)
@@ -203,6 +208,41 @@ module PlaceOS::Api::Llm
       def known_id?(id : String) : Bool
         @ids.has_key?(id)
       end
+
+      # Records what an accepted op does to the tree that later insert_shape
+      # checks rely on.
+      def register_structure(op : Hash(String, JSON::Any)) : Nil
+        case op["op"]?.try(&.as_s?)
+        when "delete"
+          targets = op["targets"]?.try(&.as_a?) || [] of JSON::Any
+          ([op["target"]?] + targets).each do |target|
+            entry, _ = resolve(target)
+            if (anchor = entry.try(&.["a"]?).try(&.as_s?))
+              @deleted << anchor
+            end
+          end
+        when "reorder", "group", "ungroup", "replace_element"
+          @restructured = true
+        end
+      end
+
+      # The editor's rule for insert_shape: `after` is a direct child of
+      # `parent`, the root when none is named. Nil when it holds or cannot be
+      # judged from the index: `after` or `parent` made in this batch, `after`
+      # deleted earlier in it, or the tree restructured by an earlier op.
+      def sibling_issue(parent : Hash(String, JSON::Any)?, after : Hash(String, JSON::Any)) : String?
+        return nil if restructured?
+        return nil unless after.has_key?("p")
+        anchor = after["a"]?.try(&.as_s?)
+        return nil if anchor && @deleted.includes?(anchor)
+
+        expected = nil
+        if parent && parent["tag"]?.try(&.as_s?) != "svg"
+          expected = parent["a"]?.try(&.as_s?)
+          return nil unless expected
+        end
+        after["p"]?.try(&.as_s?) == expected ? nil : "after must be a child of the parent"
+      end
     end
 
     # The editor's `canvasExpansionIssue`: the new bounds keep the whole old
@@ -290,13 +330,20 @@ module PlaceOS::Api::Llm
         shape = op["kind"]?.try(&.as_s?)
         return "insert_shape needs a kind" unless shape
         return "#{shape} is not a shape that can be inserted" unless Schema::SHAPES.includes?(shape)
+        parent = nil
         if present?(op["parent"]?)
-          _, issue = index.resolve(op["parent"])
+          parent, issue = index.resolve(op["parent"])
           return "parent: #{issue}" if issue
         end
+        if (tag = parent.try(&.["tag"]?).try(&.as_s?)) && !{"g", "svg"}.includes?(tag)
+          return "parent must be the root or a <g>, not <#{tag}>"
+        end
         if present?(op["after"]?)
-          _, issue = index.resolve(op["after"])
+          after, issue = index.resolve(op["after"])
           return "after: #{issue}" if issue
+          if after && (misplaced = index.sibling_issue(parent, after))
+            return misplaced
+          end
         end
         geometry = Schema::SHAPE_GEOMETRY_ATTRIBUTES[shape]
         (op["attrs"]?.try(&.as_a?) || [] of JSON::Any).each do |attr|

src/placeos-rest-api/utilities/llm/prompt.cr

@@ -285,6 +285,25 @@ module PlaceOS::Api::Llm
       parts.join(' ')
     end
 
+    # `request` again with its answer's rejected operations and the reasons,
+    # asking for the whole proposal corrected. Nothing from the answer was
+    # applied, so the map in the request is still the current state.
+    def self.repair(request : Request, envelope : Ops::Envelope, issues : Hash(String, String)) : Request
+      answered = envelope.ops.map_with_index do |op, position|
+        {"op_id" => JSON::Any.new(Ops.id_for(position))}.merge(compact_op(op))
+      end
+      text = String.build do |io|
+        io << "REPAIR\n"
+        io << "Your previous answer to this request has not been applied: the application rejected the operations listed under REJECTED, so the map is exactly as described above. "
+        io << "Answer again with the whole corrected proposal. Keep every operation that was not rejected as it was, fix each rejected operation or leave it out, and add nothing else.\n"
+        io << "PREVIOUS ANSWER summary: " << envelope.summary << '\n' if envelope.summary.presence
+        io << "PREVIOUS ANSWER ops: " << answered.to_json << '\n'
+        io << "REJECTED\n"
+        issues.each { |id, reason| io << id << ": " << reason << '\n' }
+      end
+      request.copy_with(parts: request.parts + [TextPart.new(text)] of Part)
+    end
+
     # Earlier turns of this edit: the instruction, the ops answered and what the
     # person did with them.
     def self.history_text(history : Array(Turn)) : String