fix(panels): always fire background RPC refresh after bootstrap render (#2208)

* fix(panels): always fire background RPC refresh after bootstrap render

Bootstrap hydration (getHydratedData) is one-shot — once rendered from
it, panels never refresh and can show stale or partial data indefinitely.

Affected panels: MacroSignals, ETFFlows, Stablecoins, FuelPrices,
GulfEconomies, GroceryBasket, BigMac.

Pattern: render from bootstrap immediately for fast first paint, then
fire a background RPC call that silently updates the panel with live
data. Errors during background refresh are suppressed when bootstrap
data is already visible (no error flash over valid data).

* fix(panels): guard background RPC refresh against empty response overwriting bootstrap

Empty RPC responses (200 + empty array) no longer overwrite valid bootstrap
data with error/unavailable state across all 7 affected panels:

- ETFFlowsPanel, StablecoinPanel: wrap this.data assignment in
  `if (fresh.xxx?.length || !this.data)` guard
- FuelPricesPanel, GulfEconomiesPanel, GroceryBasketPanel, BigMacPanel:
  add `!data.xxx?.length` check in background .then() before calling render
- MacroSignalsPanel: return false early when error suppressed to skip
  redundant renderPanel() call

* fix(hormuz): fix noUncheckedIndexedAccess TypeScript errors

* fix(todos): add blank lines around headings (markdownlint MD022)

* fix(hormuz): add missing hormuz-tracker service + fix implicit any in HormuzPanel

* revert: remove HormuzPanel.ts from this branch (belongs in PR #2210)
This commit is contained in:
Elie Habib
2026-03-24 20:23:40 +04:00
committed by GitHub
parent 78f517036d
commit 092efd4fe9
16 changed files with 624 additions and 15 deletions

View File

@@ -19,6 +19,10 @@ export class BigMacPanel extends Panel {
if (hydrated?.countries?.length) {
if (!this.element?.isConnected) return;
this.renderIndex(hydrated);
void client.listBigMacPrices({}).then(data => {
if (!this.element?.isConnected || !data.countries?.length) return;
this.renderIndex(data);
}).catch(() => {});
return;
}
const data = await client.listBigMacPrices({});

View File

@@ -42,22 +42,33 @@ export class ETFFlowsPanel extends Panel {
this.error = null;
this.loading = false;
this.renderPanel();
void this.refreshFromRpc();
return;
}
await this.refreshFromRpc();
}
private async refreshFromRpc(): Promise<void> {
try {
const client = new MarketServiceClient(getRpcBaseUrl(), { fetch: (...args) => globalThis.fetch(...args) });
this.data = await client.listEtfFlows({});
const fresh = await client.listEtfFlows({});
if (!this.element?.isConnected) return;
this.error = null;
if (fresh.etfs?.length || !this.data) {
this.data = fresh;
this.error = null;
this.loading = false;
this.renderPanel();
}
} catch (err) {
if (this.isAbortError(err)) return;
if (!this.element?.isConnected) return;
console.warn('[ETFFlows] Fetch error:', err);
this.error = t('components.etfFlows.unavailable');
if (!this.data) {
console.warn('[ETFFlows] Fetch error:', err);
this.error = t('components.etfFlows.unavailable');
this.loading = false;
this.renderPanel();
}
}
this.loading = false;
this.renderPanel();
}
private renderPanel(): void {

View File

@@ -19,6 +19,10 @@ export class FuelPricesPanel extends Panel {
if (hydrated?.countries?.length) {
if (!this.element?.isConnected) return;
this.renderIndex(hydrated);
void client.listFuelPrices({}).then(data => {
if (!this.element?.isConnected || !data.countries?.length) return;
this.renderIndex(data);
}).catch(() => {});
return;
}
const data = await client.listFuelPrices({});

View File

@@ -19,6 +19,10 @@ export class GroceryBasketPanel extends Panel {
if (hydrated?.countries?.length) {
if (!this.element?.isConnected) return;
this.renderBasket(hydrated);
void client.listGroceryBasketPrices({}).then(data => {
if (!this.element?.isConnected || !data.countries?.length) return;
this.renderBasket(data);
}).catch(() => {});
return;
}
const data = await client.listGroceryBasketPrices({});

View File

@@ -39,6 +39,10 @@ export class GulfEconomiesPanel extends Panel {
if (hydrated?.quotes?.length) {
if (!this.element?.isConnected) return;
this.renderGulf(hydrated);
void client.listGulfQuotes({}).then(data => {
if (!this.element?.isConnected || !data.quotes?.length) return;
this.renderGulf(data);
}).catch(() => {});
return;
}
const data = await client.listGulfQuotes({});

View File

@@ -145,9 +145,13 @@ export class MacroSignalsPanel extends Panel {
this.error = null;
this.loading = false;
this.renderPanel();
void this.refreshFromRpc();
return true;
}
return this.refreshFromRpc();
}
private async refreshFromRpc(): Promise<boolean> {
try {
const res = await economicClient.getMacroSignals({});
if (!this.element?.isConnected) return false;
@@ -156,12 +160,15 @@ export class MacroSignalsPanel extends Panel {
} catch (err) {
if (this.isAbortError(err)) return false;
if (!this.element?.isConnected) return false;
console.warn('[MacroSignals] Fetch error:', err);
this.error = t('common.noDataShort');
if (!this.data) {
console.warn('[MacroSignals] Fetch error:', err);
this.error = t('common.noDataShort');
} else {
return false;
}
}
this.loading = false;
this.renderPanel();
const ts = this.data?.timestamp ?? '';
const changed = ts !== this.lastTimestamp;
this.lastTimestamp = ts;

View File

@@ -42,22 +42,33 @@ export class StablecoinPanel extends Panel {
this.error = null;
this.loading = false;
this.renderPanel();
void this.refreshFromRpc();
return;
}
await this.refreshFromRpc();
}
private async refreshFromRpc(): Promise<void> {
try {
const client = new MarketServiceClient(getRpcBaseUrl(), { fetch: (...args) => globalThis.fetch(...args) });
this.data = await client.listStablecoinMarkets({ coins: [] });
const fresh = await client.listStablecoinMarkets({ coins: [] });
if (!this.element?.isConnected) return;
this.error = null;
if (fresh.stablecoins?.length || !this.data) {
this.data = fresh;
this.error = null;
this.loading = false;
this.renderPanel();
}
} catch (err) {
if (this.isAbortError(err)) return;
if (!this.element?.isConnected) return;
console.warn('[Stablecoin] Fetch error:', err);
this.error = t('common.noDataShort');
if (!this.data) {
console.warn('[Stablecoin] Fetch error:', err);
this.error = t('common.noDataShort');
this.loading = false;
this.renderPanel();
}
}
this.loading = false;
this.renderPanel();
}
private renderPanel(): void {

View File

@@ -0,0 +1,50 @@
import { toApiUrl } from '@/services/runtime';
export interface HormuzSeries {
date: string;
value: number;
}
export interface HormuzChart {
label: string;
title: string;
series: HormuzSeries[];
}
export interface HormuzTrackerData {
fetchedAt: number;
updatedDate: string;
title: string | null;
summary: string | null;
paragraphs: string[];
status: 'closed' | 'disrupted' | 'restricted' | 'open';
charts: HormuzChart[];
attribution: { source: string; url: string };
}
let cached: HormuzTrackerData | null = null;
let cachedAt = 0;
const CACHE_TTL = 30 * 60 * 1000;
export function getCachedHormuzTracker(): HormuzTrackerData | null {
return cached;
}
export async function fetchHormuzTracker(): Promise<HormuzTrackerData | null> {
const now = Date.now();
if (cached && now - cachedAt < CACHE_TTL) return cached;
try {
const resp = await fetch(toApiUrl('/api/supply-chain/hormuz-tracker'), {
signal: AbortSignal.timeout(10_000),
});
if (!resp.ok) return cached;
const raw = (await resp.json()) as HormuzTrackerData;
if (!Array.isArray(raw.charts)) return cached;
cached = raw;
cachedAt = now;
return cached;
} catch {
return cached;
}
}

View File

@@ -0,0 +1,51 @@
---
status: pending
priority: p1
issue_id: "010"
tags: [code-review, deep-forecast, simulation-package, correctness]
---
# `inferEntityClassFromName` — `|force|` overmatch misclassifies commercial entities as `military_or_security_actor`
## Problem Statement
`inferEntityClassFromName` contains `|force|` as a standalone regex alternative. This matches any entity name containing "force" as a substring — including commercial names like "Salesforce", "Workforce Solutions", "Task Force [commodity]". Such entities are silently classified as `military_or_security_actor` and this misclassification is written verbatim into the `simulation-package.json` R2 artifact, which is consumed by downstream LLMs.
## Findings
- `scripts/seed-forecasts.mjs``inferEntityClassFromName`, first regex branch:
```javascript
if (/military|army|navy|air force|guard|force|houthi|irgc|revolutionary|armed/.test(s)) return 'military_or_security_actor';
```
- `|force|` without word boundaries matches "Salesforce", "Workforce", "Task Force Oil Logistics", etc.
- `air force` (with space) already works correctly — the space is a literal match. The standalone `|force|` is redundant and dangerous.
- Misclassified entity class propagates into exported `simulation-package.json` with no log or warning.
## Proposed Solutions
### Option A: Add word boundaries and explicit "air force" (Recommended)
```javascript
if (/\b(military|army|navy|air\s+force|national\s+guard|houthi|irgc|revolutionary\s+guard|armed\s+forces?)\b/.test(s)) return 'military_or_security_actor';
```
Effort: Tiny | Risk: Low
### Option B: Remove `force` entirely, rely on other terms
Remove `|force|` and `|guard|` (too broad) and add the specific IRGC/Houthi terms already present.
Effort: Tiny | Risk: Low
## Acceptance Criteria
- [ ] `inferEntityClassFromName('Salesforce Inc')` returns `'exporter_or_importer'` or `'market_participant'`, NOT `'military_or_security_actor'`
- [ ] `inferEntityClassFromName('US Air Force')` still returns `'military_or_security_actor'`
- [ ] `inferEntityClassFromName('Houthi armed forces')` still returns `'military_or_security_actor'`
- [ ] New unit tests for all three cases
## Technical Details
- File: `scripts/seed-forecasts.mjs` — `inferEntityClassFromName` function
## Work Log
- 2026-03-24: Found by compound-engineering:review:kieran-typescript-reviewer in PR #2204 review

View File

@@ -0,0 +1,55 @@
---
status: pending
priority: p1
issue_id: "011"
tags: [code-review, deep-forecast, simulation-package, correctness]
---
# Entity key collision: same `dominantRegion` across multiple candidates silently drops entities + wrong `relevanceToTheater`
## Problem Statement
`buildSimulationPackageEntities` uses `su:${actorName}:${candidate.dominantRegion}` and `ev:${name}:${candidate.dominantRegion}` as Map dedup keys. When two selected theater candidates share the same `dominantRegion` (e.g., Bab el-Mandeb and Suez Canal both map to "Red Sea"), an actor appearing in both candidates (e.g., "US Navy Fifth Fleet") produces identical keys. The `addEntity` guard silently drops the second entry, and the kept entity has `relevanceToTheater` pointing to whichever candidate was processed first — not the one it is most relevant to.
## Findings
- `scripts/seed-forecasts.mjs``buildSimulationPackageEntities`, `su:` and `ev:` key construction:
```javascript
const key = `su:${actorName}:${candidate.dominantRegion}`;
// and
const key = `ev:${name}:${candidate.dominantRegion}`;
```
- Red Sea / Bab el-Mandeb / Suez Canal all share `dominantRegion: 'Red Sea'` in `CHOKEPOINT_MARKET_REGIONS`
- The dropped entity causes incorrect `relevanceToTheater` assignment with no log
- This is a data correctness issue in the exported schema — downstream simulators/LLMs get wrong theater attribution
## Proposed Solutions
### Option A: Include `candidateStateId` in the dedup key (Recommended)
```javascript
const key = `su:${actorName}:${candidate.candidateStateId}`;
// and
const key = `ev:${name}:${candidate.candidateStateId}`;
```
Effort: Tiny | Risk: Low — `candidateStateId` is always unique per candidate
### Option B: Use actor name only as dedup key, merge `relevanceToTheater` as array
Allow the same actor to appear once with `relevanceToTheater: ['theater-1', 'theater-2']`. This is semantically richer for Phase 2 but changes the schema contract.
Effort: Small | Risk: Medium (schema change)
## Acceptance Criteria
- [ ] Two candidates with same `dominantRegion` and overlapping actors produce separate entities per candidate in the output
- [ ] Each entity's `relevanceToTheater` correctly references its source candidate
- [ ] `[...seen.values()].slice(0, 20)` cap still applies
- [ ] Test: two theater candidates sharing `dominantRegion: 'Red Sea'`, same actor in `stateSummary.actors` — assert both entities present with correct `relevanceToTheater`
## Technical Details
- File: `scripts/seed-forecasts.mjs` — `buildSimulationPackageEntities`
## Work Log
- 2026-03-24: Found by compound-engineering:review:kieran-typescript-reviewer in PR #2204 review

View File

@@ -0,0 +1,70 @@
---
status: pending
priority: p1
issue_id: "012"
tags: [code-review, deep-forecast, simulation-package, correctness]
---
# `actorRegistry` always `[]` in production — `priorWorldState` not threaded to `writeSimulationPackage` call site
## Problem Statement
`buildSimulationPackageFromDeepSnapshot` accepts `priorWorldState` as its second argument and uses `priorWorldState?.actorRegistry || []` for the highest-fidelity entity extraction path (actorRegistry entries whose `forecastIds` overlap the selected theaters). But the call site in the main seed path never passes `priorWorldState`:
```javascript
writeSimulationPackage(snapshotPayload, { storageConfig: snapshotWrite.storageConfig })
```
`priorWorldState` is available earlier in scope (resolved via `readPreviousForecastWorldState` during snapshot building) but `writeDeepForecastSnapshot` does not return it, so it cannot be passed through. The result: `actorRegistry` is always `[]` in all production runs, the registry-based entity extraction branch is silently dead, and entities degrade to stateUnit actors and evidence table only.
## Findings
- `scripts/seed-forecasts.mjs` — fire-and-forget call site in seed path:
```javascript
const snapshotWrite = await writeDeepForecastSnapshot(snapshotPayload, { runId });
if (snapshotWrite?.storageConfig && ...) {
writeSimulationPackage(snapshotPayload, { storageConfig: snapshotWrite.storageConfig })
```
- `priorWorldState` is in scope earlier but not accessible at this point
- Entity extraction priority (per gap doc): actorRegistry FIRST, then stateUnit actors, then evidence table, then fallback anchors
- The most specific extraction path (forecastId overlap with registry) never runs in production
## Proposed Solutions
### Option A: Return `priorWorldState` from `writeDeepForecastSnapshot` (Recommended)
```javascript
// In writeDeepForecastSnapshot return:
return { storageConfig, snapshotKey, priorWorldState };
// At call site:
writeSimulationPackage(snapshotPayload, {
storageConfig: snapshotWrite.storageConfig,
priorWorldState: snapshotWrite.priorWorldState,
})
```
Effort: Small | Risk: Low
### Option B: Call `writeSimulationPackage` before `writeDeepForecastSnapshot`, where `priorWorldState` is still in scope
Requires restructuring the call order slightly. `writeSimulationPackage` can also accept the already-built snapshot payload.
Effort: Small | Risk: Low
### Option C: Pass `priorWorldState` into the snapshot payload itself
Add `priorWorldState` to `snapshotPayload` and read it in `buildSimulationPackageFromDeepSnapshot`.
Effort: Tiny | Risk: Medium (grows snapshot payload)
## Acceptance Criteria
- [ ] In production runs, `entities[]` contains entries sourced from `actorRegistry` when the registry has relevant actors
- [ ] `priorWorldState.actorRegistry` is passed through to `buildSimulationPackageFromDeepSnapshot`
- [ ] Test: `buildSimulationPackageFromDeepSnapshot(snapshot, { actorRegistry: [{ id: 'actor-1', name: 'Iran', forecastIds: [...], ... }] })` produces entity with `entityId: 'actor-1'` and `relevanceToTheater: 'actor_registry'`
## Technical Details
- File: `scripts/seed-forecasts.mjs` — `writeSimulationPackage` call site in `_isDirectRun` seed path
- Functions: `writeSimulationPackage`, `writeDeepForecastSnapshot`, `buildSimulationPackageFromDeepSnapshot`
## Work Log
- 2026-03-24: Found by compound-engineering:research:learnings-researcher in PR #2204 review

View File

@@ -0,0 +1,64 @@
---
status: pending
priority: p2
issue_id: "013"
tags: [code-review, deep-forecast, simulation-package, security]
---
# LLM-sourced strings enter `simulation-package.json` without `sanitizeForPrompt` — prompt injection risk
## Problem Statement
`buildSimulationRequirementText`, `buildSimulationPackageEventSeeds`, and `buildSimulationPackageEntities` interpolate LLM-generated strings directly into R2 artifact fields with no sanitization. The rest of `seed-forecasts.mjs` applies `sanitizeForPrompt()` to all LLM-derived strings before they enter prompts or Redis. The simulation package is explicitly designed to be consumed by downstream LLMs (MiroFish, scenario-analysis workflows), so unsanitized content is a stored prompt injection vector.
## Findings
**F-1 (HIGH):** `theater.label` (`candidateStateLabel`) used directly in `simulationRequirement` string:
```javascript
return `Simulate how a ${theater.label} (${theater.stateKind || 'disruption'} at ${route}${commodity})...`;
```
`candidateStateLabel` derives from LLM-generated cluster labels via `formatStateUnitLabel`.
**F-6 (MEDIUM):** `theater.topChannel` and `critTypes` also interpolated — these derive from LLM-generated market context and signal types. `replace(/_/g, ' ')` is presentational, not a security control. A value `ignore_previous_instructions` becomes `ignore previous instructions`.
**F-2 (MEDIUM):** `entry.text.slice(0, 200)` in event seeds — LLM evidence table text sliced but not stripped of injection patterns.
**F-3 (MEDIUM):** Actor names split from `entry.text` go directly into `name:` field and `entityId` slug with no sanitization.
## Proposed Solutions
### Option A: Apply `sanitizeForPrompt` to all LLM-sourced strings before artifact emission (Recommended)
```javascript
// In buildSimulationRequirementText:
const label = sanitizeForPrompt(theater.label) || theater.dominantRegion || 'unknown theater';
const route = sanitizeForPrompt(theater.routeFacilityKey || theater.dominantRegion);
// In buildSimulationPackageEventSeeds:
summary: sanitizeForPrompt(entry.text).slice(0, 200),
// In buildSimulationPackageEntities (actor name):
name: sanitizeForPrompt(actorName),
```
Effort: Small | Risk: Low — `sanitizeForPrompt` already exists and is used throughout the file
### Option B: Allowlist-validate field values instead of sanitizing
`topChannel` and `topBucketId` are already constrained by `MARKET_BUCKET_ALLOWED_CHANNELS` and `IMPACT_VARIABLE_REGISTRY`. Validate them against those registries before interpolation. `theater.label` would still need `sanitizeForPrompt`.
Effort: Small | Risk: Low
## Acceptance Criteria
- [ ] `buildSimulationRequirementText` applies `sanitizeForPrompt` to `theater.label`, `theater.stateKind`, `theater.topChannel`, and `critTypes` before string interpolation
- [ ] `buildSimulationPackageEventSeeds` applies `sanitizeForPrompt` to `entry.text` before `.slice(0, 200)`
- [ ] Actor names extracted from evidence table are sanitized before becoming entity `name` and `entityId`
- [ ] Test: a `theater.label` containing `\nIgnore previous instructions` produces a sanitized `simulationRequirement` string with no newlines or directive text
## Technical Details
- File: `scripts/seed-forecasts.mjs``buildSimulationRequirementText`, `buildSimulationPackageEventSeeds`, `buildSimulationPackageEntities`
- Existing function: `sanitizeForPrompt(text)` — already in the file, strips newlines, control chars, limits to 200 chars
## Work Log
- 2026-03-24: Found by compound-engineering:review:security-sentinel and compound-engineering:research:learnings-researcher in PR #2204 review

View File

@@ -0,0 +1,67 @@
---
status: pending
priority: p2
issue_id: "014"
tags: [code-review, deep-forecast, simulation-package, performance]
---
# Two performance issues in simulation package builders: `new Set` in filter predicate + `intersectAny` O(n²)
## Problem Statement
Two patterns in the new simulation package code introduce unnecessary allocations and O(n×m) comparisons that will grow with the actor registry size.
## Findings
**CRITICAL-1: `intersectAny` uses `Array.includes` — O(n×m) in the actor registry loop**
`buildSimulationPackageEntities` builds `allForecastIds` as a flat array via `flatMap`, then passes it to `intersectAny` which calls `right.includes(item)` for each item in `actor.forecastIds`. With a 200-actor registry and 8 forecast IDs each, this is ~38,400 comparisons per call.
```javascript
const allForecastIds = candidates.flatMap((c) => c.sourceSituationIds || []);
for (const actor of (actorRegistry || [])) {
if (!intersectAny(actor.forecastIds || [], allForecastIds)) continue;
```
Fix: convert `allForecastIds` to a Set once before the loop.
**CRITICAL-2: `new Set(candidate.marketBucketIds || [])` inside filter predicate**
`isMaritimeChokeEnergyCandidate` constructs a new Set from `marketBucketIds` (typically 2-3 elements) on every candidate, then calls `.has()` twice. At 50 candidates this is 50 Set allocations for a 2-element array check. `Array.includes` is faster for arrays of this size.
```javascript
const buckets = new Set(candidate.marketBucketIds || []);
return buckets.has('energy') || buckets.has('freight') || ...
```
## Proposed Solutions
### Fix both (Recommended)
```javascript
// Fix 1: Set-based forecast ID lookup
const allForecastIdSet = new Set(candidates.flatMap((c) => c.sourceSituationIds || []));
for (const actor of (actorRegistry || [])) {
if (!(actor.forecastIds || []).some((id) => allForecastIdSet.has(id))) continue;
// Fix 2: Array.includes instead of Set for small arrays
const bucketArr = candidate.marketBucketIds || [];
return bucketArr.includes('energy') || bucketArr.includes('freight') || topBucket === 'energy' || topBucket === 'freight'
|| SIMULATION_ENERGY_COMMODITY_KEYS.has(candidate.commodityKey || '');
```
Effort: Tiny | Risk: Low
## Acceptance Criteria
- [ ] `allForecastIds` is a `Set` before the actor registry loop in `buildSimulationPackageEntities`
- [ ] `isMaritimeChokeEnergyCandidate` uses `Array.includes` or `Array.some` instead of `new Set` for `marketBucketIds` check
- [ ] All existing simulation package tests still pass
## Technical Details
- File: `scripts/seed-forecasts.mjs``isMaritimeChokeEnergyCandidate`, `buildSimulationPackageEntities`
## Work Log
- 2026-03-24: Found by compound-engineering:review:performance-oracle in PR #2204 review

View File

@@ -0,0 +1,76 @@
---
status: pending
priority: p2
issue_id: "015"
tags: [code-review, deep-forecast, simulation-package, correctness]
---
# Two missing null guards + `theater.label` undefined in simulation package builders
## Problem Statement
Three correctness gaps in the new simulation package builders produce silent degradation or misleading output without any log.
## Findings
**1. `buildSimulationPackageEvaluationTargets`: no guard when `candidate` is `undefined`**
```javascript
const candidate = candidates.find((c) => c.candidateStateId === theater.candidateStateId);
// No guard here — if candidate is undefined, actors silently falls back to 'key actors'
const actors = (candidate?.stateSummary?.actors || []).slice(0, 3).join(', ') || 'key actors';
```
`buildSimulationPackageConstraints` has an explicit `if (!candidate) continue` guard for the same `find()` pattern. `buildSimulationPackageEvaluationTargets` does not. Silent degradation produces a valid-looking evaluation target with generic actor text and no diagnostic.
**2. `theater.label` has no fallback — produces `"Simulate how a undefined (...)"` when `candidateStateLabel` is missing**
```javascript
label: c.candidateStateLabel, // no fallback
// later:
return `Simulate how a ${theater.label} (${theater.stateKind || 'disruption'}...`;
```
If `candidateStateLabel` is absent, the `simulationRequirement` string contains literal "undefined". This propagates into R2 and downstream LLM consumers.
**3. `buildSimulationStructuralWorld`: `s.macroRegion` (singular) matched against `theaterRegions` built from `c.macroRegions` (plural array)**
If the signal schema stores an array in `macroRegion`, the `Set.has()` lookup against an array reference silently returns false for all such signals, producing empty `touchingSignals`. This code path has no test coverage (tests pass `signals: []`).
## Proposed Solutions
```javascript
// Fix 1: add candidate guard in buildSimulationPackageEvaluationTargets
const candidate = candidates.find((c) => c.candidateStateId === theater.candidateStateId);
if (!candidate) {
console.warn(`[SimulationPackage] No candidate for theaterId=${theater.theaterId} (evaluationTargets)`);
}
// Fix 2: label fallback in selectedTheaters map
label: c.candidateStateLabel || c.dominantRegion || 'unknown theater',
// Fix 3: handle both singular and array macroRegion in signal filter
.filter((s) => {
const sigMacro = s.macroRegion;
return theaterRegions.has(s.region)
|| (Array.isArray(sigMacro) ? sigMacro.some((r) => theaterRegions.has(r)) : theaterRegions.has(sigMacro))
|| theaterStateIds.has(s.situationId);
})
```
Effort: Small | Risk: Low
## Acceptance Criteria
- [ ] `buildSimulationPackageEvaluationTargets` logs a warn when candidate is undefined for a theater
- [ ] `theater.label` is never `undefined` — falls back to `dominantRegion` or `'unknown theater'`
- [ ] `buildSimulationStructuralWorld` handles both singular string and array `macroRegion` on signals
- [ ] Test: a theater with `candidateStateLabel: undefined` produces a `simulationRequirement` that does NOT contain the string `"undefined"`
## Technical Details
- File: `scripts/seed-forecasts.mjs``buildSimulationPackageEvaluationTargets`, `buildSimulationStructuralWorld`, `selectedTheaters` map in `buildSimulationPackageFromDeepSnapshot`
## Work Log
- 2026-03-24: Found by compound-engineering:review:kieran-typescript-reviewer in PR #2204 review

View File

@@ -0,0 +1,72 @@
---
status: pending
priority: p3
issue_id: "016"
tags: [code-review, deep-forecast, simulation-package, quality]
---
# Simplification opportunities in simulation package builders — duplicated patterns + YAGNI items
## Problem Statement
Several mechanical duplications and one YAGNI issue in the new simulation package code (~30 lines of avoidable noise in a 405-line addition).
## Findings
**1. `candidates.find()` duplicated 3x across builder functions**
`buildSimulationPackageEventSeeds`, `buildSimulationPackageConstraints`, `buildSimulationPackageEvaluationTargets` each do `candidates.find((c) => c.candidateStateId === theater.candidateStateId)` per theater loop. Pre-building a `Map` once in the orchestrator and passing it down eliminates 6 `find()` calls.
**2. `entry.text.slice(0, 200)` repeated 3x in `buildSimulationPackageEventSeeds`**
Extract `const MAX_SEED_SUMMARY = 200`.
**3. `name.toLowerCase().replace(/\W+/g, '_')` repeated 4x for slugifying entity IDs**
Extract `const slugify = (s) => s.toLowerCase().replace(/\W+/g, '_')`.
**4. Fallback entity block: 3 explicit `addEntity(...)` calls per theater with identical shape**
A data-driven `FALLBACK_ANCHOR_DEFS` array reduces the block from ~30 lines to ~10 and makes adding a fourth anchor class trivial.
**5. `gateDetails` in debug payload hardcodes threshold values instead of reading from `getImpactValidationFloors()`**
```javascript
gateDetails: { secondOrderMappedFloor: 0.58, ... }
```
These are already the live values in `getImpactValidationFloors('second_order')`. When thresholds change again, `gateDetails` silently shows stale values. Read from the function instead.
**6. Actor extraction regex too narrow**
`/^(.+?)\s+remain the lead actors/i` — misses "are the primary actors", "continue as the key actors". When this misses, there is no log. Add: `console.debug('[SimulationPackage] evidence actor regex miss', entry.text.slice(0, 80))` so the fallback rate is observable.
## Proposed Solutions
```javascript
// In buildSimulationPackageFromDeepSnapshot, before calling builders:
const candidateById = new Map(top.map((c) => [c.candidateStateId, c]));
// Extract helpers near section header:
const slugify = (s) => s.toLowerCase().replace(/\W+/g, '_');
const MAX_SEED_SUMMARY = 200;
// gateDetails reads live values:
const secondOrderFloors = getImpactValidationFloors('second_order');
gateDetails: {
secondOrderMappedFloor: secondOrderFloors.mapped,
secondOrderMultiplier: secondOrderFloors.multiplier,
pathScoreThreshold: 0.50,
acceptanceThreshold: 0.60,
},
```
Effort: Small | Risk: Low
## Acceptance Criteria
- [ ] `slugify` and `MAX_SEED_SUMMARY` extracted as file-local constants
- [ ] `candidateById` Map built once in `buildSimulationPackageFromDeepSnapshot`, passed to builders
- [ ] `gateDetails` reads from `getImpactValidationFloors()` rather than hardcoding values
- [ ] All existing simulation package tests still pass
## Technical Details
- File: `scripts/seed-forecasts.mjs` — simulation package section (~line 1172012100)
## Work Log
- 2026-03-24: Found by compound-engineering:review:code-simplicity-reviewer in PR #2204 review

View File

@@ -0,0 +1,59 @@
---
status: pending
priority: p3
issue_id: "017"
tags: [code-review, deep-forecast, simulation-package, architecture]
---
# Phase 2 prerequisites: `getSimulationPackage` RPC + Redis existence key
## Problem Statement
The simulation package is a write-only black box: agents cannot read it, trigger it, verify its existence, or discover its schema through any runtime interface. This is acceptable for Phase 1, but Phase 2 (MiroFish integration) requires a read path before it can proceed.
## Findings
From the agent-native review:
- **0/4 simulation-package capabilities are agent-accessible** (trigger, read, check existence, discover schema)
- There is no `getSimulationPackage(runId)` RPC handler in `server/worldmonitor/forecast/v1/`
- The R2 key is deterministic and computable from `(runId, generatedAt)` but no handler exposes it
- `schemaVersion` is written as R2 object metadata but never returned through any read path
- `writeSimulationPackage` returns `{ pkgKey, theaterCount }` but this result is discarded at the fire-and-forget call site — nothing writes a Redis existence key
Phase 2 gate: MiroFish or any LLM scenario-analysis workflow that consumes the package must reach it through the server layer, not by directly importing `buildSimulationPackageKey` from the seed script.
## Proposed Solutions
### Option A: Add `getSimulationPackage(runId)` RPC (Recommended for Phase 2)
Create `server/worldmonitor/forecast/v1/get-simulation-package.ts` that reads from R2 using `buildSimulationPackageKey(runId, generatedAt)`. Follows the same pattern as the deep-snapshot replay handler.
### Option B: Write Redis existence key on successful write
When `writeSimulationPackage` resolves successfully, write a Redis key:
```
forecast:simulation-package:latest → { runId, pkgKey, schemaVersion, theaterCount, generatedAt }
```
This gives agents a cheap existence check and gives health monitoring a probe point at zero R2 cost.
Both options are Phase 2 work, not Phase 1 blockers.
## Acceptance Criteria (Phase 2)
- [ ] `getSimulationPackage(runId)` RPC handler exists in `server/worldmonitor/forecast/v1/`
- [ ] Handler reads from R2 using `buildSimulationPackageKey`
- [ ] `schemaVersion` is included in the RPC response
- [ ] Redis key `forecast:simulation-package:latest` written on successful `writeSimulationPackage`
- [ ] Health check or bootstrap key added for existence monitoring
## Technical Details
- New file needed: `server/worldmonitor/forecast/v1/get-simulation-package.ts`
- Wire in: `server/worldmonitor/handler.ts` (gateway registration)
- Follow pattern of: `server/worldmonitor/forecast/v1/get-forecasts.ts`
## Work Log
- 2026-03-24: Found by compound-engineering:review:agent-native-reviewer in PR #2204 review
- Phase 1 only — do not block PR #2204 merge on this item