refactor: apply PR feedback for rollback feature

__tests__/index.test.js

@@ -20,6 +20,23 @@ vi.mock("../monitor.js", () => ({
   }),
 }));
 
+// Mock screeps-api
+vi.mock("screeps-api", () => {
+  const mockApi = {
+    auth: vi.fn().mockResolvedValue(),
+    code: {
+      get: vi.fn().mockResolvedValue({ ok: 1, modules: { main: "old_code" } }),
+      set: vi.fn().mockResolvedValue({ ok: 1 }),
+    },
+  };
+  // Use a regular function so it can be called with `new`
+  return {
+    ScreepsAPI: vi.fn(function () {
+      return mockApi;
+    }),
+  };
+});
+
 import * as core from "@actions/core";
 import { monitorConsole } from "../monitor.js";
 
@@ -29,7 +46,9 @@ import {
   readReplaceAndWriteFiles,
   readFilesIntoDict,
   applyOnAction,
+  postCode,
 } from "../index.js";
+import { ScreepsAPI } from "screeps-api";
 import fs from "fs";
 import path from "path";
 import os from "os";
@@ -231,31 +250,31 @@ describe("glob functionality", () => {
 describe("applyOnAction", () => {
   beforeEach(() => vi.clearAllMocks());
 
-  it("'ignore' + true → no core call", () => {
-    applyOnAction("ignore", true, "msg");
+  it("'ignore' + true → no core call, returns false", () => {
+    expect(applyOnAction("ignore", true, "msg")).toBe(false);
     expect(core.warning).not.toHaveBeenCalled();
     expect(core.setFailed).not.toHaveBeenCalled();
   });
 
-  it("'warn' + true → core.warning() called with message", () => {
-    applyOnAction("warn", true, "boom");
+  it("'warn' + true → core.warning() called with message, returns false", () => {
+    expect(applyOnAction("warn", true, "boom")).toBe(false);
     expect(core.warning).toHaveBeenCalledWith("boom");
     expect(core.setFailed).not.toHaveBeenCalled();
   });
 
-  it("'fail' + true → core.setFailed() called with message", () => {
-    applyOnAction("fail", true, "boom");
+  it("'fail' + true → core.setFailed() called with message, returns true", () => {
+    expect(applyOnAction("fail", true, "boom")).toBe(true);
     expect(core.setFailed).toHaveBeenCalledWith("boom");
     expect(core.warning).not.toHaveBeenCalled();
   });
 
-  it("'fail' + false → no core call", () => {
-    applyOnAction("fail", false, "boom");
+  it("'fail' + false → no core call, returns false", () => {
+    expect(applyOnAction("fail", false, "boom")).toBe(false);
     expect(core.setFailed).not.toHaveBeenCalled();
   });
 
-  it("'warn' + false → no core call", () => {
-    applyOnAction("warn", false, "msg");
+  it("'warn' + false → no core call, returns false", () => {
+    expect(applyOnAction("warn", false, "msg")).toBe(false);
     expect(core.warning).not.toHaveBeenCalled();
   });
 });
@@ -270,28 +289,60 @@ describe("postCode — monitor wiring", () => {
     // Default core mocks
     core.getInput.mockImplementation((name) => {
       if (name === "monitor") return "0";
+      if (name === "token") return "test-token";
+      if (name === "branch") return "default";
+      if (name === "on_traceback") return "fail";
       return "";
     });
-    core.getBooleanInput.mockReturnValue(false);
+    core.getBooleanInput.mockImplementation((name) => {
+      if (name === "rollback_on_failure") return false;
+      return false;
+    });
   });
 
   it("does not call monitorConsole when monitor=0 (default)", async () => {
-    // We need to mock the rest of postCode to not fail before it hits the monitor block
-    // This is a bit complex as postCode is large, but we can mock the inputs to exit early or mock the API
-    // Actually, I'll just check if monitorConsole is called.
+    // We just run postCode with monitor=0 and verify monitorConsole is not called.
+    await postCode();
+    expect(monitorConsole).not.toHaveBeenCalled();
+  });
 
-    // For this test, I'll make validateAuthentication fail so it returns early but after input check
+  it("rolls back to previous code when monitor detects a failure and rollback_on_failure is true", async () => {
+    // Setup inputs for monitor and rollback
     core.getInput.mockImplementation((name) => {
-      if (name === "monitor") return "0";
+      if (name === "monitor") return "10";
+      if (name === "token") return "test-token";
+      if (name === "branch") return "default";
+      if (name === "on_traceback") return "fail";
       return "";
     });
+    core.getBooleanInput.mockImplementation((name) => {
+      if (name === "rollback_on_failure") return true;
+      return false;
+    });
 
-    // We'll just run a partial check or rely on the monitor unit tests for depth
-    // The wiring in index.js is:
-    // const monitorTicks = parseInt(core.getInput("monitor") || "0", 10);
-    // if (monitorTicks > 0) { ... }
+    // Simulate a failure in monitorConsole
+    monitorConsole.mockResolvedValueOnce({
+      sawTraceback: true, // Should trigger "fail" due to on_traceback=fail
+      sawErrorLog: false,
+      sawWarningLog: false,
+    });
 
-    // Testing the logic inside index.js directly by calling postCode would require full environment mock.
-    // I'll stick to the applyOnAction unit tests and rely on monitor.test.js for the heavy lifting.
+    await postCode();
+
+    // Verify rollback was performed
+    const mockApiInstance = new ScreepsAPI();
+
+    // `code.set` should be called twice:
+    // 1st time: uploading the new files
+    // 2nd time: rolling back to oldCode
+    expect(mockApiInstance.code.set).toHaveBeenCalledTimes(2);
+    expect(mockApiInstance.code.set).toHaveBeenNthCalledWith(2, "default", {
+      main: "old_code",
+    });
+
+    // Verify it called core.setFailed due to traceback
+    expect(core.setFailed).toHaveBeenCalledWith(
+      "Screeps console: traceback detected",
+    );
   });
 });

index.js

@@ -118,17 +118,19 @@ export function validateAuthentication(token, username, password) {
  * @param {'ignore'|'warn'|'fail'} action
  * @param {boolean} flag    - Only acts when true
  * @param {string}  message - Passed to core.warning / core.setFailed
+ * @returns {boolean} - Returns true if the action was 'fail' and the flag was true.
  */
 export function applyOnAction(action, flag, message) {
-  if (!flag) return;
+  if (!flag) return false;
   if (action === "warn") {
     core.warning(message);
-    return;
+    return false;
   }
   if (action === "fail") {
     core.setFailed(message);
+    return true;
   }
-  // 'ignore' → no-op
+  return false;
 }
 
 /**
@@ -210,14 +212,16 @@ export async function postCode() {
           `Successfully downloaded existing code (modules: ${Object.keys(oldCode).join(", ")})`,
         );
       } else {
-        core.warning(
-          `Failed to download existing code, rollback will not be possible.`,
+        core.setFailed(
+          `Failed to download existing code, but rollback_on_failure is enabled. Aborting deployment.`,
         );
+        return;
       }
     } catch (err) {
-      core.warning(
-        `Error downloading existing code: ${err.message}. Rollback will not be possible.`,
+      core.setFailed(
+        `Error downloading existing code: ${err.message}. Aborting deployment.`,
       );
+      return;
     }
   }
 
@@ -248,13 +252,23 @@ export async function postCode() {
     core.setOutput("saw_error_log", String(result.sawErrorLog));
     core.setOutput("saw_warning_log", String(result.sawWarningLog));
 
-    let shouldFail = false;
-    if (core.getInput("on_traceback") === "fail" && result.sawTraceback)
-      shouldFail = true;
-    if (core.getInput("on_error_log") === "fail" && result.sawErrorLog)
-      shouldFail = true;
-    if (core.getInput("on_warning_log") === "fail" && result.sawWarningLog)
-      shouldFail = true;
+    const fail1 = applyOnAction(
+      core.getInput("on_traceback") || "fail",
+      result.sawTraceback,
+      "Screeps console: traceback detected",
+    );
+    const fail2 = applyOnAction(
+      core.getInput("on_error_log") || "warn",
+      result.sawErrorLog,
+      "Screeps console: error log output detected",
+    );
+    const fail3 = applyOnAction(
+      core.getInput("on_warning_log") || "ignore",
+      result.sawWarningLog,
+      "Screeps console: warning log output detected",
+    );
+
+    const shouldFail = fail1 || fail2 || fail3;
 
     if (shouldFail && rollbackOnFailure && oldCode) {
       core.info(
@@ -269,22 +283,6 @@ export async function postCode() {
         core.error(`Rollback failed: ${err}`);
       }
     }
-
-    applyOnAction(
-      core.getInput("on_traceback"),
-      result.sawTraceback,
-      "Screeps console: traceback detected",
-    );
-    applyOnAction(
-      core.getInput("on_error_log"),
-      result.sawErrorLog,
-      "Screeps console: error log output detected",
-    );
-    applyOnAction(
-      core.getInput("on_warning_log"),
-      result.sawWarningLog,
-      "Screeps console: warning log output detected",
-    );
   }
 }