From 092efd4fe98005dcb7f79c2bf479ba9a9762adda Mon Sep 17 00:00:00 2001 From: Elie Habib Date: Tue, 24 Mar 2026 20:23:40 +0400 Subject: [PATCH] fix(panels): always fire background RPC refresh after bootstrap render (#2208) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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) --- src/components/BigMacPanel.ts | 4 + src/components/ETFFlowsPanel.ts | 23 ++++-- src/components/FuelPricesPanel.ts | 4 + src/components/GroceryBasketPanel.ts | 4 + src/components/GulfEconomiesPanel.ts | 4 + src/components/MacroSignalsPanel.ts | 13 +++- src/components/StablecoinPanel.ts | 23 ++++-- src/services/hormuz-tracker.ts | 50 ++++++++++++ ...nferentityclassfromname-force-overmatch.md | 51 +++++++++++++ ...-p1-entity-key-collision-same-domregion.md | 55 ++++++++++++++ ...nding-p1-actorregistry-dead-at-callsite.md | 70 +++++++++++++++++ ...-p2-simulation-package-prompt-injection.md | 64 ++++++++++++++++ ...imulation-package-perf-set-intersectany.md | 67 ++++++++++++++++ ...ng-p2-simulation-package-missing-guards.md | 76 +++++++++++++++++++ ...g-p3-simulation-package-simplifications.md | 72 ++++++++++++++++++ ...simulation-package-phase2-prerequisites.md | 59 ++++++++++++++ 16 files changed, 624 insertions(+), 15 deletions(-) create mode 100644 src/services/hormuz-tracker.ts create mode 100644 todos/010-pending-p1-inferentityclassfromname-force-overmatch.md create mode 100644 todos/011-pending-p1-entity-key-collision-same-domregion.md create mode 100644 todos/012-pending-p1-actorregistry-dead-at-callsite.md create mode 100644 todos/013-pending-p2-simulation-package-prompt-injection.md create mode 100644 todos/014-pending-p2-simulation-package-perf-set-intersectany.md create mode 100644 todos/015-pending-p2-simulation-package-missing-guards.md create mode 100644 todos/016-pending-p3-simulation-package-simplifications.md create mode 100644 todos/017-pending-p3-simulation-package-phase2-prerequisites.md diff --git a/src/components/BigMacPanel.ts b/src/components/BigMacPanel.ts index badd5793c..04a5adc02 100644 --- a/src/components/BigMacPanel.ts +++ b/src/components/BigMacPanel.ts @@ -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({}); diff --git a/src/components/ETFFlowsPanel.ts b/src/components/ETFFlowsPanel.ts index c3af0c07b..746e4eca1 100644 --- a/src/components/ETFFlowsPanel.ts +++ b/src/components/ETFFlowsPanel.ts @@ -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 { 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 { diff --git a/src/components/FuelPricesPanel.ts b/src/components/FuelPricesPanel.ts index 8775f60be..2a4e4e306 100644 --- a/src/components/FuelPricesPanel.ts +++ b/src/components/FuelPricesPanel.ts @@ -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({}); diff --git a/src/components/GroceryBasketPanel.ts b/src/components/GroceryBasketPanel.ts index 09cdeb70f..269d2dc0f 100644 --- a/src/components/GroceryBasketPanel.ts +++ b/src/components/GroceryBasketPanel.ts @@ -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({}); diff --git a/src/components/GulfEconomiesPanel.ts b/src/components/GulfEconomiesPanel.ts index 890851043..643cfe2ae 100644 --- a/src/components/GulfEconomiesPanel.ts +++ b/src/components/GulfEconomiesPanel.ts @@ -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({}); diff --git a/src/components/MacroSignalsPanel.ts b/src/components/MacroSignalsPanel.ts index 05a4b0d62..c8aae3f3e 100644 --- a/src/components/MacroSignalsPanel.ts +++ b/src/components/MacroSignalsPanel.ts @@ -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 { 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; diff --git a/src/components/StablecoinPanel.ts b/src/components/StablecoinPanel.ts index 14a102bfb..bbd14d21b 100644 --- a/src/components/StablecoinPanel.ts +++ b/src/components/StablecoinPanel.ts @@ -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 { 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 { diff --git a/src/services/hormuz-tracker.ts b/src/services/hormuz-tracker.ts new file mode 100644 index 000000000..18a120b7e --- /dev/null +++ b/src/services/hormuz-tracker.ts @@ -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 { + 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; + } +} diff --git a/todos/010-pending-p1-inferentityclassfromname-force-overmatch.md b/todos/010-pending-p1-inferentityclassfromname-force-overmatch.md new file mode 100644 index 000000000..fc018ef3a --- /dev/null +++ b/todos/010-pending-p1-inferentityclassfromname-force-overmatch.md @@ -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 diff --git a/todos/011-pending-p1-entity-key-collision-same-domregion.md b/todos/011-pending-p1-entity-key-collision-same-domregion.md new file mode 100644 index 000000000..4d2a668bc --- /dev/null +++ b/todos/011-pending-p1-entity-key-collision-same-domregion.md @@ -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 diff --git a/todos/012-pending-p1-actorregistry-dead-at-callsite.md b/todos/012-pending-p1-actorregistry-dead-at-callsite.md new file mode 100644 index 000000000..50e70e701 --- /dev/null +++ b/todos/012-pending-p1-actorregistry-dead-at-callsite.md @@ -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 diff --git a/todos/013-pending-p2-simulation-package-prompt-injection.md b/todos/013-pending-p2-simulation-package-prompt-injection.md new file mode 100644 index 000000000..e17c43299 --- /dev/null +++ b/todos/013-pending-p2-simulation-package-prompt-injection.md @@ -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 diff --git a/todos/014-pending-p2-simulation-package-perf-set-intersectany.md b/todos/014-pending-p2-simulation-package-perf-set-intersectany.md new file mode 100644 index 000000000..1ad1c5909 --- /dev/null +++ b/todos/014-pending-p2-simulation-package-perf-set-intersectany.md @@ -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 diff --git a/todos/015-pending-p2-simulation-package-missing-guards.md b/todos/015-pending-p2-simulation-package-missing-guards.md new file mode 100644 index 000000000..657c3561e --- /dev/null +++ b/todos/015-pending-p2-simulation-package-missing-guards.md @@ -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 diff --git a/todos/016-pending-p3-simulation-package-simplifications.md b/todos/016-pending-p3-simulation-package-simplifications.md new file mode 100644 index 000000000..74021d866 --- /dev/null +++ b/todos/016-pending-p3-simulation-package-simplifications.md @@ -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 11720–12100) + +## Work Log + +- 2026-03-24: Found by compound-engineering:review:code-simplicity-reviewer in PR #2204 review diff --git a/todos/017-pending-p3-simulation-package-phase2-prerequisites.md b/todos/017-pending-p3-simulation-package-phase2-prerequisites.md new file mode 100644 index 000000000..19e13cf47 --- /dev/null +++ b/todos/017-pending-p3-simulation-package-phase2-prerequisites.md @@ -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