Fix ScriptRunner handling crashed state by jmthomas · Pull Request #2563 · OpenC3/cosmos

jmthomas · 2025-11-24T16:07:29Z

This would replace #2552 which had good ideas but I wanted to disallow retry for a crashed script ... it doesn't make sense to retry because unless you modify the script it will continue to crash. Also I wanted to explicitly handle all the script states in the case statement (spawning and init were not handled).

aikido-pr-checks · 2025-11-24T16:08:14Z

openc3/python/openc3/utilities/running_script.py

            RunningScript.instance.script_binding[0]["__file__"] = procedure_name
-            exec(instrumented_script, RunningScript.instance.script_binding[0], RunningScript.instance.script_binding[1])
+            exec(
+                instrumented_script, RunningScript.instance.script_binding[0], RunningScript.instance.script_binding[1]


Unsafe exec usage can lead to remote code execution - high severity
Using exec with expressions based on user input can execute arbitrary code.

Remediation: If you must use exec, create a allowlist for the input that goes into it to filter the strings.
^{View details in Aikido Security}

codecov · 2025-11-24T16:08:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.18%. Comparing base (976eb3b) to head (ad2970f).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2563      +/-   ##
==========================================
- Coverage   79.21%   79.18%   -0.04%     
==========================================
  Files         662      662              
  Lines       51218    51271      +53     
  Branches      736      736              
==========================================
+ Hits        40573    40599      +26     
- Misses      10565    10592      +27     
  Partials       80       80

Flag	Coverage Δ
python	`81.29% <ø> (ø)`
ruby-api	`84.70% <ø> (+0.13%)`	⬆️
ruby-backend	`82.12% <100.00%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jmthomas · 2025-11-24T16:10:19Z

openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue

        })
    },
    async scriptComplete() {
-      this.fatal = false


We're no longer marking fatal lines ... the output shows which line is fatal and the script simply stops with the status set to 'Crashed'

jmthomas · 2025-11-24T16:12:05Z

openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue

+      if (this.state === 'completed_errors') {
+        // Displaying 'Completed_errors' is not very user friendly
+        this.state = 'completed'
+      }


Rather than always setting to 'Stopped', we preserve the state unless 'completed_errors' which I just translate. The error will be in the script output.

Couldn't this cause users to just see "completed" at the top and exit out without noticing the errors? I feel like there should be some sort of indication of errors where we display state. Maybe an alert icon, or shade it orange/yellow?

Execution Status remains with completed_errors, which is good. I do tend to agree that the UI should make it clear that the script did encounter errors.

The script still completed but it potentially had errors like Check Errors or even logical errors (divide by zero). The user clicked Retry or Go and the script completed. So I'm not sure what you mean by exit out. The status is in the script execution status. Perhaps we do textcolor changes of the status where completed_errors would display Completed but it would be orange. Crashed / Killed is red and all others green. Thoughts?

Can we make completed_errors state display in the UI as Completed with Errors? I feel like that's the most user-friendly.

That makes the State box much wider to be readable

Could we add an alert icon or shade the box a color? Maybe also a tooltip, but that might not be necessary.

By "exit out," I mean leave the page (close the browser tab or navigate away)

jmthomas · 2025-11-24T16:12:52Z

openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue

+        case 'completed_errors':
+        case 'stopped':
+        case 'crashed':
+        case 'killed':


These are all the possible 'completed' states. The script is done in the backend.

jmthomas · 2025-11-24T16:13:37Z

openc3/lib/openc3/top_level.rb

    end
  end

+  require 'openc3/utilities/logger'


Moving this fixes the circular dependency in Enterprise tests where OpenC3.disable_warnings is not defined.

jmthomas · 2025-11-24T16:13:52Z

openc3/python/openc3/utilities/running_script.py

 from openc3.tools.test_runner.test import SkipTestCase
 from openc3.script.suite import Group
 from openc3.utilities.script_instrumentor import ScriptInstrumentor
-import openc3.utilities.target_file_importer


jmthomas · 2025-11-24T16:14:16Z

openc3/python/openc3/utilities/running_script.py

            # All ask and prompt dialogs should include a 'Cancel' button
            # If they cancel we wait so they can potentially stop
-            if input == "Cancel":
+            if user_input == "Cancel":


input shadows a built-in so renamed

jmthomas · 2025-11-24T16:16:49Z

openc3/python/openc3/utilities/running_script.py

                kill_thread(self, RunningScript.output_thread)
                RunningScript.output_thread = None
            self.mark_completed()
-            self.current_filename = None


This is actually a bug because we're setting the current_filename method to None which basically removes it. I think any call to RunningScript.instance.current_filename() would crash after this. Maybe didn't matter because this is in Exception handling and we're already complete.

jmthomas · 2025-11-24T16:17:37Z

openc3/python/openc3/utilities/running_script.py

+            formatted_lines = traceback.format_exception(exc_type, exc_value, exc_traceback)
+            # Print the last 4 lines to show the exception, the ^^^^ line,
+            # the line itself, and the filename / line number
+            Logger.error("".join(formatted_lines[-4:]))


Changed to show the actual line number of the exception. I noticed that previous we just print only the exception which doesn't show where it is. Makes it a lot harder to debug.

clayandgen

Curious, do you have a new formatter or were these formatting changes manual? (or perhaps this file just hadn't been modified in some time)

jmthomas · 2025-11-24T19:38:28Z

Not sure if the reformatted was due to pylance or black. I think the former.

clayandgen

Big fan of this update!

Fix ScriptRunner handling crashed state

8512227

jmthomas requested review from clayandgen, ryan-pratt and ryanmelt and removed request for clayandgen, ryan-pratt and ryanmelt November 24, 2025 16:07

aikido-pr-checks bot reviewed Nov 24, 2025

View reviewed changes

jmthomas commented Nov 24, 2025

View reviewed changes

jmthomas mentioned this pull request Nov 24, 2025

Fix script crashed state and retry in the script runner UI #2534 #2552

Closed

clayandgen reviewed Nov 24, 2025

View reviewed changes

Colorize state and fix playwright

ad2970f

jmthomas requested a review from clayandgen November 24, 2025 23:13

clayandgen approved these changes Nov 25, 2025

View reviewed changes

jmthomas merged commit 0706d53 into main Nov 25, 2025
48 of 50 checks passed

jmthomas deleted the script_states branch November 25, 2025 17:25

Conversation

jmthomas commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aikido-pr-checks bot Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayandgen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmthomas commented Nov 24, 2025

Uh oh!

clayandgen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmthomas commented Nov 24, 2025 •

edited

Loading

aikido-pr-checks bot Nov 24, 2025 •

edited

Loading

codecov bot commented Nov 24, 2025 •

edited

Loading

clayandgen left a comment •

edited

Loading