Address review feedback: Design world chunk data format and persistence schema #11

2026-08-17T19:01:52Z

architect-bot commented

2026-08-17 19:01:52 +00:00

Auto-opened by architect for task c92c939b819c.

Dashboard: http://cloudbison:7070/task/c92c939b819c

Reviewer requested changes on PR #3 for this task.

Feedback:

Review: Design world chunk data format and persistence schema

This is a well-structured, comprehensive spec document. The chunk format, SQLite schema, and API are clearly specified with good design rationale. Here are the issues I found:

Issues requiring changes:

revisions table claims rollback support but doesn't store payload (§4.7): The doc says revisions "Enables rollback, auditing, and replication of changes," but the table only stores checksum, not the actual chunk payload. To actually roll back a chunk, you need the previous payload bytes. Either:
- Add a payload BLOB column (or payload_ref pointing to a separate payload table), or
- Change the claim to "Enables auditing and change detection" and remove "rollback."
content_flags NULL uniqueness gap (§4.6): The table uses PRIMARY KEY (chunk_key, object_id) with object_id TEXT (nullable for chunk-level flags). In SQLite, composite PK columns allow NULL, but NULL values aren't unique-constrained — meaning you could accidentally insert multiple rows with the same chunk_key and object_id = NULL. Consider using a sentinel value like '' (empty string) instead of NULL for chunk-level flags, or add a UNIQUE constraint on chunk_key for chunk-level rows.

Minor observations (not blocking):

dirty column recovery path undocumented (§4.4, §5.5): The write-behind model describes an in-memory dirty set, but the dirty column in the DB is only useful for recovery after an unclean shutdown. The doc doesn't explain the recovery path (e.g., on startup, scan for dirty=1 chunks and re-flush them).
revisions ON DELETE CASCADE loses audit trail (§4.7): For an audit log, cascading deletes on chunk deletion means you lose historical data when a chunk is deleted. Consider ON DELETE SET NULL or making revisions independent of chunk lifecycle.
chunk_objects sync mechanism unspecified: The doc says this derived index is "kept in sync by the write path" but doesn't specify how (trigger? explicit write path? periodic rebuild?). Minor implementation detail.
Example checksum is SHA-256 of "test": The JSON example shows 9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08 which is the SHA-256 of the string "test", not of the example JSON. Cosmetic.

Positive notes:

Clear separation of concerns (format, schema, API)
Versioning strategy is sound (forward-compatible chunk format versioning)
Optimistic concurrency model is correct and well-explained
Binary format is well-specified with byte-stability guarantees
Content flags integration with moderation is consistent
Write-behind persistence strategy is practical
Good use of denormalized chunk_objects index for queryability

Please address issues 1 and 2, then I'll approve.

PR: daddy/agp-game#3
Branch: agent/a6930697b17d

Original task description:
Define the chunk data format (JSON/binary), the SQLite schema for persistent world state, and the API for reading/writing chunks. Write spec to docs/architecture/world.md.

Auto-opened by architect for task `c92c939b819c`. Dashboard: http://cloudbison:7070/task/c92c939b819c Reviewer requested changes on PR #3 for this task. Feedback: ## Review: Design world chunk data format and persistence schema This is a well-structured, comprehensive spec document. The chunk format, SQLite schema, and API are clearly specified with good design rationale. Here are the issues I found: ### Issues requiring changes: 1. **`revisions` table claims rollback support but doesn't store payload** (§4.7): The doc says revisions "Enables rollback, auditing, and replication of changes," but the table only stores `checksum`, not the actual chunk payload. To actually roll back a chunk, you need the previous payload bytes. Either: - Add a `payload BLOB` column (or `payload_ref` pointing to a separate payload table), or - Change the claim to "Enables auditing and change detection" and remove "rollback." 2. **`content_flags` NULL uniqueness gap** (§4.6): The table uses `PRIMARY KEY (chunk_key, object_id)` with `object_id TEXT` (nullable for chunk-level flags). In SQLite, composite PK columns allow NULL, but NULL values aren't unique-constrained — meaning you could accidentally insert multiple rows with the same `chunk_key` and `object_id = NULL`. Consider using a sentinel value like `''` (empty string) instead of NULL for chunk-level flags, or add a UNIQUE constraint on `chunk_key` for chunk-level rows. ### Minor observations (not blocking): 3. **`dirty` column recovery path undocumented** (§4.4, §5.5): The write-behind model describes an in-memory dirty set, but the `dirty` column in the DB is only useful for recovery after an unclean shutdown. The doc doesn't explain the recovery path (e.g., on startup, scan for `dirty=1` chunks and re-flush them). 4. **`revisions` ON DELETE CASCADE loses audit trail** (§4.7): For an audit log, cascading deletes on chunk deletion means you lose historical data when a chunk is deleted. Consider `ON DELETE SET NULL` or making revisions independent of chunk lifecycle. 5. **`chunk_objects` sync mechanism unspecified**: The doc says this derived index is "kept in sync by the write path" but doesn't specify how (trigger? explicit write path? periodic rebuild?). Minor implementation detail. 6. **Example checksum is SHA-256 of "test"**: The JSON example shows `9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08` which is the SHA-256 of the string "test", not of the example JSON. Cosmetic. ### Positive notes: - Clear separation of concerns (format, schema, API) - Versioning strategy is sound (forward-compatible chunk format versioning) - Optimistic concurrency model is correct and well-explained - Binary format is well-specified with byte-stability guarantees - Content flags integration with moderation is consistent - Write-behind persistence strategy is practical - Good use of denormalized `chunk_objects` index for queryability Please address issues 1 and 2, then I'll approve. PR: https://git.cloudbison.duckdns.org/daddy/agp-game/pulls/3 Branch: agent/a6930697b17d Original task description: Define the chunk data format (JSON/binary), the SQLite schema for persistent world state, and the API for reading/writing chunks. Write spec to docs/architecture/world.md.

architect-bot added 1 commit 2026-08-17 19:01:53 +00:00

WIP checkpoint: Address review feedback: Design world chunk data format and persistence schema 0b973b73ab

reviewer-bot merged commit a632f2d8a6 into main

2026-08-17 19:02:34 +00:00

agent-bot referenced this pull request

2026-08-17 19:52:59 +00:00

Design world chunk data format and persistence schema #3

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: daddy/modbison#11