Code review 2026-04-27 — d9b4b85

[Luis85]

2026-04-27 — d9b4b85

Reviewed: 42ede76..d9b4b85 (39 commits)
Blockers: 0 | Majors: 0 | Minors: 3 | Nits: 1

---

• [MINOR] examples/product-demo/src/views/PlayView.vue:11 — hardcoded seed storage key duplicates store's computed key, silent drift risk (shipped in fix(demo): share seed storage key between PlayView and useAgentSession #159)

  details

  Problem: SEED_PERSIST_KEY = 'demo.v2.session.lastSeed.petCare' in PlayView.vue hardcodes the same string that useAgentSession.ts constructs dynamically as `${SEED_STORAGE_KEY_PREFIX}${scenarioId}` ('demo.v2.session.lastSeed.' + 'petCare').

  Why it matters: If SEED_STORAGE_KEY_PREFIX or DEFAULT_SCENARIO_ID ever changes in the store, PlayView.vue's read silently misses the new key, generating a fresh seed on every load and losing session continuity for returning users without any compile-time or test-time signal.

  Fix:

  // examples/product-demo/src/views/PlayView.vue
  -const SEED_PERSIST_KEY = 'demo.v2.session.lastSeed.petCare';
  -
  -function readPersistedSeed(): string | null {
  -  try {
  -    const raw = globalThis.localStorage?.getItem(SEED_PERSIST_KEY);
  -    return typeof raw === 'string' && raw.length > 0 ? raw : null;
  -  } catch { return null; }
  -}

  Export readPersistedSeed(scenarioId) from useAgentSession.ts and call it in PlayView.vue, or expose the storage key constant so both sides stay in sync.

• [MINOR] examples/product-demo/src/cognitionSwitcher.ts:442 — new reasoner active but old learner still wired during await buildLearningLearner (shipped in fix(demo): wire learner before reasoner on cognition swap #162)

  details

  Problem: agent.setReasoner(reasoner) is called at line 442, but activeLearner is not updated until line 453 — after await swapLearner(mode.id, reasoner) on line 448. Any AgentTicked events that fire during the buildLearningLearner async gap score outcomes from the NEW reasoner against the OLD learner.

  Why it matters: On a switch into learning mode the old NoopLearner is still wired; the first N ticks' training observations are silently discarded rather than feeding the new TfjsLearner. On a switch out of learning mode, the outgoing TfjsLearner receives one-to-several misclassified outcomes from the incoming heuristic/BDI/BT reasoner before it is disposed.

  Fix:

  // examples/product-demo/src/cognitionSwitcher.ts (onChange handler)
  -      agent.setReasoner(reasoner);
  -      activeReasoner = reasoner;
  -      activeModeId = mode.id;
  -      disposeIfOwned(previousReasoner);
  -      // Swap learners after the reasoner is in place…
  -      const learner = await swapLearner(mode.id, reasoner);
  +      // Build the new learner BEFORE committing the reasoner swap so
  +      // the agent never runs with a mismatched (reasoner, learner) pair.
  +      const learner = await swapLearner(mode.id, reasoner);
         if (disposed || myEpoch !== changeEpoch) {
  +        disposeLearner(learner);
           disposeIfOwned(reasoner);
           return;
         }
  +      agent.setReasoner(reasoner);
  +      activeReasoner = reasoner;
  +      activeModeId = mode.id;
  +      disposeIfOwned(previousReasoner);

• [MINOR] tests/examples/learningMode.train.test.ts:27 — function attachResetHandler declared before a static import statement on line 38 (shipped in fix(test): move attachResetHandler below static imports #163)

  details

  Problem: The replacement for the deleted mountResetButton import was inserted as a function declaration on line 27, above the surviving import { TEST_BACKEND } from '../setup/tfjsBackend.js' on line 38. ES module import statements are hoisted at parse time so it works at runtime, but the source order (declaration before import) is non-standard, confuses readers, and will fail under any future import/first lint rule.

  Why it matters: This pattern will silently break if the project ever enables eslint-plugin-import's import/first rule, turning a style issue into a blocked build with no obvious connection to the original change. It also signals to readers that the file was edited carelessly, undermining confidence in the surrounding test logic.

  Fix:

  -// `mountResetButton` lived in…
  -function attachResetHandler(agentId: string): void {
  -  …
  -}
   import { TEST_BACKEND } from '../setup/tfjsBackend.js';
  +
  +// `mountResetButton` lived in…
  +function attachResetHandler(agentId: string): void {
  +  …
  +}

  Move the function below all import statements.

• [NIT] scripts/review-fix-parse.mjs:105 — new Date(b.issue.createdAt) - new Date(a.issue.createdAt) relies on implicit Date.valueOf() coercion (shipped in fix(scripts): use Date.getTime() in review-fix sort comparator #164)

  details

  Problem: Arithmetic between two Date objects coerces via valueOf() implicitly.

  Why it matters: TypeScript strict mode would flag this as error TS2362/2363; if this file is ever migrated to .ts or a TypeScript-strict linter is applied to .mjs files, the implicit coercion becomes a build error.

  Fix:

  -  .sort((a, b) => new Date(b.issue.createdAt) - new Date(a.issue.createdAt));
  +  .sort((a, b) => new Date(b.issue.createdAt).getTime() - new Date(a.issue.createdAt).getTime());

---

Counter-argument to my own [MINOR] d9b4b85.1: The 'demo.v2.session.lastSeed.petCare' literal is identical to SEED_STORAGE_KEY_PREFIX + DEFAULT_SCENARIO_ID by construction, and the 'demo.v2.' prefix is tied to the demo's schema version — changing it IS a breaking change that intentionally wipes all stored state regardless, so the duplication can never silently produce a different key without the developer also intending to break compatibility. The SEED_PERSIST_KEY constant documents the full string clearly. That said, two maintenance sites for the same value is still a smell that could bite a future contributor who updates the store's prefix without grepping for consumers, so keeping the finding as MINOR is warranted.

---

• Reviewed range: 42ede764913e45c37fcd6b89cfb22d1f883c3348..d9b4b85cf67ce3eac0c665f3adec2a8043801c4b (39 commits, 192 files)
• Blockers: 0
• Majors: 0
• Minors: 3
• Nits: 1
• Counter-argument check: d9b4b85.1 tested, kept as MINOR (two maintenance sites, argument softens severity but duplication persists)
• Not reviewed: examples/product-demo/test/ Vue component unit tests and e2e placeholder (spot-checked structure only); graphify-out/ generated artifacts; package-lock.json; .claude/memory/; all docs/ sub-trees (archive, plans, specs, dep-triage-bot, docs-review-bot, plan-recon-bot, actions-bump-bot)
• Last reviewed SHA: d9b4b85cf67ce3eac0c665f3adec2a8043801c4b