From 28ea5da10371cf25d3760ef529d33f7784ea771c Mon Sep 17 00:00:00 2001 From: Fini Date: Sun, 10 May 2026 15:25:00 +0800 Subject: [PATCH] fix(ai): memoize isMobileFullScreen per plan via WeakMap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-hook caught: even after the orchestrator-level reuse fix (a720aac1), `orchestrator-sub-agent.ts` still calls `isMobileFullScreen(plan)` independently in 2 places (L374 in executeSubAgent + L734 in buildSubAgentUserPrompt). Both run AFTER the orchestrator stripped the status-bar subtask, so they see a smaller subtask count than the orchestrator's pre-strip classify. A 2-subtask [status-bar, content] plan would flip from "mobile" → "not-mobile" across the strip, and sub-agent prompt builders would then disagree with the orchestrator about chrome handling — sub- agent emits its own status bar / wraps in a phone mockup. Architectural fix: classify ONCE per plan and memoize the result on a WeakMap keyed by the plan object. Subsequent calls (whether from orchestrator, executeSubAgent, or buildSubAgentUserPrompt) return the cached pre-mutation answer. WeakMap avoids polluting the public OrchestratorPlan type and lets the cache vacate naturally when the plan goes out of scope. This subsumes the orchestrator.ts L838 local-reuse fix from a720aac1 — that path is now safe via memo too — but the explicit reuse is retained as defense-in-depth + readability (clear that the same classification value is used at two adjacent call sites). Tests: 2 new cases — mutation-survives-classify + per-plan isolation. Existing 7 cases continue to pass. --- .../orchestrator-plan-classify.test.ts | 26 +++++++++++++++++++ .../services/ai/orchestrator-plan-classify.ts | 25 ++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/apps/web/src/services/ai/__tests__/orchestrator-plan-classify.test.ts b/apps/web/src/services/ai/__tests__/orchestrator-plan-classify.test.ts index 4fb448dbb..91a371936 100644 --- a/apps/web/src/services/ai/__tests__/orchestrator-plan-classify.test.ts +++ b/apps/web/src/services/ai/__tests__/orchestrator-plan-classify.test.ts @@ -52,4 +52,30 @@ describe('isMobileFullScreen', () => { expect(isMobileFullScreen(plan(480, 812, 5))).toBe(true); expect(isMobileFullScreen(plan(481, 812, 5))).toBe(false); }); + + // 2026-05-10 Codex stop-hook — orchestrator strips status-bar subtasks + // mid-pipeline, then sub-agents re-classify the mutated plan and would + // get a different answer (smaller subtask count). The memo pins the + // first-call result so all downstream consumers agree. + + it('memoizes per plan — subtask mutation does not flip classification', () => { + const p = plan(375, 0, 2); // narrow + height-0 + 2 subtasks → mobile + expect(isMobileFullScreen(p)).toBe(true); + // Simulate the orchestrator stripping status-bar subtask: + p.subtasks.pop(); + expect(p.subtasks.length).toBe(1); + // Second call sees post-strip plan but returns cached pre-strip result. + expect(isMobileFullScreen(p)).toBe(true); + }); + + it('different plan instances are classified independently (no cross-pollination)', () => { + const p1 = plan(375, 0, 2); // mobile + const p2 = plan(375, 0, 1); // Type 0 component + expect(isMobileFullScreen(p1)).toBe(true); + expect(isMobileFullScreen(p2)).toBe(false); + // Mutating p1 doesn't change p2's cached result. + p1.subtasks.pop(); + expect(isMobileFullScreen(p1)).toBe(true); + expect(isMobileFullScreen(p2)).toBe(false); + }); }); diff --git a/apps/web/src/services/ai/orchestrator-plan-classify.ts b/apps/web/src/services/ai/orchestrator-plan-classify.ts index a13e94d4e..6002a2533 100644 --- a/apps/web/src/services/ai/orchestrator-plan-classify.ts +++ b/apps/web/src/services/ai/orchestrator-plan-classify.ts @@ -1,5 +1,22 @@ import type { OrchestratorPlan } from './ai-types'; +/** + * Memoize the classification on a per-plan basis so call sites that hit + * the helper AFTER the orchestrator strips the status-bar subtask still + * see the original answer. The strip is a real mutation + * (`plan.subtasks = plan.subtasks.filter(...)`), so the second classify + * has a smaller subtask count than the first. Without this memo, a plan + * that came in as [status-bar, content] (2 items, height=0 from LLM + * emitting a non-numeric height) flips from "mobile" → "not-mobile" + * across the strip, and downstream consumers (sub-agent prompt + * builders, status-bar injection branches) disagree about chrome + * handling. Codex stop-hook 2026-05-10 caught this. + * + * WeakMap keeps the memo out of the plan's type surface and lets it + * vanish naturally when the plan goes out of scope. + */ +const memo = new WeakMap(); + /** * A plan represents a full mobile screen only when the root frame is narrow * AND tall. Narrow + auto-height (or small fixed height) is a Type 0 component @@ -14,6 +31,14 @@ import type { OrchestratorPlan } from './ai-types'; * 2026-05-09). */ export function isMobileFullScreen(plan: OrchestratorPlan): boolean { + const cached = memo.get(plan); + if (cached !== undefined) return cached; + const result = computeIsMobileFullScreen(plan); + memo.set(plan, result); + return result; +} + +function computeIsMobileFullScreen(plan: OrchestratorPlan): boolean { if (plan.rootFrame.width > 480) return false; if (plan.rootFrame.height >= 480) return true; // Width-narrow + height-zero/auto: distinguish a single Type 0