fix(ai): memoize isMobileFullScreen per plan via WeakMap
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.
This commit is contained in:
parent
0f30526676
commit
28ea5da103
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<OrchestratorPlan, boolean>();
|
||||
|
||||
/**
|
||||
* 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
|
||||
|
|
|
|||
Loading…
Reference in a new issue