feat(nakama): persist agent runtime activity#8
Conversation
![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 agent runtime counters and a bounded activity log to the Nakama backend and Unity client. It adds a new RPC secondspawn_agent_activity_add, updates existing RPCs to track decisions and stats, and implements profile bootstrapping in the Unity client. Feedback focused on addressing potential race conditions in the backend storage operations, validating client-provided date strings, ensuring proper clamping and finiteness of numeric metrics to prevent serialization issues, and optimizing the Unity authentication flow by removing redundant network requests.
| nk: nkruntime.Nakama, | ||
| payload: string | ||
| ): string { | ||
| var context = getOrCreateAgentContext(ctx, nk); |
There was a problem hiding this comment.
This RPC follows a read-modify-write pattern that is susceptible to race conditions. If multiple requests (e.g., from the client and a game server) attempt to update the agent context concurrently, one update may overwrite another because Nakama storage operations are not atomic here. Consider using optimistic concurrency control by reading the object version and providing it to nk.storageWrite.
| function normalizeAgentActivity(context: any, request: any): any { | ||
| var kind = normalizeAgentActivityKind(request.kind); | ||
| var summary = trimString(request.summary); | ||
| if (!summary) { | ||
| throw new Error("agent activity summary is required"); | ||
| } | ||
|
|
||
| return { | ||
| id: trimString(request.id) || newActivityId(context), | ||
| kind: kind, | ||
| summary: summary, | ||
| occurred_at: trimString(request.occurred_at) || new Date().toISOString(), | ||
| source: trimString(request.source) || "client", | ||
| metrics: request.metrics || {} | ||
| }; | ||
| } |
There was a problem hiding this comment.
The occurred_at field is taken directly from the client request without validation. If an invalid date string is provided, it could cause issues when the Unity client or other services attempt to parse the activity log. It's safer to validate the date and fall back to the current server time if it's invalid.
function normalizeAgentActivity(context: any, request: any): any {
var kind = normalizeAgentActivityKind(request.kind);
var summary = trimString(request.summary);
if (!summary) {
throw new Error("agent activity summary is required");
}
var occurredAt = trimString(request.occurred_at) || new Date().toISOString();
if (isNaN(new Date(occurredAt).getTime())) {
occurredAt = new Date().toISOString();
}
return {
id: trimString(request.id) || newActivityId(context),
kind: kind,
summary: summary,
occurred_at: occurredAt,
source: trimString(request.source) || "client",
metrics: request.metrics || {}
};
}| function applyActivityMetrics(runtime: any, metrics: any): void { | ||
| runtime.offline_seconds += positiveMetric(metrics.offline_seconds); | ||
| runtime.decision_count += positiveMetric(metrics.decisions_made || metrics.decision_count); | ||
| runtime.fallback_decision_count += positiveMetric(metrics.fallback_decisions || metrics.fallback_decision_count); | ||
| runtime.move_intent_count += positiveMetric(metrics.move_intents || metrics.move_intent_count); | ||
| runtime.say_intent_count += positiveMetric(metrics.say_intents || metrics.say_intent_count); | ||
| runtime.stop_intent_count += positiveMetric(metrics.stop_intents || metrics.stop_intent_count); | ||
| runtime.interact_intent_count += positiveMetric(metrics.interact_intents || metrics.interact_intent_count); | ||
| } |
There was a problem hiding this comment.
The metrics from the client are added to the runtime counters without immediate clamping. While ensureAgentRuntime performs clamping, it is only called at the beginning of the RPC or during profile retrieval. Large values in the request could cause the counters to exceed intended bounds in storage until the next time the profile is fully processed. Clamping should be applied during the update.
| function applyActivityMetrics(runtime: any, metrics: any): void { | |
| runtime.offline_seconds += positiveMetric(metrics.offline_seconds); | |
| runtime.decision_count += positiveMetric(metrics.decisions_made || metrics.decision_count); | |
| runtime.fallback_decision_count += positiveMetric(metrics.fallback_decisions || metrics.fallback_decision_count); | |
| runtime.move_intent_count += positiveMetric(metrics.move_intents || metrics.move_intent_count); | |
| runtime.say_intent_count += positiveMetric(metrics.say_intents || metrics.say_intent_count); | |
| runtime.stop_intent_count += positiveMetric(metrics.stop_intents || metrics.stop_intent_count); | |
| runtime.interact_intent_count += positiveMetric(metrics.interact_intents || metrics.interact_intent_count); | |
| } | |
| function applyActivityMetrics(runtime: any, metrics: any): void { | |
| runtime.offline_seconds = clampNumber(runtime.offline_seconds + positiveMetric(metrics.offline_seconds), 0, 1000000000); | |
| runtime.decision_count = clampNumber(runtime.decision_count + positiveMetric(metrics.decisions_made || metrics.decision_count), 0, 1000000000); | |
| runtime.fallback_decision_count = clampNumber(runtime.fallback_decision_count + positiveMetric(metrics.fallback_decisions || metrics.fallback_decision_count), 0, 1000000000); | |
| runtime.move_intent_count = clampNumber(runtime.move_intent_count + positiveMetric(metrics.move_intents || metrics.move_intent_count), 0, 1000000000); | |
| runtime.say_intent_count = clampNumber(runtime.say_intent_count + positiveMetric(metrics.say_intents || metrics.say_intent_count), 0, 1000000000); | |
| runtime.stop_intent_count = clampNumber(runtime.stop_intent_count + positiveMetric(metrics.stop_intents || metrics.stop_intent_count), 0, 1000000000); | |
| runtime.interact_intent_count = clampNumber(runtime.interact_intent_count + positiveMetric(metrics.interact_intents || metrics.interact_intent_count), 0, 1000000000); | |
| } |
| function positiveMetric(value: any): number { | ||
| var numberValue = Number(value || 0); | ||
| if (isNaN(numberValue) || numberValue < 0) { | ||
| return 0; | ||
| } | ||
| return Math.floor(numberValue); | ||
| } |
There was a problem hiding this comment.
The Number() constructor can return Infinity for very large values. Math.floor(Infinity) is still Infinity, and when JSON.stringify is called on an object containing Infinity, it serializes to null. This will cause the Unity client (which expects a long) to receive a default value of 0, effectively losing the counter state. Adding a check for isFinite prevents this.
| function positiveMetric(value: any): number { | |
| var numberValue = Number(value || 0); | |
| if (isNaN(numberValue) || numberValue < 0) { | |
| return 0; | |
| } | |
| return Math.floor(numberValue); | |
| } | |
| function positiveMetric(value: any): number { | |
| var numberValue = Number(value || 0); | |
| if (isNaN(numberValue) || numberValue < 0 || !isFinite(numberValue)) { | |
| return 0; | |
| } | |
| return Math.floor(numberValue); | |
| } |
| private IEnumerator BootstrapNakamaProfileAfterAuth(string authSource) | ||
| { | ||
| if (!_bootstrapProfileAfterAuth || !HasNakamaSession) | ||
| { | ||
| yield break; | ||
| } | ||
|
|
||
| AgentContextDto context = null; | ||
| yield return GetNakamaContext(result => context = result, error => | ||
| { | ||
| Debug.LogWarning($"[SecondSpawnGatewayClient] Nakama profile bootstrap failed: {error}"); | ||
| }); | ||
|
|
||
| if (context == null) | ||
| { | ||
| yield break; | ||
| } | ||
|
|
||
| yield return AddNakamaAgentActivity(new AgentActivityRecordDto | ||
| { | ||
| kind = "profile_bootstrap", | ||
| summary = $"Unity client authenticated through {authSource} and confirmed the Nakama character profile.", | ||
| source = "unity" | ||
| }, null, error => | ||
| { | ||
| Debug.LogWarning($"[SecondSpawnGatewayClient] Nakama profile activity write failed: {error}"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The BootstrapNakamaProfileAfterAuth method performs two sequential network calls: GetNakamaContext followed by AddNakamaAgentActivity. Since AddNakamaAgentActivity triggers the same getOrCreateAgentContext logic on the backend (which creates the profile if it doesn't exist) and also returns the full AgentContextDto, the initial GetNakamaContext call is redundant and adds unnecessary latency to the authentication flow.
private IEnumerator BootstrapNakamaProfileAfterAuth(string authSource)
{
if (!_bootstrapProfileAfterAuth || !HasNakamaSession)
{
yield break;
}
yield return AddNakamaAgentActivity(new AgentActivityRecordDto
{
kind = "profile_bootstrap",
summary = $"Unity client authenticated through {authSource} and confirmed the Nakama character profile.",
source = "unity"
}, null, error =>
{
Debug.LogWarning($"[SecondSpawnGatewayClient] Nakama profile activity write failed: {error}");
});
}
|
Delta after Unity retest:
Remaining Unity smoke blocker is separate from this PR and still tracked in #7: Fusion editor/domain-reload errors around |
Local code-review skill passTarget: PR #8 Engine Specialist Findings: N/A - no engine specialist configured
Testability: TESTABLE
ADR Compliance: COMPLIANTNo explicit ADR reference was found in commit messages or file headers. Architectural behavior matches the current project rules: Nakama owns durable game profile/activity state, gateway remains an AI/LLM boundary, and Unity stores no provider keys. Standards Compliance: PASS
Architecture: CLEAN
SOLID: APPROVED WITH SUGGESTIONSNo blocking SOLID issue found. Longer-term, Game-Specific Concerns
Positive Observations
Required ChangesNone from local code-review after Suggestions
Verdict: APPROVED WITH SUGGESTIONSCodex (@codex) review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Delta after Gemini review comments and GDD doc pass:
Local verification on latest head
GitHub checks are green: Gemini, please delta re-review the addressed findings when available. Gemini (@gemini-code-assist) review Note: |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Game Design Document and implements the 'Agent Runtime and Activity' system for tracking offline-agent observability. Key changes include new Nakama RPCs for activity logging and runtime counters, Unity-side profile bootstrapping, and the introduction of versioned storage writes for optimistic concurrency. Review feedback suggests refactoring the rpcAgentDecide logic to eliminate code duplication and standardizing metric naming across the system to ensure consistency between client and server data structures.
| var state = getOrCreateAgentContextState(ctx, nk); | ||
| var context = state.context; | ||
| var request = parseJson(payload || "{}", "agent decision payload"); | ||
| var world = request.world_snapshot || {}; | ||
| var allowed = request.allowed || ["move", "interact", "say", "stop"]; | ||
| var bodyTime = Number(world.body_time_seconds || context.body.time.remaining_seconds || 0); | ||
| var decision: any = null; | ||
|
|
||
| if (bodyTime > 0 && bodyTime <= context.body.agent_policy.stop_when_body_time_below) { | ||
| return JSON.stringify({ | ||
| decision = { | ||
| action: "stop", | ||
| reason: "body_time_below_policy_threshold", | ||
| confidence: 0.9 | ||
| }); | ||
| confidence: 0.9, | ||
| source: "fallback", | ||
| source_reason: "nakama_body_time_policy" | ||
| }; | ||
| recordAgentDecision(context, decision); | ||
| writeAgentContext(nk, context, state.version); | ||
| return JSON.stringify(decision); | ||
| } | ||
|
|
||
| if (arrayContains(allowed, "move")) { | ||
| var position = world.position || { x: 0, z: 0 }; | ||
| return JSON.stringify({ | ||
| decision = { | ||
| action: "move", | ||
| move: { | ||
| x: Number(position.x || 0) + 1.5, | ||
| z: Number(position.z || 0) + 0.75 | ||
| }, | ||
| reason: "prototype_safe_patrol", | ||
| confidence: 0.55 | ||
| }); | ||
| confidence: 0.55, | ||
| source: "fallback", | ||
| source_reason: "nakama_prototype_patrol" | ||
| }; | ||
| recordAgentDecision(context, decision); | ||
| writeAgentContext(nk, context, state.version); | ||
| return JSON.stringify(decision); | ||
| } | ||
|
|
||
| if (arrayContains(allowed, "say")) { | ||
| return JSON.stringify({ | ||
| decision = { | ||
| action: "say", | ||
| say: "I am keeping this body safe until the player returns.", | ||
| reason: "prototype_social_fallback", | ||
| confidence: 0.6 | ||
| }); | ||
| confidence: 0.6, | ||
| source: "fallback", | ||
| source_reason: "nakama_social_fallback" | ||
| }; | ||
| recordAgentDecision(context, decision); | ||
| writeAgentContext(nk, context, state.version); | ||
| return JSON.stringify(decision); | ||
| } | ||
|
|
||
| return JSON.stringify({ | ||
| decision = { | ||
| action: "stop", | ||
| reason: "no_allowed_action", | ||
| confidence: 0.5 | ||
| }); | ||
| confidence: 0.5, | ||
| source: "fallback", | ||
| source_reason: "nakama_no_allowed_action" | ||
| }; | ||
| recordAgentDecision(context, decision); | ||
| writeAgentContext(nk, context, state.version); | ||
| return JSON.stringify(decision); |
There was a problem hiding this comment.
The logic to record the decision, write the context, and return is duplicated in each branch of the if/else if chain. This can be refactored to improve readability and maintainability by determining the decision object first, and then calling the common logic once at the end of the function.
var state = getOrCreateAgentContextState(ctx, nk);
var context = state.context;
var request = parseJson(payload || "{}", "agent decision payload");
var world = request.world_snapshot || {};
var allowed = request.allowed || ["move", "interact", "say", "stop"];
var bodyTime = Number(world.body_time_seconds || context.body.time.remaining_seconds || 0);
var decision: any;
if (bodyTime > 0 && bodyTime <= context.body.agent_policy.stop_when_body_time_below) {
decision = {
action: "stop",
reason: "body_time_below_policy_threshold",
confidence: 0.9,
source: "fallback",
source_reason: "nakama_body_time_policy"
};
} else if (arrayContains(allowed, "move")) {
var position = world.position || { x: 0, z: 0 };
decision = {
action: "move",
move: {
x: Number(position.x || 0) + 1.5,
z: Number(position.z || 0) + 0.75
},
reason: "prototype_safe_patrol",
confidence: 0.55,
source: "fallback",
source_reason: "nakama_prototype_patrol"
};
} else if (arrayContains(allowed, "say")) {
decision = {
action: "say",
say: "I am keeping this body safe until the player returns.",
reason: "prototype_social_fallback",
confidence: 0.6,
source: "fallback",
source_reason: "nakama_social_fallback"
};
} else {
decision = {
action: "stop",
reason: "no_allowed_action",
confidence: 0.5,
source: "fallback",
source_reason: "nakama_no_allowed_action"
};
}
recordAgentDecision(context, decision);
writeAgentContext(nk, context, state.version);
return JSON.stringify(decision);| function applyActivityMetrics(runtime: any, metrics: any): void { | ||
| runtime.offline_seconds = addRuntimeMetric(runtime.offline_seconds, metrics.offline_seconds); | ||
| runtime.decision_count = addRuntimeMetric(runtime.decision_count, metrics.decisions_made || metrics.decision_count); | ||
| runtime.fallback_decision_count = addRuntimeMetric(runtime.fallback_decision_count, metrics.fallback_decisions || metrics.fallback_decision_count); | ||
| runtime.move_intent_count = addRuntimeMetric(runtime.move_intent_count, metrics.move_intents || metrics.move_intent_count); | ||
| runtime.say_intent_count = addRuntimeMetric(runtime.say_intent_count, metrics.say_intents || metrics.say_intent_count); | ||
| runtime.stop_intent_count = addRuntimeMetric(runtime.stop_intent_count, metrics.stop_intents || metrics.stop_intent_count); | ||
| runtime.interact_intent_count = addRuntimeMetric(runtime.interact_intent_count, metrics.interact_intents || metrics.interact_intent_count); | ||
| } |
There was a problem hiding this comment.
This function uses || to support alternative names for metrics, for example metrics.decisions_made || metrics.decision_count. This suggests an inconsistency in metric naming across different parts of the system (e.g., client-reported vs. internally generated). To improve code clarity and maintainability, it's recommended to standardize on a single set of metric names. The names used in the client DTOs (e.g., decisions_made, fallback_decisions) seem like a good standard to adopt everywhere. This would require updating recordAgentDecision to use the standard names when creating metrics for agent_decision activities.
|
Delta after reviewing Gemini's latest comments against the current code:
Verified latest head
Gemini (@gemini-code-assist) review |
There was a problem hiding this comment.
Code Review
This pull request implements agent runtime counters and a bounded activity log to monitor offline agent behavior and profile bootstrapping. Key updates include new DTOs and RPC endpoints in the Unity client and Nakama backend, supported by comprehensive documentation and a new Game Design Document. Feedback from the review highlights logic errors in the Nakama decision-making process that could fail to stop the agent when bodyTime is zero. Furthermore, suggestions were made to replace string timestamps with time.Time in the Go backend for better type safety and consistency.
| ProfileBootstrappedAt string `json:"profile_bootstrapped_at"` | ||
| LastProfileBootstrapAt string `json:"last_profile_bootstrap_at"` | ||
| LastActivityAt string `json:"last_activity_at"` |
There was a problem hiding this comment.
These timestamp fields should use time.Time instead of string to maintain consistency with other durable fields in this file (like CreatedAt and UpdatedAt) and to leverage Go's native RFC3339 JSON marshaling.
ProfileBootstrappedAt time.Time `json:"profile_bootstrapped_at"`\n LastProfileBootstrapAt time.Time `json:"last_profile_bootstrap_at"`\n LastActivityAt time.Time `json:"last_activity_at"`| ID string `json:"id"` | ||
| Kind string `json:"kind"` | ||
| Summary string `json:"summary"` | ||
| OccurredAt string `json:"occurred_at"` |
There was a problem hiding this comment.
Consider using time.Time for the OccurredAt field for better type safety and consistency with the rest of the character profile model.
| OccurredAt string `json:"occurred_at"` | |
| OccurredAt time.Time `json:"occurred_at"` |
| var request = parseJson(payload || "{}", "agent decision payload"); | ||
| var world = request.world_snapshot || {}; | ||
| var allowed = request.allowed || ["move", "interact", "say", "stop"]; | ||
| var bodyTime = Number(world.body_time_seconds || context.body.time.remaining_seconds || 0); |
There was a problem hiding this comment.
Using the logical OR operator (||) here will cause bodyTime to incorrectly fall back to the cached profile value when world.body_time_seconds is explicitly 0 (e.g., when the body is out of time). This can lead to the agent continuing to act when it should have stopped. Use an explicit check for undefined or null instead.
| var bodyTime = Number(world.body_time_seconds || context.body.time.remaining_seconds || 0); | |
| var bodyTime = Number((world.body_time_seconds !== undefined && world.body_time_seconds !== null) ? world.body_time_seconds : (context.body.time.remaining_seconds || 0)); |
| var bodyTime = Number(world.body_time_seconds || context.body.time.remaining_seconds || 0); | ||
| var decision: any; | ||
|
|
||
| if (bodyTime > 0 && bodyTime <= context.body.agent_policy.stop_when_body_time_below) { |
There was a problem hiding this comment.
The bodyTime > 0 check prevents the deterministic 'stop' action from triggering when the body is exactly out of time (bodyTime === 0). If the body has no time left, it should satisfy the policy threshold and stop immediately.
| if (bodyTime > 0 && bodyTime <= context.body.agent_policy.stop_when_body_time_below) { | |
| if (bodyTime <= context.body.agent_policy.stop_when_body_time_below) { |
|
Delta after latest Gemini comments:
Verified latest head
Gemini (@gemini-code-assist) review |
There was a problem hiding this comment.
Code Review
This pull request introduces an agent runtime tracking system and a bounded activity log for the Nakama backend, supported by new DTOs in Unity and updated Go structs in the gateway. Key additions include a comprehensive Game Design Document and Nakama TypeScript updates that implement optimistic concurrency via storage versions, alongside new RPCs for activity logging and deterministic fallback decisions. Feedback focuses on logic improvements within the Nakama module, specifically noting that the interact action is missing implementation in rpcAgentDecide and that frequent decision logging might flood the activity log. It was also recommended to replace Math.random() with the platform-standard nk.uuidV4() for generating unique identifiers.
| var allowed = request.allowed || ["move", "interact", "say", "stop"]; | ||
| var bodyTime = Number(world.body_time_seconds || context.body.time.remaining_seconds || 0); | ||
| var bodyTime = Number(world.body_time_seconds !== undefined && world.body_time_seconds !== null | ||
| ? world.body_time_seconds | ||
| : context.body.time.remaining_seconds || 0); | ||
| var decision: any; | ||
|
|
||
| if (bodyTime > 0 && bodyTime <= context.body.agent_policy.stop_when_body_time_below) { | ||
| return JSON.stringify({ | ||
| if (bodyTime <= context.body.agent_policy.stop_when_body_time_below) { | ||
| decision = { | ||
| action: "stop", | ||
| reason: "body_time_below_policy_threshold", | ||
| confidence: 0.9 | ||
| }); | ||
| } | ||
|
|
||
| if (arrayContains(allowed, "move")) { | ||
| confidence: 0.9, | ||
| source: "fallback", | ||
| source_reason: "nakama_body_time_policy" | ||
| }; | ||
| } else if (arrayContains(allowed, "move")) { | ||
| var position = world.position || { x: 0, z: 0 }; | ||
| return JSON.stringify({ | ||
| decision = { | ||
| action: "move", | ||
| move: { | ||
| x: Number(position.x || 0) + 1.5, | ||
| z: Number(position.z || 0) + 0.75 | ||
| }, | ||
| reason: "prototype_safe_patrol", | ||
| confidence: 0.55 | ||
| }); | ||
| } | ||
|
|
||
| if (arrayContains(allowed, "say")) { | ||
| return JSON.stringify({ | ||
| confidence: 0.55, | ||
| source: "fallback", | ||
| source_reason: "nakama_prototype_patrol" | ||
| }; | ||
| } else if (arrayContains(allowed, "say")) { | ||
| decision = { | ||
| action: "say", | ||
| say: "I am keeping this body safe until the player returns.", | ||
| reason: "prototype_social_fallback", | ||
| confidence: 0.6 | ||
| }); | ||
| confidence: 0.6, | ||
| source: "fallback", | ||
| source_reason: "nakama_social_fallback" | ||
| }; | ||
| } else { | ||
| decision = { | ||
| action: "stop", | ||
| reason: "no_allowed_action", | ||
| confidence: 0.5, | ||
| source: "fallback", | ||
| source_reason: "nakama_no_allowed_action" | ||
| }; | ||
| } |
There was a problem hiding this comment.
The rpcAgentDecide function includes interact in the default allowed set but lacks implementation logic for it. If interact is the only allowed action, the RPC will fall through to returning a stop action, which is inconsistent with the allowed list and the incrementDecisionAction helper that expects interact as a possible outcome.
| function recordAgentDecision(context: any, decision: any): void { | ||
| ensureAgentRuntime(context); | ||
| var runtime = context.body.agent_runtime; | ||
| runtime.decision_count += 1; | ||
| if (decision.source === "fallback") { | ||
| runtime.fallback_decision_count += 1; | ||
| } | ||
|
|
||
| incrementDecisionAction(runtime, decision.action); | ||
| addAgentActivity(context, { | ||
| kind: "agent_decision", | ||
| summary: "Agent chose " + trimString(decision.action || "unknown") + ": " + trimString(decision.reason || "no reason provided"), | ||
| source: "nakama", | ||
| metrics: { | ||
| decisions_made: 1 | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The recordAgentDecision function appends an agent_decision activity record for every decision made. Given the agentActivityLogLimit of 32, frequent calls to rpcAgentDecide (e.g., during an offline session) will flood the activity log, quickly pushing out more significant events like profile_bootstrap or offline_session summaries. Consider only logging a decision activity if the action changes or at the start/end of a session, while still updating the runtime counters for every decision.
| function newActivityId(context: any): string { | ||
| var playerId = sanitizeNakamaIdentifier(context.player.player_id || "player", "player"); | ||
| var randomPart = Math.floor(Math.random() * 0x100000000).toString(36); | ||
| var sequence = String((context.body.agent_activity || []).length + 1); | ||
| return "act-" + playerId + "-" + nowId() + "-" + randomPart + "-" + sequence; | ||
| } |
There was a problem hiding this comment.
While Math.random() is generally acceptable for ID generation in a single-node RPC context, Nakama provides nk.uuidV4() which is a more standard approach for generating unique identifiers within the runtime.
References
- Prefer standard library or framework-provided UUID generators over Math.random() for identifier generation to ensure uniqueness and follow platform idioms.
|
Delta after Gemini review:
Verification:
Local note: |
|
Gemini (@gemini-code-assist) please run a delta re-review on commit b17deac. Focus areas: Nakama interact fallback, duplicate decision activity suppression, and the deferred uuidV4 note above. |
There was a problem hiding this comment.
Code Review
This pull request implements agent runtime counters and a bounded activity log to monitor offline-agent behavior and profile bootstrap events. Changes span the Unity client, Go gateway, and Nakama runtime, including new RPCs for activity reporting and deterministic fallback decisions. Review feedback focuses on ensuring idempotency in activity reporting to prevent duplicate metric increments and optimizing database interactions by utilizing storage write acknowledgments to avoid redundant reads.
| addAgentActivity(context, activity); | ||
| applyActivityMetrics(context.body.agent_runtime, request.metrics || {}); | ||
| writeAgentContext(nk, context, state.version); |
There was a problem hiding this comment.
The rpcAgentActivityAdd endpoint is not idempotent. If a client retries a request (e.g., due to a network timeout), the same activity metrics will be added to the runtime counters multiple times, leading to incorrect statistics. You should check if an activity with the same ID already exists before applying metrics and writing the context.
| addAgentActivity(context, activity); | |
| applyActivityMetrics(context.body.agent_runtime, request.metrics || {}); | |
| writeAgentContext(nk, context, state.version); | |
| if (addAgentActivity(context, activity)) { | |
| applyActivityMetrics(context.body.agent_runtime, request.metrics || {}); | |
| writeAgentContext(nk, context, state.version); | |
| } |
| function addAgentActivity(context: any, activity: any): void { | ||
| ensureAgentRuntime(context); | ||
| if (!activity.id) { | ||
| activity.id = newActivityId(context); | ||
| } | ||
| if (!activity.occurred_at) { | ||
| activity.occurred_at = new Date().toISOString(); | ||
| } | ||
| if (!activity.source) { | ||
| activity.source = "nakama"; | ||
| } | ||
|
|
||
| var activities = context.body.agent_activity || []; | ||
| activities.unshift(activity); | ||
| if (activities.length > agentActivityLogLimit) { | ||
| activities = activities.slice(0, agentActivityLogLimit); | ||
| } | ||
| context.body.agent_activity = activities; | ||
| context.body.agent_runtime.activity_count += 1; | ||
| context.body.agent_runtime.last_activity_at = activity.occurred_at; | ||
| } |
There was a problem hiding this comment.
To support idempotency in activity reporting, addAgentActivity should check for existing activity IDs before appending to the log. Returning a boolean indicating whether a new activity was actually added allows the caller to decide whether to apply associated metrics.
function addAgentActivity(context: any, activity: any): boolean {
ensureAgentRuntime(context);
var activities = context.body.agent_activity || [];
if (activity.id) {
for (var i = 0; i < activities.length; i++) {
if (activities[i].id === activity.id) {
return false;
}
}
} else {
activity.id = newActivityId(context);
}
if (!activity.occurred_at) {
activity.occurred_at = new Date().toISOString();
}
if (!activity.source) {
activity.source = "nakama";
}
activities.unshift(activity);
if (activities.length > agentActivityLogLimit) {
activities = activities.slice(0, agentActivityLogLimit);
}
context.body.agent_activity = activities;
context.body.agent_runtime.activity_count += 1;
context.body.agent_runtime.last_activity_at = activity.occurred_at;
return true;
}| writeAgentContext(nk, context, ""); | ||
| var created = readAgentContext(nk, userId); | ||
| if (created) { | ||
| return { | ||
| context: created.value, | ||
| version: created.version | ||
| }; | ||
| } |
There was a problem hiding this comment.
The current implementation performs an unnecessary storage read immediately after a write to retrieve the object version. nk.storageWrite returns an array of acknowledgments that already contain the version of the written objects. You can optimize this by updating writeAgentContext to return the version and using it directly, saving a database roundtrip during the profile bootstrap process.
var context = defaultAgentContext(userId);
var version = writeAgentContext(nk, context, "");
return {
context: context,
version: version
};| function writeAgentContext(nk: nkruntime.Nakama, context: any, version: string): void { | ||
| var write: any = { | ||
| collection: collectionAgent, | ||
| key: keyAgentContext, | ||
| userId: context.player.player_id, | ||
| value: context, | ||
| permissionRead: 1, | ||
| permissionWrite: 0 | ||
| }]); | ||
| }; | ||
| if (version) { | ||
| write.version = version; | ||
| } | ||
| nk.storageWrite([write]); | ||
| } |
There was a problem hiding this comment.
Updating writeAgentContext to return the version from the storage acknowledgment allows callers to maintain an up-to-date version string without performing additional reads. This is particularly useful for the bootstrap logic to avoid redundant database operations.
| function writeAgentContext(nk: nkruntime.Nakama, context: any, version: string): void { | |
| var write: any = { | |
| collection: collectionAgent, | |
| key: keyAgentContext, | |
| userId: context.player.player_id, | |
| value: context, | |
| permissionRead: 1, | |
| permissionWrite: 0 | |
| }]); | |
| }; | |
| if (version) { | |
| write.version = version; | |
| } | |
| nk.storageWrite([write]); | |
| } | |
| function writeAgentContext(nk: nkruntime.Nakama, context: any, version: string): string { | |
| var write: any = { | |
| collection: collectionAgent, | |
| key: keyAgentContext, | |
| userId: context.player.player_id, | |
| value: context, | |
| permissionRead: 1, | |
| permissionWrite: 0 | |
| }; | |
| if (version) { | |
| write.version = version; | |
| } | |
| var acks = nk.storageWrite([write]); | |
| return acks[0].version; | |
| } |
|
Delta after Gemini review on idempotency:
Reviewed the storage write acknowledgement optimization separately:
Verification on latest head
Gemini (@gemini-code-assist) please delta re-review latest commit 2dffe65. |
There was a problem hiding this comment.
Code Review
This pull request introduces agent runtime counters and a bounded activity log to the character profile, providing observability for offline-agent sessions and profile bootstrapping. The changes include new Nakama RPCs for activity reporting, Unity client updates for automatic profile bootstrapping after authentication, and the addition of a comprehensive Game Design Document. Feedback highlights a critical bug in the Nakama storage write logic where empty string versions (used for create-only operations) are incorrectly ignored, potentially causing race conditions. Additionally, the introduction of redundant DTOs in the Unity client is flagged as a maintenance burden, and there is a recommendation to ensure model-backed decisions from the Gateway are reported back to Nakama to maintain consistent counters.
| if (version) { | ||
| write.version = version; | ||
| } |
There was a problem hiding this comment.
The check if (version) evaluates to false for an empty string (""). In Nakama's storage API, a version of "" is specifically used to indicate a "create-only" operation (the record must not already exist). By skipping the assignment when version is "", the storage write becomes an unconditional overwrite, which breaks the optimistic concurrency logic in getOrCreateAgentContextState and introduces a race condition during profile creation.
| if (version) { | |
| write.version = version; | |
| } | |
| if (typeof version === 'string') { | |
| write.version = version; | |
| } |
| private sealed class GatewayAgentDecisionRequestDto | ||
| { | ||
| public GatewayAgentContextDto context; | ||
| public WorldSnapshotDto world_snapshot; | ||
| public string[] allowed; | ||
|
|
||
| public static GatewayAgentDecisionRequestDto From(AgentDecisionRequestDto request) | ||
| { | ||
| return new GatewayAgentDecisionRequestDto | ||
| { | ||
| context = GatewayAgentContextDto.From(request?.context), | ||
| world_snapshot = request?.world_snapshot, | ||
| allowed = request?.allowed | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| [Serializable] | ||
| private sealed class GatewayAgentContextDto | ||
| { | ||
| public PlayerProfileDto player; | ||
| public GatewayBodyProfileDto body; | ||
|
|
||
| public static GatewayAgentContextDto From(AgentContextDto context) | ||
| { | ||
| return new GatewayAgentContextDto | ||
| { | ||
| player = context?.player, | ||
| body = GatewayBodyProfileDto.From(context?.body) | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| [Serializable] | ||
| private sealed class GatewayBodyProfileDto | ||
| { | ||
| public string body_id; | ||
| public string archetype_id; | ||
| public string visual_prefab_key; | ||
| public EquipmentLoadoutDto equipment; | ||
| public CharacterTraitsDto characteristics; | ||
| public BodyTimeDto time; | ||
| public CultivationDto cultivation; | ||
| public AgentPolicyDto agent_policy; | ||
| public SoulProfileDto soul; | ||
| public MemoryRecordDto[] memory; | ||
|
|
||
| public static GatewayBodyProfileDto From(BodyProfileDto body) | ||
| { | ||
| if (body == null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return new GatewayBodyProfileDto | ||
| { | ||
| body_id = body.body_id, | ||
| archetype_id = body.archetype_id, | ||
| visual_prefab_key = body.visual_prefab_key, | ||
| equipment = body.equipment, | ||
| characteristics = body.characteristics, | ||
| time = body.time, | ||
| cultivation = body.cultivation, | ||
| agent_policy = body.agent_policy, | ||
| soul = body.soul, | ||
| memory = body.memory | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The introduction of GatewayAgentDecisionRequestDto and its nested classes creates significant maintenance overhead. These classes are nearly identical to the existing AgentDecisionRequestDto hierarchy and are only used to omit the agent_runtime and agent_activity fields. Since the Go-based Gateway safely ignores extra JSON fields, it is more maintainable to send the original DTOs directly rather than maintaining a parallel set of filtered DTOs that must be manually synchronized whenever the character profile schema changes.
| reason: "no_allowed_action", | ||
| confidence: 0.5 | ||
| }); | ||
| recordAgentDecision(context, decision, nk); |
There was a problem hiding this comment.
The AgentRuntime counters (such as decision_count) are currently only updated within the Nakama rpcAgentDecide function. However, the Unity client primarily uses the Gateway's /v1/agent/decide endpoint for model-backed decisions. This results in inconsistent statistics where the majority of agent activity is not reflected in the runtime counters. Consider having the Unity client report Gateway decisions back to Nakama via the secondspawn_agent_activity_add RPC to ensure the audit trail and counters remain accurate regardless of the decision source.
|
Delta after Gemini review on latest PR #8 comments:
Verification on latest head
Gemini (@gemini-code-assist) please delta re-review latest commit 0fc87e9. |
There was a problem hiding this comment.
Code Review
This pull request introduces agent runtime telemetry and activity logging across the Unity client, Go gateway, and Nakama backend. Key additions include new DTOs and structs for tracking agent counters (decisions, intents, offline time) and a bounded activity log. The Unity client now bootstraps the player profile immediately after authentication and records gateway decisions as Nakama activity. The Nakama runtime was updated with a new RPC for activity reporting, deterministic fallback decision recording, and optimistic concurrency control for storage writes. Documentation was expanded with a comprehensive Game Design Document and updated system specs. Review feedback focused on hardening the Nakama RPCs against invalid numeric inputs and storage version conflicts, ensuring robust error handling in the Unity gateway client, and using normalized data for metric updates.
| var bodyTime = Number(world.body_time_seconds !== undefined && world.body_time_seconds !== null | ||
| ? world.body_time_seconds | ||
| : context.body.time.remaining_seconds || 0); |
There was a problem hiding this comment.
The bodyTime calculation should be guarded against NaN values. If world.body_time_seconds is provided as a non-numeric value, Number() will return NaN, causing the policy threshold check to fail and potentially allowing the agent to continue acting with invalid state. Using the existing clampNumber helper ensures a safe numeric fallback.
| var bodyTime = Number(world.body_time_seconds !== undefined && world.body_time_seconds !== null | |
| ? world.body_time_seconds | |
| : context.body.time.remaining_seconds || 0); | |
| var bodyTime = clampNumber(world.body_time_seconds !== undefined && world.body_time_seconds !== null | |
| ? world.body_time_seconds | |
| : context.body.time.remaining_seconds || 0, 0, agentRuntimeMetricMax); | |
| @@ -194,7 +206,32 @@ public IEnumerator UpdateSoulForPlayer(string playerId, UpdateSoulRequestDto req | |||
|
|
|||
| public IEnumerator Decide(AgentDecisionRequestDto request, Action<AgentDecisionDto> onSuccess, Action<string> onError = null) | |||
| { | |||
There was a problem hiding this comment.
The Decide method should validate that the request parameter is not null before proceeding. Passing a null request to the gateway DTO factory will result in a malformed payload that the server may fail to process.
{
if (request == null)
{
onError?.Invoke("Agent decision request cannot be null.");
yield break;
}| if (decision == null) | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(gatewayError)) | ||
| { | ||
| onError?.Invoke(gatewayError); | ||
| } | ||
| yield break; | ||
| } |
There was a problem hiding this comment.
If the gateway returns an empty or invalid response that results in a null decision, the onError callback might not be invoked if gatewayError is also null or whitespace. It is better to provide a default error message to ensure the caller is always notified of the failure.
if (decision == null)
{
onError?.Invoke(!string.IsNullOrWhiteSpace(gatewayError) ? gatewayError : "Gateway returned an invalid or empty decision.");
yield break;
}| confidence: 0.5 | ||
| }); | ||
| recordAgentDecision(context, decision, nk); | ||
| writeAgentContext(nk, context, state.version); |
There was a problem hiding this comment.
The RPC implementation uses optimistic concurrency control via storage versions, but it lacks a retry mechanism for handling version conflicts. In scenarios with high activity (e.g., rapid decision loops or concurrent activity reports), the writeAgentContext call may throw a 'storage version conflict' error, causing the RPC to fail and potentially leading to inconsistent state between the client and server. Consider implementing a simple retry loop that refreshes the context state on conflict.
| var activity = normalizeAgentActivity(context, request, nk); | ||
|
|
||
| if (addAgentActivity(context, activity, nk)) { | ||
| applyActivityMetrics(context.body.agent_runtime, request.metrics || {}); |
There was a problem hiding this comment.
In rpcAgentActivityAdd, it is safer to use the normalized activity.metrics instead of the raw request.metrics. This ensures that any sanitization or default values applied during the normalization phase are correctly reflected in the runtime counters.
| applyActivityMetrics(context.body.agent_runtime, request.metrics || {}); | |
| applyActivityMetrics(context.body.agent_runtime, activity.metrics); | |
|
Small docs delta after the updated body/NPC model clarification:
Verification:
Gemini (@gemini-code-assist) please include latest docs commit e833ad9 in the pending PR #8 delta review. |
There was a problem hiding this comment.
Code Review
This pull request implements agent runtime counters and a bounded activity log for the offline-agent prototype, spanning Unity client updates, Go backend profile structures, and Nakama RPC logic. It also adds a comprehensive Game Design Document and updates existing design documentation. Feedback focused on improving the robustness of the Nakama profile normalization logic to prevent runtime errors when accessing potentially missing nested objects like time and agent_policy. There was also a recommendation to evaluate the latency impact of synchronous activity logging within the Unity Decide coroutine.
| var bodyTime = Number(world.body_time_seconds !== undefined && world.body_time_seconds !== null | ||
| ? world.body_time_seconds | ||
| : context.body.time.remaining_seconds || 0); |
There was a problem hiding this comment.
The access to context.body.time.remaining_seconds is risky if the time object is missing from the profile. While defaultAgentContext initializes it, the ensureAgentRuntime migration function (line 368) only ensures the presence of body, agent_runtime, and agent_activity. If a profile is in a corrupted state or was partially migrated, this could throw a runtime error.
| var bodyTime = Number(world.body_time_seconds !== undefined && world.body_time_seconds !== null | |
| ? world.body_time_seconds | |
| : context.body.time.remaining_seconds || 0); | |
| var bodyTime = Number(world.body_time_seconds !== undefined && world.body_time_seconds !== null | |
| ? world.body_time_seconds | |
| : (context.body.time && context.body.time.remaining_seconds) || 0); |
| : context.body.time.remaining_seconds || 0); | ||
| var decision: any; | ||
|
|
||
| if (bodyTime <= context.body.agent_policy.stop_when_body_time_below) { |
There was a problem hiding this comment.
Similar to the time object, context.body.agent_policy should be checked for existence before accessing its properties to prevent runtime exceptions if the profile data is incomplete.
| if (bodyTime <= context.body.agent_policy.stop_when_body_time_below) { | |
| if (context.body.agent_policy && bodyTime <= context.body.agent_policy.stop_when_body_time_below) { |
| }; | ||
| } | ||
|
|
||
| function ensureAgentRuntime(context: any): boolean { |
There was a problem hiding this comment.
The ensureAgentRuntime function acts as a normalization and migration layer. To improve robustness, it should also ensure that other critical fields required by the agent logic (like time, agent_policy, soul, and memory) exist, especially since it initializes context.body to an empty object if it's missing.
| if (HasNakamaSession) | ||
| { | ||
| yield return AddNakamaAgentActivity(BuildGatewayDecisionActivity(decision), null, error => | ||
| { | ||
| Debug.LogWarning($"[SecondSpawnGatewayClient] Gateway decision activity write failed: {error}"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The Decide coroutine now waits for the AddNakamaAgentActivity RPC to complete before finishing. While onSuccess is called immediately after the gateway response, any logic that yield returns on Decide will be delayed by this additional network call. Consider if this activity logging should be fire-and-forget or if the latency is acceptable for the current prototype.
|
Delta after Gemini review on loaded-profile robustness and Unity decision latency:
Verification on latest head 99fcae6: pm run build in �ackend/nakama passed.pm test in �ackend/nakama passed.
Unity caveat: this delta touches C#, but clean-worktree Unity batchmode compile remains blocked in this repo family by the existing Package Manager issue noted earlier, so Editor/MCP compile verification is still required before a no-caveat merge claim. Gemini (@gemini-code-assist) please delta re-review latest commit 99fcae6. |
There was a problem hiding this comment.
Code Review
This pull request implements the Actor and Body Model and an Agent Runtime tracking system, distinguishing between durable player consciousness and synthetic bodies. Key changes include new Unity DTOs, backend profile updates in Go, and Nakama TypeScript logic for managing operational counters and activity logs. The Unity client now supports profile bootstrapping and records gateway decisions. A comprehensive Game Design Document was also added. Review feedback focuses on improving Nakama storage efficiency by utilizing version strings from storage writes to eliminate redundant read operations and redundant data normalization.
| writeAgentContext(nk, context, ""); | ||
| var created = readAgentContext(nk, userId); | ||
| if (created) { | ||
| return { | ||
| context: created.value, | ||
| version: created.version | ||
| }; | ||
| } |
There was a problem hiding this comment.
The pattern of performing a storageRead immediately after a storageWrite to retrieve the object version is inefficient. Nakama's storageWrite returns an array of StorageAck objects, each containing the version of the written object. You should update writeAgentContext to return this version and use it directly.
References
- Identify performance bottlenecks and avoid unnecessary operations.
| writeAgentContext(nk, context, existing.version); | ||
| var rewritten = readAgentContext(nk, userId); | ||
| if (rewritten) { | ||
| return { | ||
| context: ensureAgentContext(rewritten.value, userId), | ||
| version: rewritten.version | ||
| }; | ||
| } |
There was a problem hiding this comment.
Similar to the initialization logic, this block performs an unnecessary storageRead after a write. The new version is already available in the return value of nk.storageWrite. Additionally, the call to ensureAgentContext on line 282 is redundant as the context was already normalized before the write.
References
- Avoid redundant calculations and unnecessary I/O operations.
| function writeAgentContext(nk: nkruntime.Nakama, context: any, version: string): void { | ||
| var write: any = { | ||
| collection: collectionAgent, | ||
| key: keyAgentContext, | ||
| userId: context.player.player_id, | ||
| value: context, | ||
| permissionRead: 1, | ||
| permissionWrite: 0 | ||
| }]); | ||
| }; | ||
| if (typeof version === "string") { | ||
| write.version = version; | ||
| } | ||
| nk.storageWrite([write]); | ||
| } |
There was a problem hiding this comment.
This function should return the version string provided by Nakama's storageWrite to allow callers to update their state without an additional read operation.
| function writeAgentContext(nk: nkruntime.Nakama, context: any, version: string): void { | |
| var write: any = { | |
| collection: collectionAgent, | |
| key: keyAgentContext, | |
| userId: context.player.player_id, | |
| value: context, | |
| permissionRead: 1, | |
| permissionWrite: 0 | |
| }]); | |
| }; | |
| if (typeof version === "string") { | |
| write.version = version; | |
| } | |
| nk.storageWrite([write]); | |
| } | |
| function writeAgentContext(nk: nkruntime.Nakama, context: any, version: string): string { | |
| var write: any = { | |
| collection: collectionAgent, | |
| key: keyAgentContext, | |
| userId: context.player.player_id, | |
| value: context, | |
| permissionRead: 1, | |
| permissionWrite: 0 | |
| }; | |
| if (typeof version === "string") { | |
| write.version = version; | |
| } | |
| var acks = nk.storageWrite([write]); | |
| return (acks && acks.length > 0) ? acks[0].version : ""; | |
| } |
1 participant
Summary
agent_runtimecounters and boundedagent_activityhistory to the player profile contextsecondspawn_agent_activity_addfor Unity/client-reported bootstrap and offline-session activityVerification
npm run build && npm testinbackend/nakamago test ./...inbackend/gatewaynpx --yes markdownlint-cli2 README.md ROADMAP.md CHANGELOG.md CONTRIBUTING.md SECURITY.md AGENTS.md .claude/CLAUDE.md "docs/**/*.md"rg -n "—" CHANGELOG.md ROADMAP.md docs backend Unity AGENTS.md .claude/CLAUDE.mdreturned no matchesUnity Smoke Note
Play Mode smoke is blocked by an existing Photon Fusion editor/codegen issue, tracked separately in #7. Console output includes
Invalid AssetDatabase path: /Projects/Second-Spawn/Unity/Assets/Photon/Fusion/CodeGen/Fusion.CodeGen.csand Fusion/iOS Xcode reference noise. This PR does not modify Photon vendor code.