ronantakizawa/codereview-bench huggingface dataset:
Repository: louislam/uptime-kuma
File: server/notification-providers/twilio.js (JavaScript)

Code before:

    /**
     * @inheritdoc
     */
    async send(notification, msg, monitorJSON = null, heartbeatJSON = null) {
        const okMsg = "Sent Successfully.";

        let apiKey = notification.twilioApiKey ? notification.twilioApiKey : notification.twilioAccountSID;
        let twilioMessagingServiceSID = notification.twilioMessagingServiceSID ? notification.twilioMessagingServiceSID : null;

        try {
            let config = {
                headers: {
                    "Content-Type": "application/x-www-form-urlencoded;charset=utf-8",
                    "Authorization": "Basic " + Buffer.from(apiKey + ":" + notification.twilioAuthToken).toString("base64"),
                }
            };
            config = this.getAxiosConfigWithProxy(config);

            let data = new URLSearchParams();
            data.append("To", notification.twilioToNumber);
            data.append("From", notification.twilioFromNumber);
            data.append("Body", msg);

            if (twilioMessagingServiceSID !== null) {
                data.append("MessagingServiceSid", twilioMessagingServiceSID);
            }

            await axios.post(`https://api.twilio.com/2010-04-01/Accounts/${(notification.twilioAccountSID)}/Messages.json`, data, config);

            return okMsg;
        } catch (error) {
            this.throwGeneralAxiosError(error);
        }
    }

}

module.exports = Twilio;


Review comment (suggestion):
```suggestion
            if (notification.twilioMessagingServiceSID) {
                data.append("MessagingServiceSid", notification.twilioMessagingServiceSID);
```

Code after:

    /**
     * @inheritdoc
     */
    async send(notification, msg, monitorJSON = null, heartbeatJSON = null) {
        const okMsg = "Sent Successfully.";

        let apiKey = notification.twilioApiKey ? notification.twilioApiKey : notification.twilioAccountSID;

        try {
            let config = {
                headers: {
                    "Content-Type": "application/x-www-form-urlencoded;charset=utf-8",
                    "Authorization": "Basic " + Buffer.from(apiKey + ":" + notification.twilioAuthToken).toString("base64"),
                }
            };
            config = this.getAxiosConfigWithProxy(config);

            let data = new URLSearchParams();
            data.append("To", notification.twilioToNumber);
            data.append("From", notification.twilioFromNumber);
            data.append("Body", msg);
            if (notification.twilioMessagingServiceSID) {
                data.append("MessagingServiceSid", notification.twilioMessagingServiceSID);
            }

            await axios.post(`https://api.twilio.com/2010-04-01/Accounts/${(notification.twilioAccountSID)}/Messages.json`, data, config);

            return okMsg;
        } catch (error) {
            this.throwGeneralAxiosError(error);
        }
    }

}

module.exports = Twilio;
Repository: louislam/uptime-kuma
File: server/monitor-types/postgres.js (JavaScript)

Code before:
const { MonitorType } = require("./monitor-type");
const { UP } = require("../../src/util");
const dayjs = require("dayjs");
const { postgresQuery } = require("../util-server");

class PostgresMonitorType extends MonitorType {
    name = "postgres";

    /**
     * @inheritdoc
     */
    async check(monitor, heartbeat, _server) {
        let startTime = dayjs().valueOf();

        await postgresQuery(
            monitor.databaseConnectionString,
            monitor.databaseQuery || "SELECT 1"
        );

        heartbeat.msg = "";
        heartbeat.status = UP;
        heartbeat.ping = dayjs().valueOf() - startTime;
    }
}

module.exports = {
    PostgresMonitorType,
};


Review comment (refactor):
Could you inline this function into `PostgresMonitorType`?
I think this was not a good utility to start with and this makes this monitor a lot more readable 😉

Code after:
const { MonitorType } = require("./monitor-type");
const { log, UP } = require("../../src/util");
const dayjs = require("dayjs");
const postgresConParse = require("pg-connection-string").parse;
const { Client } = require("pg");

class PostgresMonitorType extends MonitorType {
    name = "postgres";

    /**
     * @inheritdoc
     */
    async check(monitor, heartbeat, _server) {
        let startTime = dayjs().valueOf();

        let query = monitor.databaseQuery;
        // No query provided by user, use SELECT 1
        if (!query || (typeof query === "string" && query.trim() === "")) {
            query = "SELECT 1";
        }
        await this.postgresQuery(monitor.databaseConnectionString, query);

        heartbeat.msg = "";
        heartbeat.status = UP;
        heartbeat.ping = dayjs().valueOf() - startTime;
    }

    /**
     * Run a query on Postgres
     * @param {string} connectionString The database connection string
     * @param {string} query The query to validate the database with
     * @returns {Promise<(string[] | object[] | object)>} Response from
     * server
     */
    async postgresQuery(connectionString, query) {
        return new Promise((resolve, reject) => {
            const config = postgresConParse(connectionString);

            // Fix #3868, which true/false is not parsed to boolean
            if (typeof config.ssl === "string") {
Repository: louislam/uptime-kuma
File: server/monitor-types/postgres.js (JavaScript)

Code before:
const { MonitorType } = require("./monitor-type");
const { log, UP } = require("../../src/util");
const dayjs = require("dayjs");
const postgresConParse = require("pg-connection-string").parse;
const { Client } = require("pg");

class PostgresMonitorType extends MonitorType {
    name = "postgres";

    /**
     * @inheritdoc
     */
    async check(monitor, heartbeat, _server) {
        let startTime = dayjs().valueOf();

        await postgresQuery(monitor.databaseConnectionString, monitor.databaseQuery || "SELECT 1");

        heartbeat.msg = "";
        heartbeat.status = UP;
        heartbeat.ping = dayjs().valueOf() - startTime;
    }

    /**
     * Run a query on Postgres
     * @param {string} connectionString The database connection string
     * @param {string} query The query to validate the database with
     * @returns {Promise<(string[] | object[] | object)>} Response from
     * server
     */
    async postgresQuery(connectionString, query) {
        return new Promise((resolve, reject) => {
            const config = postgresConParse(connectionString);
    
            // Fix #3868, which true/false is not parsed to boolean
            if (typeof config.ssl === "string") {
                config.ssl = config.ssl === "true";
            }
    
            if (config.password === "") {
                // See https://github.com/brianc/node-postgres/issues/1927
                reject(new Error("Password is undefined."));

Review comment (suggestion):
```suggestion
        await this.postgresQuery(monitor.databaseConnectionString, monitor.databaseQuery || "SELECT 1");
```

Code after:
const { MonitorType } = require("./monitor-type");
const { log, UP } = require("../../src/util");
const dayjs = require("dayjs");
const postgresConParse = require("pg-connection-string").parse;
const { Client } = require("pg");

class PostgresMonitorType extends MonitorType {
    name = "postgres";

    /**
     * @inheritdoc
     */
    async check(monitor, heartbeat, _server) {
        let startTime = dayjs().valueOf();

        let query = monitor.databaseQuery;
        // No query provided by user, use SELECT 1
        if (!query || (typeof query === "string" && query.trim() === "")) {
            query = "SELECT 1";
        }
        await this.postgresQuery(monitor.databaseConnectionString, query);

        heartbeat.msg = "";
        heartbeat.status = UP;
        heartbeat.ping = dayjs().valueOf() - startTime;
    }

    /**
     * Run a query on Postgres
     * @param {string} connectionString The database connection string
     * @param {string} query The query to validate the database with
     * @returns {Promise<(string[] | object[] | object)>} Response from
     * server
     */
    async postgresQuery(connectionString, query) {
        return new Promise((resolve, reject) => {
            const config = postgresConParse(connectionString);

            // Fix #3868, which true/false is not parsed to boolean
            if (typeof config.ssl === "string") {
                config.ssl = config.ssl === "true";
Repository: louislam/uptime-kuma
File: test/backend-test/test-system-service.js (JavaScript)

Code before:
const { describe, it, beforeEach } = require("node:test");
const assert = require("node:assert");
const { SystemServiceMonitorType } = require("../../server/monitor-types/system-service");
const { DOWN } = require("../../src/util");

describe("SystemServiceMonitorType", () => {
    let monitorType;
    let heartbeat;

    beforeEach(() => {
        monitorType = new SystemServiceMonitorType();
        heartbeat = {
            status: DOWN,
            msg: "",
        };
    });

    it("should handle non-existent service gracefully", async (t) => {
        // Skip this test on macOS/Docker runners where systemctl doesn't exist
        if (process.platform !== "linux" && process.platform !== "win32") {
            t.skip("Skipping integration test on unsupported platform");
            return;
        }

        const monitor = { system_service_name: "non-existent-service-12345" };

        try {
            await monitorType.check(monitor, heartbeat);
        } catch (e) {
            // Expected failure for non-existent service
        }

Review comment (bug):
> Tests: I replaced the mock-based unit test with an integration test. It now tests the public check() API directly by attempting to query a non-existent service. This verifies that the actual command execution pipeline (systemctl/powershell) and error handling are working correctly on the host OS, as requested.

Please explain.

As written, this test does not verify that the functionality works correctly. It only verifies the failure case—i.e. that check() reports an error when it is expected to fail.

While validating the error path is useful, it does not demonstrate that the implementation succeeds when given valid input.

Please rework the test cases to also cover a positive scenario, so that we verify both of the below, on each windows/linux:

- when it should fail, it fails, and
- when it should work, it works

This likely requires a short CI modifiction, that is fine.

Code after:
const { describe, test, beforeEach, afterEach } = require("node:test");
const assert = require("node:assert");
const { SystemServiceMonitorType } = require("../../server/monitor-types/system-service");
const { DOWN, UP } = require("../../src/util");
const process = require("process");
const { execSync } = require("node:child_process");

/**
 * Check if the test should be skipped.
 * @returns {boolean} True if the test should be skipped
 */
function shouldSkip() {
    if (process.platform === "win32") {
        return false;
    }
    if (process.platform !== "linux") {
        return true;
    }

    // We currently only support systemd as an init system on linux
    // -> Check if PID 1 is systemd (or init which maps to systemd)
    try {
        const pid1Comm = execSync("ps -p 1 -o comm=", { encoding: "utf-8" }).trim();
        return ![ "systemd", "init" ].includes(pid1Comm);
    } catch (e) {
        return true;
    }
}

describe("SystemServiceMonitorType", { skip: shouldSkip() }, () => {
    let monitorType;
Repository: louislam/uptime-kuma
File: server/monitor-types/system-service.js (JavaScript)

Code before:
                "-NoProfile",
                "-NonInteractive",
                "-Command",
                // Single quotes around the service name
                `(Get-Service -Name '${safeServiceName}').Status`
            ];

            execFile(cmd, args, { timeout: 5000 }, (error, stdout, stderr) => {
                let output = (stderr || stdout || "").toString().trim();
                if (output.length > 200) {
                    output = output.substring(0, 200) + "...";
                }

                if (error || stderr) {
                    heartbeat.msg = output || `Service '${serviceName}' is not running/found.`;
                    reject(new Error(heartbeat.msg));
                    return;
                }

                if (output === "Running") {
                    heartbeat.status = UP;
                    heartbeat.msg = `Service '${serviceName}' is running.`;
                    resolve();
                } else {
                    heartbeat.msg = `Service '${serviceName}' is ${output}.`;
                    reject(new Error(heartbeat.msg));
                }
            });
        });
    }
}

module.exports = {
    SystemServiceMonitorType,
};


Review comment (suggestion):
```suggestion
                    reject(new Error(`Service '${serviceName}' is ${output}.`));
```

Code after:
                "-Command",
                // Single quotes around the service name
                `(Get-Service -Name '${serviceName.replaceAll("'", "''")}').Status`
            ];

            execFile(cmd, args, { timeout: 5000 }, (error, stdout, stderr) => {
                let output = (stderr || stdout || "").toString().trim();
                if (output.length > 200) {
                    output = output.substring(0, 200) + "...";
                }

                if (error || stderr) {
                    reject(new Error(`Service '${serviceName}' is not running/found.`));
                    return;
                }

                if (output === "Running") {
                    heartbeat.status = UP;
                    heartbeat.msg = `Service '${serviceName}' is running.`;
                    resolve();
                } else {
                    reject(new Error(`Service '${serviceName}' is ${output}.`));
                }
            });
        });
    }
}

module.exports = {
    SystemServiceMonitorType,
};
Repository: louislam/uptime-kuma
File: test/backend-test/test-system-service.js (JavaScript)

Code before:
        try {
            await monitorType.check(monitor, heartbeat);
        } catch (e) {
            // Query a non-existent service to force an error/down state.
            // This works correctly on both 'systemctl' and 'Get-Service'.
        }

        assert.strictEqual(heartbeat.status, DOWN);
    });

    it("should fail gracefully with invalid characters", async () => {
        // Mock platform for validation logic test
        Object.defineProperty(process, "platform", {
            value: "linux",
            configurable: true,
        });

        const monitor = {
            system_service_name: "invalid&service;name",
        };

        try {
            await monitorType.check(monitor, heartbeat);
        } catch (e) {
            // Expected validation error
        }

        assert.strictEqual(heartbeat.status, DOWN);
    });

    it("should throw error on unsupported platforms", async () => {
        // This test mocks the platform, so it can run anywhere.
        Object.defineProperty(process, "platform", {
            value: "darwin",
            configurable: true,
        });

        const monitor = {
            system_service_name: "test-service",
        };

        await assert.rejects(async () => await monitorType.check(monitor, heartbeat), /not supported/);
    });
});


Review comment (suggestion):
please use assert.throws instead of this try-catch block

https://nodejs.org/api/assert.html#assertthrowsfn-error-message

Code after:

    test("check() fails gracefully with invalid characters", async () => {
        // Mock platform for validation logic test
        Object.defineProperty(process, "platform", {
            value: "linux",
            configurable: true,
        });

        const monitor = {
            system_service_name: "invalid&service;name",
        };

        // Expected validation error
        await assert.rejects(monitorType.check(monitor, heartbeat));

        assert.strictEqual(heartbeat.status, DOWN);
    });

    test("check() throws on unsupported platforms", async () => {
        // This test mocks the platform, so it can run anywhere.
        Object.defineProperty(process, "platform", {
            value: "darwin",
            configurable: true,
        });

        const monitor = {
            system_service_name: "test-service",
        };

        await assert.rejects(monitorType.check(monitor, heartbeat), /not supported/);
    });
});
Repository: louislam/uptime-kuma
File: test/backend-test/test-system-service.js (JavaScript)

Code before:

        // Query a non-existent service to force an error/down state.
        // We pass the promise directly to assert.rejects, avoiding unnecessary async wrappers.
        await assert.rejects(monitorType.check(monitor, heartbeat));

        assert.strictEqual(heartbeat.status, DOWN);
    });

    it("should fail gracefully with invalid characters", async () => {
        // Mock platform for validation logic test
        Object.defineProperty(process, "platform", {
            value: "linux",
            configurable: true,
        });

        const monitor = {
            system_service_name: "invalid&service;name",
        };

        // Expected validation error
        await assert.rejects(monitorType.check(monitor, heartbeat));

        assert.strictEqual(heartbeat.status, DOWN);
    });

    it("should throw error on unsupported platforms", async () => {
        // This test mocks the platform, so it can run anywhere.
        Object.defineProperty(process, "platform", {
            value: "darwin",
            configurable: true,
        });

        const monitor = {
            system_service_name: "test-service",
        };

        await assert.rejects(monitorType.check(monitor, heartbeat), /not supported/);
    });
});


Review comment (suggestion):
```suggestion
    test("check() throws on unsupported platforms", async () => {
```

Code after:
        // Query a non-existent service to force an error/down state.
        // We pass the promise directly to assert.rejects, avoiding unnecessary async wrappers.
        await assert.rejects(monitorType.check(monitor, heartbeat));

        assert.strictEqual(heartbeat.status, DOWN);
    });

    test("check() fails gracefully with invalid characters", async () => {
        // Mock platform for validation logic test
        Object.defineProperty(process, "platform", {
            value: "linux",
            configurable: true,
        });

        const monitor = {
            system_service_name: "invalid&service;name",
        };

        // Expected validation error
        await assert.rejects(monitorType.check(monitor, heartbeat));

        assert.strictEqual(heartbeat.status, DOWN);
    });

    test("check() throws on unsupported platforms", async () => {
        // This test mocks the platform, so it can run anywhere.
        Object.defineProperty(process, "platform", {
            value: "darwin",
            configurable: true,
        });

        const monitor = {
            system_service_name: "test-service",
        };

        await assert.rejects(monitorType.check(monitor, heartbeat), /not supported/);
    });
});
Repository: louislam/uptime-kuma
File: test/backend-test/test-system-service.js (JavaScript)

Code before:
const { describe, it, beforeEach, afterEach } = require("node:test");
const assert = require("node:assert");
const { SystemServiceMonitorType } = require("../../server/monitor-types/system-service");
const { DOWN, UP } = require("../../src/util");
const process = require("process");
const { execSync } = require("node:child_process");

let isSystemd = false;
let isWindows = process.platform === "win32";

if (process.platform === "linux") {
    try {
        // Check if PID 1 is systemd (or init which maps to systemd)
        const pid1Comm = execSync("ps -p 1 -o comm=", { encoding: "utf-8" }).trim();
        if (pid1Comm === "systemd" || pid1Comm === "init") {
            isSystemd = true;
        }
    } catch (e) {
        // Command failed, likely not systemd
    }
}

const shouldRun = isWindows || isSystemd;

// With Linux and no Systemd (ARM64), the test is skipped.
if (process.platform === "linux" && !isSystemd) {

Review comment (suggestion):
```suggestion
const { describe, test, beforeEach, afterEach } = require("node:test");
```

Code after:
const { describe, test, beforeEach, afterEach } = require("node:test");
const assert = require("node:assert");
const { SystemServiceMonitorType } = require("../../server/monitor-types/system-service");
const { DOWN, UP } = require("../../src/util");
const process = require("process");
const { execSync } = require("node:child_process");

/**
 * Check if the test should be skipped.
 * @returns {boolean} True if the test should be skipped
 */
function shouldSkip() {
    if (process.platform === "win32") {
        return false;
    }
    if (process.platform !== "linux") {
        return true;
    }

    // We currently only support systemd as an init system on linux
    // -> Check if PID 1 is systemd (or init which maps to systemd)
    try {
        const pid1Comm = execSync("ps -p 1 -o comm=", { encoding: "utf-8" }).trim();
        return ![ "systemd", "init" ].includes(pid1Comm);
    } catch (e) {
        return true;
Repository: louislam/uptime-kuma
File: test/backend-test/notification-providers/test-ntlm.js (JavaScript)

Code before:
const { describe, test } = require("node:test");
const assert = require("node:assert");

const hash = require("../../server/modules/axios-ntlm/lib/hash");

describe("createPseudoRandomValue()", () => {
    test("returns a hexadecimal string with the requested length", () => {
        for (const length of [0, 8, 16, 32, 64]) {
            const result = hash.createPseudoRandomValue(length);
            assert.strictEqual(typeof result, "string");
            assert.strictEqual(result.length, length);
            assert.ok(/^[0-9a-f]*$/.test(result));
        }
    });

    test("returns unique values across multiple calls with the same length", () => {
        const length = 16;
        const iterations = 10;
        const results = new Set();

        for (let i = 0; i < iterations; i++) {
            results.add(hash.createPseudoRandomValue(length));
        }

        assert.strictEqual(results.size, iterations);
    });
});


Review comment (suggestion):
```suggestion
        for (const length of [ 0, 8, 16, 32, 64 ]) {
```

Code after:
const { describe, test } = require("node:test");
const assert = require("node:assert");

const hash = require("../../../server/modules/axios-ntlm/lib/hash");

describe("createPseudoRandomValue()", () => {
    test("returns a hexadecimal string with the requested length", () => {
        for (const length of [ 0, 8, 16, 32, 64 ]) {
            const result = hash.createPseudoRandomValue(length);
            assert.strictEqual(typeof result, "string");
            assert.strictEqual(result.length, length);
            assert.ok(/^[0-9a-f]*$/.test(result));
        }
    });

    test("returns unique values across multiple calls with the same length", () => {
        const length = 16;
        const iterations = 10;
        const results = new Set();

        for (let i = 0; i < iterations; i++) {
            results.add(hash.createPseudoRandomValue(length));
        }

        assert.strictEqual(results.size, iterations);
    });
});