From 5996f9e5658a498347db6695bf8dd2df43516e55 Mon Sep 17 00:00:00 2001
From: Nathan Glenn <garfieldnate@gmail.com>
Date: Wed, 26 Jun 2024 00:13:37 -0500
Subject: [PATCH] Don't allow disabling SVS from substates

SVS keeps track of a stack of states which need to correspond to the current
goal state. `state_creation_callback` and `state_deletion_callback` are called
when goal states are created/deleted, and SVS updates its stack.
`state_deletion_callback` has an `assert` to check that the state passed to it
is the same one that is being popped; the two stacks must always stay in sync,
and the assert checks for this invariant.

If disabling SVS is allowed in substates, we can break the invariant like this:

* Start with SVS enabled (which is the default). Let's call the top state S1.
* Go two subgoals deep (S3)
* After S3 returns to S2, enable SVS again
* When S2 returns, `svs::state_deletion_callback` will be called. `state_stack`
will have S3 on top, since we skipped `svs::state_deletion_callback` while SVS
was disabled.
* Therefore, `assert(state == s->get_state());` will fail

To prevent this case, we fail attempts to disable SVS while within a subgoal.
This is probably a corner-case, but it ensures that we can maintain the
invariant and not throw an `assert` exception (or hit undefined behavior when
we call `pop_back` on an empty `std::vector`!).

See #475.
---
 Core/CLI/src/cli_svs.cpp     | 8 ++++++++
 Core/SVS/src/svs.h           | 5 +++++
 Core/SVS/src/svs_interface.h | 4 ++++
 3 files changed, 17 insertions(+)

diff --git a/Core/CLI/src/cli_svs.cpp b/Core/CLI/src/cli_svs.cpp
index 7275b377ba..d7411d0779 100644
--- a/Core/CLI/src/cli_svs.cpp
+++ b/Core/CLI/src/cli_svs.cpp
@@ -47,6 +47,10 @@ bool CommandLineInterface::DoSVS(const std::vector<std::string>& args)
             }
             else
             {
+                if (thisAgent->svs->is_in_substate()) {
+                    m_Result << "Cannot disable Spatial Visual System while in a substate.";
+                    return false;
+                }
                 thisAgent->svs->set_enabled(false);
                 m_Result << "Spatial Visual System disabled.";
             }
@@ -72,6 +76,10 @@ bool CommandLineInterface::DoSVS(const std::vector<std::string>& args)
             }
             else
             {
+                if (thisAgent->svs->is_in_substate()) {
+                    m_Result << "Cannot disable Spatial Visual System in substates while in a substate.";
+                    return false;
+                }
                 thisAgent->svs->set_enabled_in_substates(false);
                 m_Result << "Spatial Visual System disabled in substates.";
             }
diff --git a/Core/SVS/src/svs.h b/Core/SVS/src/svs.h
index a25bec6cb4..677c920244 100644
--- a/Core/SVS/src/svs.h
+++ b/Core/SVS/src/svs.h
@@ -191,6 +191,11 @@ class svs : public svs_interface, public cliproxy
             return "";
         }
 
+        bool is_in_substate()
+        {
+            return state_stack.size() > 1;
+        }
+
         // dirty bit is true only if there has been a new command
         //   from soar or from SendSVSInput
         //   (no need to recheck filters)
diff --git a/Core/SVS/src/svs_interface.h b/Core/SVS/src/svs_interface.h
index 1d5217ae9a..551fb1a65d 100644
--- a/Core/SVS/src/svs_interface.h
+++ b/Core/SVS/src/svs_interface.h
@@ -19,6 +19,10 @@ class svs_interface
         virtual void set_enabled(bool newSetting) = 0;
         virtual bool is_enabled_in_substates() = 0;
         virtual void set_enabled_in_substates(bool newSetting) = 0;
+        /**
+         * Indicates that the current top of the state stack is for a subgoal/substate
+         */
+        virtual bool is_in_substate() = 0;
 };
 
 svs_interface* make_svs(agent* a);