feat(nakama): add actor profile registry#12
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Local code-review skill passTarget: PR #12 Engine Specialist Findings: N/A - no engine configured
Testability: TESTABLE
ADR Compliance: NO ADRS FOUNDNo direct ADR reference was found in the implementation commit or file headers. The change stays within ADR 0010 direction: Nakama owns game-backend profile/storage extensions, while LLM provider calls remain outside Nakama. Standards Compliance: PASS
Architecture: CLEAN
SOLID: APPROVED WITH SUGGESTIONSThe Nakama module is getting larger, but this follows the current single-module prototype pattern. Once these RPC surfaces settle, split actor profile helpers into a separate runtime module file if Nakama bundling allows it cleanly. Game-Specific Concerns
Positive Observations
Required ChangesNone from local code-review. Suggestions
Verdict: APPROVED WITH SUGGESTIONSVerification observed:
Codex Cloud review was requested in the PR body, but the connector replied that code review usage limits are reached. Gemini review is still pending. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces actor profile management and memory tracking by adding new RPC endpoints and storage logic. The review feedback highlights critical issues in the data normalization functions, specifically where the use of the logical OR operator (||) incorrectly overwrites valid 0 values for character stats and body timers with default values. Additionally, a suggestion was made to improve the normalization of the cultivation object to ensure all fields are correctly defaulted and clamped.
| function normalizeStats(stats: any): any { | ||
| var defaults = defaultCharacterStats(); | ||
| return { | ||
| level: clampNumber(stats.level || defaults.level, 1, 100), | ||
| vitality: clampNumber(stats.vitality || defaults.vitality, 1, 9999), | ||
| force: clampNumber(stats.force || defaults.force, 1, 9999), | ||
| agility: clampNumber(stats.agility || defaults.agility, 1, 9999), | ||
| focus: clampNumber(stats.focus || defaults.focus, 1, 9999), | ||
| resilience: clampNumber(stats.resilience || defaults.resilience, 1, 9999), | ||
| max_health: clampNumber(stats.max_health || defaults.max_health, 1, 999999), | ||
| max_energy: clampNumber(stats.max_energy || defaults.max_energy, 0, 999999), | ||
| attack_power: clampNumber(stats.attack_power || defaults.attack_power, 0, 999999), | ||
| defense_power: clampNumber(stats.defense_power || defaults.defense_power, 0, 999999) | ||
| }; | ||
| } |
There was a problem hiding this comment.
Using the || operator for defaults in normalizeStats will cause valid 0 values for max_energy, attack_power, and defense_power to be overwritten by their non-zero defaults. A check for undefined or null should be used instead to allow 0 as a valid input.
| function normalizeStats(stats: any): any { | |
| var defaults = defaultCharacterStats(); | |
| return { | |
| level: clampNumber(stats.level || defaults.level, 1, 100), | |
| vitality: clampNumber(stats.vitality || defaults.vitality, 1, 9999), | |
| force: clampNumber(stats.force || defaults.force, 1, 9999), | |
| agility: clampNumber(stats.agility || defaults.agility, 1, 9999), | |
| focus: clampNumber(stats.focus || defaults.focus, 1, 9999), | |
| resilience: clampNumber(stats.resilience || defaults.resilience, 1, 9999), | |
| max_health: clampNumber(stats.max_health || defaults.max_health, 1, 999999), | |
| max_energy: clampNumber(stats.max_energy || defaults.max_energy, 0, 999999), | |
| attack_power: clampNumber(stats.attack_power || defaults.attack_power, 0, 999999), | |
| defense_power: clampNumber(stats.defense_power || defaults.defense_power, 0, 999999) | |
| }; | |
| } | |
| function normalizeStats(stats: any): any { | |
| var defaults = defaultCharacterStats(); | |
| var get = function (v: any, d: any) { return (v !== undefined && v !== null) ? v : d; }; | |
| return { | |
| level: clampNumber(get(stats.level, defaults.level), 1, 100), | |
| vitality: clampNumber(get(stats.vitality, defaults.vitality), 1, 9999), | |
| force: clampNumber(get(stats.force, defaults.force), 1, 9999), | |
| agility: clampNumber(get(stats.agility, defaults.agility), 1, 9999), | |
| focus: clampNumber(get(stats.focus, defaults.focus), 1, 9999), | |
| resilience: clampNumber(get(stats.resilience, defaults.resilience), 1, 9999), | |
| max_health: clampNumber(get(stats.max_health, defaults.max_health), 1, 999999), | |
| max_energy: clampNumber(get(stats.max_energy, defaults.max_energy), 0, 999999), | |
| attack_power: clampNumber(get(stats.attack_power, defaults.attack_power), 0, 999999), | |
| defense_power: clampNumber(get(stats.defense_power, defaults.defense_power), 0, 999999) | |
| }; | |
| } |
| function normalizeBodyTime(time: any): any { | ||
| return { | ||
| remaining_seconds: clampNumber(time.remaining_seconds || 86400, 0, 31536000), | ||
| max_seconds: clampNumber(time.max_seconds || 86400, 1, 31536000), | ||
| danger_drain_rate: clampNumber(time.danger_drain_rate || 1, 0, 1000) | ||
| }; | ||
| } |
There was a problem hiding this comment.
In normalizeBodyTime, using || for remaining_seconds and danger_drain_rate will incorrectly reset them to defaults if they are set to 0. This could lead to critical bugs in gameplay timers where an actor with no time left is suddenly reset to a full day.
| function normalizeBodyTime(time: any): any { | |
| return { | |
| remaining_seconds: clampNumber(time.remaining_seconds || 86400, 0, 31536000), | |
| max_seconds: clampNumber(time.max_seconds || 86400, 1, 31536000), | |
| danger_drain_rate: clampNumber(time.danger_drain_rate || 1, 0, 1000) | |
| }; | |
| } | |
| function normalizeBodyTime(time: any): any { | |
| var get = function (v: any, d: any) { return (v !== undefined && v !== null) ? v : d; }; | |
| return { | |
| remaining_seconds: clampNumber(get(time.remaining_seconds, 86400), 0, 31536000), | |
| max_seconds: clampNumber(get(time.max_seconds, 86400), 1, 31536000), | |
| danger_drain_rate: clampNumber(get(time.danger_drain_rate, 1), 0, 1000) | |
| }; | |
| } |
| profile.body.stats = normalizeStats(profile.body.stats || {}); | ||
| profile.body.characteristics = normalizeTraits(profile.body.characteristics || {}); | ||
| profile.body.time = normalizeBodyTime(profile.body.time || {}); | ||
| profile.body.cultivation = profile.body.cultivation || { tier: "Awakening", progress_xp: 0 }; |
There was a problem hiding this comment.
The cultivation object is not fully normalized. If it exists but is missing fields like tier or progress_xp, they won't be defaulted or clamped. It's better to normalize its properties individually to ensure data integrity.
| profile.body.cultivation = profile.body.cultivation || { tier: "Awakening", progress_xp: 0 }; | |
| profile.body.cultivation = profile.body.cultivation || {}; | |
| profile.body.cultivation.tier = trimString(profile.body.cultivation.tier) || "Awakening"; | |
| profile.body.cultivation.progress_xp = clampNumber(profile.body.cultivation.progress_xp, 0, 1000000000); |
|
Unity client delta added after the initial PR:
Additional verification on latest head
Unity caveat:
Gemini (@gemini-code-assist) please include latest commit 254d3c0 in the pending PR #12 review. |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive system for managing actor profiles and memories, including new DTOs in Unity and RPC endpoints in the Nakama backend. The changes allow for retrieving actor profiles and adding memories, supported by a suite of normalization and storage helper functions. Feedback focuses on ensuring that normalization changes are persisted to storage during profile retrieval, optimizing the creation flow by returning storage acknowledgments to avoid redundant database reads, and implementing length validation for actor IDs to prevent storage key failures.
| var state = getOrCreateActorProfileState(ctx, nk, request); | ||
| return JSON.stringify(state.profile); |
There was a problem hiding this comment.
The rpcActorProfileGet function should persist any normalization changes made by ensureActorProfile. Currently, if ensureActorProfile (called inside getOrCreateActorProfileState) fills in missing defaults or bootstraps the agent_runtime, these changes are returned to the client but never saved to storage. This mirrors the logic in rpcProfileGet (lines 61-63). Consider updating ensureActorProfile to return a boolean indicating if changes were made, and performing a writeActorProfile if so.
| writeActorProfile(nk, profile, ""); | ||
| var created = readActorProfile(nk, ownerId, actorId); | ||
| if (created) { | ||
| return { | ||
| profile: ensureActorProfile(created.value, ownerId, actorId), | ||
| version: created.version | ||
| }; | ||
| } |
There was a problem hiding this comment.
Efficiency: writeActorProfile can return the storage acks from nk.storageWrite. This allows getOrCreateActorProfileState to obtain the new object version without performing an additional readActorProfile call immediately after writing. This reduces database load during actor creation.
var acks = writeActorProfile(nk, profile, "");
return {
profile: profile,
version: acks[0].version
};| }; | ||
| } | ||
|
|
||
| function writeActorProfile(nk: nkruntime.Nakama, profile: any, version: string): void { |
There was a problem hiding this comment.
Update writeActorProfile to return the storage acks to support the efficiency improvement in getOrCreateActorProfileState.
| function writeActorProfile(nk: nkruntime.Nakama, profile: any, version: string): void { | |
| function writeActorProfile(nk: nkruntime.Nakama, profile: any, version: string): nkruntime.StorageAck[] { |
| if (typeof version === "string") { | ||
| write.version = version; | ||
| } | ||
| nk.storageWrite([write]); |
There was a problem hiding this comment.
Return the result of nk.storageWrite to provide the version info to callers.
| nk.storageWrite([write]); | |
| return nk.storageWrite([write]); |
| function normalizeActorId(actorId: any): string { | ||
| var normalized = sanitizeNakamaIdentifier(trimString(actorId), ""); | ||
| if (!normalized) { | ||
| throw new Error("actor_id is required"); | ||
| } | ||
| return normalized; | ||
| } |
There was a problem hiding this comment.
Validation: actor_id should be validated for length. Nakama storage keys have a limit of 128 bytes. Since the key is prefixed with profile: (8 bytes), the actor_id should be restricted to 120 characters to prevent storageWrite failures when using long identifiers.
function normalizeActorId(actorId: any): string {
var normalized = sanitizeNakamaIdentifier(trimString(actorId), "");
if (!normalized) {
throw new Error("actor_id is required");
}
if (normalized.length > 120) {
throw new Error("actor_id is too long (max 120 characters)");
}
return normalized;
}
|
Delta after Gemini review on actor normalization:
Verification on latest head
Unity caveat remains unchanged: clean-worktree Unity batchmode is blocked before script compilation by the existing Package Manager Gemini (@gemini-code-assist) please delta re-review latest commit b894bdd. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Actor Profile system, including Unity DTOs, gateway client methods, and Nakama backend RPCs for retrieving and updating actor profiles and memories. Key additions include ActorProfileDto, ActorProfileRequestDto, and backend logic for managing actor state in Nakama storage. Feedback suggests optimizing the backend implementation by persisting normalized profiles during retrieval to prevent redundant processing and leveraging storageWrite acknowledgments to avoid unnecessary read operations when creating new profiles.
| var state = getOrCreateActorProfileState(ctx, nk, request); | ||
| return JSON.stringify(state.profile); |
There was a problem hiding this comment.
The rpcActorProfileGet function currently returns the profile after normalization, but it does not persist any changes made by ensureActorProfile back to storage. This means that if a profile requires migration or default field population, it will be re-processed on every GET request.
Consider refactoring ensureActorProfile to return a boolean indicating if changes were made, and then conditionally calling writeActorProfile here, similar to the pattern used in rpcProfileGet (lines 61-63).
| writeActorProfile(nk, profile, ""); | ||
| var created = readActorProfile(nk, ownerId, actorId); | ||
| if (created) { | ||
| return { | ||
| profile: ensureActorProfile(created.value, ownerId, actorId), | ||
| version: created.version | ||
| }; |
There was a problem hiding this comment.
This implementation performs a redundant storageRead immediately after a storageWrite to retrieve the object version. nk.storageWrite returns an array of StorageAck objects, each containing the new version of the written object.
You can optimize this by updating writeActorProfile to return the version string from the write acknowledgment, eliminating the need for the second read.
var version = writeActorProfile(nk, profile, "");
return {
profile: ensureActorProfile(profile, ownerId, actorId),
version: version
};| }; | ||
| } | ||
|
|
||
| function writeActorProfile(nk: nkruntime.Nakama, profile: any, version: string): void { |
There was a problem hiding this comment.
Update this function to return the version string from the nk.storageWrite acknowledgment to allow callers to avoid redundant reads.
| function writeActorProfile(nk: nkruntime.Nakama, profile: any, version: string): void { | |
| function writeActorProfile(nk: nkruntime.Nakama, profile: any, version: string): string { | |
| var write: any = { | |
| collection: collectionActor, | |
| key: actorStorageKey(profile.actor_id), | |
| userId: profile.owner_player_id, | |
| value: profile, | |
| permissionRead: 1, | |
| permissionWrite: 0 | |
| }; | |
| if (typeof version === "string" && version !== "") { | |
| write.version = version; | |
| } | |
| var acks = nk.storageWrite([write]); | |
| return acks[0].version; | |
| } |
|
Delta after Gemini review on actor profile retrieval:
Verification on latest head d32614f: pm run build in �ackend/nakama passed.pm test in �ackend/nakama passed.
Gemini (@gemini-code-assist) please delta re-review latest commit d32614f. |
There was a problem hiding this comment.
Code Review
This pull request introduces actor profiles and actor-specific memory management to the Nakama backend and Unity client. Key changes include the addition of ActorProfileDto and related request objects, new RPC endpoints for retrieving actor profiles and adding memories, and comprehensive normalization logic for character stats and attributes. Feedback focuses on several critical improvements: reducing actorIdMaxLength to 56 to prevent storage key length violations, eliminating redundant database reads by utilizing the version returned from storageWrite, handling potential version conflicts during memory updates, and resolving structural inconsistencies in the ActorProfileDto to align with existing player profile patterns.
| var rpcIdActorMemoryAdd = "secondspawn_actor_memory_add"; | ||
| var agentActivityLogLimit = 32; | ||
| var agentRuntimeMetricMax = 1000000000; | ||
| var actorIdMaxLength = 64; |
There was a problem hiding this comment.
The actorIdMaxLength of 64 is too high because the storage key generated in actorStorageKey (line 962) prepends profile:. Nakama storage keys have a hard limit of 64 characters. If a user provides an actor_id of 64 characters, the resulting key will be 72 characters, causing storageWrite to fail.
| var actorIdMaxLength = 64; | |
| var actorIdMaxLength = 56; |
| public MemoryRecordDto[] memory; | ||
| public AgentRuntimeDto agent_runtime; | ||
| public AgentActivityRecordDto[] agent_activity; |
There was a problem hiding this comment.
The ActorProfileDto contains memory, agent_runtime, and agent_activity fields at the root level, but these fields also exist within the BodyProfileDto (lines 47-49). This duplication creates inconsistency between the player profile (which uses body.memory) and the actor profile. It is recommended to move these behavioral state fields into the body object for actors as well, ensuring a consistent interface for all entities.
|
|
||
| var profile = defaultActorProfile(ownerId, actorId, request); | ||
| writeActorProfile(nk, profile, ""); | ||
| var created = readActorProfile(nk, ownerId, actorId); |
There was a problem hiding this comment.
This readActorProfile call is redundant. The nk.storageWrite method returns an array of acknowledgment objects that include the new version of the stored object. You can use the version from the write result instead of performing an additional database read.
| } | ||
| state.profile.memory = upsertMemoryRecord(state.profile.memory || [], memory); | ||
| state.profile.updated_at = new Date().toISOString(); | ||
| writeActorProfile(nk, state.profile, state.version); |
There was a problem hiding this comment.
This call to writeActorProfile uses a specific version for optimistic concurrency control but does not handle potential version conflict errors. If multiple requests attempt to update the same actor's memory simultaneously, one will fail with a storage conflict. Consider adding a retry loop or catching the error to provide a more robust implementation.
|
Delta after Gemini review on actor profile storage safety:
Verification on latest head
Gemini (@gemini-code-assist) please delta re-review latest commit bdf5940. |
There was a problem hiding this comment.
Code Review
This pull request introduces actor profile management, adding new DTOs in Unity and corresponding RPC endpoints in the Nakama backend for retrieving actor profiles and adding memories. The backend implementation includes logic for actor profile storage, normalization, and state management. Feedback focuses on optimizing the profile retrieval process by avoiding expensive deep comparisons using JSON.stringify and addressing a potential race condition during the initial creation of actor profiles to prevent unhandled storage conflict errors.
| var before = JSON.stringify(existing.value || {}); | ||
| var profile = ensureActorProfile(existing.value || {}, ownerId, actorId); | ||
| if (JSON.stringify(profile) !== before) { |
There was a problem hiding this comment.
Using JSON.stringify for deep comparison on every profile retrieval is inefficient, especially as the actor profile grows with memory and activity logs. Consider tracking a dirty flag within ensureActorProfile or comparing specific critical fields to determine if a write-back is necessary.
| writeActorProfile(nk, profile, ""); | ||
| var created = readActorProfile(nk, ownerId, actorId); |
There was a problem hiding this comment.
There is a potential race condition here. If two concurrent requests attempt to create the same actorId for the same user, the second writeActorProfile call with version "" (create-only) will throw a storage conflict error. Consider wrapping the initial write in a try-catch block to gracefully handle the case where the profile was created by a parallel request.
References
- Ensure that invalid states or race conditions in storage operations are safely handled, particularly during object creation.
JOY (JOY)
force-pushed
the
feat/character-stats-animation
branch
from
ca363a0 to
c7d00a8
Compare
JOY (JOY)
force-pushed
the
feat/actor-profile-registry
branch
from
bdf5940 to
ca4087e
Compare
|
Delta after Gemini review and stack rebase:
Verification on latest head
Gemini (@gemini-code-assist) please delta re-review latest commit ca4087e. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Local code-review delta passTarget: PR #12 latest head Engine Specialist Findings: N/A - no engine configured
Testability: TESTABLE
ADR Compliance: COMPLIANTThe change stays within ADR 0010: actor profiles are game-backend state in Nakama runtime modules. No separate game API gateway or client-authoritative state mutation is introduced. Standards Compliance: PASS
Architecture: CLEANActor profiles remain separate from player account/current-body state and support many NPC-like bodies with their own stats, traits, soul, memory, policy, runtime, and activity. SOLID: APPROVED WITH SUGGESTIONSNo blocking issue found. Split Nakama runtime modules later if the single prototype file becomes harder to maintain. Required ChangesNone. Suggestions
Verdict: APPROVED WITH SUGGESTIONSGemini delta review is currently blocked by daily quota, but this local pass verifies the latest Gemini findings were addressed and CI is green. |
1 participant
Summary
secondspawn_actor_profile_getandsecondspawn_actor_memory_add.secondspawn_actorstorage object keyed by actor id.Stack
This PR is stacked on #11 (
feat/character-stats-animation). Retarget todevafter #8 and #11 merge.Verification
npm run buildinbackend/nakamapassed.npm testinbackend/nakamapassed..github/workflows/markdown-lint.yml.git diff --checkpassed.Notes
Gemini (@gemini-code-assist) review
Codex (@codex) review