fix(plugins): protect modified files on upgrade and surface diff failures

openc3-cosmos-cmd-tlm-api/spec/controllers/plugins_controller_spec.rb

@@ -98,6 +98,15 @@
       expect(json["files"]).to eq(["INST/screen.txt"])
     end
 
+    it "returns internal_server_error when the diff fails" do
+      allow(OpenC3::PluginModel).to receive(:modified_diff).and_raise("boom")
+
+      post :modified_diff, params: {scope: "DEFAULT", plugin_hash: '{"name":"x"}'}
+      expect(response).to have_http_status(:internal_server_error)
+      json = JSON.parse(response.body)
+      expect(json["message"]).to eq("boom")
+    end
+
     it "returns bad_request on invalid plugin_hash JSON" do
       post :modified_diff, params: {scope: "DEFAULT", plugin_hash: "not json"}
       expect(response).to have_http_status(:bad_request)

openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/ModifiedPluginDialog.vue

@@ -20,13 +20,18 @@
         <v-spacer />
       </v-toolbar>
       <v-card-text class="pa-3 card-container">
-        <!-- Deleting the plugin: offer to delete the orphaned modified files
-             (unchanged, destructive behavior). -->
-        <template v-if="pluginDelete">
+        <!-- Deleting the plugin, or upgrading without Version History (Core,
+             or Enterprise with it disabled): offer to delete the modified
+             files (destructive). Kept modified files override the plugin. -->
+        <template v-if="deleteFlow">
           <div>
             Plugin {{ plugin }} has modified files. Would you like to delete the
             existing modified files?
           </div>
+          <div v-if="!pluginDelete" class="mt-2">
+            Modified files that are not deleted continue to override the new
+            plugin's versions of those files.
+          </div>
           <v-list-item
             v-for="(target, index) in modifiedTargets"
             :key="index"
@@ -49,14 +54,19 @@
           />
         </template>
 
-        <!-- Upgrading: warn (no options) about modified files that actually
-             differ from the new plugin. The plugin's version is taken and the
-             prior content saved to Version History. -->
+        <!-- Upgrading with Version History: warn (no options) about modified
+             files that actually differ from the new plugin. The plugin's
+             version is taken and the prior content saved to Version History. -->
         <template v-else>
           <div v-if="loading" class="d-flex align-center py-2">
             <v-progress-circular indeterminate size="20" class="mr-3" />
             Checking which modified files differ from the new plugin...
           </div>
+          <div v-else-if="diffError" data-test="modified-plugin-diff-error">
+            Unable to check which of {{ plugin }}'s modified files differ from
+            the new plugin: {{ diffError }}. If you continue, all modified files
+            are kept and continue to override the new plugin's versions.
+          </div>
           <div v-else-if="diffFiles.length === 0">
             None of {{ plugin }}'s modified files conflict with the new plugin.
             Installing will proceed normally.
@@ -122,6 +132,9 @@ export default {
       default: null,
     },
     pluginDelete: Boolean,
+    // Whether the Enterprise Version History backend is enabled. Without it
+    // an upgrade can't preserve modified content, so offer deletion instead.
+    versionHistory: Boolean,
   },
   emits: ['update:modelValue', 'submit', 'cancel'],
   data() {
@@ -133,9 +146,13 @@ export default {
       // the incoming plugin.
       loading: false,
       diffFiles: [],
+      diffError: null,
     }
   },
   computed: {
+    deleteFlow() {
+      return this.pluginDelete || !this.versionHistory
+    },
     show: {
       get() {
         return this.modelValue
@@ -146,7 +163,7 @@ export default {
     },
   },
   created() {
-    if (this.pluginDelete) {
+    if (this.deleteFlow) {
       this.loadModifiedFiles()
     } else {
       this.loadDiff()
@@ -181,12 +198,19 @@ export default {
       try {
         const response = await Api.post('/openc3-api/plugins/modified_diff', {
           data: { plugin_hash: JSON.stringify(this.pluginHash) },
+          headers: {
+            Accept: 'application/json',
+            'Content-Type': 'application/json',
+            // Reported inline in the dialog rather than the global banner
+            'Ignore-Errors': '500,502,503,504',
+          },
         })
         this.diffFiles = response.data.files || []
-      } catch {
-        // If the dry run fails, fall back to letting the install proceed with
-        // no version-history capture rather than blocking the upgrade.
+      } catch (error) {
+        // Report the failure rather than claiming no conflicts. The install
+        // can still proceed; with no files listed, every modified file is kept.
         this.diffFiles = []
+        this.diffError = error.response?.data?.message || error.message
       } finally {
         this.loading = false
       }
@@ -198,7 +222,7 @@ export default {
     submit() {
       let installFromPlugin = []
       const deleteFiles = []
-      if (this.pluginDelete) {
+      if (this.deleteFlow) {
         if (this.deleteModified) {
           for (const target of this.modifiedTargets) {
             target.files.forEach((f) => deleteFiles.push(f.fullName))

openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/admin/tabs/PluginsTab.vue

@@ -154,6 +154,7 @@
       :targets="pluginTargets(currentPlugin)"
       :plugin-hash="pluginHashTmp"
       :plugin-delete="pluginDelete"
+      :version-history="scriptVersionsEnabled"
       @submit="modifiedSubmit"
     />
     <!-- <download-dialog v-model="showDownloadDialog" /> -->
@@ -437,11 +438,17 @@ export default {
           byTarget[targetName] ||= []
           byTarget[targetName].push(fullName)
         }
-        for (const [targetName, files] of Object.entries(byTarget)) {
-          await Api.post(`/openc3-api/targets/${targetName}/delete_modified`, {
-            data: { files },
-          })
-        }
+        const deletePromises = Object.entries(byTarget).map(
+          async ([targetName, files]) => {
+            await Api.post(
+              `/openc3-api/targets/${targetName}/delete_modified`,
+              {
+                data: { files },
+              },
+            )
+          },
+        )
+        await Promise.all(deletePromises)
       }
       if (this.pluginDelete) {
         this.deletePlugin(this.currentPlugin)
@@ -528,28 +535,30 @@ export default {
         .catch(console.error)
       this.update()
     },
-    migrateToUv: function (plugin) {
-      this.$dialog
-        .confirm(
+    migrateToUv: async function (plugin) {
+      try {
+        await this.$dialog.confirm(
           `Migrate plugin ${plugin} to a per-plugin UV virtual environment?`,
           {
             okText: 'Migrate',
             cancelText: 'Cancel',
           },
         )
-        .then(() => {
-          Api.post(`/openc3-api/plugins/${plugin}/migrate_to_uv`)
-            .then((response) => {
-              this.alert = `Started migrating plugin ${plugin} to UV ...`
-              this.alertType = 'success'
-              this.showAlert = true
-              setTimeout(() => {
-                this.showAlert = false
-                this.updateProcesses()
-              }, 5000)
-            })
-            .catch(console.error)
-        })
+        try {
+          await Api.post(`/openc3-api/plugins/${plugin}/migrate_to_uv`)
+          this.alert = `Started migrating plugin ${plugin} to UV ...`
+          this.alertType = 'success'
+          this.showAlert = true
+          setTimeout(() => {
+            this.showAlert = false
+            this.updateProcesses()
+          }, 5000)
+        } catch (error) {
+          console.error(error)
+        }
+      } catch {
+        // user cancelled, do nothing
+      }
     },
     upgradePlugin(plugin) {
       this.resetControlState()

openc3/lib/openc3/models/plugin_model.rb

@@ -428,11 +428,9 @@ def self.install_phase2(plugin_hash, scope:, gem_file_path: nil, validate_only:
     # Dry run: which modified files would this plugin's install supersede?
     # Returns a list of "TARGET/path" names whose live (modified) content
     # differs from the rendered plugin content. Read-only; no side effects.
+    # Errors propagate so callers can tell a failed check from an empty diff.
     def self.modified_diff(plugin_hash, scope:)
       install_phase2(plugin_hash, scope: scope, diff_only: true)
-    rescue => e
-      Logger.warn("PluginModel.modified_diff failed: #{e.message}")
-      []
     end
 
     def initialize(

openc3/lib/openc3/models/target_model.rb

@@ -761,11 +761,12 @@ def deploy(gem_path, variables, validate_only: false, upgrade_context: nil)
     # Version History is an Enterprise feature. The plugin deploy runs in the
     # cmd-tlm-api process, which (unlike script-runner-api) doesn't otherwise
     # require the store, so lazily load it on demand. Returns false in Core
-    # builds where the Enterprise gem isn't present.
+    # builds where the Enterprise gem isn't present, and when the store is
+    # disabled (OPENC3_VERSION_HISTORY_DIR unset): its commits no-op there, so
+    # versioning would drop the modified copy without preserving it.
     def version_store_available?
-      return true if defined?(::VersionStore)
-      require 'openc3-enterprise/utilities/version_store'
-      defined?(::VersionStore) ? true : false
+      require 'openc3-enterprise/utilities/version_store' unless defined?(::VersionStore)
+      defined?(::VersionStore) && ::VersionStore.enabled? ? true : false
     rescue LoadError
       false
     end

openc3/spec/models/plugin_model_spec.rb

@@ -381,10 +381,9 @@ module OpenC3
         expect(PluginModel.modified_diff({"name" => "name"}, scope: "DEFAULT")).to eql(["TGT/screen.txt"])
       end
 
-      it "returns an empty array and logs when install_phase2 raises" do
+      it "raises when install_phase2 raises rather than reporting no differences" do
         expect(PluginModel).to receive(:install_phase2).and_raise("boom")
-        expect(Logger).to receive(:warn).with(/modified_diff failed: boom/)
-        expect(PluginModel.modified_diff({"name" => "name"}, scope: "DEFAULT")).to eql([])
+        expect { PluginModel.modified_diff({"name" => "name"}, scope: "DEFAULT") }.to raise_error("boom")
       end
     end
 

openc3/spec/models/target_model_spec.rb

@@ -360,6 +360,24 @@ def shadow_response(body)
         end
       end
 
+      describe "#version_store_available?" do
+        it "is true when the store is present and enabled" do
+          stub_const("VersionStore", double("VersionStore", enabled?: true))
+          expect(model.send(:version_store_available?)).to be true
+        end
+
+        it "is false when the store is present but disabled" do
+          stub_const("VersionStore", double("VersionStore", enabled?: false))
+          expect(model.send(:version_store_available?)).to be false
+        end
+
+        it "is false when the enterprise store gem is absent" do
+          hide_const("VersionStore") if defined?(VersionStore)
+          allow(model).to receive(:require).with("openc3-enterprise/utilities/version_store").and_raise(LoadError)
+          expect(model.send(:version_store_available?)).to be false
+        end
+      end
+
       describe "#apply_upgrade_version" do
         let(:store) { double("VersionStore") }
         before(:each) { stub_const("VersionStore", store) }

playwright/tests/admin/plugins.p.spec.ts

@@ -513,9 +513,14 @@ test.describe(() => {
     await expect(page.locator('.v-dialog:has-text("Variables")')).toBeVisible()
     await page.locator('data-test=edit-submit').click()
     await expect(page.locator('.v-dialog:has-text("Modified")')).toBeVisible()
-    // The modified file is a script, so it gets a per-file choice that
-    // defaults to "Install file from plugin" (recoverable via Version
-    // History). Just confirm to take the plugin's version.
+    // Without Version History (Core, or Enterprise with it disabled) the
+    // dialog offers to delete the modified files; check it to take the
+    // plugin's version. With Version History there is no checkbox and the
+    // plugin's version is taken, with the prior content versioned.
+    const deleteModified = page.locator('text=DELETE MODIFIED')
+    if (await deleteModified.isVisible()) {
+      await deleteModified.click()
+    }
     await page.locator('data-test=modified-plugin-submit').click()
     await expect(page.locator('[data-test=plugin-alert]')).toContainText(
       'Started installing',