throw error on character failing to load

[liamzebedee]

Relates to:

Risks

Background

What does this PR do?

What kind of change is this?

Improvement

Why are we doing this? Any context or related work?

Users pass character paths via command-line. If the character file fails to be found / parsed, Eliza just launches the default character. This is counterintuitive for a newcomer like myself, since the error logged is immediately cleared from the screen when the chat loop is started. More intuitive to just throw directly.

Documentation changes needed?

Testing

Where should a reviewer start?

Detailed testing steps

[snobbee]

@liamzebedee This throws an error when a character cannot be loaded. While this approach makes the failure explicit, it interrupts the entire process, which might not be ideal depending on the use case. Below are some suggestions for improvement:

1. Stop the process instead of throwing the error:

Instead of throwing the error, consider stopping the process gracefully after logging the error. This can be beneficial for the following reasons:

• User Experience: A controlled shutdown with a descriptive message is less abrupt and helps the user understand what went wrong.
• Data Integrity: Stopping the process ensures no partial state is left behind, avoiding any unintended side effects or corrupted data.
• Debugging: Providing a meaningful error message and clean termination can make debugging simpler for developers and users.
• Scalability: In larger systems, propagating the error upwards could lead to cascading failures. Handling it locally and stopping the process reduces this risk.

Here’s an example of how you might refactor this:

export function loadCharacters(charactersArg: string): Character[] {
    const loadedCharacters: Character[] = [];
    // [...]
    for (const path of parsePaths(charactersArg)) {
        try {
            const character = loadCharacterFromPath(path);
            loadedCharacters.push(character);
        } catch (e) {
            console.error(`Error loading character from ${path}: ${e.message}`);
            console.error(`Stopping the process to ensure integrity.`);
            process.exit(1); // Gracefully stop the process
        }
    }
    // [...]
    return loadedCharacters;
}

2. Add Unit Tests for Edge Cases:
It’s important to validate how the function handles edge cases and ensure that it behaves as expected in all scenarios. Suggestions for unit tests:

• Character Load Failure: Test the function with an invalid path to ensure the error is logged and the process stops gracefully.
• Empty Input: Validate that passing an empty charactersArg results in an empty loadedCharacters array.
• Valid and Invalid Mix: Test a scenario where charactersArg contains a mix of valid and invalid paths. Verify that valid characters are loaded successfully before the process stops.
• Large Input Set: Test the function’s behavior when handling a large number of paths to ensure no unexpected errors arise.

Here’s a sample Jest test case outline for handling failures:

test("loadCharacters gracefully stops on error", () => {
    const mockExit = jest.spyOn(process, "exit").mockImplementation(() => {
        throw new Error("process.exit called");
    });

    const mockLog = jest.spyOn(console, "error").mockImplementation(() => {});

    const invalidPath = "invalid/path";
    const validPath = "valid/path";

    jest.mock("./path-parser", () => ({
        parsePaths: jest.fn(() => [validPath, invalidPath]),
    }));

    jest.mock("./character-loader", () => ({
        loadCharacterFromPath: jest
            .fn()
            .mockImplementationOnce(() => ({ name: "Valid Character" }))
            .mockImplementationOnce(() => {
                throw new Error("Invalid path");
            }),
    }));

    expect(() => loadCharacters("input")).toThrow("process.exit called");

    expect(mockLog).toHaveBeenCalledWith(expect.stringContaining("Error loading character"));
    expect(mockExit).toHaveBeenCalledWith(1);

    mockLog.mockRestore();
    mockExit.mockRestore();
});

Let me know if you have any questions! 👍

[jkbrooks]

can we address this merge conflict core/src/cli/index.ts @liamzebedee @snobbee

[odilitime]

Obseleted by #426

[snobbee]

@jkbrooks sounds like the changes in this PR were already addressed by another PR #426, therefore we could close this one