Skip to content

feat(spike): implement save/load hook feasibility#3

Merged
SolshineCode merged 2 commits into
mainfrom
spike/save-load-hooks
May 7, 2026
Merged

feat(spike): implement save/load hook feasibility#3
SolshineCode merged 2 commits into
mainfrom
spike/save-load-hooks

Conversation

@SolshineCode
Copy link
Copy Markdown
Owner

Implements save/load hook feasibility spike with complete test suite.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the Product Requirements Document and initial feasibility spikes for Claude Kingdoms, featuring Python scripts for save injection and a Java-based observability manager. The review identified critical bugs, including malformed XML in the Maven configuration, an incorrectly formatted ZIP save file, and a character-vs-byte length error in the HTTP server implementation. Further improvements were suggested regarding file encoding consistency, the removal of hardcoded absolute paths for better portability, and alignment with Java naming conventions for boolean getters.

Comment thread saves/test_save.zip
Comment on lines +1 to +39
{
"version": 1,
"name": "Spike Test Save",
"date": "400-01-01",
"player": 0,
"civilizations": [
{
"id": 0,
"name": "Test Civ",
"player": true,
"cities": []
}
],
"cities": [
{
"id": 0,
"name": "Capital",
"civilization": 0,
"x": 0,
"y": 0,
"resources": {
"food": 100,
"production": 150,
"science": 200,
"gold": 50,
"culture": 25,
"faith": 10
}
}
],
"units": [],
"map": {
"width": 1,
"height": 1,
"tiles": [
{"x": 0, "y": 0, "type": "plains", "resources": []}
]
}
} No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The file test_save.zip contains raw JSON text. According to the PRD and Unciv's format, a save file should be a ZIP archive containing a file named savegame. This file will likely fail to load in the game engine.

<groupId>com.claudekingdoms</groupId>
<artifactId>observability-spike</artifactId>
<version>1.0.0-SNAPSHOT</version>
<name>Observability Hook Feasibility Spike</modelVersion>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Typo in the closing tag for <name>. It should be </name> instead of </modelVersion>.

    <name>Observability Hook Feasibility Spike</name>

<version>1.0.0-SNAPSHOT</version>
<name>Observability Hook Feasibility Spike</modelVersion>

<parameter=
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Invalid XML syntax <parameter=. This should likely be the opening tag for <properties>. Also, the file appears to be missing the closing </properties> and </project> tags.

    <properties>


def main():
# Create a log directory if it doesn't exist
log_dir = Path("/mnt/c/Users/caleb/projects/ClaudeKingdoms/spikes/hook_logs")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The log directory path is hardcoded to a specific absolute path on your local machine. This makes the script non-portable. Consider using a relative path or making it configurable via an environment variable.

Suggested change
log_dir = Path("/mnt/c/Users/caleb/projects/ClaudeKingdoms/spikes/hook_logs")
log_dir = Path(__file__).parent.parent / "spikes" / "hook_logs"

Comment on lines +116 to +121
private void sendResponse(HttpExchange exchange, int statusCode, String response) throws IOException {
exchange.sendResponseHeaders(statusCode, response.length());
try (OutputStream os = exchange.getResponseBody()) {
os.write(response.getBytes());
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

response.length() returns the number of characters, but sendResponseHeaders requires the length in bytes. This can cause issues with multi-byte characters (e.g., UTF-8). It's safer to convert the string to bytes first and use the byte array's length.

    private void sendResponse(HttpExchange exchange, int statusCode, String response) throws IOException {
        byte[] responseBytes = response.getBytes(java.nio.charset.StandardCharsets.UTF_8);
        exchange.sendResponseHeaders(statusCode, responseBytes.length);
        try (OutputStream os = exchange.getResponseBody()) {
            os.write(responseBytes);
        }
    }

Comment on lines +63 to +65
try (FileWriter writer = new FileWriter(saveFile)) {
writer.write(json);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

FileWriter uses the default system encoding, which can lead to issues when moving save files between different operating systems. It is better to explicitly specify StandardCharsets.UTF_8.

Suggested change
try (FileWriter writer = new FileWriter(saveFile)) {
writer.write(json);
}
try (java.io.Writer writer = new java.io.OutputStreamWriter(new java.io.FileOutputStream(saveFile), java.nio.charset.StandardCharsets.UTF_8)) {
writer.write(json);
}

*/
public GameState loadGame(String saveName) throws IOException {
File saveFile = new File(savesDirectory, saveName + ".json");
String json = new String(Files.readAllBytes(Paths.get(saveFile.getAbsolutePath())));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When reading the save file, the default encoding is used. To ensure consistency with the writer, explicitly use StandardCharsets.UTF_8.

Suggested change
String json = new String(Files.readAllBytes(Paths.get(saveFile.getAbsolutePath())));
String json = new String(Files.readAllBytes(Paths.get(saveFile.getAbsolutePath())), java.nio.charset.StandardCharsets.UTF_8);

public int getMoves() { return moves; }
public void setMoves(int moves) { this.moves = moves; }

public boolean isHasActed() { return hasActed; }
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The getter name isHasActed is redundant and doesn't follow standard Java naming conventions for booleans. hasActed() or isActed() would be more idiomatic.

Suggested change
public boolean isHasActed() { return hasActed; }
public boolean hasActed() { return hasActed; }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant