Files
worldmonitor/todos/022-complete-p2-simulation-package-runid-mismatch-not-checked.md
Elie Habib 01f6057389 feat(simulation): MiroFish Phase 2 — theater-limited simulation runner (#2220)
* feat(simulation): MiroFish Phase 2 — theater-limited simulation runner

Adds the simulation execution layer that consumes simulation-package.json
and produces simulation-outcome.json for maritime chokepoint + energy/logistics
theaters, closing the WorldMonitor → MiroFish handoff loop.

Changes:
- scripts/seed-forecasts.mjs: 2-round LLM simulation runner (prompt builders,
  JSON extractor, runTheaterSimulation, writeSimulationOutcome, task queue
  with NX dedup lock, runSimulationWorker poll loop)
- scripts/process-simulation-tasks.mjs: standalone worker entry point
- proto: GetSimulationOutcome RPC + make generate
- server/worldmonitor/forecast/v1/get-simulation-outcome.ts: RPC handler
- server/gateway.ts: slow tier for get-simulation-outcome
- api/health.js: simulationOutcomeLatest in STANDALONE + ON_DEMAND keys
- tests: 14 new tests for simulation runner functions

* fix(simulation): address P1/P2 code review findings from PR #2220

Security (P1 #018):
- sanitizeForPrompt() applied to all entity/seed fields interpolated into
  Round 1 prompt (entityId, class, stance, seedId, type, timing)
- sanitizeForPrompt() applied to actorId and entityIds in Round 2 prompt
- sanitizeForPrompt() + length caps applied to all LLM array fields written
  to R2 (dominantReactions, stabilizers, invalidators, keyActors, timingMarkers)

Validation (P1 #019):
- Added validateRunId() regex guard
- Applied in enqueueSimulationTask() and processNextSimulationTask() loop

Type safety (P1 #020):
- Added isOutcomePointer() and isPackagePointer() type guards in TS handlers
- Replaced unsafe as-casts with runtime-validated guards in both handlers

Correctness (P2 #022):
- Log warning when pkgPointer.runId does not match task runId

Architecture (P2 #024):
- isMaritimeChokeEnergyCandidate() accepts both flat and nested topBucketId
- Call site simplified to pass theater directly

Performance (P2 #025):
- SIMULATION_ROUND1_MAX_TOKENS raised 1800 to 2200
- Added max 3 initialReactions instruction to Round 1 prompt

Maintainability (P2 #026):
- Simulation pointer keys exported from server/_shared/cache-keys.ts
- Both TS handlers import from shared location

Documentation (P2 #027):
- Strengthened runId no-op description in proto and OpenAPI spec

* fix(todos): add blank lines around lists in markdown todo files

* style(api): reformat openapi yaml to match linter output

* test(simulation): add flat-shape filter test + getSimulationOutcome handler coverage

Two tests identified as missing during PR #2220 review:

1. isMaritimeChokeEnergyCandidate flat-shape tests — covers the || candidate.topBucketId
   normalization added in the P1/P2 review pass. The existing tests only used the nested
   marketContext.topBucketId shape; this adds the flat root-field shape that arrives from
   the simulation-package.json JSON (selectedTheaters entries have topBucketId at root).

2. getSimulationOutcome handler structural tests — verifies the isOutcomePointer guard,
   found:false NOT_FOUND return, found:true success path, note population on runId mismatch,
   and redis_unavailable error string. Follows the readSrc static-analysis pattern used
   elsewhere in server-handlers.test.mjs (handler imports Redis so full integration test
   would require a test Redis instance).
2026-03-25 13:55:59 +04:00

2.8 KiB

status, priority, issue_id, tags
status priority issue_id tags
complete p2 022
code-review
architecture
simulation-runner
correctness

pkgPointer.runId never compared to task runId — can silently simulate wrong package

Problem Statement

In processNextSimulationTask, after claiming a task for runId=A, the code reads SIMULATION_PACKAGE_LATEST_KEY which returns the latest package pointer — not necessarily the one for run A. If a new simulation package for run B is written to Redis while task A is still queued, the worker picks up task A but processes run B's package data. The outcome is written under run A's runId but contains run B content. No warning is logged, no error is returned. This is especially relevant in Phase 3 when per-run lookup becomes active.

Findings

F-1 (HIGH): pkgPointer.runId is read but never compared to the task's runId:

// scripts/seed-forecasts.mjs ~line 15697
const pkgPointer = await redisGet(url, token, SIMULATION_PACKAGE_LATEST_KEY);
if (!pkgPointer?.pkgKey) { ... return { status: 'failed', reason: 'no_package_pointer' }; }
// Missing: if (pkgPointer.runId && pkgPointer.runId !== runId) { ... abort ... }

const pkgData = await getR2JsonObject(storageConfig, pkgPointer.pkgKey);
// pkgData.runId !== runId — proceeds to simulate and write outcome under wrong runId

Proposed Solutions

const pkgPointer = await redisGet(url, token, SIMULATION_PACKAGE_LATEST_KEY);
if (!pkgPointer?.pkgKey) { ... return failed; }

// Guard: skip if package is for a different run
if (pkgPointer.runId && pkgPointer.runId !== runId) {
  console.warn(`  [Simulation] Package mismatch: task=${runId} pkg=${pkgPointer.runId} — skipping`);
  await completeSimulationTask(runId);
  return { status: 'skipped', reason: 'package_run_mismatch', runId };
}

This is non-breaking: if pkgPointer.runId is absent (old format), the guard is skipped and behavior is unchanged.

Effort: Small | Risk: Low

Option B: Accept current behavior, document explicitly

Add a comment explaining that "latest wins" is intentional and document the Phase 3 migration path. Safe for Phase 2 where only one run stream exists.

Acceptance Criteria

  • Guard added: if pkgPointer.runId !== runId, task is completed and { status: 'skipped', reason: 'package_run_mismatch' } returned
  • Log line emitted on mismatch for operational visibility
  • Test: enqueue task for runId A, set package pointer to runId B — processNextSimulationTask returns skipped/package_run_mismatch

Technical Details

  • File: scripts/seed-forecasts.mjsprocessNextSimulationTask (~line 15697)

Work Log

  • 2026-03-24: Found by compound-engineering:review:architecture-strategist in PR #2220 review