From b5d75a9b5f5367c628d4322b43c6a176eacff4b2 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 5 May 2026 09:17:43 -0600 Subject: [PATCH 1/6] feat(scriptrunner): add S3 versioning and script lifecycle tracking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enable bucket versioning on OPENC3_CONFIG_BUCKET via versitygw, threading a sibling /versions volume + VGW_VERSIONING_DIR through the openc3-buckets image and entrypoint. Add list_object_versions and version_id-aware get/head/delete to AwsBucket. Introduce ScriptLifecycleModel to track per-version state (saved/validated/reviewed/executed) with usernames and timestamps; saves over a reviewed version return 409 unless force_taint is sent, in which case the new version is flagged with provenance back to the prior reviewer. Auto-validate inline on save (syntax + mnemonic) to record validated_at for API-driven saves. Remove Script.lock entirely along with the editor-locking UI now that versioning eliminates overwrite risk. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- compose.yaml | 3 + openc3-buckets/Dockerfile | 6 +- openc3-buckets/Dockerfile-ubi | 6 +- openc3-buckets/docker-entrypoint.sh | 17 +- .../admin/tabs/settings/EditorSettings.vue | 21 -- .../src/tools/scriptrunner/ScriptRunner.vue | 106 +--------- .../app/controllers/scripts_controller.rb | 119 ++++++++--- .../config/routes.rb | 2 - .../controllers/scripts_controller_spec.rb | 73 ++----- .../spec/models/script_spec.rb | 36 +--- openc3/bin/openc3cli | 3 + .../openc3/models/script_lifecycle_model.rb | 191 ++++++++++++++++++ openc3/lib/openc3/utilities/aws_bucket.rb | 64 +++++- openc3/lib/openc3/utilities/bucket.rb | 17 +- openc3/lib/openc3/utilities/script.rb | 24 +-- openc3/lib/openc3/utilities/target_file.rb | 11 +- .../models/script_lifecycle_model_spec.rb | 191 ++++++++++++++++++ playwright/tests/script-runner/api.s.spec.ts | 8 - .../tests/script-runner/prompts.p.spec.ts | 8 - 19 files changed, 608 insertions(+), 298 deletions(-) create mode 100644 openc3/lib/openc3/models/script_lifecycle_model.rb create mode 100644 openc3/spec/models/script_lifecycle_model_spec.rb diff --git a/compose.yaml b/compose.yaml index ee634c90c6..22d108ac4d 100644 --- a/compose.yaml +++ b/compose.yaml @@ -22,6 +22,7 @@ services: image: "${OPENC3_REGISTRY}/${OPENC3_NAMESPACE}/openc3-buckets${OPENC3_IMAGE_SUFFIX}:${OPENC3_TAG}" volumes: - "openc3-object-v:/data" + - "openc3-object-versions-v:/versions" - "./cacert.pem:/devel/cacert.pem:z" restart: "unless-stopped" logging: @@ -34,6 +35,7 @@ services: ROOT_SECRET_KEY: "${OPENC3_BUCKET_PASSWORD}" OPENC3_SR_BUCKET_USERNAME: "${OPENC3_SR_BUCKET_USERNAME}" OPENC3_SR_BUCKET_PASSWORD: "${OPENC3_SR_BUCKET_PASSWORD}" + VGW_VERSIONING_DIR: "/versions" SSL_CERT_FILE: "/devel/cacert.pem" CURL_CA_BUNDLE: "/devel/cacert.pem" REQUESTS_CA_BUNDLE: "/devel/cacert.pem" @@ -326,6 +328,7 @@ volumes: openc3-redis-v: {} openc3-redis-ephemeral-v: {} openc3-object-v: {} + openc3-object-versions-v: {} openc3-gems-v: {} openc3-cmd-tlm-api-tmp-v: {} openc3-script-runner-api-tmp-v: {} diff --git a/openc3-buckets/Dockerfile b/openc3-buckets/Dockerfile index a9c60480c0..9eb7f173b6 100644 --- a/openc3-buckets/Dockerfile +++ b/openc3-buckets/Dockerfile @@ -34,9 +34,9 @@ RUN apk update && \ chmod +x /usr/bin/docker-entrypoint.sh && \ addgroup -g ${GROUP_ID} -S ${IMAGE_GROUP} && \ adduser -u ${USER_ID} -G ${IMAGE_GROUP} -s /bin/ash -S ${IMAGE_USER} && \ - mkdir /data && \ - chown -R ${USER_ID}:${IMAGE_GROUP} /data && \ - chmod -R 777 /data + mkdir /data /versions && \ + chown -R ${USER_ID}:${IMAGE_GROUP} /data /versions && \ + chmod -R 777 /data /versions # Switch to user USER ${USER_ID}:${GROUP_ID} diff --git a/openc3-buckets/Dockerfile-ubi b/openc3-buckets/Dockerfile-ubi index dcc47b4131..cc4d0615a3 100644 --- a/openc3-buckets/Dockerfile-ubi +++ b/openc3-buckets/Dockerfile-ubi @@ -31,9 +31,9 @@ RUN microdnf update -y && \ microdnf clean all && \ rm -rf /var/cache/yum && \ chmod +x /usr/bin/docker-entrypoint.sh && \ - mkdir -p /data && \ - chown 1001:1001 /data && \ - chmod -R 777 /data + mkdir -p /data /versions && \ + chown 1001:1001 /data /versions && \ + chmod -R 777 /data /versions USER 1001 EXPOSE 9000 diff --git a/openc3-buckets/docker-entrypoint.sh b/openc3-buckets/docker-entrypoint.sh index 00890013cb..de32696d98 100644 --- a/openc3-buckets/docker-entrypoint.sh +++ b/openc3-buckets/docker-entrypoint.sh @@ -24,6 +24,13 @@ if [ "${1}" = "versitygw" ] || [ "${1#-}" != "$1" ]; then # Create IAM directory if it doesn't exist mkdir -p "${VGW_IAM_DIR}" + # versitygw posix backend requires the versioning dir to live OUTSIDE the + # data root (it errors with "the root directory contains the directory ..." + # otherwise), so this must be a separately mounted path. + if [ -n "${VGW_VERSIONING_DIR}" ]; then + mkdir -p "${VGW_VERSIONING_DIR}" + fi + # Pre-create ScriptRunner user account if credentials are provided and different from root # versitygw expects accounts in a users.json file with accessAccounts structure if [ -n "${OPENC3_SR_BUCKET_USERNAME}" ] && [ -n "${OPENC3_SR_BUCKET_PASSWORD}" ]; then @@ -55,9 +62,15 @@ EOF fi # If no arguments provided, use defaults with IAM enabled - # Note: --iam-dir is a global option that must come BEFORE the backend command + # Note: --iam-dir is a global option that must come BEFORE the backend command; + # --versioning-dir is a posix backend flag, so it goes after "posix" and before + # the data dir argument. if [ $# -eq 0 ]; then - set -- versitygw --iam-dir "${VGW_IAM_DIR}" "${VGW_BACKEND}" "${VGW_BACKEND_ARG}" + if [ "${VGW_BACKEND}" = "posix" ] && [ -n "${VGW_VERSIONING_DIR}" ]; then + set -- versitygw --iam-dir "${VGW_IAM_DIR}" "${VGW_BACKEND}" --versioning-dir "${VGW_VERSIONING_DIR}" "${VGW_BACKEND_ARG}" + else + set -- versitygw --iam-dir "${VGW_IAM_DIR}" "${VGW_BACKEND}" "${VGW_BACKEND_ARG}" + fi else set -- versitygw "$@" fi diff --git a/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/settings/EditorSettings.vue b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/settings/EditorSettings.vue index 4e8a00cf7b..3c9c56a8c7 100644 --- a/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/settings/EditorSettings.vue +++ b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/settings/EditorSettings.vue @@ -39,15 +39,6 @@ data-test="default-language" /> - - - mdi-pencil-off - {{ lockedBy }} is editing this script. Editor is in read-only mode - -
0 }, - isLocked: function () { - if (!this.lockingEnabled) { - return false - } - return !!this.lockedBy - }, // Returns the currently shown filename fullFilename: function () { if (this.currentFilename) return this.currentFilename @@ -1241,17 +1201,6 @@ export default { }, }, watch: { - isLocked: function (val) { - this.showEditingToast = val - if (!this.suiteRunner) { - this.startOrGoDisabled = val - } - if (this.readOnlyUser == false && val == false && !this.inline) { - this.editor.setReadOnly(val) - } else { - this.editor.setReadOnly(true) - } - }, fullFilename: function (filename) { this.filenameSelect = filename if (!this.inline) { @@ -1295,16 +1244,6 @@ export default { .catch((error) => { // Do nothing }) - try { - const lockingResponse = await this.api.get_setting( - 'script_runner_locking', - ) - if (lockingResponse !== null && lockingResponse !== undefined) { - this.lockingEnabled = lockingResponse - } - } catch (error) { - // Keep default (true) - } this.updateOverridesCount() @@ -1466,7 +1405,6 @@ export default { this.editor.container.remove() }, unmounted() { - this.unlockFile() if (this.updateInterval != null) { clearInterval(this.updateInterval) } @@ -1815,7 +1753,7 @@ export default { } }, onChange(event) { - // Don't track changes when we're running or read-only (locked) + // Don't track changes when we're running or the editor is read-only if (this.scriptId || this.editor.getReadOnly() === true) { return } @@ -2518,7 +2456,6 @@ export default { } }, newFile() { - this.unlockFile() this.filename = NEW_FILENAME this.currentFilename = null this.tempFilename = null @@ -2700,9 +2637,8 @@ class TestSuite(Suite): if (response.data.success) { file['success'] = response.data.success } - const locked = response.data.locked const breakpoints = response.data.breakpoints - this.setFile({ file, locked, breakpoints }, true) + this.setFile({ file, breakpoints }, true) this.saveAllowed = true }) .catch((error) => { @@ -2717,17 +2653,10 @@ class TestSuite(Suite): }) }, // Called by the FileOpenDialog to set the file contents - setFile({ file, locked, breakpoints }, local = false) { + setFile({ file, breakpoints }, local = false) { this.files = {} // Clear the cached file list // Split off the ' *' which indicates a file is modified on the server let newFilename = file.name.split('*')[0] - if (local === false) { - // We only need to unlock if the file is different - if (this.filename !== newFilename) { - this.unlockFile() // first unlock what was just being edited - this.lockedBy = locked - } - } this.filename = newFilename if (!this.inline) { // Update the URL with the filename @@ -2900,7 +2829,6 @@ class TestSuite(Suite): this.alertText = `Error saving file. Code: ${response.status} Text: ${response.statusText}` this.showAlert = true } - this.lockFile() // Ensure this file is locked for editing this.doResize() }) .catch(({ response }) => { @@ -3050,34 +2978,6 @@ class TestSuite(Suite): .filter((key) => allMarkers[key].type === 'fullLine') .forEach((marker) => this.editor.session.removeMarker(marker)) }, - confirmLocalUnlock: function () { - this.$dialog - .confirm( - 'Are you sure you want to unlock this script for editing? If another user is editing this script, your changes might conflict with each other.', - { - okText: 'Force Unlock', - cancelText: 'Cancel', - }, - ) - .then(() => { - this.lockedBy = null - return this.lockFile() // Re-lock it as this user so it's locked for anyone else who opens it - }) - }, - lockFile: function () { - if (!this.readOnlyUser) { - return Api.post(`/script-api/scripts/${this.filename}/lock`) - } - }, - unlockFile: function () { - if ( - this.filename !== NEW_FILENAME && - !this.readOnly && - !this.readOnlyUser - ) { - Api.post(`/script-api/scripts/${this.filename}/unlock`) - } - }, backToNewScript: async function () { // Disconnect from the current script this.scriptDisconnect() diff --git a/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb b/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb index 089f1cbd7b..f42739e8cd 100644 --- a/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb +++ b/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb @@ -17,6 +17,7 @@ require 'json' require 'openc3/utilities/script' +require 'openc3/models/script_lifecycle_model' class ScriptsController < ApplicationController # This REGEX is also found in running_script.rb @@ -57,15 +58,14 @@ def body file = Script.body(scope, name) if file - locked = Script.locked?(scope, name) - unless locked - Script.lock(scope, name, username()) - end breakpoints = Script.get_breakpoints(scope, name) + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) results = { contents: file, breakpoints: breakpoints, - locked: locked + version_id: lifecycle.latest_version_id, + lifecycle: lifecycle.latest_record, + locked_for_review: lifecycle.locked_for_review? } if ((File.extname(name) == '.py') and (file =~ PYTHON_SUITE_REGEX)) or ((File.extname(name) != '.py') and (file =~ SUITE_REGEX)) results_suites, results_error, success = Script.process_suite(name, file, username: username(), scope: scope) @@ -88,8 +88,53 @@ def create args = params.permit(:text, breakpoints: []) args[:scope] = scope args[:name] = name - Script.create(args) + args[:username] = username() + + # Refuse to overwrite a reviewed version unless the caller explicitly + # opts into tainting it. The frontend confirms with the user before retrying + # with force_taint=true. + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + force_taint = params[:force_taint].to_s == 'true' + if lifecycle.locked_for_review? && !force_taint + prior = lifecycle.latest_record || {} + render json: { + status: 'locked_for_review', + reviewed_by: prior['reviewed_by'], + reviewed_at: prior['reviewed_at'], + reviewed_notes: prior['reviewed_notes'], + version_id: lifecycle.latest_version_id + }, status: :conflict + return + end + + prev_reviewed_version_id = lifecycle.latest_version_id if lifecycle.locked_for_review? + prev_reviewer = lifecycle.latest_record&.dig('reviewed_by') if lifecycle.locked_for_review? + + write_result = Script.create(args) results = {} + if write_result.is_a?(String) + results['version_id'] = write_result + if force_taint && prev_reviewed_version_id + lifecycle.record_taint( + version_id: write_result, + username: username(), + prev_version_id: prev_reviewed_version_id, + prev_reviewer: prev_reviewer + ) + else + lifecycle.record_save(version_id: write_result, username: username()) + end + + # Auto-validate inline so scripts saved via API still acquire a + # validated_at timestamp. Combines syntax + mnemonic check; both must + # pass for the version to be marked validated. + validation = inline_validate(name, params[:text]) + results['validation'] = validation + if validation && validation[:passed] + lifecycle.record_validation(version_id: write_result, username: username()) + end + end + if ((File.extname(name) == '.py') and (params[:text] =~ PYTHON_SUITE_REGEX)) or ((File.extname(name) != '.py') and (params[:text] =~ SUITE_REGEX)) results_suites, results_error, success = Script.process_suite(name, params[:text], username: username(), scope: scope) results['suites'] = results_suites @@ -119,29 +164,16 @@ def run running_script_id = Script.run(scope, name, suite_runner, disconnect, environment, user_full_name(), username(), line_no, end_line_no) if running_script_id OpenC3::Logger.info("Script started: #{name}", scope: scope, user: username()) + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + if lifecycle.latest_version_id + lifecycle.record_execution(version_id: lifecycle.latest_version_id, username: username(), disconnect: disconnect) + end render plain: running_script_id.to_s else head :not_found end end - def lock - return unless authorization('script_edit') - scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true) - return unless scope - Script.lock(scope, name, username()) - render status: :ok - end - - def unlock - return unless authorization('script_edit') - scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true) - return unless scope - locked_by = Script.locked?(scope, name) - Script.unlock(scope, name) if username() == locked_by - render status: :ok - end - def destroy return unless authorization('script_edit') scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true) @@ -176,8 +208,18 @@ def syntax # Extract the target that this script lives under target_name = name.split('/')[0] return unless authorization('script_run', target_name: target_name) - script = Script.syntax(name, request.body.read) + body = request.body.read + script = Script.syntax(name, body) if script + # Record a validated_at on the latest version when the explicit syntax + # check is invoked from the UI and passes. Scope is optional in this + # endpoint (the route doesn't include it); skip recording when absent. + if script['title'].to_s.include?('Successful') && params[:scope] + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: params[:scope]) + if lifecycle.latest_version_id + lifecycle.record_validation(version_id: lifecycle.latest_version_id, username: username()) + end + end render json: script else head :error @@ -205,4 +247,33 @@ def delete_all_breakpoints OpenC3::Store.del("#{scope}__script-breakpoints") head :ok end + + private + + # Combined syntax + mnemonic check used to decide whether to mark a save as + # validated. Mnemonic check is a hard fail when applicable (script-engine + # languages); for ruby/python it's still browser-side, so this method only + # exercises the syntax check for those. + def inline_validate(name, text) + return { passed: false, syntax: nil, mnemonics: nil } if text.nil? + syntax = Script.syntax(name, text) + syntax_passed = syntax && syntax['title'].to_s.include?('Successful') + + mnemonics_passed = true + mnemonics = nil + extension = File.extname(name).to_s.downcase + if extension != '.rb' && extension != '.py' + begin + mnemonics = Script.mnemonics(name, text) + mnemonics_passed = mnemonics && mnemonics['title'].to_s.include?('Successful') + rescue => e + mnemonics = { 'title' => 'Mnemonics Check Skipped', 'description' => e.message } + mnemonics_passed = false + end + end + + { passed: syntax_passed && mnemonics_passed, syntax: syntax, mnemonics: mnemonics } + rescue => e + { passed: false, error: e.message } + end end diff --git a/openc3-cosmos-script-runner-api/config/routes.rb b/openc3-cosmos-script-runner-api/config/routes.rb index 6ea164b263..15ed65ba56 100644 --- a/openc3-cosmos-script-runner-api/config/routes.rb +++ b/openc3-cosmos-script-runner-api/config/routes.rb @@ -24,8 +24,6 @@ get "/scripts/*name" => "scripts#body", format: false, defaults: { format: 'html' } post "/scripts/*name/run(/:disconnect)" => "scripts#run", format: false, defaults: { format: 'html' } post "/scripts/*name/delete" => "scripts#destroy", format: false, defaults: { format: 'html' } - post "/scripts/*name/lock" => "scripts#lock" - post "/scripts/*name/unlock" => "scripts#unlock" post "/scripts/*name/syntax" => "scripts#syntax" post "/scripts/*name/mnemonics" => "scripts#mnemonics" post "/scripts/*name/instrumented" => "scripts#instrumented" diff --git a/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb b/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb index 4f1c2275a4..d9fe3449da 100644 --- a/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb +++ b/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb @@ -14,6 +14,7 @@ require "rails_helper" require "openc3/utilities/aws_bucket" require 'openc3/utilities/script' +require 'openc3/models/script_lifecycle_model' RSpec.describe ScriptsController, type: :controller do before(:each) do @@ -128,8 +129,9 @@ it "does not pass params which aren't permitted" do expect(Script).to receive(:create) do |params| - # Check that we don't pass extra params - expect(params.keys).to eql(%w[text breakpoints scope name]) + # Check that we don't pass extra params (username is added by the + # controller from the auth header, not from the request body) + expect(params.keys).to eql(%w[text breakpoints scope name username]) end post :create, params: {scope: "DEFAULT", name: "script.rb", text: "text", breakpoints: [1], other: "nope"} expect(response).to have_http_status(:ok) @@ -156,8 +158,6 @@ breakpoints = [1, 5] expect(Script).to receive(:body).with("DEFAULT", "INST/procedures/test.rb").and_return(script_content) - expect(Script).to receive(:locked?).with("DEFAULT", "INST/procedures/test.rb").and_return(false) - expect(Script).to receive(:lock).with("DEFAULT", "INST/procedures/test.rb", "anonymous") expect(Script).to receive(:get_breakpoints).with("DEFAULT", "INST/procedures/test.rb").and_return(breakpoints) get :body, params: {scope: "DEFAULT", name: "INST/procedures/test.rb"} @@ -166,24 +166,27 @@ json = JSON.parse(response.body) expect(json["contents"]).to eq(script_content) expect(json["breakpoints"]).to eq(breakpoints) - expect(json["locked"]).to eq(false) + # Fresh model has no recorded versions yet + expect(json["version_id"]).to be_nil + expect(json["locked_for_review"]).to eq(false) end - it "does not lock script if already locked" do + it "exposes the lifecycle of the latest version when one exists" do script_content = "puts 'Hello World'" - breakpoints = [1, 5] + OpenC3::ScriptLifecycleModel.get_or_build(name: "INST/procedures/test.rb", scope: "DEFAULT") + .record_save(version_id: "01ABCDEFGHJKMNPQRSTVWXYZ", username: "alice") + .record_review(version_id: "01ABCDEFGHJKMNPQRSTVWXYZ", username: "bob") expect(Script).to receive(:body).with("DEFAULT", "INST/procedures/test.rb").and_return(script_content) - expect(Script).to receive(:locked?).with("DEFAULT", "INST/procedures/test.rb").and_return(true) - expect(Script).not_to receive(:lock) - expect(Script).to receive(:get_breakpoints).with("DEFAULT", "INST/procedures/test.rb").and_return(breakpoints) + expect(Script).to receive(:get_breakpoints).with("DEFAULT", "INST/procedures/test.rb").and_return([]) get :body, params: {scope: "DEFAULT", name: "INST/procedures/test.rb"} expect(response).to have_http_status(:ok) json = JSON.parse(response.body) - expect(json["contents"]).to eq(script_content) - expect(json["locked"]).to eq(true) + expect(json["version_id"]).to eq("01ABCDEFGHJKMNPQRSTVWXYZ") + expect(json["locked_for_review"]).to eq(true) + expect(json["lifecycle"]["reviewed_by"]).to eq("bob") end it "processes suite files correctly" do @@ -192,8 +195,6 @@ suites_data = "{\"suites\":[]}" expect(Script).to receive(:body).with("DEFAULT", "INST/procedures/test.rb").and_return(script_content) - expect(Script).to receive(:locked?).with("DEFAULT", "INST/procedures/test.rb").and_return(false) - expect(Script).to receive(:lock).with("DEFAULT", "INST/procedures/test.rb", "anonymous") expect(Script).to receive(:get_breakpoints).with("DEFAULT", "INST/procedures/test.rb").and_return(breakpoints) expect(Script).to receive(:process_suite).with("INST/procedures/test.rb", script_content, username: "anonymous", scope: "DEFAULT").and_return([suites_data, nil, true]) @@ -212,8 +213,6 @@ suites_data = "{\"suites\":[]}" expect(Script).to receive(:body).with("DEFAULT", "INST/procedures/test.py").and_return(script_content) - expect(Script).to receive(:locked?).with("DEFAULT", "INST/procedures/test.py").and_return(false) - expect(Script).to receive(:lock).with("DEFAULT", "INST/procedures/test.py", "anonymous") expect(Script).to receive(:get_breakpoints).with("DEFAULT", "INST/procedures/test.py").and_return(breakpoints) expect(Script).to receive(:process_suite).with("INST/procedures/test.py", script_content, username: "anonymous", scope: "DEFAULT").and_return([suites_data, nil, true]) @@ -240,48 +239,6 @@ end end - describe "lock" do - it "locks a script" do - expect(Script).to receive(:lock).with("DEFAULT", "INST/procedures/test.rb", "anonymous") - - post :lock, params: {scope: "DEFAULT", name: "INST/procedures/test.rb"} - - expect(response).to have_http_status(:ok) - end - - it "handles authorization failure" do - post :lock, params: {name: "INST/procedures/test.rb"} - - expect(response).to have_http_status(:unauthorized) - end - end - - describe "unlock" do - it "unlocks a script that is locked by the user" do - expect(Script).to receive(:locked?).with("DEFAULT", "INST/procedures/test.rb").and_return("anonymous") - expect(Script).to receive(:unlock).with("DEFAULT", "INST/procedures/test.rb") - - post :unlock, params: {scope: "DEFAULT", name: "INST/procedures/test.rb"} - - expect(response).to have_http_status(:ok) - end - - it "does not unlock a script locked by another user" do - expect(Script).to receive(:locked?).with("DEFAULT", "INST/procedures/test.rb").and_return("another_user") - expect(Script).not_to receive(:unlock) - - post :unlock, params: {scope: "DEFAULT", name: "INST/procedures/test.rb"} - - expect(response).to have_http_status(:ok) - end - - it "handles authorization failure" do - post :unlock, params: {name: "INST/procedures/test.rb"} - - expect(response).to have_http_status(:unauthorized) - end - end - describe "destroy" do it "destroys a script" do expect(Script).to receive(:destroy).with("DEFAULT", "INST/procedures/test.rb") diff --git a/openc3-cosmos-script-runner-api/spec/models/script_spec.rb b/openc3-cosmos-script-runner-api/spec/models/script_spec.rb index b23b4bf54f..fd30edbae3 100644 --- a/openc3-cosmos-script-runner-api/spec/models/script_spec.rb +++ b/openc3-cosmos-script-runner-api/spec/models/script_spec.rb @@ -30,36 +30,6 @@ end end - describe "self.lock, self.unlock, self.locked?" do - it "locks and unlocks scripts" do - name = "script.rb" - username = "user1" - - # Not locked initially - expect(Script.locked?("DEFAULT", name)).to be false - - # Lock the script - Script.lock("DEFAULT", name, username) - expect(Script.locked?("DEFAULT", name)).to eq username - - # Unlock the script - Script.unlock("DEFAULT", name) - expect(Script.locked?("DEFAULT", name)).to be false - end - - it "handles modified script names with asterisks" do - name = "script.rb*" - username = "user1" - - Script.lock("DEFAULT", name, username) - expect(Script.locked?("DEFAULT", name)).to eq username - expect(Script.locked?("DEFAULT", "script.rb")).to eq username - - Script.unlock("DEFAULT", name) - expect(Script.locked?("DEFAULT", name)).to be false - end - end - describe "self.get_breakpoints" do it "returns breakpoints for a script" do name = "script.rb" @@ -99,7 +69,7 @@ } allow(Script).to receive(:body).and_return(nil) - expect(OpenC3::TargetFile).to receive(:create).with("DEFAULT", "new_script.rb", "puts 'Hello'") + expect(OpenC3::TargetFile).to receive(:create).with("DEFAULT", "new_script.rb", "puts 'Hello'", username: nil) Script.create(params) @@ -117,7 +87,7 @@ } allow(Script).to receive(:body).and_return("puts 'Original'") - expect(OpenC3::TargetFile).to receive(:create).with("DEFAULT", "existing_script.rb", "puts 'Updated'") + expect(OpenC3::TargetFile).to receive(:create).with("DEFAULT", "existing_script.rb", "puts 'Updated'", username: nil) Script.create(params) @@ -153,7 +123,7 @@ } allow(Script).to receive(:body).and_return(nil) - expect(OpenC3::TargetFile).to receive(:create).with("DEFAULT", "script.rb", "puts 'Hello'") + expect(OpenC3::TargetFile).to receive(:create).with("DEFAULT", "script.rb", "puts 'Hello'", username: nil) # Pre-store breakpoints OpenC3::Store.hset("DEFAULT__script-breakpoints", "script.rb", [5, 10].to_json) diff --git a/openc3/bin/openc3cli b/openc3/bin/openc3cli index 59bb3b5143..165b1b52c8 100755 --- a/openc3/bin/openc3cli +++ b/openc3/bin/openc3cli @@ -1443,6 +1443,9 @@ if not ARGV[0].nil? # argument(s) given end # Always ensure the scriptrunner policy is in place since it is required for script execution client.ensure_scriptrunner_policy(ENV['OPENC3_CONFIG_BUCKET'], ENV['OPENC3_LOGS_BUCKET']) + # Enable versioning on the config bucket so script edits, plugin uploads, + # and other config writes accumulate version history. Idempotent. + client.ensure_versioning_enabled(ENV['OPENC3_CONFIG_BUCKET']) if ENV['OPENC3_CONFIG_BUCKET'] when 'runmigrations' if ARGV[1] == '--help' || ARGV[1] == '-h' diff --git a/openc3/lib/openc3/models/script_lifecycle_model.rb b/openc3/lib/openc3/models/script_lifecycle_model.rb new file mode 100644 index 0000000000..13b635c1b5 --- /dev/null +++ b/openc3/lib/openc3/models/script_lifecycle_model.rb @@ -0,0 +1,191 @@ +# encoding: ascii-8bit + +# Copyright 2026 OpenC3, Inc. +# All Rights Reserved. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. +# See LICENSE.md for more details. +# +# This file may also be used under the terms of a commercial license +# if purchased from OpenC3, Inc. + +require 'openc3/models/model' + +module OpenC3 + # Tracks per-version lifecycle events for a script: when it was saved (and by + # whom), when its syntax + mnemonic check passed, when it was reviewed/signed + # off, and each time it was executed (with or without targets connected). + # Also records taint provenance — when a reviewed version is overridden, the + # next saved version is marked tainted with a pointer back to the prior + # reviewed version + reviewer. + # + # Stored under key "#{scope}__script-lifecycle", one hash field per script + # name. The field value is a JSON document that holds a `versions` map keyed + # by S3 VersionId. + class ScriptLifecycleModel < Model + PRIMARY_KEY = 'script-lifecycle' + + attr_accessor :versions, :latest_version_id + + def self.get(name:, scope:) + super("#{scope}__#{PRIMARY_KEY}", name: name) + end + + def self.names(scope:) + super("#{scope}__#{PRIMARY_KEY}") + end + + def self.all(scope:) + super("#{scope}__#{PRIMARY_KEY}") + end + + def self.from_json(json, scope:) + json = JSON.parse(json, allow_nan: true, create_additions: true) if String === json + raise "json data is nil" if json.nil? + self.new(**json.transform_keys(&:to_sym), scope: scope) + end + + # Returns the model for this script, building an empty one in memory (not + # yet persisted) if no entry exists. Most transition recording flows want + # this — they don't care whether a row already existed. + def self.get_or_build(name:, scope:) + existing = get(name: name, scope: scope) + return from_json(existing, scope: scope) if existing + self.new(name: name, scope: scope) + end + + def initialize( + name:, + scope:, + versions: nil, + latest_version_id: nil, + updated_at: nil, + plugin: nil + ) + super("#{scope}__#{PRIMARY_KEY}", name: name, updated_at: updated_at, plugin: plugin, scope: scope) + @versions = versions || {} + @latest_version_id = latest_version_id + end + + def as_json(*_a) + { + 'name' => @name, + 'versions' => @versions, + 'latest_version_id' => @latest_version_id, + 'updated_at' => @updated_at, + 'scope' => @scope + } + end + + # Derived primary state for a single version. A version that has been + # reviewed counts as 'reviewed' even if executions happened later — those + # are recorded as orthogonal events. + def state_of(version_id) + rec = @versions[version_id] + return 'unknown' unless rec + return 'reviewed' if rec['reviewed_at'] + return 'validated' if rec['validated_at'] + 'new' + end + + def latest_state + state_of(@latest_version_id) + end + + # True iff saving over this script should be refused unless the caller + # explicitly opts into tainting. Reviewed = read-only. + def locked_for_review? + latest_state == 'reviewed' + end + + def latest_record + @latest_version_id ? @versions[@latest_version_id] : nil + end + + # Record a fresh save. New version becomes the latest. + def record_save(version_id:, username:, timestamp: nil) + timestamp ||= Time.now.utc.iso8601 + rec = @versions[version_id] ||= {} + rec['saved_by'] = username + rec['saved_at'] = timestamp + @latest_version_id = version_id + persist + self + end + + # Record a save that overrides a previously reviewed version. The new + # version is flagged tainted with provenance back to the prior reviewer. + # Review of a tainted version cleans it (caller invokes record_review + # later — taint flag is preserved as historical fact, but state derivation + # treats reviewed as authoritative). + def record_taint(version_id:, username:, prev_version_id:, prev_reviewer:, timestamp: nil) + timestamp ||= Time.now.utc.iso8601 + rec = @versions[version_id] ||= {} + rec['saved_by'] = username + rec['saved_at'] = timestamp + rec['tainted'] = true + rec['tainted_from_version_id'] = prev_version_id + rec['tainted_from_reviewed_by'] = prev_reviewer + @latest_version_id = version_id + persist + self + end + + # Record that the syntax + mnemonic check passed for this version. Idempotent. + def record_validation(version_id:, username: nil, timestamp: nil) + return self unless @versions[version_id] + timestamp ||= Time.now.utc.iso8601 + @versions[version_id]['validated_at'] = timestamp + @versions[version_id]['validated_by'] = username if username + persist + self + end + + # Record sign-off. Latest reviewer wins (reviews can happen multiple times). + def record_review(version_id:, username:, notes: nil, timestamp: nil) + return self unless @versions[version_id] + timestamp ||= Time.now.utc.iso8601 + @versions[version_id]['reviewed_at'] = timestamp + @versions[version_id]['reviewed_by'] = username + @versions[version_id]['reviewed_notes'] = notes if notes + persist + self + end + + # Append an execution event. type: 'executed' for connected runs, + # 'executed_disconnect' for disconnect-mode runs. + def record_execution(version_id:, username:, disconnect: false, timestamp: nil) + return self unless @versions[version_id] + timestamp ||= Time.now.utc.iso8601 + @versions[version_id]['executions'] ||= [] + @versions[version_id]['executions'] << { + 'type' => disconnect ? 'executed_disconnect' : 'executed', + 'username' => username, + 'at' => timestamp + } + persist + self + end + + # Record a restore operation: a new version_id was created by re-PUTting + # the body of a prior version. The new version becomes latest. + def record_restore(version_id:, username:, restored_from_version_id:, timestamp: nil) + timestamp ||= Time.now.utc.iso8601 + rec = @versions[version_id] ||= {} + rec['saved_by'] = username + rec['saved_at'] = timestamp + rec['restored_from_version_id'] = restored_from_version_id + @latest_version_id = version_id + persist + self + end + + # Persist the current in-memory state. Upsert semantics — first call is an + # insert, subsequent calls overwrite. + def persist + create(force: true) + end + end +end diff --git a/openc3/lib/openc3/utilities/aws_bucket.rb b/openc3/lib/openc3/utilities/aws_bucket.rb index cccf6a59ae..12983b37e8 100644 --- a/openc3/lib/openc3/utilities/aws_bucket.rb +++ b/openc3/lib/openc3/utilities/aws_bucket.rb @@ -191,6 +191,18 @@ def ensure_scriptrunner_policy(config_bucket, logs_bucket) end end + # Idempotently enable bucket versioning. Versitygw needs the gateway started + # with --versioning-dir for this to succeed; if it isn't, versitygw responds + # with VersioningNotConfigured and we surface a warning rather than failing + # bucket bootstrap. + def ensure_versioning_enabled(bucket) + options = { bucket: bucket, versioning_configuration: { status: 'Enabled' } } + options[:checksum_algorithm] = "SHA256" if @use_checksum + @client.put_bucket_versioning(options) + rescue Aws::S3::Errors::NotImplemented, Aws::S3::Errors::ServiceError, Aws::S3::Errors::InternalError => e + Logger.warn("put_bucket_versioning for #{bucket} not supported by S3 backend: #{e.message}") + end + def exist?(bucket) @client.head_bucket({ bucket: bucket }) true @@ -204,17 +216,48 @@ def delete(bucket) end end - def get_object(bucket:, key:, path: nil, range: nil) + def get_object(bucket:, key:, path: nil, range: nil, version_id: nil) + args = { bucket: bucket, key: key, range: range } + args[:version_id] = version_id if version_id if path - @client.get_object(bucket: bucket, key: key, response_target: path, range: range) + @client.get_object(**args, response_target: path) else - @client.get_object(bucket: bucket, key: key, range: range) + @client.get_object(**args) end # If the key is not found return nil rescue Aws::S3::Errors::NoSuchKey nil end + # Lists all versions (and delete markers) of objects matching a prefix. + # Each Versions entry has: key, version_id, is_latest, last_modified, size, etag + # Each DeleteMarkers entry has: key, version_id, is_latest, last_modified + # Returns { versions: [...], delete_markers: [...] }. + def list_object_versions(bucket:, prefix: nil, max_request: 1000, max_total: 100_000) + key_marker = nil + version_id_marker = nil + versions = [] + delete_markers = [] + while true + resp = @client.list_object_versions({ + bucket: bucket, + max_keys: max_request, + prefix: prefix, + key_marker: key_marker, + version_id_marker: version_id_marker + }) + versions.concat(resp.versions || []) + delete_markers.concat(resp.delete_markers || []) + break if (versions.length + delete_markers.length) >= max_total + break unless resp.is_truncated + key_marker = resp.next_key_marker + version_id_marker = resp.next_version_id_marker + end + { versions: versions, delete_markers: delete_markers } + rescue Aws::S3::Errors::NoSuchBucket + raise NotFound, "Bucket '#{bucket}' does not exist." + end + def list_objects(bucket:, prefix: nil, max_request: 1000, max_total: 100_000) token = nil result = [] @@ -287,11 +330,10 @@ def list_files(bucket:, path:, only_directories: false, metadata: false) end # get metadata for a specific object - def head_object(bucket:, key:) - @client.head_object({ - bucket: bucket, - key: key - }) + def head_object(bucket:, key:, version_id: nil) + args = { bucket: bucket, key: key } + args[:version_id] = version_id if version_id + @client.head_object(args) rescue Aws::S3::Errors::NotFound raise NotFound, "Object '#{bucket}/#{key}' does not exist." end @@ -331,8 +373,10 @@ def check_object(bucket:, key:, retries: true) false end - def delete_object(bucket:, key:) - @client.delete_object(bucket: bucket, key: key) + def delete_object(bucket:, key:, version_id: nil) + args = { bucket: bucket, key: key } + args[:version_id] = version_id if version_id + @client.delete_object(args) rescue Exception => e Logger.error("Error deleting object bucket: #{bucket}, key: #{key}: #{e.message}") end diff --git a/openc3/lib/openc3/utilities/bucket.rb b/openc3/lib/openc3/utilities/bucket.rb index 324aab7400..37e721b770 100644 --- a/openc3/lib/openc3/utilities/bucket.rb +++ b/openc3/lib/openc3/utilities/bucket.rb @@ -45,6 +45,13 @@ def ensure_scriptrunner_policy(config_bucket, logs_bucket) # No-op by default, implemented by AwsBucket for local mode end + # Idempotently enable bucket versioning. Implementations should swallow + # backend-not-supported errors so missing versioning support never blocks + # bucket bootstrap. + def ensure_versioning_enabled(bucket) + raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" + end + def exist?(bucket) raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" end @@ -53,7 +60,7 @@ def delete(bucket) raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" end - def get_object(bucket:, key:, path: nil, range: nil) + def get_object(bucket:, key:, path: nil, range: nil, version_id: nil) raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" end @@ -61,6 +68,12 @@ def list_objects(bucket:, prefix: nil, max_request: nil, max_total: nil) raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" end + # List all object versions (and delete markers) under a key prefix. + # Returns an array of hashes — see AwsBucket implementation for the shape. + def list_object_versions(bucket:, prefix: nil, max_request: nil, max_total: nil) + raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" + end + def list_files(bucket:, path:, only_directories: false, metadata: false) raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" end @@ -73,7 +86,7 @@ def check_object(bucket:, key:, retries: true) raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" end - def delete_object(bucket:, key:) + def delete_object(bucket:, key:, version_id: nil) raise NotImplementedError, "#{self.class} has not implemented method '#{__method__}'" end diff --git a/openc3/lib/openc3/utilities/script.rb b/openc3/lib/openc3/utilities/script.rb index 3885075fae..82a1ccd6b8 100644 --- a/openc3/lib/openc3/utilities/script.rb +++ b/openc3/lib/openc3/utilities/script.rb @@ -29,23 +29,6 @@ def self.all(scope, target = nil) super(scope, nil, target: target) # No path matchers end - def self.lock(scope, name, username) - name = name.split('*')[0] # Split '*' that indicates modified - OpenC3::Store.hset("#{scope}__script-locks", name, username) - end - - def self.unlock(scope, name) - name = name.split('*')[0] # Split '*' that indicates modified - OpenC3::Store.hdel("#{scope}__script-locks", name) - end - - def self.locked?(scope, name) - name = name.split('*')[0] # Split '*' that indicates modified - locked_by = OpenC3::Store.hget("#{scope}__script-locks", name) - locked_by ||= false - locked_by - end - def self.get_breakpoints(scope, name) breakpoints = OpenC3::Store.hget("#{scope}__script-breakpoints", name.split('*')[0]) # Split '*' that indicates modified return JSON.parse(breakpoints, allow_nan: true, create_additions: true) if breakpoints @@ -162,11 +145,15 @@ def self.process_suite(name, contents, new_process: true, username: nil, scope:) return stdout_results, stderr_results, success end + # Returns the new S3 VersionId (String) when the bucket write produced a new + # version, true when no new version was needed, false on failure, or nil when + # the text was unchanged from the existing body and no write happened. def self.create(params) + write_result = nil existing = body(params[:scope], params[:name]) # Commit if there is no existing or something has changed if existing.nil? or existing != params[:text] - super(params[:scope], params[:name], params[:text]) + write_result = super(params[:scope], params[:name], params[:text], username: params[:username]) end breakpoints = params[:breakpoints] if breakpoints @@ -177,6 +164,7 @@ def self.create(params) breakpoints.as_json().to_json(allow_nan: true)) end end + write_result end def self.delete_temp(scope) diff --git a/openc3/lib/openc3/utilities/target_file.rb b/openc3/lib/openc3/utilities/target_file.rb index 65b39e96d9..140c69fe0d 100644 --- a/openc3/lib/openc3/utilities/target_file.rb +++ b/openc3/lib/openc3/utilities/target_file.rb @@ -120,26 +120,31 @@ def self.body(scope, name) end end - def self.create(scope, name, text, content_type: 'text/plain') + # Returns the new S3 VersionId (String) on success when the bucket is + # versioned, true on success when not versioned, or false on failure. + def self.create(scope, name, text, content_type: 'text/plain', username: nil) return false unless text if ENV['OPENC3_LOCAL_MODE'] OpenC3::LocalMode.put_target_file("#{scope}/targets_modified/#{name}", text, scope: scope) end client = Bucket.getClient() - client.put_object( + metadata = username ? { 'username' => username } : nil + resp = client.put_object( # Use targets_modified to save modifications # This keeps the original target clean (read-only) bucket: ENV['OPENC3_CONFIG_BUCKET'], key: "#{scope}/targets_modified/#{name}", body: text, content_type: content_type, + metadata: metadata, ) # Wait for the object to exist if client.check_object( bucket: ENV['OPENC3_CONFIG_BUCKET'], key: "#{scope}/targets_modified/#{name}", ) - true + version_id = resp.respond_to?(:version_id) ? resp.version_id : nil + (version_id && !version_id.empty?) ? version_id : true else false end diff --git a/openc3/spec/models/script_lifecycle_model_spec.rb b/openc3/spec/models/script_lifecycle_model_spec.rb new file mode 100644 index 0000000000..af4ae729e8 --- /dev/null +++ b/openc3/spec/models/script_lifecycle_model_spec.rb @@ -0,0 +1,191 @@ +# encoding: ascii-8bit + +# Copyright 2026 OpenC3, Inc. +# All Rights Reserved. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. +# See LICENSE.md for more details. +# +# This file may also be used under the terms of a commercial license +# if purchased from OpenC3, Inc. + +require 'spec_helper' +require 'openc3/models/script_lifecycle_model' + +module OpenC3 + describe ScriptLifecycleModel, type: :model do + before(:each) do + mock_redis() + end + + let(:scope) { 'DEFAULT' } + let(:name) { 'INST/procedures/test.rb' } + let(:v1) { '01KQW7ZHJGQZK9Q3QVZNHQQ287' } + let(:v2) { '01KQW7ZHY72RW7SH3F2YPDKKJK' } + let(:v3) { '01KQW7ZJ9X70B4MEYS9WS1V3ZK' } + + describe '.get_or_build' do + it 'returns an empty model when nothing is persisted' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + expect(model.versions).to eq({}) + expect(model.latest_version_id).to be_nil + expect(model.latest_state).to eq('unknown') + end + + it 'rehydrates a persisted model' do + ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + rehydrated = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + expect(rehydrated.latest_version_id).to eq(v1) + expect(rehydrated.versions[v1]['saved_by']).to eq('alice') + end + end + + describe 'state derivation' do + it 'returns new for a fresh save' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + expect(model.latest_state).to eq('new') + expect(model.locked_for_review?).to be(false) + end + + it 'returns validated after record_validation' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_validation(version_id: v1) + expect(model.latest_state).to eq('validated') + expect(model.locked_for_review?).to be(false) + end + + it 'returns reviewed after record_review and locks the script' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_validation(version_id: v1) + .record_review(version_id: v1, username: 'bob', notes: 'looks good') + expect(model.latest_state).to eq('reviewed') + expect(model.locked_for_review?).to be(true) + expect(model.versions[v1]['reviewed_by']).to eq('bob') + expect(model.versions[v1]['reviewed_notes']).to eq('looks good') + end + end + + describe '#record_save' do + it 'advances latest_version_id with each new version' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + model.record_save(version_id: v1, username: 'alice') + model.record_save(version_id: v2, username: 'bob') + expect(model.latest_version_id).to eq(v2) + expect(model.versions.keys).to contain_exactly(v1, v2) + end + + it 'preserves prior versions immutably' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_validation(version_id: v1) + .record_save(version_id: v2, username: 'alice') + expect(model.versions[v1]['validated_at']).not_to be_nil + expect(model.versions[v2]['validated_at']).to be_nil + end + end + + describe '#record_taint' do + it 'flags the new version with provenance from the prior reviewed version' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_review(version_id: v1, username: 'bob') + .record_taint(version_id: v2, username: 'alice', prev_version_id: v1, prev_reviewer: 'bob') + expect(model.latest_version_id).to eq(v2) + expect(model.versions[v2]['tainted']).to be(true) + expect(model.versions[v2]['tainted_from_version_id']).to eq(v1) + expect(model.versions[v2]['tainted_from_reviewed_by']).to eq('bob') + expect(model.locked_for_review?).to be(false) + end + + it 'leaves the prior reviewed version reviewed' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_review(version_id: v1, username: 'bob') + .record_taint(version_id: v2, username: 'alice', prev_version_id: v1, prev_reviewer: 'bob') + expect(model.state_of(v1)).to eq('reviewed') + end + end + + describe 'review of a tainted version cleans it' do + it 'flips state to reviewed even though tainted flag remains' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_review(version_id: v1, username: 'bob') + .record_taint(version_id: v2, username: 'alice', prev_version_id: v1, prev_reviewer: 'bob') + .record_validation(version_id: v2) + .record_review(version_id: v2, username: 'bob', notes: 're-reviewed after edit') + expect(model.latest_state).to eq('reviewed') + expect(model.locked_for_review?).to be(true) + expect(model.versions[v2]['tainted']).to be(true) + expect(model.versions[v2]['reviewed_by']).to eq('bob') + end + end + + describe '#record_review (multiple reviews allowed)' do + it 'overwrites prior review fields with the latest sign-off' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_review(version_id: v1, username: 'bob', notes: 'first pass') + .record_review(version_id: v1, username: 'carol', notes: 'second pass') + expect(model.versions[v1]['reviewed_by']).to eq('carol') + expect(model.versions[v1]['reviewed_notes']).to eq('second pass') + end + end + + describe '#record_execution' do + it 'appends connected and disconnect runs as separate events' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_execution(version_id: v1, username: 'alice', disconnect: true) + .record_execution(version_id: v1, username: 'alice', disconnect: false) + execs = model.versions[v1]['executions'] + expect(execs.size).to eq(2) + expect(execs[0]['type']).to eq('executed_disconnect') + expect(execs[1]['type']).to eq('executed') + end + end + + describe '#record_restore' do + it 'creates a new version with restored_from pointer' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_save(version_id: v2, username: 'bob') + .record_restore(version_id: v3, username: 'alice', restored_from_version_id: v1) + expect(model.latest_version_id).to eq(v3) + expect(model.versions[v3]['restored_from_version_id']).to eq(v1) + end + end + + describe 'transitions on missing versions are no-ops' do + it 'does not crash when validating a version that was never saved' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_validation(version_id: v1) + expect(model.versions).to eq({}) + end + end + + describe 'JSON round-trip' do + it 'preserves all fields through save and reload' do + ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_validation(version_id: v1) + .record_review(version_id: v1, username: 'bob', notes: 'ok') + .record_execution(version_id: v1, username: 'alice', disconnect: false) + .record_taint(version_id: v2, username: 'alice', prev_version_id: v1, prev_reviewer: 'bob') + + rehydrated = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + expect(rehydrated.latest_version_id).to eq(v2) + expect(rehydrated.versions[v1]['validated_at']).not_to be_nil + expect(rehydrated.versions[v1]['reviewed_by']).to eq('bob') + expect(rehydrated.versions[v1]['executions'].size).to eq(1) + expect(rehydrated.versions[v2]['tainted']).to be(true) + end + end + end +end diff --git a/playwright/tests/script-runner/api.s.spec.ts b/playwright/tests/script-runner/api.s.spec.ts index f6f11a8f6e..9f4ec644d2 100644 --- a/playwright/tests/script-runner/api.s.spec.ts +++ b/playwright/tests/script-runner/api.s.spec.ts @@ -36,14 +36,6 @@ async function openFile(page, utils, filename) { await page.locator(`text=${filename}`).click() await page.locator('[data-test=file-open-save-submit-btn]').click() await expect(page.locator('.v-dialog')).not.toBeVisible() - - // Check for potential " is editing this script" - // This can happen if we had to do a retry on this test - const someone = page.getByText('is editing this script') - if (await someone.isVisible()) { - await page.locator('[data-test="unlock-button"]').click() - await page.locator('[data-test="confirm-dialog-force unlock"]').click() - } } async function runScript(page, utils, filename, callback = async () => {}) { diff --git a/playwright/tests/script-runner/prompts.p.spec.ts b/playwright/tests/script-runner/prompts.p.spec.ts index d47e586cb5..9b32144dde 100644 --- a/playwright/tests/script-runner/prompts.p.spec.ts +++ b/playwright/tests/script-runner/prompts.p.spec.ts @@ -333,14 +333,6 @@ async function openFile(page, utils, filename) { await page.locator(`text=${filename}`).click() await page.locator('[data-test=file-open-save-submit-btn]').click() await expect(page.locator('.v-dialog')).not.toBeVisible() - - // Check for potential " is editing this script" - // This can happen if we had to do a retry on this test - const someone = page.getByText('is editing this script') - if (await someone.isVisible()) { - await page.locator('[data-test="unlock-button"]').click() - await page.locator('[data-test="confirm-dialog-force unlock"]').click() - } } async function runScript(page, utils, filename) { From 5c6288d6ce56d8080121a7649a27f7e4c8dcb8b7 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 5 May 2026 09:32:37 -0600 Subject: [PATCH 2/6] feat(scriptrunner): version history, sign-off, and restore endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add four new endpoints under /script-api/scripts/*name: GET versions (S3 versions merged with lifecycle metadata, newest-first), GET version (body of a specific version_id), POST restore (re-PUT a historical body with the same review-lock + force_taint semantics as save), and POST review (sign-off, gated by a new can_approve_script? predicate that's no-op in Core and overridable in Enterprise's existing Authorization module). Sign-off requires the supplied version_id to match the current latest_version_id, returning 409 version_mismatch if the script was edited between display and click. Refactor ScriptLifecycleModel into nested validated/reviewed blocks plus a richer executions array (disconnect, environment, suite_runner, running_script_id) so the UI can render three independent badges (validated/reviewed/executed) that fill in with metadata as each facet acquires data. Final execution status stays in ScriptStatusModel — dereferenced via running_script_id — to avoid cross-process writes from the spawned RunningScript. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../app/controllers/scripts_controller.rb | 253 ++++++++++++++++-- .../config/routes.rb | 7 + .../controllers/scripts_controller_spec.rb | 162 ++++++++++- .../openc3/models/script_lifecycle_model.rb | 103 ++++--- openc3/lib/openc3/utilities/authorization.rb | 8 + .../models/script_lifecycle_model_spec.rb | 81 ++++-- 6 files changed, 526 insertions(+), 88 deletions(-) diff --git a/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb b/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb index f42739e8cd..1533c087d4 100644 --- a/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb +++ b/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb @@ -96,19 +96,17 @@ def create lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) force_taint = params[:force_taint].to_s == 'true' if lifecycle.locked_for_review? && !force_taint - prior = lifecycle.latest_record || {} + prior_reviewed = lifecycle.latest_record&.dig('reviewed') || {} render json: { status: 'locked_for_review', - reviewed_by: prior['reviewed_by'], - reviewed_at: prior['reviewed_at'], - reviewed_notes: prior['reviewed_notes'], + reviewed: prior_reviewed, version_id: lifecycle.latest_version_id }, status: :conflict return end prev_reviewed_version_id = lifecycle.latest_version_id if lifecycle.locked_for_review? - prev_reviewer = lifecycle.latest_record&.dig('reviewed_by') if lifecycle.locked_for_review? + prev_reviewer = lifecycle.latest_record&.dig('reviewed', 'by') if lifecycle.locked_for_review? write_result = Script.create(args) results = {} @@ -125,14 +123,17 @@ def create lifecycle.record_save(version_id: write_result, username: username()) end - # Auto-validate inline so scripts saved via API still acquire a - # validated_at timestamp. Combines syntax + mnemonic check; both must - # pass for the version to be marked validated. + # Auto-validate inline so every saved version acquires a validated + # block. Combines syntax + mnemonic check; pass/fail and any error + # lines are recorded for the UI's validated badge. validation = inline_validate(name, params[:text]) results['validation'] = validation - if validation && validation[:passed] - lifecycle.record_validation(version_id: write_result, username: username()) - end + lifecycle.record_validation( + version_id: write_result, + passed: validation[:passed], + errors: validation[:errors] || [], + username: username() + ) end if ((File.extname(name) == '.py') and (params[:text] =~ PYTHON_SUITE_REGEX)) or ((File.extname(name) != '.py') and (params[:text] =~ SUITE_REGEX)) @@ -166,7 +167,14 @@ def run OpenC3::Logger.info("Script started: #{name}", scope: scope, user: username()) lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) if lifecycle.latest_version_id - lifecycle.record_execution(version_id: lifecycle.latest_version_id, username: username(), disconnect: disconnect) + lifecycle.record_execution( + version_id: lifecycle.latest_version_id, + username: username(), + disconnect: disconnect, + environment: environment, + suite_runner: suite_runner, + running_script_id: running_script_id + ) end render plain: running_script_id.to_s else @@ -211,13 +219,25 @@ def syntax body = request.body.read script = Script.syntax(name, body) if script - # Record a validated_at on the latest version when the explicit syntax - # check is invoked from the UI and passes. Scope is optional in this - # endpoint (the route doesn't include it); skip recording when absent. - if script['title'].to_s.include?('Successful') && params[:scope] + # Update the validated block on the latest version when an explicit + # syntax check is invoked from the UI. Scope is optional on this route + # (path has only the name); skip recording when absent. + if params[:scope] + passed = script['title'].to_s.include?('Successful') + errors = [] + unless passed + desc = script['description'] + lines = desc.is_a?(String) ? (JSON.parse(desc) rescue [desc]) : Array(desc) + errors.concat(lines.compact.map(&:to_s)) + end lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: params[:scope]) if lifecycle.latest_version_id - lifecycle.record_validation(version_id: lifecycle.latest_version_id, username: username()) + lifecycle.record_validation( + version_id: lifecycle.latest_version_id, + passed: passed, + errors: errors, + username: username() + ) end end render json: script @@ -248,14 +268,193 @@ def delete_all_breakpoints head :ok end + # GET /scripts/*name/versions — list S3 versions merged with lifecycle data. + # Returns versions newest-first. + def versions + return unless authorization('script_view') + scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true) + return unless scope + bucket_name = ENV['OPENC3_CONFIG_BUCKET'] + key = "#{scope}/targets_modified/#{name}" + bucket = OpenC3::Bucket.getClient() + s3 = bucket.list_object_versions(bucket: bucket_name, prefix: key) + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + versions = s3[:versions].select { |v| v.key == key }.map do |v| + record = lifecycle.versions[v.version_id] || {} + { + version_id: v.version_id, + size: v.size, + last_modified: v.last_modified, + is_latest: v.is_latest, + etag: v.etag, + state: lifecycle.state_of(v.version_id), + saved_by: record['saved_by'], + saved_at: record['saved_at'], + validated: record['validated'], + reviewed: record['reviewed'], + executions: record['executions'] || [], + tainted: record['tainted'] == true, + tainted_from_version_id: record['tainted_from_version_id'], + tainted_from_reviewed_by: record['tainted_from_reviewed_by'], + restored_from_version_id: record['restored_from_version_id'] + } + end + delete_markers = s3[:delete_markers].select { |dm| dm.key == key }.map do |dm| + { version_id: dm.version_id, last_modified: dm.last_modified, is_latest: dm.is_latest, deleted: true } + end + render json: { versions: versions, delete_markers: delete_markers, latest_version_id: lifecycle.latest_version_id } + rescue => e + log_error(e) + render json: { status: 'error', message: e.message }, status: :internal_server_error + end + + # GET /scripts/*name/version?version_id=... — return body of a specific version. + def version_body + return unless authorization('script_view') + scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true) + return unless scope + version_id = params[:version_id] + if version_id.nil? || version_id.empty? + render json: { status: 'error', message: 'version_id required' }, status: :bad_request + return + end + bucket = OpenC3::Bucket.getClient() + resp = bucket.get_object( + bucket: ENV['OPENC3_CONFIG_BUCKET'], + key: "#{scope}/targets_modified/#{name}", + version_id: version_id + ) + if resp && resp.body + body = File.extname(name) == '.bin' ? (resp.body.binmode; resp.body.read) : resp.body.read.force_encoding('UTF-8') + render plain: body + else + head :not_found + end + rescue => e + log_error(e) + render json: { status: 'error', message: e.message }, status: :internal_server_error + end + + # POST /scripts/*name/restore body {version_id, force_taint?} — re-PUT the + # body of an old version. Same review-lock semantics as save: refused with + # 409 if the latest version is reviewed unless force_taint=true, in which + # case the new (restored) version is marked tainted with provenance. + def restore + return unless authorization('script_edit') + scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true) + return unless scope + version_id = params[:version_id] + if version_id.nil? || version_id.empty? + render json: { status: 'error', message: 'version_id required' }, status: :bad_request + return + end + + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + force_taint = params[:force_taint].to_s == 'true' + if lifecycle.locked_for_review? && !force_taint + prior_reviewed = lifecycle.latest_record&.dig('reviewed') || {} + render json: { + status: 'locked_for_review', + reviewed: prior_reviewed, + version_id: lifecycle.latest_version_id + }, status: :conflict + return + end + + prev_reviewed_version_id = lifecycle.latest_version_id if lifecycle.locked_for_review? + prev_reviewer = lifecycle.latest_record&.dig('reviewed', 'by') if lifecycle.locked_for_review? + + bucket = OpenC3::Bucket.getClient() + bucket_name = ENV['OPENC3_CONFIG_BUCKET'] + key = "#{scope}/targets_modified/#{name}" + src = bucket.get_object(bucket: bucket_name, key: key, version_id: version_id) + if src.nil? || src.body.nil? + head :not_found + return + end + body = src.body.read + + write_result = OpenC3::TargetFile.create(scope, name, body, username: username()) + new_version_id = write_result.is_a?(String) ? write_result : nil + + if new_version_id + if force_taint && prev_reviewed_version_id + lifecycle.record_taint( + version_id: new_version_id, + username: username(), + prev_version_id: prev_reviewed_version_id, + prev_reviewer: prev_reviewer + ) + # record_taint sets latest_version_id; layer the restore provenance on top + lifecycle.versions[new_version_id]['restored_from_version_id'] = version_id + lifecycle.persist + else + lifecycle.record_restore( + version_id: new_version_id, + username: username(), + restored_from_version_id: version_id + ) + end + end + + OpenC3::Logger.info("Script restored: #{name} from #{version_id}", scope: scope, user: username()) + render json: { version_id: new_version_id, restored_from_version_id: version_id } + rescue => e + log_error(e) + render json: { status: 'error', message: e.message }, status: :internal_server_error + end + + # POST /scripts/*name/review body {version_id, notes} — sign off on the + # specified version. The version_id must match the current latest_version_id + # (otherwise the script has been edited since the reviewer loaded it, and + # they'd be signing off on a different version than they reviewed). + def review + return unless authorization('script_edit') + scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true) + return unless scope + + unless can_approve_script?(scope: scope, token: request.headers['HTTP_AUTHORIZATION']) + render json: { status: 'error', message: 'not authorized to approve scripts' }, status: :forbidden + return + end + + version_id = params[:version_id] + if version_id.nil? || version_id.empty? + render json: { status: 'error', message: 'version_id required' }, status: :bad_request + return + end + notes = params[:notes] + + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + if lifecycle.latest_version_id != version_id + render json: { + status: 'version_mismatch', + message: 'A newer version exists; reload before signing off', + latest_version_id: lifecycle.latest_version_id + }, status: :conflict + return + end + + lifecycle.record_review(version_id: version_id, username: username(), notes: notes) + OpenC3::Logger.info("Script reviewed: #{name} #{version_id}", scope: scope, user: username()) + render json: { + version_id: version_id, + reviewed: lifecycle.versions[version_id]['reviewed'] + } + rescue => e + log_error(e) + render json: { status: 'error', message: e.message }, status: :internal_server_error + end + private # Combined syntax + mnemonic check used to decide whether to mark a save as # validated. Mnemonic check is a hard fail when applicable (script-engine # languages); for ruby/python it's still browser-side, so this method only - # exercises the syntax check for those. + # exercises the syntax check for those. Returns a hash with :passed, + # :errors (flattened lines from both checks for the UI), :syntax, :mnemonics. def inline_validate(name, text) - return { passed: false, syntax: nil, mnemonics: nil } if text.nil? + return { passed: false, errors: ['no text'], syntax: nil, mnemonics: nil } if text.nil? syntax = Script.syntax(name, text) syntax_passed = syntax && syntax['title'].to_s.include?('Successful') @@ -272,8 +471,20 @@ def inline_validate(name, text) end end - { passed: syntax_passed && mnemonics_passed, syntax: syntax, mnemonics: mnemonics } + errors = [] + if syntax && !syntax_passed + desc = syntax['description'] + lines = desc.is_a?(String) ? (JSON.parse(desc) rescue [desc]) : Array(desc) + errors.concat(lines.compact.map(&:to_s)) + end + if mnemonics && !mnemonics_passed + desc = mnemonics['description'] + lines = desc.is_a?(String) ? (JSON.parse(desc) rescue [desc]) : Array(desc) + errors.concat(lines.compact.map(&:to_s)) + end + + { passed: syntax_passed && mnemonics_passed, errors: errors, syntax: syntax, mnemonics: mnemonics } rescue => e - { passed: false, error: e.message } + { passed: false, errors: [e.message] } end end diff --git a/openc3-cosmos-script-runner-api/config/routes.rb b/openc3-cosmos-script-runner-api/config/routes.rb index 15ed65ba56..a03e4e321f 100644 --- a/openc3-cosmos-script-runner-api/config/routes.rb +++ b/openc3-cosmos-script-runner-api/config/routes.rb @@ -27,6 +27,13 @@ post "/scripts/*name/syntax" => "scripts#syntax" post "/scripts/*name/mnemonics" => "scripts#mnemonics" post "/scripts/*name/instrumented" => "scripts#instrumented" + # Version history / sign-off / restore. The :version_id route uses a + # placeholder since the wildcard *name greedily consumes path segments, + # so we keep the version_id as a query param on the GET body endpoint. + get "/scripts/*name/versions" => "scripts#versions", format: false, defaults: { format: 'html' } + get "/scripts/*name/version" => "scripts#version_body", format: false, defaults: { format: 'html' } + post "/scripts/*name/restore" => "scripts#restore", format: false, defaults: { format: 'html' } + post "/scripts/*name/review" => "scripts#review", format: false, defaults: { format: 'html' } # Must be last so /run, /delete, etc will match first post "/scripts/*name" => "scripts#create", format: false, defaults: { format: 'html' } diff --git a/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb b/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb index d9fe3449da..014cbe82c8 100644 --- a/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb +++ b/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb @@ -186,7 +186,7 @@ json = JSON.parse(response.body) expect(json["version_id"]).to eq("01ABCDEFGHJKMNPQRSTVWXYZ") expect(json["locked_for_review"]).to eq(true) - expect(json["lifecycle"]["reviewed_by"]).to eq("bob") + expect(json["lifecycle"]["reviewed"]["by"]).to eq("bob") end it "processes suite files correctly" do @@ -347,4 +347,164 @@ expect(response).to have_http_status(:unauthorized) end end + + describe "versions" do + let(:name) { "INST/procedures/test.rb" } + let(:scope) { "DEFAULT" } + let(:key) { "DEFAULT/targets_modified/INST/procedures/test.rb" } + + def mock_s3_version(version_id, key:, is_latest: false, size: 100, last_modified: Time.now, etag: '"abc"') + double('s3_version', version_id: version_id, key: key, is_latest: is_latest, + size: size, last_modified: last_modified, etag: etag) + end + + it "returns versions newest-first merged with lifecycle data" do + OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: "V1", username: "alice") + .record_validation(version_id: "V1", passed: true) + .record_save(version_id: "V2", username: "bob") + .record_validation(version_id: "V2", passed: true) + .record_review(version_id: "V2", username: "carol", notes: "lgtm") + + bucket = instance_double(OpenC3::AwsBucket) + allow(OpenC3::Bucket).to receive(:getClient).and_return(bucket) + expect(bucket).to receive(:list_object_versions).and_return({ + versions: [ + mock_s3_version("V2", key: key, is_latest: true), + mock_s3_version("V1", key: key, is_latest: false) + ], + delete_markers: [] + }) + + get :versions, params: {scope: scope, name: name} + expect(response).to have_http_status(:ok) + json = JSON.parse(response.body) + expect(json["versions"].length).to eq(2) + expect(json["versions"][0]["version_id"]).to eq("V2") + expect(json["versions"][0]["state"]).to eq("reviewed") + expect(json["versions"][0]["reviewed"]["by"]).to eq("carol") + expect(json["versions"][1]["state"]).to eq("validated") + expect(json["versions"][1]["validated"]["passed"]).to be(true) + expect(json["latest_version_id"]).to eq("V2") + end + + it "handles authorization failure" do + get :versions, params: {name: name} + expect(response).to have_http_status(:unauthorized) + end + end + + describe "version_body" do + it "returns the body of a specific version" do + bucket = instance_double(OpenC3::AwsBucket) + allow(OpenC3::Bucket).to receive(:getClient).and_return(bucket) + body_io = StringIO.new("puts 'old version'") + resp = double('s3_resp', body: body_io) + expect(bucket).to receive(:get_object).with( + bucket: anything, + key: "DEFAULT/targets_modified/INST/procedures/test.rb", + version_id: "V1" + ).and_return(resp) + + get :version_body, params: {scope: "DEFAULT", name: "INST/procedures/test.rb", version_id: "V1"} + expect(response).to have_http_status(:ok) + expect(response.body).to eq("puts 'old version'") + end + + it "returns 400 when version_id is missing" do + get :version_body, params: {scope: "DEFAULT", name: "INST/procedures/test.rb"} + expect(response).to have_http_status(:bad_request) + end + + it "returns 404 when version is not found" do + bucket = instance_double(OpenC3::AwsBucket) + allow(OpenC3::Bucket).to receive(:getClient).and_return(bucket) + expect(bucket).to receive(:get_object).and_return(nil) + + get :version_body, params: {scope: "DEFAULT", name: "INST/procedures/test.rb", version_id: "BOGUS"} + expect(response).to have_http_status(:not_found) + end + end + + describe "review" do + let(:name) { "INST/procedures/test.rb" } + let(:scope) { "DEFAULT" } + + it "records review when version_id matches latest" do + OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: "V1", username: "alice") + + post :review, params: {scope: scope, name: name, version_id: "V1", notes: "ok"} + expect(response).to have_http_status(:ok) + json = JSON.parse(response.body) + expect(json["reviewed"]["by"]).to eq("anonymous") + expect(json["reviewed"]["notes"]).to eq("ok") + + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + expect(lifecycle.locked_for_review?).to be(true) + end + + it "rejects review when version_id is stale" do + OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: "V1", username: "alice") + .record_save(version_id: "V2", username: "bob") + + post :review, params: {scope: scope, name: name, version_id: "V1", notes: "stale"} + expect(response).to have_http_status(:conflict) + json = JSON.parse(response.body) + expect(json["status"]).to eq("version_mismatch") + expect(json["latest_version_id"]).to eq("V2") + end + + it "rejects review when approver predicate denies" do + OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: "V1", username: "alice") + + allow_any_instance_of(ScriptsController).to receive(:can_approve_script?).and_return(false) + + post :review, params: {scope: scope, name: name, version_id: "V1", notes: "x"} + expect(response).to have_http_status(:forbidden) + end + + it "returns 400 when version_id is missing" do + post :review, params: {scope: scope, name: name} + expect(response).to have_http_status(:bad_request) + end + end + + describe "create with review-lock" do + let(:name) { "INST/procedures/test.rb" } + let(:scope) { "DEFAULT" } + + it "returns 409 when latest version is reviewed and force_taint is not set" do + OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: "V1", username: "alice") + .record_review(version_id: "V1", username: "bob", notes: "lgtm") + + expect(Script).not_to receive(:create) + + post :create, params: {scope: scope, name: name, text: "new"} + expect(response).to have_http_status(:conflict) + json = JSON.parse(response.body) + expect(json["status"]).to eq("locked_for_review") + expect(json["reviewed"]["by"]).to eq("bob") + end + + it "marks the new version tainted when force_taint=true" do + OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: "V1", username: "alice") + .record_review(version_id: "V1", username: "bob", notes: "lgtm") + + expect(Script).to receive(:create).and_return("V2") + + post :create, params: {scope: scope, name: name, text: "new", force_taint: 'true'} + expect(response).to have_http_status(:ok) + + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + expect(lifecycle.latest_version_id).to eq("V2") + expect(lifecycle.versions["V2"]["tainted"]).to be(true) + expect(lifecycle.versions["V2"]["tainted_from_version_id"]).to eq("V1") + expect(lifecycle.versions["V2"]["tainted_from_reviewed_by"]).to eq("bob") + end + end end diff --git a/openc3/lib/openc3/models/script_lifecycle_model.rb b/openc3/lib/openc3/models/script_lifecycle_model.rb index 13b635c1b5..77b9b240dc 100644 --- a/openc3/lib/openc3/models/script_lifecycle_model.rb +++ b/openc3/lib/openc3/models/script_lifecycle_model.rb @@ -14,16 +14,27 @@ require 'openc3/models/model' module OpenC3 - # Tracks per-version lifecycle events for a script: when it was saved (and by - # whom), when its syntax + mnemonic check passed, when it was reviewed/signed - # off, and each time it was executed (with or without targets connected). - # Also records taint provenance — when a reviewed version is overridden, the - # next saved version is marked tainted with a pointer back to the prior - # reviewed version + reviewer. + # Tracks per-version lifecycle facets for a script. The UI surfaces these as + # three badges (validated, reviewed, executed) that start greyed out and + # fill in as each facet acquires data. + # + # Per-version record shape: + # saved_by, saved_at — base metadata, set on every save + # validated: { passed, errors, at, by } | nil + # reviewed: { by, notes, at } | nil + # executions: [ { by, at, disconnect, environment, suite_runner, running_script_id } ] + # tainted, tainted_from_version_id, tainted_from_reviewed_by — provenance + # flags set when a save overrides a previously reviewed version + # restored_from_version_id — set when this version was created via restore + # + # Final execution status (completed / completed_errors / stopped / etc.) is + # NOT denormalized into the lifecycle — the frontend dereferences the linked + # running_script_id against ScriptStatusModel to display live status on the + # executed badge. Avoids cross-process writes from the spawned RunningScript. # # Stored under key "#{scope}__script-lifecycle", one hash field per script - # name. The field value is a JSON document that holds a `versions` map keyed - # by S3 VersionId. + # name. The field value is a JSON document holding a `versions` map keyed by + # S3 VersionId. class ScriptLifecycleModel < Model PRIMARY_KEY = 'script-lifecycle' @@ -48,8 +59,7 @@ def self.from_json(json, scope:) end # Returns the model for this script, building an empty one in memory (not - # yet persisted) if no entry exists. Most transition recording flows want - # this — they don't care whether a row already existed. + # yet persisted) if no entry exists. def self.get_or_build(name:, scope:) existing = get(name: name, scope: scope) return from_json(existing, scope: scope) if existing @@ -79,25 +89,27 @@ def as_json(*_a) } end - # Derived primary state for a single version. A version that has been - # reviewed counts as 'reviewed' even if executions happened later — those - # are recorded as orthogonal events. + # Derived primary state used for the UI badge color. Each badge is + # independently rendered from its own block in the record, but for places + # that need a single string (logs, list views) we fold the most-advanced + # facet into a primary state. def state_of(version_id) rec = @versions[version_id] return 'unknown' unless rec - return 'reviewed' if rec['reviewed_at'] - return 'validated' if rec['validated_at'] - 'new' + return 'executed' if rec['executions']&.any? + return 'reviewed' if rec['reviewed'] + return 'validated' if rec['validated'] + 'unknown' end def latest_state state_of(@latest_version_id) end - # True iff saving over this script should be refused unless the caller - # explicitly opts into tainting. Reviewed = read-only. + # Reviewed = read-only. Saving over the latest version requires force_taint. def locked_for_review? - latest_state == 'reviewed' + rec = latest_record + !!(rec && rec['reviewed']) end def latest_record @@ -115,11 +127,9 @@ def record_save(version_id:, username:, timestamp: nil) self end - # Record a save that overrides a previously reviewed version. The new - # version is flagged tainted with provenance back to the prior reviewer. - # Review of a tainted version cleans it (caller invokes record_review - # later — taint flag is preserved as historical fact, but state derivation - # treats reviewed as authoritative). + # Save that overrides a previously reviewed version. Tainted with provenance. + # Review of a tainted version cleans it (state derives from the reviewed + # block; the tainted flag is preserved as historical fact). def record_taint(version_id:, username:, prev_version_id:, prev_reviewer:, timestamp: nil) timestamp ||= Time.now.utc.iso8601 rec = @versions[version_id] ||= {} @@ -133,12 +143,18 @@ def record_taint(version_id:, username:, prev_version_id:, prev_reviewer:, times self end - # Record that the syntax + mnemonic check passed for this version. Idempotent. - def record_validation(version_id:, username: nil, timestamp: nil) + # Record a syntax + mnemonic check result. Re-validation overwrites the + # prior validated block; the latest result wins. + # @param errors [Array] human-readable error lines (empty when passed=true) + def record_validation(version_id:, passed:, errors: [], username: nil, timestamp: nil) return self unless @versions[version_id] timestamp ||= Time.now.utc.iso8601 - @versions[version_id]['validated_at'] = timestamp - @versions[version_id]['validated_by'] = username if username + @versions[version_id]['validated'] = { + 'passed' => passed, + 'errors' => errors || [], + 'at' => timestamp, + 'by' => username + } persist self end @@ -147,30 +163,35 @@ def record_validation(version_id:, username: nil, timestamp: nil) def record_review(version_id:, username:, notes: nil, timestamp: nil) return self unless @versions[version_id] timestamp ||= Time.now.utc.iso8601 - @versions[version_id]['reviewed_at'] = timestamp - @versions[version_id]['reviewed_by'] = username - @versions[version_id]['reviewed_notes'] = notes if notes + @versions[version_id]['reviewed'] = { + 'by' => username, + 'notes' => notes, + 'at' => timestamp + } persist self end - # Append an execution event. type: 'executed' for connected runs, - # 'executed_disconnect' for disconnect-mode runs. - def record_execution(version_id:, username:, disconnect: false, timestamp: nil) + # Record an execution with the launch settings. Final status is not stored + # here — the UI dereferences running_script_id against ScriptStatusModel + # to show live status on the executed badge. + def record_execution(version_id:, username:, disconnect: false, environment: nil, suite_runner: nil, running_script_id: nil, timestamp: nil) return self unless @versions[version_id] timestamp ||= Time.now.utc.iso8601 @versions[version_id]['executions'] ||= [] @versions[version_id]['executions'] << { - 'type' => disconnect ? 'executed_disconnect' : 'executed', - 'username' => username, - 'at' => timestamp + 'by' => username, + 'at' => timestamp, + 'disconnect' => disconnect, + 'environment' => environment, + 'suite_runner' => suite_runner, + 'running_script_id' => running_script_id } persist self end - # Record a restore operation: a new version_id was created by re-PUTting - # the body of a prior version. The new version becomes latest. + # Restore: a new version_id created by re-PUTting the body of a prior version. def record_restore(version_id:, username:, restored_from_version_id:, timestamp: nil) timestamp ||= Time.now.utc.iso8601 rec = @versions[version_id] ||= {} @@ -182,8 +203,8 @@ def record_restore(version_id:, username:, restored_from_version_id:, timestamp: self end - # Persist the current in-memory state. Upsert semantics — first call is an - # insert, subsequent calls overwrite. + # Persist current state. Upsert semantics — first call inserts, subsequent + # calls overwrite. def persist create(force: true) end diff --git a/openc3/lib/openc3/utilities/authorization.rb b/openc3/lib/openc3/utilities/authorization.rb index 5f4b7c588f..a22433a254 100644 --- a/openc3/lib/openc3/utilities/authorization.rb +++ b/openc3/lib/openc3/utilities/authorization.rb @@ -54,6 +54,14 @@ def authorize(permission: nil, target_name: nil, packet_name: nil, interface_nam def user_info(_token) {} # Enterprise does stuff here end + + # Predicate for who is allowed to sign off (review) on a script. Core + # has no role model, so anyone authenticated can approve. Enterprise + # overrides this in openc3-enterprise/utilities/authorization to check + # for the existing approver role. + def can_approve_script?(scope:, token: nil) + true + end end end end diff --git a/openc3/spec/models/script_lifecycle_model_spec.rb b/openc3/spec/models/script_lifecycle_model_spec.rb index af4ae729e8..4c30d804da 100644 --- a/openc3/spec/models/script_lifecycle_model_spec.rb +++ b/openc3/spec/models/script_lifecycle_model_spec.rb @@ -44,30 +44,41 @@ module OpenC3 end describe 'state derivation' do - it 'returns new for a fresh save' do + it 'returns unknown until validation runs' do model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) .record_save(version_id: v1, username: 'alice') - expect(model.latest_state).to eq('new') + expect(model.latest_state).to eq('unknown') expect(model.locked_for_review?).to be(false) end - it 'returns validated after record_validation' do + it 'returns validated after record_validation regardless of pass/fail' do model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) .record_save(version_id: v1, username: 'alice') - .record_validation(version_id: v1) + .record_validation(version_id: v1, passed: false, errors: ['line 3: syntax error']) expect(model.latest_state).to eq('validated') - expect(model.locked_for_review?).to be(false) + expect(model.versions[v1]['validated']['passed']).to be(false) + expect(model.versions[v1]['validated']['errors']).to eq(['line 3: syntax error']) end it 'returns reviewed after record_review and locks the script' do model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) .record_save(version_id: v1, username: 'alice') - .record_validation(version_id: v1) + .record_validation(version_id: v1, passed: true) .record_review(version_id: v1, username: 'bob', notes: 'looks good') expect(model.latest_state).to eq('reviewed') expect(model.locked_for_review?).to be(true) - expect(model.versions[v1]['reviewed_by']).to eq('bob') - expect(model.versions[v1]['reviewed_notes']).to eq('looks good') + expect(model.versions[v1]['reviewed']['by']).to eq('bob') + expect(model.versions[v1]['reviewed']['notes']).to eq('looks good') + end + + it 'returns executed once at least one execution is recorded' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_validation(version_id: v1, passed: true) + .record_review(version_id: v1, username: 'bob') + .record_execution(version_id: v1, username: 'alice', disconnect: false, running_script_id: 99) + expect(model.latest_state).to eq('executed') + expect(model.locked_for_review?).to be(true) # executing doesn't unlock end end @@ -83,10 +94,10 @@ module OpenC3 it 'preserves prior versions immutably' do model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) .record_save(version_id: v1, username: 'alice') - .record_validation(version_id: v1) + .record_validation(version_id: v1, passed: true) .record_save(version_id: v2, username: 'alice') - expect(model.versions[v1]['validated_at']).not_to be_nil - expect(model.versions[v2]['validated_at']).to be_nil + expect(model.versions[v1]['validated']['passed']).to be(true) + expect(model.versions[v2]['validated']).to be_nil end end @@ -118,12 +129,12 @@ module OpenC3 .record_save(version_id: v1, username: 'alice') .record_review(version_id: v1, username: 'bob') .record_taint(version_id: v2, username: 'alice', prev_version_id: v1, prev_reviewer: 'bob') - .record_validation(version_id: v2) + .record_validation(version_id: v2, passed: true) .record_review(version_id: v2, username: 'bob', notes: 're-reviewed after edit') expect(model.latest_state).to eq('reviewed') expect(model.locked_for_review?).to be(true) expect(model.versions[v2]['tainted']).to be(true) - expect(model.versions[v2]['reviewed_by']).to eq('bob') + expect(model.versions[v2]['reviewed']['by']).to eq('bob') end end @@ -133,21 +144,40 @@ module OpenC3 .record_save(version_id: v1, username: 'alice') .record_review(version_id: v1, username: 'bob', notes: 'first pass') .record_review(version_id: v1, username: 'carol', notes: 'second pass') - expect(model.versions[v1]['reviewed_by']).to eq('carol') - expect(model.versions[v1]['reviewed_notes']).to eq('second pass') + expect(model.versions[v1]['reviewed']['by']).to eq('carol') + expect(model.versions[v1]['reviewed']['notes']).to eq('second pass') + end + end + + describe '#record_validation' do + it 'overwrites the validated block on re-validation (latest result wins)' do + model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) + .record_save(version_id: v1, username: 'alice') + .record_validation(version_id: v1, passed: false, errors: ['oops']) + .record_validation(version_id: v1, passed: true, errors: []) + expect(model.versions[v1]['validated']['passed']).to be(true) + expect(model.versions[v1]['validated']['errors']).to eq([]) end end describe '#record_execution' do - it 'appends connected and disconnect runs as separate events' do + it 'appends executions with launch metadata' do model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) .record_save(version_id: v1, username: 'alice') - .record_execution(version_id: v1, username: 'alice', disconnect: true) - .record_execution(version_id: v1, username: 'alice', disconnect: false) + .record_execution( + version_id: v1, username: 'alice', disconnect: true, + environment: [{ key: 'A', value: '1' }], + suite_runner: { mode: 'all' }, + running_script_id: 42 + ) + .record_execution(version_id: v1, username: 'alice', disconnect: false, running_script_id: 43) execs = model.versions[v1]['executions'] expect(execs.size).to eq(2) - expect(execs[0]['type']).to eq('executed_disconnect') - expect(execs[1]['type']).to eq('executed') + expect(execs[0]['disconnect']).to be(true) + expect(execs[0]['running_script_id']).to eq(42) + expect(execs[0]['environment']).to eq([{ key: 'A', value: '1' }]) + expect(execs[0]['suite_runner']).to eq({ mode: 'all' }) + expect(execs[1]['running_script_id']).to eq(43) end end @@ -165,7 +195,7 @@ module OpenC3 describe 'transitions on missing versions are no-ops' do it 'does not crash when validating a version that was never saved' do model = ScriptLifecycleModel.get_or_build(name: name, scope: scope) - .record_validation(version_id: v1) + .record_validation(version_id: v1, passed: true) expect(model.versions).to eq({}) end end @@ -174,16 +204,17 @@ module OpenC3 it 'preserves all fields through save and reload' do ScriptLifecycleModel.get_or_build(name: name, scope: scope) .record_save(version_id: v1, username: 'alice') - .record_validation(version_id: v1) + .record_validation(version_id: v1, passed: true, errors: []) .record_review(version_id: v1, username: 'bob', notes: 'ok') - .record_execution(version_id: v1, username: 'alice', disconnect: false) + .record_execution(version_id: v1, username: 'alice', running_script_id: 7) .record_taint(version_id: v2, username: 'alice', prev_version_id: v1, prev_reviewer: 'bob') rehydrated = ScriptLifecycleModel.get_or_build(name: name, scope: scope) expect(rehydrated.latest_version_id).to eq(v2) - expect(rehydrated.versions[v1]['validated_at']).not_to be_nil - expect(rehydrated.versions[v1]['reviewed_by']).to eq('bob') + expect(rehydrated.versions[v1]['validated']['passed']).to be(true) + expect(rehydrated.versions[v1]['reviewed']['by']).to eq('bob') expect(rehydrated.versions[v1]['executions'].size).to eq(1) + expect(rehydrated.versions[v1]['executions'][0]['running_script_id']).to eq(7) expect(rehydrated.versions[v2]['tainted']).to be(true) end end From ff897dd05b19f136f9204eb740a22efc81b77815 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 5 May 2026 10:33:10 -0600 Subject: [PATCH 3/6] feat(scriptrunner): lifecycle badges, sign-off, and Validate menu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add three lifecycle badges (validated/reviewed/executed) to the Script Runner toolbar, each greyed out and filling in with status as its facet acquires data. Reviews now carry a decision (approved | changes_requested); only approved decisions lock the script for further saves. The reviewed badge renders bright green on approval, dull green when an earlier version was approved but the current isn't, and red on changes_requested. Executed badge dereferences the latest run's running_script_id against ScriptStatusModel and colors by terminal state (green/yellow/red), with a live tonal-blue while the user is running it in-session. Combine the Script -> Syntax Check and Mnemonic Check menu items into a single Script -> Validate that records the result on the validated block, and surface validation failures from the inline auto-validate-on-save in the same dialog. Replace the now-removed Script.lock UI with a 409 locked_for_review handler that prompts the user to taint, plus a sign-off dialog with separate Approve and Request Changes buttons. Bug fix: save-on-Start triggered the locked_for_review 409 even when the content was unchanged. Frontend now skips non-menu saves when nothing has been modified, and the controller treats identical-body saves as no-ops that bypass the lock check. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../scriptrunner/ScriptLifecycleBadges.vue | 309 +++++++++++ .../src/tools/scriptrunner/ScriptRunner.vue | 484 ++++++++++++++++-- .../app/controllers/scripts_controller.rb | 61 ++- .../config/routes.rb | 1 + .../controllers/scripts_controller_spec.rb | 28 +- .../openc3/models/script_lifecycle_model.rb | 33 +- .../models/script_lifecycle_model_spec.rb | 47 ++ 7 files changed, 891 insertions(+), 72 deletions(-) create mode 100644 openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptLifecycleBadges.vue diff --git a/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptLifecycleBadges.vue b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptLifecycleBadges.vue new file mode 100644 index 0000000000..184e5b9aaf --- /dev/null +++ b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptLifecycleBadges.vue @@ -0,0 +1,309 @@ + + + + + + + diff --git a/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue index a65c3c2617..9cc9edec3f 100644 --- a/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue +++ b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue @@ -165,6 +165,14 @@ />
+ +
+ @@ -645,6 +660,65 @@ /> + + + Review Version + +

+ Approve marks the version reviewed and locks it + against further saves until the lock is explicitly overridden (which + marks the new version tainted). Request Changes + records review feedback without locking — the author can keep editing. +

+

+ Version {{ versionId }} +

+ + + + + + Cancel + + + Request Changes + + + Approve + + + + + + diff --git a/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb b/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb index 8cb740d831..f4f50b6c84 100644 --- a/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb +++ b/openc3-cosmos-script-runner-api/app/controllers/scripts_controller.rb @@ -113,6 +113,20 @@ def create prev_reviewed_version_id = lifecycle.latest_version_id if lifecycle.locked_for_review? prev_reviewer = lifecycle.latest_record&.dig('reviewed', 'by') if lifecycle.locked_for_review? + # First-edit baseline capture: plugins install scripts under + # {scope}/targets/{name} (the read-only baseline prefix) but the + # versioned object lives at {scope}/targets_modified/{name}. Without + # this step the user's first save would become V1 and the + # plugin-installed original would never appear in history. So before + # writing the user's edit, copy targets/ -> targets_modified/ once and + # record it in the lifecycle as the original baseline. + if !text_unchanged && lifecycle.latest_version_id.nil? + capture_original_baseline(scope, name, lifecycle) + # Reload so subsequent record_save / record_taint sees the baseline + # we just inserted as latest. + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + end + write_result = Script.create(args) results = {} if write_result.is_a?(String) @@ -347,6 +361,29 @@ def versions render json: { status: 'error', message: e.message }, status: :internal_server_error end + # GET /scripts/*name/latest — small-payload endpoint used by the editor's + # poll loop. Returns the latest version_id (so the editor can show its + # "newer version exists" banner when someone else saves) plus the latest + # version's lifecycle record. With the lifecycle in the response, the + # reviewed and executed badges update in near-real-time for any user + # watching the script — when user A signs off or runs, user B's badges + # transition on the next 10s poll without a manual reload. + def latest_version + return unless authorization('script_view') + scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true) + return unless scope + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + render json: { + latest_version_id: lifecycle.latest_version_id, + lifecycle: lifecycle.latest_record, + locked_for_review: lifecycle.locked_for_review?, + had_prior_approved_review: lifecycle.had_prior_approved_review? + } + rescue => e + log_error(e) + render json: { status: 'error', message: e.message }, status: :internal_server_error + end + # GET /scripts/*name/version?version_id=... — return body of a specific version. def version_body return unless authorization('script_view') @@ -358,11 +395,22 @@ def version_body return end bucket = OpenC3::Bucket.getClient() - resp = bucket.get_object( - bucket: ENV['OPENC3_CONFIG_BUCKET'], - key: "#{scope}/targets_modified/#{name}", - version_id: version_id - ) + begin + resp = bucket.get_object( + bucket: ENV['OPENC3_CONFIG_BUCKET'], + key: "#{scope}/targets_modified/#{name}", + version_id: version_id + ) + rescue Aws::Errors::ServiceError => e + # Backends (real AWS S3, etc.) reject lookups with versionIds + # that aren't valid for that bucket — for example null-version markers + # returned by list_object_versions on a bucket whose versioning was + # only just enabled. Surface as 404 with the backend's reason rather + # than letting the controller blow up with a 500. + OpenC3::Logger.warn("get_object(version_id=#{version_id}) failed for #{scope}/#{name}: #{e.message}", scope: scope) + render json: { status: 'error', message: "Version unavailable: #{e.message}" }, status: :not_found + return + end if resp && resp.body body = File.extname(name) == '.bin' ? (resp.body.binmode; resp.body.read) : resp.body.read.force_encoding('UTF-8') render plain: body @@ -406,7 +454,13 @@ def restore bucket = OpenC3::Bucket.getClient() bucket_name = ENV['OPENC3_CONFIG_BUCKET'] key = "#{scope}/targets_modified/#{name}" - src = bucket.get_object(bucket: bucket_name, key: key, version_id: version_id) + begin + src = bucket.get_object(bucket: bucket_name, key: key, version_id: version_id) + rescue Aws::Errors::ServiceError => e + OpenC3::Logger.warn("restore get_object(version_id=#{version_id}) failed for #{scope}/#{name}: #{e.message}", scope: scope) + render json: { status: 'error', message: "Version unavailable: #{e.message}" }, status: :not_found + return + end if src.nil? || src.body.nil? head :not_found return @@ -495,6 +549,33 @@ def review private + # Copy {scope}/targets/{name} -> {scope}/targets_modified/{name} and record + # it in the lifecycle as the original/baseline version. No-op when there's + # no targets/ baseline (file was created in Script Runner directly) or when + # targets_modified/ already exists (lifecycle should already be tracking it). + def capture_original_baseline(scope, name, lifecycle) + bucket = OpenC3::Bucket.getClient() + config_bucket = ENV['OPENC3_CONFIG_BUCKET'] + modified_key = "#{scope}/targets_modified/#{name}" + # Already-versioned content (e.g. an earlier plugin install that wrote + # to targets_modified/ directly) shows up in /versions on its own — + # nothing to back-fill here. + return if bucket.get_object(bucket: config_bucket, key: modified_key) + + orig_resp = bucket.get_object(bucket: config_bucket, key: "#{scope}/targets/#{name}") + return unless orig_resp && orig_resp.body + orig_body = orig_resp.body.read + orig_body = orig_body.force_encoding('UTF-8') unless File.extname(name) == '.bin' + + baseline_version_id = OpenC3::TargetFile.create(scope, name, orig_body, username: '') + return unless baseline_version_id.is_a?(String) + lifecycle.record_save(version_id: baseline_version_id, username: '') + rescue => e + # Don't block the user's save just because we couldn't snapshot the + # baseline — log it and let the regular save proceed. + OpenC3::Logger.warn("baseline capture failed for #{scope}/#{name}: #{e.message}", scope: scope) + end + # Combined syntax + mnemonic check used to decide whether to mark a save as # validated. Mnemonic check is a hard fail when applicable (script-engine # languages); for ruby/python it's still browser-side, so this method only diff --git a/openc3-cosmos-script-runner-api/config/routes.rb b/openc3-cosmos-script-runner-api/config/routes.rb index 403df2f10d..2f044e9ce1 100644 --- a/openc3-cosmos-script-runner-api/config/routes.rb +++ b/openc3-cosmos-script-runner-api/config/routes.rb @@ -21,6 +21,14 @@ get "/ping" => "scripts#ping" get "/scripts" => "scripts#index" delete "/scripts/temp_files" => "scripts#delete_temp" + # Specific GET routes must come BEFORE the catchall body route, since + # Rails routes match in declaration order and *name is greedy — without + # this ordering a request to /scripts/foo.rb/versions matches body with + # name="foo.rb/versions". + get "/scripts/*name/versions" => "scripts#versions", format: false, defaults: { format: 'html' } + get "/scripts/*name/version" => "scripts#version_body", format: false, defaults: { format: 'html' } + get "/scripts/*name/latest" => "scripts#latest_version", format: false, defaults: { format: 'html' } + # Catchall body route — must be last among GETs. get "/scripts/*name" => "scripts#body", format: false, defaults: { format: 'html' } post "/scripts/*name/run(/:disconnect)" => "scripts#run", format: false, defaults: { format: 'html' } post "/scripts/*name/delete" => "scripts#destroy", format: false, defaults: { format: 'html' } @@ -28,11 +36,6 @@ post "/scripts/*name/mnemonics" => "scripts#mnemonics" post "/scripts/*name/validate" => "scripts#validate", format: false, defaults: { format: 'html' } post "/scripts/*name/instrumented" => "scripts#instrumented" - # Version history / sign-off / restore. The :version_id route uses a - # placeholder since the wildcard *name greedily consumes path segments, - # so we keep the version_id as a query param on the GET body endpoint. - get "/scripts/*name/versions" => "scripts#versions", format: false, defaults: { format: 'html' } - get "/scripts/*name/version" => "scripts#version_body", format: false, defaults: { format: 'html' } post "/scripts/*name/restore" => "scripts#restore", format: false, defaults: { format: 'html' } post "/scripts/*name/review" => "scripts#review", format: false, defaults: { format: 'html' } # Must be last so /run, /delete, etc will match first diff --git a/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb b/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb index c001caa553..3ad8610295 100644 --- a/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb +++ b/openc3-cosmos-script-runner-api/spec/controllers/scripts_controller_spec.rb @@ -527,4 +527,80 @@ def mock_s3_version(version_id, key:, is_latest: false, size: 100, last_modified expect(lifecycle.versions["V2"]["tainted_from_reviewed_by"]).to eq("bob") end end + + describe "create — first-edit baseline capture" do + let(:name) { "INST/procedures/baseline_test.rb" } + let(:scope) { "DEFAULT" } + let(:original_body) { "puts 'original from plugin'" } + let(:user_edit) { "puts 'user edit'" } + let(:bucket) { instance_double(OpenC3::AwsBucket) } + + it "writes targets/ baseline as the first version before user edit" do + allow(OpenC3::Bucket).to receive(:getClient).and_return(bucket) + # First Script.body call from the controller's text_unchanged check — + # nothing in targets_modified/ yet, so the original is what flows back. + allow(Script).to receive(:body).with(scope, name).and_return(original_body) + # Inside capture_original_baseline: targets_modified/ does not yet exist, + # targets/ does. Return the original body wrapped to look like an S3 resp. + allow(bucket).to receive(:get_object) + .with(bucket: anything, key: "#{scope}/targets_modified/#{name}") + .and_return(nil) + orig_resp = double(body: StringIO.new(original_body)) + allow(bucket).to receive(:get_object) + .with(bucket: anything, key: "#{scope}/targets/#{name}") + .and_return(orig_resp) + # Baseline put_object returns a synthetic version id — TargetFile.create + # uses bucket.put_object directly. Stub both put + check. + baseline_put = double(version_id: "BASELINE") + allow(bucket).to receive(:put_object).and_return(baseline_put) + allow(bucket).to receive(:check_object).and_return(true) + # Then Script.create runs — return the user's version_id. + allow(Script).to receive(:create).and_return("USER_V1") + + post :create, params: {scope: scope, name: name, text: user_edit} + expect(response).to have_http_status(:ok) + + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + expect(lifecycle.versions.keys).to include("BASELINE", "USER_V1") + expect(lifecycle.versions["BASELINE"]["saved_by"]).to eq("") + expect(lifecycle.versions["USER_V1"]["saved_by"]).to eq("anonymous") + expect(lifecycle.latest_version_id).to eq("USER_V1") + end + + it "skips baseline capture when targets_modified/ already exists" do + allow(OpenC3::Bucket).to receive(:getClient).and_return(bucket) + allow(Script).to receive(:body).with(scope, name).and_return("existing modified body") + # targets_modified/ already there — capture_original_baseline early-returns. + modified_resp = double(body: StringIO.new("existing modified body")) + allow(bucket).to receive(:get_object) + .with(bucket: anything, key: "#{scope}/targets_modified/#{name}") + .and_return(modified_resp) + # targets/ get_object should never be called in this branch. + expect(bucket).not_to receive(:get_object) + .with(bucket: anything, key: "#{scope}/targets/#{name}") + allow(Script).to receive(:create).and_return("USER_V1") + + post :create, params: {scope: scope, name: name, text: user_edit} + expect(response).to have_http_status(:ok) + + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + expect(lifecycle.versions.keys).to contain_exactly("USER_V1") + end + + it "skips baseline capture when text is unchanged from original" do + allow(OpenC3::Bucket).to receive(:getClient).and_return(bucket) + allow(Script).to receive(:body).with(scope, name).and_return(original_body) + # Controller still calls Script.create (which internally no-ops on + # identical content), but the baseline-capture branch should be skipped + # so no bucket.get_object call is made and no lifecycle entry is recorded. + expect(bucket).not_to receive(:get_object) + allow(Script).to receive(:create).and_return(nil) + + post :create, params: {scope: scope, name: name, text: original_body} + expect(response).to have_http_status(:ok) + + lifecycle = OpenC3::ScriptLifecycleModel.get_or_build(name: name, scope: scope) + expect(lifecycle.versions).to be_empty + end + end end diff --git a/openc3/lib/openc3/utilities/aws_bucket.rb b/openc3/lib/openc3/utilities/aws_bucket.rb index 12983b37e8..18646c7ee3 100644 --- a/openc3/lib/openc3/utilities/aws_bucket.rb +++ b/openc3/lib/openc3/utilities/aws_bucket.rb @@ -115,6 +115,7 @@ def ensure_scriptrunner_policy(config_bucket, logs_bucket) "Principal": ["#{sr_username}"], "Action": [ "s3:ListBucket", + "s3:ListBucketVersions", "s3:GetBucketLocation" ], "Resource": "#{@aws_arn}:s3:::#{config_bucket}" @@ -123,7 +124,10 @@ def ensure_scriptrunner_policy(config_bucket, logs_bucket) "Sid": "ScriptRunnerReadTargets", "Effect": "Allow", "Principal": ["#{sr_username}"], - "Action": "s3:GetObject", + "Action": [ + "s3:GetObject", + "s3:GetObjectVersion" + ], "Resource": "#{@aws_arn}:s3:::#{config_bucket}/*" }, { From 5f96213f06e00eabdcbce08c8609aed62c445f7e Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Mon, 18 May 2026 15:00:46 -0600 Subject: [PATCH 5/6] Revert Script lifecycle but maintain versioning --- .../app/controllers/info_controller.rb | 7 +- .../admin/tabs/settings/EditorSettings.vue | 21 + .../scriptrunner/ScriptLifecycleBadges.vue | 280 ------ .../src/tools/scriptrunner/ScriptRunner.vue | 905 ++++-------------- .../ScriptVersionHistoryDialog.vue | 358 +++---- .../app/controllers/scripts_controller.rb | 373 +------- .../config/routes.rb | 5 +- .../controllers/scripts_controller_spec.rb | 332 ++----- .../spec/models/script_spec.rb | 30 + .../openc3/models/script_lifecycle_model.rb | 235 ----- openc3/lib/openc3/utilities/authorization.rb | 8 - openc3/lib/openc3/utilities/aws_bucket.rb | 6 + openc3/lib/openc3/utilities/script.rb | 17 + .../models/script_lifecycle_model_spec.rb | 269 ------ playwright/tests/script-runner/api.s.spec.ts | 8 + .../tests/script-runner/prompts.p.spec.ts | 8 + 16 files changed, 583 insertions(+), 2279 deletions(-) delete mode 100644 openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptLifecycleBadges.vue delete mode 100644 openc3/lib/openc3/models/script_lifecycle_model.rb delete mode 100644 openc3/spec/models/script_lifecycle_model_spec.rb diff --git a/openc3-cosmos-cmd-tlm-api/app/controllers/info_controller.rb b/openc3-cosmos-cmd-tlm-api/app/controllers/info_controller.rb index c65bcfd710..57a4824faa 100644 --- a/openc3-cosmos-cmd-tlm-api/app/controllers/info_controller.rb +++ b/openc3-cosmos-cmd-tlm-api/app/controllers/info_controller.rb @@ -17,7 +17,12 @@ rescue LoadError class InfoController < ApplicationController def info - render json: { version: OPENC3_VERSION, license: 'OpenC3', enterprise: false } + render json: { + version: OPENC3_VERSION, + license: 'OpenC3', + enterprise: false, + local_mode: !ENV['OPENC3_LOCAL_MODE'].to_s.empty? + } end end end diff --git a/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/settings/EditorSettings.vue b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/settings/EditorSettings.vue index 3c9c56a8c7..4e8a00cf7b 100644 --- a/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/settings/EditorSettings.vue +++ b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/settings/EditorSettings.vue @@ -39,6 +39,15 @@ data-test="default-language" /> + - - - - - - diff --git a/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue index 1e294889f3..f1b9d98cdf 100644 --- a/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue +++ b/openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/scriptrunner/ScriptRunner.vue @@ -43,6 +43,37 @@ /> + + mdi-pencil-off + {{ lockedBy }} is editing this script. Editor is in read-only mode + +
mdi-connection -
+
- Reload File - Back to New Script + Reload File
- -
- -
- - Locked for review — - {{ lockedReviewerLabel }} - - - - - View History - - - Edit Anyway - -
- - - You're editing past - {{ lockedReviewerLabel }}'s approval. Saving will create a new version marked - tainted. - - -
- - A newer version of this script was saved. Reload to see the - changes - . - - - - Reload - -
- - - -
- - Locked for review — {{ lockedReviewerLabel }} - - - - Edit Anyway - -
- - -
- - A newer version of this script was saved. - - - - - Reload - -
- @@ -731,6 +617,13 @@ :persistent="true" @status="promptDialogCallback" /> + - - - - Review Version - -

- Approve marks the version reviewed and locks it - against further saves until the lock is explicitly overridden (which - marks the new version tainted). Request Changes - records review feedback without locking — the author can keep editing. -

-

- Version {{ versionId }} -

- - - - - - Cancel - - - Request Changes - - - Approve - - - -