From c29dd49f9f539f1cae03b3aeb84002ee9663c721 Mon Sep 17 00:00:00 2001
From: Eric Lau <ericglau@outlook.com>
Date: Fri, 24 Jan 2025 15:37:19 -0500
Subject: [PATCH] Fix error conditions when warnings occur in validation output
 (#94)

---
 CHANGELOG.md                   |  4 ++++
 package.json                   |  2 +-
 src/internal/Core.sol          | 30 ++++++++++++++++++++++--------
 test/Upgrades.t.sol            | 13 +++++++++++++
 test/contracts/Validations.sol |  9 +++++++++
 5 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a1295fc..28af34d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Changelog
 
+## 0.3.8 (2025-01-24)
+
+- Fix error conditions when warnings occur in validation output. ([#94](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/pull/94))
+
 ## 0.3.7 (2025-01-13)
 
 - Update documentation links. ([#88](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/pull/88))
diff --git a/package.json b/package.json
index 2544e05..348b437 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@openzeppelin/foundry-upgrades",
-  "version": "0.3.7",
+  "version": "0.3.8",
   "description": "Foundry library for deploying and managing upgradeable contracts",
   "license": "MIT",
   "files": [
diff --git a/src/internal/Core.sol b/src/internal/Core.sol
index 23c8fc9..9ed1bb4 100644
--- a/src/internal/Core.sol
+++ b/src/internal/Core.sol
@@ -345,15 +345,29 @@ library Core {
         string memory stdout = string(result.stdout);
 
         // CLI validate command uses exit code to indicate if the validation passed or failed.
-        // As an extra precaution, we also check stdout for "SUCCESS" to ensure it actually ran.
-        if (result.exitCode == 0 && stdout.toSlice().contains("SUCCESS".toSlice())) {
-            return;
-        } else if (result.stderr.length > 0) {
-            // Validations failed to run
-            revert(string(abi.encodePacked("Failed to run upgrade safety validation: ", string(result.stderr))));
+        if (result.exitCode == 0) {
+            // As an extra precaution, we also check stdout for "SUCCESS" to ensure it actually ran.
+            if (stdout.toSlice().contains("SUCCESS".toSlice())) {
+                if (result.stderr.length > 0) {
+                    // Prints warnings from stderr
+                    console.log(string(result.stderr));
+                }
+                return;
+            } else {
+                revert(string(abi.encodePacked("Failed to run upgrade safety validation: ", stdout)));
+            }
         } else {
-            // Validations ran but some contracts were not upgrade safe
-            revert(string(abi.encodePacked("Upgrade safety validation failed:\n", stdout)));
+            if (stdout.toSlice().contains("FAILED".toSlice())) {
+                if (result.stderr.length > 0) {
+                    // Prints warnings from stderr
+                    console.log(string(result.stderr));
+                }
+                // Validations ran but some contracts were not upgrade safe
+                revert(string(abi.encodePacked("Upgrade safety validation failed:\n", stdout)));
+            } else {
+                // Validations failed to run
+                revert(string(abi.encodePacked("Failed to run upgrade safety validation: ", string(result.stderr))));
+            }
         }
     }
 
diff --git a/test/Upgrades.t.sol b/test/Upgrades.t.sol
index c60f8e6..b018760 100644
--- a/test/Upgrades.t.sol
+++ b/test/Upgrades.t.sol
@@ -333,6 +333,19 @@ contract UpgradesTest is Test {
             opts
         );
     }
+
+    function testWarningAndError() public {
+        Options memory opts;
+        opts.unsafeAllow = "state-variable-immutable";
+
+        Invoker i = new Invoker();
+        try i.validateImplementation("Validations.sol:HasWarningAndError", opts) {
+            fail();
+        } catch Error(string memory reason) {
+            strings.slice memory slice = reason.toSlice();
+            assertTrue(slice.contains("Use of delegatecall is not allowed".toSlice()));
+        }
+    }
 }
 
 contract Invoker {
diff --git a/test/contracts/Validations.sol b/test/contracts/Validations.sol
index 2a2fd76..bcc9970 100644
--- a/test/contracts/Validations.sol
+++ b/test/contracts/Validations.sol
@@ -79,3 +79,12 @@ contract NamespacedV2_UpgradesFrom_Ok {
         uint256 c;
     }
 }
+
+contract HasWarningAndError {
+    uint256 immutable x = 1; // allow `state-variable-immutable` using option to turn into a warning
+
+    function unsafe() public {
+        (bool s, ) = msg.sender.delegatecall("");
+        s;
+    }
+}