fix(microservices): ack malformed and released commands, align flag defaults

openc3/lib/openc3/config/config_parser.rb

@@ -319,8 +319,10 @@ def self.handle_nil(value)
     # primitive. All other values are simply returned.
     #
     # @param value [Object]
+    # @param default [Object] Returned if value is nil
     # @return [true|false|Object]
-    def self.handle_true_false(value)
+    def self.handle_true_false(value, default: nil)
+      return default if value.nil?
       if String === value
         case value.upcase
         when 'TRUE'

openc3/lib/openc3/microservices/interface_microservice.rb

@@ -156,8 +156,8 @@ def run
               next 'SUCCESS'
             end
             if msg_hash.key?('interface_cmd')
-              params = JSON.parse(msg_hash['interface_cmd'], allow_nan: true, create_additions: true)
               begin
+                params = JSON.parse(msg_hash['interface_cmd'], allow_nan: true, create_additions: true)
                 @logger.info "#{@interface.name}: interface_cmd: #{params['cmd_name']} #{params['cmd_params'].join(' ')}"
                 @interface.interface_cmd(params['cmd_name'], *params['cmd_params'])
                 InterfaceStatusModel.set(@interface.as_json(), queued: true, scope: @scope)
@@ -168,8 +168,8 @@ def run
               next 'SUCCESS'
             end
             if msg_hash.key?('protocol_cmd')
-              params = JSON.parse(msg_hash['protocol_cmd'], allow_nan: true, create_additions: true)
               begin
+                params = JSON.parse(msg_hash['protocol_cmd'], allow_nan: true, create_additions: true)
                 @logger.info "#{@interface.name}: protocol_cmd: #{params['cmd_name']} #{params['cmd_params'].join(' ')} read_write: #{params['read_write']} index: #{params['index']}"
                 @interface.protocol_cmd(params['cmd_name'], *params['cmd_params'], read_write: params['read_write'], index: params['index'])
                 InterfaceStatusModel.set(@interface.as_json(), queued: true, scope: @scope)
@@ -232,21 +232,28 @@ def run
 
           target_name = msg_hash['target_name']
           if target_name and not @interface.cmd_target_enabled[target_name]
+            # A released critical command was already acked as pending so it must get a result
+            next "Target #{target_name} is disabled" if release_critical
             next nil # Return and don't ack given target_name if disabled
           end
 
           cmd_name = msg_hash['cmd_name']
-          manual = ConfigParser.handle_true_false(msg_hash['manual'])
+          manual = ConfigParser.handle_true_false(msg_hash['manual'], default: false)
           cmd_params = nil
           range_check = true
           raw = false
           cmd_buffer = nil
           hazardous_check = nil
           if msg_hash['cmd_params']
-            cmd_params = JSON.parse(msg_hash['cmd_params'], allow_nan: true, create_additions: true)
-            range_check = ConfigParser.handle_true_false(msg_hash['range_check'])
-            raw = ConfigParser.handle_true_false(msg_hash['raw'])
-            hazardous_check = ConfigParser.handle_true_false(msg_hash['hazardous_check'])
+            begin
+              cmd_params = JSON.parse(msg_hash['cmd_params'], allow_nan: true, create_additions: true)
+            rescue => e
+              @logger.error "#{@interface.name}: Invalid cmd_params: #{e.message}"
+              next "Invalid cmd_params: #{e.message}"
+            end
+            range_check = ConfigParser.handle_true_false(msg_hash['range_check'], default: true)
+            raw = ConfigParser.handle_true_false(msg_hash['raw'], default: false)
+            hazardous_check = ConfigParser.handle_true_false(msg_hash['hazardous_check'], default: true)
           elsif msg_hash['cmd_buffer']
             cmd_buffer = msg_hash['cmd_buffer']
           end
@@ -278,6 +285,7 @@ def run
                 command.received_count = orig_command.received_count
                 command.received_time = Time.now
               else
+                next "Target #{command.target_name} is disabled" if release_critical
                 next nil # Don't ack disabled targets
               end
             rescue RangeError => e
@@ -325,7 +333,7 @@ def run
               end
             end
 
-            validate = ConfigParser.handle_true_false(msg_hash['validate'])
+            validate = ConfigParser.handle_true_false(msg_hash['validate'], default: true)
             begin
               if @interface.connected?
                 result = true
@@ -347,7 +355,7 @@ def run
                 @count += 1
                 @metric.set(name: 'interface_cmd_total', value: @count, type: 'counter') if @metric
 
-                log_message = ConfigParser.handle_true_false(msg_hash['log_message'])
+                log_message = ConfigParser.handle_true_false(msg_hash['log_message'], default: true)
                 if log_message
                   @logger.info(msg_hash['cmd_string'], user: msg_hash['username'], scope: @scope)
                 end

openc3/python/openc3/microservices/interface_microservice.py

@@ -201,8 +201,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis):
                     self.interface.stop_raw_logging()
                 return "SUCCESS"
             if msg_hash.get(b"interface_cmd"):
-                params = json.loads(msg_hash[b"interface_cmd"])
                 try:
+                    params = json.loads(msg_hash[b"interface_cmd"])
                     str_params = " ".join([str(i) for i in params["cmd_params"]])
                     self.logger.info(f"{self.interface.name}: interface_cmd: {params['cmd_name']} {str_params}")
                     self.interface.interface_cmd(params["cmd_name"], *params["cmd_params"])
@@ -213,8 +213,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis):
                     return str(e)
                 return "SUCCESS"
             if msg_hash.get(b"protocol_cmd"):
-                params = json.loads(msg_hash[b"protocol_cmd"])
                 try:
+                    params = json.loads(msg_hash[b"protocol_cmd"])
                     str_params = " ".join([str(i) for i in params["cmd_params"]])
                     self.logger.info(
                         f"{self.interface.name}: protocol_cmd: {params['cmd_name']} {str_params} read_write: {params['read_write']} index: {params['index']}"
@@ -286,18 +286,27 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis):
                     )
                     return str(e)
 
-        target_name = msg_hash[b"target_name"].decode()
+        target_name = msg_hash.get(b"target_name")
+        target_name = target_name.decode() if target_name is not None else None
         if target_name and not self.interface.cmd_target_enabled.get(target_name, False):
+            # A released critical command was already acked as pending so it must get a result
+            if release_critical:
+                return f"Target {target_name} is disabled"
             return None  # Return and don't ack given target_name if disabled
-        cmd_name = msg_hash[b"cmd_name"].decode()
+        cmd_name = msg_hash.get(b"cmd_name")
+        cmd_name = cmd_name.decode() if cmd_name is not None else None
         manual = ConfigParser.handle_true_false(msg_hash.get(b"manual", b"FALSE").decode())
         cmd_params = None
         range_check = True
         raw = False
         cmd_buffer = None
         hazardous_check = None
         if msg_hash.get(b"cmd_params") is not None:
-            cmd_params = json.loads(msg_hash.get(b"cmd_params"), cls=JsonDecoder)
+            try:
+                cmd_params = json.loads(msg_hash.get(b"cmd_params"), cls=JsonDecoder)
+            except ValueError as e:
+                self.logger.error(f"{self.interface.name}: Invalid cmd_params: {e}")
+                return f"Invalid cmd_params: {e}"
             range_check = ConfigParser.handle_true_false(msg_hash.get(b"range_check", b"TRUE").decode())
             raw = ConfigParser.handle_true_false(msg_hash.get(b"raw", b"FALSE").decode())
             hazardous_check = ConfigParser.handle_true_false(msg_hash.get(b"hazardous_check", b"TRUE").decode())
@@ -324,6 +333,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis):
                 if enabled_target_name == "UNKNOWN" and target_name:
                     enabled_target_name = target_name
                 if not self.interface.cmd_target_enabled.get(enabled_target_name, False):
+                    if release_critical:
+                        return f"Target {command.target_name} is disabled"
                     return None  # Don't ack disabled targets
 
                 orig_command = System.commands.packet(command.target_name, command.packet_name)
@@ -393,10 +404,11 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis):
                     if command.validator and validate:
                         try:
                             result, reason = command.validator.pre_check(command)
-                        except Exception:
+                        except Exception as e:
                             result = False
-                            reason = traceback.format_exc()
-                        if not result:
+                            reason = str(e)
+                        # Explicitly check for False to allow None to represent unknown
+                        if result is False:
                             message = f"pre_check returned false for {command.extra['cmd_string']} due to {reason}"
                             raise WriteRejectError(message)
 
@@ -419,9 +431,9 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis):
                     if command.validator and validate:
                         try:
                             result, reason = command.validator.post_check(command)
-                        except Exception:
+                        except Exception as e:
                             result = False
-                            reason = traceback.format_exc()
+                            reason = str(e)
                         command.extra["cmd_success"] = result
                         if reason:
                             command.extra["cmd_reason"] = reason
@@ -430,7 +442,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis):
                     CommandTopic.write_packet(command, scope=self.scope)
                     InterfaceStatusModel.set(self.interface.as_json(), queued=True, scope=self.scope)
 
-                    if not result:
+                    # Explicitly check for False to allow None to represent unknown
+                    if result is False:
                         message = f"post_check returned false for {command.extra['cmd_string']} due to {reason}"
                         raise WriteRejectError(message)
 

openc3/python/test/microservices/test_interface_microservice.py

@@ -777,13 +777,14 @@ def test_process_cmd_with_all_fields_and_missing_optional_fields(self):
         result = handler.process_cmd(topic, msg_id, minimal_msg_hash, None)
         self.assertEqual(result, "SUCCESS")
 
-        # Missing target_name raises KeyError (required field uses direct [] access)
-        with self.assertRaises(KeyError):
-            handler.process_cmd(topic, msg_id, {b"cmd_name": b"ABORT"}, None)
-
-        # Missing cmd_name raises KeyError (required field uses direct [] access)
-        with self.assertRaises(KeyError):
-            handler.process_cmd(topic, msg_id, {b"target_name": b"INST"}, None)
+        # Missing target_name or cmd_name is acked with an error rather than raising
+        cmd_params = json.dumps({}).encode()
+        result = handler.process_cmd(topic, msg_id, {b"cmd_name": b"ABORT", b"cmd_params": cmd_params}, None)
+        self.assertIsInstance(result, str)
+        self.assertNotEqual(result, "SUCCESS")
+        result = handler.process_cmd(topic, msg_id, {b"target_name": b"INST", b"cmd_params": cmd_params}, None)
+        self.assertIsInstance(result, str)
+        self.assertNotEqual(result, "SUCCESS")
 
         # Disabled target returns None (no ack)
         handler.interface.cmd_target_enabled["INST"] = False
@@ -953,6 +954,89 @@ def test_process_cmd_command_error_and_hazardous_branches(self):
         )
         self.assertEqual(result, "Interface not connected: INST_INT")
 
+    def test_process_cmd_acks_malformed_json(self):
+        """Malformed JSON in a directive or cmd_params is acked with an error
+        instead of killing the command handler thread."""
+        im = InterfaceMicroservice("DEFAULT__INTERFACE__INST_INT")
+        self.addCleanup(im.shutdown)
+        handler = im.handler_thread
+        msg_id = f"{int(time.time() * 1000)}-0"
+
+        for directive in [b"interface_cmd", b"protocol_cmd"]:
+            result = handler.process_cmd("{DEFAULT__CMD}INTERFACE__INST_INT", msg_id, {directive: b"{not json"}, None)
+            self.assertIsInstance(result, str)
+            self.assertNotEqual(result, "SUCCESS")
+
+        result = handler.process_cmd(
+            "{DEFAULT__CMD}TARGET__INST",
+            msg_id,
+            {b"target_name": b"INST", b"cmd_name": b"ABORT", b"cmd_params": b"{not json"},
+            None,
+        )
+        self.assertIn("Invalid cmd_params", result)
+
+    def test_process_cmd_acks_released_critical_cmd_for_disabled_target(self):
+        """A released critical command was already acked as pending, so a target
+        disabled in the meantime must produce an error ack, not silence."""
+        im = InterfaceMicroservice("DEFAULT__INTERFACE__INST_INT")
+        self.addCleanup(im.shutdown)
+        handler = im.handler_thread
+        msg_id = f"{int(time.time() * 1000)}-0"
+
+        critical_model = Mock()
+        critical_model.approver = "approver"
+        critical_model.cmd_hash = {
+            b"target_name": b"INST",
+            b"cmd_name": b"ABORT",
+            b"cmd_params": json.dumps({}).encode(),
+            b"cmd_string": b"cmd('INST ABORT')",
+        }
+        handler.interface.cmd_target_enabled["INST"] = False
+        with patch("openc3.microservices.interface_microservice.CriticalCmdModel", create=True) as model_class:
+            model_class.get_model.return_value = critical_model
+            result = handler.process_cmd(
+                "{DEFAULT__CMD}INTERFACE__INST_INT", msg_id, {b"release_critical": b"abc-123"}, None
+            )
+        self.assertEqual(result, "Target INST is disabled")
+
+    def test_process_cmd_validator_results(self):
+        """Only an explicit False from pre_check/post_check rejects a command (None
+        means unknown), and a raising validator reports its message, not a traceback."""
+        im = InterfaceMicroservice("DEFAULT__INTERFACE__INST_INT")
+        thread = threading.Thread(target=im.run)
+        thread.start()
+        self.addCleanup(thread.join, 5)
+        self.addCleanup(im.shutdown)
+        time.sleep(0.1)
+
+        handler = im.handler_thread
+        topic = "{DEFAULT__CMD}TARGET__INST"
+        msg_id = f"{int(time.time() * 1000)}-0"
+        msg_hash = {
+            b"target_name": b"INST",
+            b"cmd_name": b"ABORT",
+            b"cmd_params": json.dumps({}).encode(),
+            b"cmd_string": b"cmd('INST ABORT')",
+        }
+
+        validator = Mock()
+        packet = System.commands.packet("INST", "ABORT")
+        packet.validator = validator
+        self.addCleanup(setattr, packet, "validator", None)
+
+        validator.pre_check.return_value = (None, None)
+        validator.post_check.return_value = (None, None)
+        self.assertEqual(handler.process_cmd(topic, msg_id, msg_hash, None), "SUCCESS")
+
+        validator.pre_check.return_value = (False, "not ready")
+        result = handler.process_cmd(topic, msg_id, msg_hash, None)
+        self.assertEqual(result, "pre_check returned false for cmd('INST ABORT') due to not ready")
+
+        validator.pre_check.return_value = (True, None)
+        validator.post_check.side_effect = RuntimeError("post boom")
+        result = handler.process_cmd(topic, msg_id, msg_hash, None)
+        self.assertEqual(result, "post_check returned false for cmd('INST ABORT') due to post boom")
+
     def test_process_cmd_decodes_ruby_nan_cmd_params(self):
         """cmd_params from a Ruby caller encode NaN as a json_class Float object;
         it must reach build_cmd as a float, not a dict."""

openc3/spec/config/config_parser_spec.rb

@@ -537,6 +537,13 @@ module OpenC3
         expect(ConfigParser.handle_true_false("HI")).to eql "HI"
         expect(ConfigParser.handle_true_false(5.0)).to eql 5.0
       end
+
+      it "returns the default for nil" do
+        expect(ConfigParser.handle_true_false(nil)).to be_nil
+        expect(ConfigParser.handle_true_false(nil, default: true)).to be true
+        expect(ConfigParser.handle_true_false(nil, default: false)).to be false
+        expect(ConfigParser.handle_true_false('FALSE', default: true)).to be false
+      end
     end
 
     describe "self.handle_true_false_strict" do