Apply 5 patches from Codex Phase A Gate round 2 review against spec
v19.1 (FROZEN at openpencil-docs commit 526791f):
- BLOCK 1: `SharedSkiaContext::new(provider) -> Result<Self>` single-arg
per spec §3.3. Provider owns surface configuration; constructor queries
GL viewport / sample count / stencil bits via glow after make_current
returns (option C — no trait change, no caller-side `SurfaceConfig`).
`dpi` field on `SurfaceConfig` was dead and is dropped.
- BLOCK 2(a): `glow()` returns `Option<&Arc<glow::Context>>` (borrow,
not clone) per spec §3.3. Hot-path callers clone explicitly.
- BLOCK 2(b): mobile `on_pause` drops `glow_handle` alongside surface
per spec §3.4 — backing GL context is invalid once activity backgrounds.
- CONCERN 1: `default_framebuffer_id` is now a required trait method
(no default body); explicit overrides on `GlutinProvider` (0),
`EglPbufferProvider` (0), `EaglProvider` (unimplemented! Step 1f),
`AndroidEglProvider` (0). Forces Step 1f mobile impls to specify the
non-zero CAEAGLLayer-backed FBO rather than silently inheriting 0.
- CONCERN 2: new `tests/resize_smoke.rs` with two raster-backed tests —
grow 400×300→800×600→400×300 paints through `NativeBackend` without
panic; resize span emits on grow / shrink / 0×0 clamp paths.
- NIT: stale "Spec mini-patch pending" comments rewritten to reflect
v19.1 frozen state.
cargo build / test / clippy / fmt all green on macOS local.
Open-source codebase convention: all source-code comments in English.
Translates Chinese comments across openpencil-shell-{core,native,web}
.rs and Cargo.toml files. Logic, identifiers, and string literals
unchanged; the literal CJK fixture "Hello 你好" in raster_text_smoke
stays since it exercises the textlayout CJK path.
Applies Codex Phase A Gate round 1 review (3 BLOCK + 2 CONCERN + 1 NIT)
against the Task 2 SharedSkiaContext + NativeBackend implementation.
BLOCK 1 — `ProviderError::from_msg` `pub(crate)` blocked the Linux EGL
pbuffer test helper from constructing typed provider errors. Promoted
to `pub` so out-of-tree provider impls (test pbuffer, future Step 1f
mobile providers) can produce diagnostically-identical errors.
BLOCK 2 — Linux GPU smoke + chrome-stub-composition tests silently
returned `Ok(())` on EGL pbuffer setup failure, turning acceptance #3 /
#4 into false positives on hosted CI without GPU. Now gated by
`STEP1A_REQUIRE_GPU=1`: real-GPU runners panic on setup failure;
dev / hostless runs surface an explicit `INCONCLUSIVE` marker before
returning. Mirrors the macOS `catch_unwind` skip path in the same file.
BLOCK 3 — `tests/memory_loop.rs` was running 100 cycles against
`SharedSkiaContext::inert_for_test()` (every Option<> field None), so
the RSS budget proved nothing about real allocation lifecycle. Renamed
constructor to `inert_for_lifecycle_test()` (clearer intent) and split
the test into:
- Phase 0 warmup (100 inert + 100 raster) so Skia's lazy
glyph/path/binding caches are populated before measurement;
- Phase 1 lifecycle idempotence (100 inert);
- Phase 2 real-resource cycle: raster surface on macOS / Windows
(winit::EventLoop main-thread-only on macOS; Win Actions runner
has no GPU per spec §8.1), full EGL pbuffer + GL surface on Linux
when `STEP1A_REQUIRE_GPU=1`, raster fallback otherwise.
Budget kept at 5 % per acceptance #6 with a 1.5 MB absolute floor to
absorb macOS sysinfo's coarse RSS sampling jitter on small baselines.
CONCERN 1 — `GlContextProvider` had three non-spec methods (`resize`,
`size`, `default_framebuffer_id`). Audit:
- `resize`: actually used by `SharedSkiaContext::resize` (window /
pbuffer resize → Skia FBO rewrap). KEPT, spec mini-patch
documented in comment, escalation needed for spec v19 → v19.1.
- `default_framebuffer_id`: used by `SharedSkiaContext::new` /
`resize` for the FBO id Skia wraps; iOS EAGL provider (Step 1f)
will need non-zero values. KEPT, same escalation path.
- `size`: unused anywhere. DELETED (YAGNI), along with the unused
`size: (u32, u32)` field on `GlutinProvider` and the iOS / Android
stub impls.
CONCERN 2 — `glow_handle: Option<Arc<glow::Context>>` deviates from
spec v19 lines 120-125 + 191 (`Arc<glow::Context>`). Real lifecycle
needs the handle droppable: teardown releases the loaded function
table, `inert_for_lifecycle_test` has no GL backing, Step 1f Android
`on_pause` must drop alongside the EGL context. KEPT as Option<Arc>,
spec mini-patch documented for v19.1 escalation.
NIT 1 — Removed Task 1 link-check helper `placeholder()`. Task 2's
full re-export chain (`SharedSkiaContext`, `NativeBackend`, …)
already proves shell-core ↔ shell-native linkage; placeholder is
YAGNI now.
Verification (macOS local):
- cargo build -p openpencil-shell-native: clean
- cargo test -p openpencil-shell-native: 12/12 pass (8 binaries)
- cargo clippy -p openpencil-shell-native --tests --all-targets
-- -D warnings: clean
- cargo fmt -p openpencil-shell-native -- --check: clean
- memory_loop stress 8 consecutive runs: 8/8 pass
Anchor v19 pivot at the workspace level: vendor Jian as a git submodule
pinned to fork commit ad13ce6 (P0.5 mini-gate GO; skia-safe 0.78 → 0.97 +
new pub draw_on_canvas adapter), wire jian-core / jian-skia / jian-host-desktop
as path deps with explicit version per spec §12.2, and re-export the
Jian render/geometry/scene types from shell-core so shell-native can
translate the OP RenderBackend facade into jian DrawOp commands.
shell-core stays wasm32-clean: only jian-core (already wasm32-validated
in P0.5) plus glam / bitflags / thiserror / tracing land here.
shell-native picks up the full P0-pinned GL stack (skia-safe 0.97.0,
glutin 0.32.3, glutin-winit 0.5.0, glow 0.17.0, winit 0.30.13,
raw-window-handle 0.6.2, scopeguard 1.2) plus jian-skia (textlayout)
and target-gated jian-host-desktop (default-features = false, no `run`
feature so we skip Jian's softbuffer raster present path — OP owns its
own GPU swap_buffers per spec §3.6).
Adds OP RenderBackend trait + Rect / Color (with RED/GREEN/BLUE/BLACK/
WHITE/TRANSPARENT named constants per spec §5.2) + TextLayout facade
that wraps jian_core::render::TextRun explicitly (TextRun has no Default
impl, fields enumerated to honour spec §5.2 round-2 CONCERN-1 fix).
Boundary checks all pass:
- wasm32 shell-web metadata: no jian-host-desktop / jian-skia
- aarch64-linux-android shell-native metadata: no jian-host-desktop
- shell-core src: no glutin / skia_safe / winit / glow imports
Tasks 2-4 (SharedSkiaContext + NativeBackend + ShellEvent mapping +
acceptance) follow per plan v7.
P0 dep-stack probe (Step 1a) cleared all three OS targets in CI
run 25358457742:
- macOS aarch64: full window+GL probe (cross-API state + readback) PASS
- Linux x86_64 (hosted runner): link-time PASS, runtime DEFERRED
(LINUX_GPU_DEFERRED_NO_RUNNER) — Xvfb GLX limitation; same skip as
bevy / rust-skia / iced CI.
- Windows x86_64 (hosted runner): link-time PASS, runtime DEFERRED
(WINDOWS_GPU_DEFERRED_NO_RUNNER per spec §8.2).
Pin versions captured in
`openpencil-docs/superpowers/notes/2026-05-05-skia-glow-loader-compat-probe.md`.
Reverts:
- transient `[dev-dependencies]` block in shell-native Cargo.toml
(skia-safe / glutin / glutin-winit / glow / raw-window-handle /
scopeguard / dev-only winit override).
- transient `tests/p0_probe.rs` + `examples/p0_probe.rs`.
- transient workflow steps that gated `--ignored P0_PROBE_GATE` and the
Xvfb / freetype / mesa apt installs that only the probe needed.
Kept:
- prod winit dep features `["x11", "wayland", "wayland-csd-adwaita",
"rwh_06"]` — needed for Linux to satisfy winit's
`compile_error!("...not supported by winit")` guard. Stage F may
trim this when RenderBackend lands.
- workflow's libxkbcommon / libwayland apt install — winit's link-time
deps for the features above.
- `.gitattributes` — enforces `eol=lf` so future cross-OS rustfmt stays
green.
Task 1 will reintroduce skia-safe / glutin / glow / raw-window-handle
/ scopeguard as permanent prod deps when SharedSkiaContext +
RenderBackend land.
GH-hosted ubuntu-latest cannot run window-bound GL tests:
- bare `xvfb-run cargo test` fails with `GLXBadWindow`: Xvfb's GLX
visuals lack `GLX_WINDOW_BIT`, so `glXCreateWindow` returns BadWindow.
- `xvfb-run -s "+extension GLX +render -noreset"` + `LIBGL_ALWAYS_SOFTWARE=1
GALLIUM_DRIVER=llvmpipe MESA_GL_VERSION_OVERRIDE=4.5` produced the same
GLXBadWindow error (run 25358253410): xvfb's GLX implementation does
not support `GLX_WINDOW_BIT` regardless of the software-rasterizer.
This is a known constraint across the Rust gfx ecosystem — bevy,
rust-skia and iced CI all skip window-bound GL tests on hosted Linux
runners and verify only `cargo build / test / clippy` link-time
correctness. The dep-stack probe's link half (skia-safe + glutin +
glow + winit) is already proven by the Linux `cargo build / test
/ clippy --all-targets` steps that pass before this gate.
Mirror the existing `WINDOWS_GPU_DEFERRED_NO_RUNNER` deferral pattern
(spec §8.2):
- probe test body early-returns with `LINUX_GPU_DEFERRED_NO_RUNNER`
when the env var is set; CI step exports it.
- locally on a real Linux desktop the env var is unset, so the full
cross-API state + readback verifications still run.
macOS retains the full window+GL path (CI + local), which alone
covers spec §7.2(2) "cross-API GL state visibility" and §6.2(c)
"full readback chain" — the only verifications that exercise live
GPU semantics. Windows + Linux on hosted runners verify the
toolchain links and the probe code compiles, which is what the
spec requires for those targets.
Linux P0 probe was failing with `GLXBadWindow` because:
- bare `xvfb-run` brings up Xvfb with default args (no `+extension GLX`);
the X server then advertises no GLX FBConfigs, so winit's X11 backend
fails when glutin tries to create a GL window.
- the runner has no GPU, so even with GLX enabled mesa would not pick a
hardware visual; without a software fallback configured glutin cannot
resolve `ContextApi::OpenGl`.
Fix:
- pass `xvfb-run -s "-screen 0 1280x1024x24 +extension GLX +render
-noreset"` so Xvfb advertises a 24-bit GLX-capable visual.
- set `LIBGL_ALWAYS_SOFTWARE=1`, `GALLIUM_DRIVER=llvmpipe`, and
`MESA_GL_VERSION_OVERRIDE=4.5` so mesa loads llvmpipe (CPU
rasterizer) and reports a desktop-GL version high enough for skia.
These env vars propagate naturally from the workflow shell down through
xvfb-run → cargo → the spawned `cargo run --example p0_probe`
subprocess (probe runs each verification in a fresh process so winit's
EventLoop singleton guard doesn't trip).
Linux `cargo test --workspace` failed at link time:
/usr/bin/ld: cannot find -lfreetype: No such file or directory
/usr/bin/ld: cannot find -lfontconfig: No such file or directory
collect2: error: ld returned 1 exit status
skia-safe 0.97 (P0 probe transient dev-dep) links against the system
freetype + fontconfig on Linux. The GitHub-hosted ubuntu-latest runner
ships only the runtime libs; we need the `-dev` packages so `cc` can
resolve `-lfreetype` / `-lfontconfig` during link.
macOS and Windows do not link against these (skia-bindings uses
CoreText / DirectWrite respectively), so the install step stays
gated on `runner.os == 'Linux'`.
Two unrelated CI failures on the P0 probe gate matrix, fixed together
because both gate the same workflow:
1. ubuntu-latest: winit 0.30 with `default-features = false` triggers
`compile_error!("The platform you're compiling for is not supported by
winit")` because no Linux backend (`x11` / `wayland`) is enabled.
Adds explicit `["x11", "wayland", "wayland-csd-adwaita", "rwh_06"]`
features so the prod skeleton dep compiles on every desktop OS.
macOS / Windows backends auto-activate via `cfg(target_os)`, so they
don't need explicit features.
2. windows-latest: `cargo fmt --check` failed with `Incorrect newline
style` — actions/checkout normalized .rs files to CRLF on the
Windows runner, but rustfmt.toml pins `newline_style = "Unix"`.
Adds `.gitattributes` enforcing `eol=lf` on all text (and explicit
`*.rs` / `*.toml`) so checkouts stay LF on every platform.
Both fixes are minimal and scoped to the P0 probe gate. The transient
dev-dep block (skia-safe / glutin / glow / etc.) is unchanged.
Drives the three-OS CI matrix verification of the skia-safe + glutin +
glow + winit dep stack per Step 1a spec §7.
- examples/p0_probe.rs: stencil_visibility + readback chain runner (must
own a real OS main thread because winit on macOS rejects
EventLoop::new() from cargo test worker threads).
- tests/p0_probe.rs: subprocess-invoke wrapper, gated
#[ignore = "P0_PROBE_GATE"] so default cargo test stays untouched.
- Cargo.toml: add transient [target.'cfg(not(target_arch = "wasm32"))'.
dev-dependencies] block (skia-safe 0.97 + glutin 0.32.3 + glutin-winit
0.5.0 + glow 0.17.0 + raw-window-handle 0.6.2 + scopeguard 1.2.0 +
winit defaults). Pinned to versions resolved in /tmp/skia-glow-probe.
- .github/workflows/rust-check.yml: install Linux GL prereqs (xvfb,
mesa, libxkbcommon, libwayland) and add a P0-probe-gate step running
cargo test --ignored on each OS (Linux through xvfb-run; Windows
early-returns per spec §8.2 WINDOWS_GPU_DEFERRED_NO_RUNNER).
All three artefacts are TRANSIENT — reverted in a follow-up cleanup
commit after CI is green and the loader-compat notes commit lands.
Task 1 owns the permanent integration.
Codex flagged: my previous repair regex matched 3/4/6/8 hex digits,
but \`pen-renderer/paint-utils.ts::parseColor\` only handles
lengths 3, 6, and 8 — the length-4 branch falls through to the
gray fallback. So a raw 4-digit string like \`F00A\` got the \`#\`
prepended and looked like a valid \`#F00A\` color downstream, but
the renderer still painted gray. Net effect: traded one broken
render path (raw-string → fallback) for another (length-4 → fallback)
while masking the schema error so upstream callers couldn't see
it had a problem.
Tightened RAW_HEX_RE to only the three lengths parseColor actually
accepts. 4-digit strings now stay un-prefixed so the schema error
stays visible to tooling that flags malformed hex.
Test updated: drops the F00A → #F00A case from the "repairs N-digit
shapes" matrix and adds a dedicated negative-test case asserting
F00A survives normalization unchanged. Comment in RAW_HEX_RE also
captures the parseColor support matrix and the rationale for not
expanding 4-digit shorthand here — that would require an actual
RGBA-to-RRGGBBAA expansion (e.g. F00A → #FF0000AA), which is a
separate concern that belongs in the renderer or a dedicated
shorthand expander, not in a schema repair pass.
M2.7 food-app run shipped the page root with
fill: [{ type: 'solid', color: 'FFF8F0' }]
(no leading \`#\`). The renderer's hex parser failed → root frame
fell back to its default gray fill → the warm-food cream page bg
disappeared and the whole design read as a generic gray app
instead of the warm-light theme. Bottom nav and other surfaces
were similarly affected when sub-agents emitted raw 6-digit hex
without the prefix.
normalizer now adds the missing \`#\` in place when:
- entry is a SolidFill with a string color
- color starts with neither \`#\` nor \`$\` (so we don't touch
variable refs)
- color matches one of the four hex shapes the renderer accepts:
\`/^[0-9A-Fa-f]{3}([0-9A-Fa-f]([0-9A-Fa-f]{2}([0-9A-Fa-f]{2})?)?)?$/\`
— exactly 3, 4, 6, or 8 hex digits. 5 and 7 digit strings
intentionally don't match (those aren't repairable hex).
Same repair applies to:
- gradient stop colors (linear_gradient + radial_gradient)
- stroke.fill colors (M2.7 also drops the prefix on stroke colors)
6 new tests cover: 6-digit repair, 3/4/8-digit shapes, valid hex
unchanged, \$color-* refs unchanged, non-hex strings (named
colors / partial / 5-7 digit) untouched, stroke fill repair,
gradient stop repair. Verified by temporarily commenting out the
repair calls — 4 tests correctly fail "expected '#FFF8F0' to be
'FFF8F0'", confirming the regression coverage actually exercises
the bug condition.
Codex flagged: the previous image-card test had \`height: 180\` with
an image fill_container child + a moderately long caption. With
the way fitContentHeight resolves a fill_container image's height
(returns 0 when no parent height context), the natural height
landed at ~120 — well below the declared 180 — so the bug
condition \`natural > declared\` never fired and the assertion
\`changed === false\` would have passed even with image-card back
in CARD_ROLES.
Rewrite to actually exercise the regression:
- Drop declared height to 80 (a tight 1:3.75 crop).
- Use a multi-paragraph caption that wraps to ~10 lines at the
card's 300px width — natural height lands at ~210, well past 80.
- Add a sanity assertion (\`fitContentHeight(card) > 80\`) before the
no-change check so future edits to the test fixture can't
silently re-introduce the vacuous-pass shape without setting off
this guard.
- Mirror the same shape under \`role: 'card'\` and assert it DOES
get expanded. The role-based gate is the whole point of the
fix; asserting the contrast across two near-identical fixtures
makes the regression's blast radius and behavior obvious.
Verified by temporarily putting \`image-card\` back into CARD_ROLES:
the test correctly fails with "expected true to be false". With
the fix in place, all 6 tests pass.
Codex flagged: \`image-card\` was in CARD_ROLES, so a 16:9 photo
tile or a 1:1 thumbnail could get silently switched to fit_content
when its computed natural height exceeded the declared one
(image+caption pattern: caption text wraps past the photo crop,
fitContentHeight returns more than the fixed height, my pass
auto-expanded). That breaks the intended visual proportion —
\`image-card\` exists precisely to lock in a fixed crop / aspect
ratio.
Removed \`image-card\` from CARD_ROLES with a scope note explaining
the rationale. Authors who want an image card to grow with content
should use the generic \`role: 'card'\` with an image child instead.
Other card-family roles (card, stat-card, pricing-card,
feature-card, testimonial, event-card, product-card) keep the
auto-expand because they're text-content first and overflow there
is the bug we're trying to fix.
New regression test seeds an image-card with a 16:9 crop + a long
wrapped caption that pushes natural height past the declared 180,
asserts the height stays at 180 and the pass returns false.
Image #44 banner shipped with the "Order now" button cut in half:
\`featured-promo-card { role: 'card', height: 165, clipContent: true }\`
held a vertical content stack (badge + title + body + button) whose
natural height was ~220px on the model's wrapped column width. The
card role default sets \`clipContent: true\` to keep image children
inside rounded corners, so the overflow got rendered then clipped at
y=165, making the bottom row of content disappear.
New \`expandOverflowingFixedHeightCards\` post-pass:
- Walks the tree.
- For each frame whose \`role\` is in CARD_ROLES (card, stat-card,
pricing-card, feature-card, image-card, testimonial, event-card,
product-card) AND \`height\` is a positive number AND
\`fitContentHeight(node) > height\`, switches \`height\` to
\`'fit_content'\`.
- Returns true if any card was patched.
Why fit_content, not removing clipContent: clipContent is what makes
nested image children respect the card's rounded corners. Removing
it would un-clip the button (good) but un-clip the image edges (bad
— image bleeds past the card's corner radius). Just letting the
card grow keeps both invariants right.
Also wired in: \`design-canvas-ops.ts\` calls the new pass right
after \`injectMissingNavSurfaceFill(pageRoot)\` in the streaming /
dispatcher post-pass chain. The card-overflow fix runs ONCE per
post-pass invocation on the page root, so all card-family children
on the page get checked together.
Side fix: button role default for tab-style buttons (parent role is
bottom-tab-bar / tab-bar / tab-row AND layout='vertical') now
returns \`padding: [6, 4], gap: 4\` instead of falling through to
the text-button \`[12, 24]\` default. Only affects the case where
the model omits padding on the tab cell — sub-agents that emit
explicit padding still win (per applyDefaults' missing-only rule).
5 new tests cover: banner-style overflow gets fit_content, fitting
content stays at fixed, non-card roles never get touched, already
auto-sizing cards stay alone, and overflow detection walks into
nested sections.
Two visible regressions in Image #44:
1. Bottom nav reverted to no-background even though earlier runs
worked. GPT-5.5 wrapped its bottom nav in a single-child section:
root > frame{role:'section',id:'bottom-tabs-root'}
> frame{role:'bottom-tab-bar'} > [tabs]
The inject pass only walked DIRECT children of root and bailed on
the section wrapper. Now we hop one level when the wrapper is a
single-child section AND its sole child is a nav-role frame, so
the nested nav gets the surface fill + position-aware shadow.
Multi-child sections still bail (those are real content sections,
not wrappers).
2. Banner "Order now" CTA shipped with white text + dark icon. My
prior contrast fix used a luminance-delta threshold of 0.4, but
#0F172A icon vs #F97316 (orange accent) actually has delta 0.48
— the threshold said "good contrast, leave it alone" while the
user sees an obvious mismatch with the white text label.
Wrong axis: the user's complaint is about CONSISTENCY (icon
should read as the same token as text), not raw contrast.
Refactored fixButtonForegroundContrast:
PASS 1 — find a "reference" foreground from sibling text fill
(after refs resolve). The model's own text color is the
authoritative signal for what the button's foreground should
look like, regardless of what bg/fg luminance suggests.
PASS 2 — for each icon_font sibling, override when its
resolved hex differs from the reference fg. Icon-only
buttons (no text sibling) fall back to a luminance-based
check at threshold 0.5 — catches dark-on-dark / light-on-
light pairs that motivated the original rule, without the
false-negative on saturated mid-luminance bgs (orange).
3 new tests: wrapper-section nav reach, multi-child wrapper bail,
and the regression test for the original "dark-on-dark icon-only
button" still passing under the new luminance-fallback path.
141 tests in the affected suites all green.
Side effect: applyNavSurfaceFill now bails entirely (returns false)
when the nav already has a fill — earlier version still added a
shadow even when fill was preserved, which violated the
"preserves sub-agent intent" semantics the existing tests rely on.
Image #42 logs showed every image fetch URL came out wrapped twice:
http://localhost:3000/api/local-asset?path=%2Fapi%2Fai%2Fimage-proxy%3Furl%3D...
The image-search pipeline correctly returned
\`/api/ai/image-proxy?url=...\` thumbUrls (so browser fetches go
through the dev server, which can reach openverse via the system
proxy). But \`isLocalAssetPath\` only excluded \`data:\`/\`https?:\`/
\`blob:\` from local-asset bridging — anything else, including
absolute paths starting with \`/api/\`, was treated as a file-system
asset and re-wrapped through \`/api/local-asset?path=\`. That bridge
handler then 404s because the encoded path \`/api/ai/image-proxy?...\`
isn't a real file. Net effect: every search-found image stayed at
the placeholder visual even though the search succeeded.
Add a same-origin route carve-out:
/^\/(?:api|_)\//
Paths under those prefixes are runtime endpoints (Nitro \`/api/*\`,
Vite \`/_/*\`), not file system assets, so they pass through the
resolver as-is. \`/assets/hero.png\` and similar absolute file-style
paths still go through the local-asset bridge.
New regression test covers /api/ai/image-proxy, /api/local-asset,
and /_/* paths returning false from isLocalAssetPath, and verifies
ordinary /assets/... paths still return true.
Codex flagged: the previous version cleared the AbortController
timeout in a finally{} block right after \`await fetch()\`, but
fetch() resolves as soon as the response headers arrive — the
body read happened later in the \`reader.read()\` loop with no
timeout protection. An upstream that drip-feeds bytes (or stops
mid-stream) would leave the dev server hanging on
reader.read() forever.
Single AbortController + timeout now covers the entire request
lifecycle (DNS + TLS + headers + body). The clearTimeout moves to
the outer finally{} so it fires regardless of return path
(success, 4xx, 5xx, abort) but never AHEAD of the body read.
Side benefit: AbortError thrown by the controller's timeout (or
by the size-cap controller.abort()) now lands in the catch clause
with a distinguishable error.name === 'AbortError'. Translate it
to a 504 Gateway Timeout when the timeout was the cause, so the
caller can distinguish a slow-upstream from a generic
fetch failure (502).
Codex flagged: the previous version did
\`Buffer.from(await upstream.arrayBuffer())\` which buffers the
entire upstream body into memory with no upper bound. Wikimedia
Commons originals can be 100 MB+, and a malicious request could
point at any arbitrarily-large file on an allow-listed host (an
upstream big enough to OOM the dev server is reachable behind
plenty of legitimate-looking URLs).
Hard 16 MiB cap on every proxied response:
- Read upstream's declared Content-Length first; reject (413) if
it already advertises more than the cap, before reading a single
byte.
- Stream the body via \`getReader()\`, accumulate in chunks, and
bail (cancel reader, abort fetch, return 413) the moment total
bytes cross the cap. Subsequent chunks are never buffered.
- Move the timeout from \`AbortSignal.timeout(15000)\` to a manual
AbortController so the same controller can also abort on
size-limit hit.
16 MiB sits well above any reasonable thumbnail and even high-res
4K JPEGs (~5–8 MiB), but well below the territory that risks
heap pressure from a single fetch.
Image #40 logs showed three "Failed to load image" errors for
api.openverse.org/...thumb/ URLs even though the search-pipeline
successfully fetched URLs from openverse via the dev server (the
HTTPS_PROXY fix from 3a6f8480 routes server-side fetches through the
local proxy). The browser-side image loader doesn't go through the
same dispatcher: \`new Image(); img.src = url\` does a direct
browser fetch that ignores HTTP_PROXY env vars, so on a machine that
requires the proxy to reach openverse.org the canvas paints the
placeholder visual even though the search-pipeline already found a
valid image URL.
New endpoint \`/api/ai/image-proxy?url=<encoded-url>\`:
- Proxies image bytes through the dev server.
- Reuses \`configureProxyDispatcher\` so the upstream fetch routes
through HTTPS_PROXY (same path as image-search).
- Allow-lists known image hosts (openverse, wikimedia, flickr's
static CDN) to prevent the dev server being used as an open
proxy. Unknown hosts get 403.
- Forwards Content-Type and Cache-Control from upstream.
- Sets Access-Control-Allow-Origin: * so canvas readback works.
\`mapOpenverseResult\` and \`mapWikimediaPages\` now return thumbUrl
wrapped via \`viaImageProxy(externalUrl)\`. The browser fetches
\`/api/ai/image-proxy?url=...\` (same-origin, no proxy needed),
the server fetches the upstream (with proxy), bytes flow back,
the canvas paints the photo. Test expectations updated to assert
the proxy wrapper.
Net effect: with HTTPS_PROXY set, image search end-to-end (search
results found AND images actually load in canvas) works on
proxy-required dev machines. With no proxy env var set
(production / CI), the cascade is still well-behaved — proxy
dispatcher is a no-op, server fetch is direct, no proxy wrapping
is necessary but it doesn't hurt either (the endpoint just adds
a hop).
The food-app run on warm-light theme shipped a bottom-tab-bar with
a valid \$color-surface (white) fill, but the page bg
(\$color-bg-deep) is cream #FFF8F0. The luminance delta between
white and cream is ~0.03 — visually indistinguishable, so the user
reads the nav as having no background even though it does. Image #40
made this concrete: the nav fill landed correctly per live-doc
inspection, but the screenshot still showed icons floating over an
unbroken cream background.
The inject pass already set the surface fill. To survive the
low-fill-contrast case we also stamp a soft shadow:
- bottom-tab-bar → upward shadow (offsetY: -4) lifts the nav off
the content above. A downward shadow would clip off-screen.
- top-app-bar / top-nav-bar / navbar → downward shadow
(offsetY: 4). An upward shadow would cling to the screen edge.
- nav / tab-bar / tab-row → ambiguous position, default downward.
Shadow specs (offsetY: ±4, blur: 12, spread: 0, color: #0000000F)
match conventional iOS/Android nav lift values and survive on
ANY page bg color, not just cream — even on dark themes the
extra subtle shadow is invisible (already-dark page) without
breaking the design.
Existing effects on the nav are preserved — sub-agents that
intentionally emit a drop-shadow / glow keep their declaration.
3 new tests cover: bottom-nav gets upward shadow,
top-nav variants get downward shadow, sub-agent's existing
effects survive the inject pass.
Codex flagged: the previous fix (b3180534) read
\`doc.themes[SEMANTIC_PALETTE_THEME_AXIS]\` as the dark-mode
discriminator, but \`seedDocVariablesFromStyleGuide\` — the only
production writer of theme-related doc state — writes ONLY
\`doc.variables\`, never \`doc.themes\`. So on a real
orchestrator-emitted dark-mode design the axis is empty, the test
\`modeAxis[0] === 'Dark'\` is false, and the cascade still served
the LIGHT palette. The "production" dark-mode signal was wired to
nothing.
Fix reads the active page root's fill via \`detectThemeFromNode\`, the
same heuristic \`resolveTreeRoles\` uses at its entry point to set
\`ctx.theme\` for role defaults. The page root's fill is what the
model / user actually painted as the page background, regardless
of whether any themes axis was ever populated, so it's the
production-truthful signal.
New \`detectActivePageMode()\` helper reads the doc store + canvas
store's activePageId, finds the first frame on that page, and
runs \`detectThemeFromNode\`. Defaults to 'light' when no page root
exists or it has no fill — same conservative bias as before.
Test updated to seed a dark page-root fill on the live doc store
(replaces the prior \`themes['Mode']\` axis seed which never
matched production state). Also explicitly nulls the themes axis
to confirm the fill-based detection is the active code path.
Codex flagged: the previous cascade (965e143e) gated step-2 mode
selection on a `themeHint` parameter that no caller actually
supplied, so every dark-mode generation with an unseeded
\$color-accent button got served the LIGHT palette hex
(#2563EB blue) instead of the DARK one (#60A5FA light blue).
With identical luminance assumptions in both branches the
contrast pass picked the wrong fg for genuinely dark-mode docs.
`resolveColorMaybeRef` now drops the unused themeHint param and
reads `doc.themes[SEMANTIC_PALETTE_THEME_AXIS]` ('Mode' axis)
directly. First value of that axis === 'Dark' → step-2 uses the
dark palette; otherwise light. The function is self-contained,
no plumbing required at call sites, and the fallback honors
whichever mode the doc currently advertises.
New regression test seeds doc.themes={ Mode: ['Dark', 'Light'] }
with no doc.variables, asserts that an unseeded \$color-accent
button gets the dark-palette accent (#60A5FA, lum ≈ 0.6) and
therefore the dark fg color (#0F172A) on the text child. If
step-2 had stayed locked on Light, the test would expect #FFFFFF
and fail.
Codex flagged the previous fix (1c08ac3f, "skip on unresolved ref")
as too conservative: a sub-agent's button with `\$color-accent` bg
and a text child WITHOUT a fill ended up with no fill at all when
doc.variables hadn't been seeded yet — text falls back to black,
which is invisible on a dark accent button.
Better fix: extend `resolveColorMaybeRef` with a step-2 fallback
into the built-in semantic palette (`getSemanticPaletteHex`). Every
core token (`color-accent`, `color-bg`, `color-text-primary`, etc.)
has a known hex for both `Light` and `Dark` modes, so the cascade
now is:
1. Doc-seeded variables (user's palette).
2. Built-in semantic palette for the requested mode.
3. Original ref string (unresolvable) — caller bails.
In the food-app + GPT-5.5 path, step 1 already worked because
`seedDocVariablesFromStyleGuide` runs before the sub-agents.
The fallback is for paths that bypass seeding (test fixtures,
external MCP callers, mid-flight states), not the common case.
Skip-on-NaN behavior stays — it now only triggers for genuinely
unknown tokens (`\$color-foobar` or similar), where any guess is
worse than leaving the model's existing fill alone.
Tests:
- New: `\$color-accent` ref resolves to #2563EB via semantic palette
even with no doc.variables, contrast pass picks white fg correctly
for the unfilled text child.
- New: `\$color-mystery-token` (not in palette) — step-2 misses,
contrast pass skips, existing text fill survives.
- Replaces the prior "skips on unresolved" test which over-asserted
the conservative path.
Codex flagged: the previous version (ddf6580f) treated a NaN
luminance — what `hexLuminance` returns when the bg color is still
a `\$color-accent` ref because the doc's variables haven't been
seeded — as a dark bg and painted white text. If the user's palette
later resolves \$color-accent to a LIGHT hex (e.g. cream
#FFE4B5), the white text becomes invisible on the light bg. Same
risk in the inverted direction with the original code, which
defaulted to dark text on unknown bg.
Either guess can ship a visually broken button. Skip the contrast
pass entirely when we can't resolve the bg ref to a hex — text /
icon retain whatever fill they already carry, which is at least
visible (the model's default text color, usually dark). A later
post-pass invocation, after variables get seeded, re-runs and
applies contrast cleanly with a real luminance value.
`needsContrastOverride` already returns false when either luminance
is non-finite, so the icon-override branch was already safe; only
the text branch and the (now-removed) NaN→white default needed the
fix.
New regression test in role-resolver.test.ts covers the unseeded-ref
case: explicit dark text fill on an unresolvable accent button
survives untouched.
3 new cases covering the regression fixed in ddf6580f:
- dark icon on dark button → overridden to white (low contrast triggers)
- intentional brand-red icon on white button → preserved (delta > 0.4)
- unfilled icon_font on dark button → still gets contrast fill (no
regression on the original "fill if missing" branch)
User reported icons inside accent-color buttons rendering dark while
text on the same button rendered white — visible on the "Burger" tab,
the "Order now" CTA, and the round filter icon-button in the food-app
generation. Two compounding causes in `fixButtonForegroundContrast`:
1. The luminance check ran on the raw `fill[0].color` even when that
was a `\$color-accent` variable ref. parseInt('\$c…', 16) returns
NaN, NaN<0.5 evaluates to false, the dark-fg branch wins, and the
contrast pass was painting dark-on-orange. Resolve the ref via
the doc store's variables + active theme before luminance.
(NaN luminance now falls through to the white branch — better
than the previous silent dark default when resolution misses.)
2. The icon_font child path skipped any node that "already has a
visible fill". Models reflexively stamp `icon_font.fill` to a
dark text color (the prompt lists `fill` as a property), so on
an accent button the contrast pass left the dark icon next to
the now-white text. Split the icon_font branch off the text
branch: when the icon already has a fill, RESOLVE it and check
contrast against the (resolved) bg; if the luminance delta is
below the WCAG-graphical threshold (0.4), override with the
contrast fg.
Why not unconditionally override icon_font fill: an intentional
brand-color icon on a near-white button (red notification dot, blue
brand mark) has a contrast delta well above 0.4 and survives. Only
the dark-on-dark / light-on-light pairs that motivated the bug get
rewritten.
`text` and `path` branches keep their original behavior — text is
where models intentionally express accent colors, and path stroke
icons get filled by the existing stroke-fallback path.
The food-app live doc had two unfilled placeholders because the model
emitted 3-keyword queries that Openverse returned zero results for
("burger combo fries", "sakura sushi platter"). The 2-keyword forms
have plenty of matches (240 each).
The previous skill text said "2-3 English keywords" which the model
read as "3 is fine"; concrete examples like `image_search_query:
"burger fries combo"` reinforced the 3-word habit. Updated to:
- "Strongly prefer 2 keywords; never more than 3"
- Explanation of WHY (strict AND-search, concrete zero-result vs hit
comparisons for "burger fries" / "sushi platter")
- Rule for the 3rd keyword: only when it's a strong common-phrase
noun ("iced latte" yes, "iced latte coffee" no)
- All worked examples in elements.md row 44 updated to 2 keywords
("burger fries", "sushi platter", "chicken bowl")
The server-side fallback (commit 93e5847f) catches 3-keyword
zero-results by retrying with 2 words, so this is a quality nudge
on top of a working safety net — fewer retries means tighter
relevance and faster fill.
Two of five food-app placeholder images shipped unfilled because
Openverse returned `[]` for the model's 3-keyword queries:
- "burger combo fries" → 0 results
- "sakura sushi platter" → 0 results
The same queries truncated to the first two words have plenty:
- "burger fries" → 240 results
- "sushi platter" → 240 results
Openverse uses strict AND-search across all keywords, so a 3-word
query that includes any low-frequency or non-matching token
zero-results even when the photos exist. The skill prompt already
nudges models toward "2-3 English keywords" but they often pick three
when the brief mentions a third descriptor (e.g. "Tasty BURGER COMBO
fries" → "burger combo fries").
Endpoint now cascades:
1. Openverse with full query.
2. If `[]` and query has > 2 words: re-query with first 2 words.
3. If still nothing usable: fall through to Wikimedia (existing path)
with the same 2-word retry safety net.
Returning the original empty result was wrong: the placeholder stays
unfilled even though a satisfactory photo for "burger fries" was one
keyword-trim away. The trade-off is losing a small amount of relevance
on the dropped 3rd keyword — but that's better than no photo at all,
and the model still drives the first two keywords which carry the
core subject.
Visible regression in the food-app screenshot: the top-left avatar
(44×44 orange circle, role='button', single child text 'A') rendered
with the 'A' visibly off-center. Live doc inspection showed
`padding: [12, 24]` on the avatar — 24px horizontal padding × 2 = 48
exceeded the 44px frame width, the layout engine clamped to the
negative content area, and the centered text ended up shifted off
the visual center of the orange circle.
Root cause: the default 'button' role rule unconditionally returns
`padding: [12, 24]`, which fits a typical text-button (~44 tall × wide
enough for label + horizontal pad). When the model emits role='button'
on what's actually an avatar / icon-action shape (small square, single
short child), the same default collides with the small fixed width.
Fix: in the default branch (no special parent-role context), inspect
the node's explicit width/height. When BOTH are numeric AND ≤ 60 AND
the default 24px horizontal padding wouldn't fit (`width < 24*2`), skip
the text-button defaults entirely — return only the layout / centering
shape (no padding, no cornerRadius, no height). The 60px ceiling is
the standard icon-button / avatar size band; the 24*2 fit-check keeps
the full text-button defaults active for everything wider than ~48px.
Why no cornerRadius default in this branch: avatar-style frames
typically supply `cornerRadius: width/2` themselves to get a circle.
The 8px text-button default would silently override it.
Note: applyDefaults only fills missing properties, so this only
affects nodes the model didn't explicitly stamp padding on. Models
that DO emit padding stay untouched (correct — the model is the
source of truth when it commits to a value).
Visible regression in the GPT-5.5 food-app run: the bottom nav
shipped with no surface fill (floating icons on the cream root
background), even though `injectMissingNavSurfaceFill` was wired in
and verified to add a fill on top-level nav-role frames. Live doc
inspection showed `bottom-tab-bar` carrying `fill: undefined`
post-generation — the inject pass simply never ran.
Root cause: `orchestrator-sub-agent.ts` runs the dispatcher branch
(Strategy A `<op_tool>` element-tools AND Strategy B JSONL-in-
batch_design) and **early-returns before** reaching
`applyPostStreamingTreeHeuristics(rootId)` further down in the
function. That post-pass is what runs:
- normalizeStrokeFillSchema
- unwrapFakePhoneMockups
- resolveTreeRoles + resolveTreePostPass
- normalizeTreeLayout
- stripRedundantSectionFills
- injectMissingNavSurfaceFill
- publish (forcePageResync)
Skipping it on the dispatcher path means EVERY sub-agent that emits
via element tools or JSONL fallback bypasses role resolution, layout
normalization, redundant-fill stripping, AND nav-surface injection.
The streaming path was the only branch that fired the cleanup.
Fix: call `applyPostStreamingTreeHeuristics(subtask.parentFrameId ??
plan.rootFrame.id)` right before the dispatcher branch returns, when
at least one node was inserted. The post-pass walks up to the page
root via `getParentOf()` for the inject step, so passing the section
root that the dispatcher inserted into is correct.
This also un-blocks several heuristics that depend on the full
subtree being in the store: button width / frame height equalization,
clipContent on cards-with-image-children, and theme detection on the
sub-agent's root (which feeds icon/text color defaults).
The previous commit ran `bun install` while ~/.npmrc set
`registry=https://registry.npmmirror.com/`, so every tgz URL in
bun.lock got rewritten from the empty-default-registry form (`""`)
to an explicit `https://registry.npmmirror.com/...` URL. That pinned
the entire workspace install to a regional mirror that other
contributors and CI don't have access to.
Re-ran the install with ~/.npmrc temporarily moved aside so undici's
new entry lands with the workspace's normal "" URL convention while
every other dep's URL is reset to "" too. apps/web's workspace block
now has `"undici": "^7.22.0"` while the package metadata block keeps
the same empty-URL shape as the rest of the lockfile.
No content / version changes — only URL field resets.
Previous version did `require('undici')` inside a try/catch on the
theory that would let it run on both CJS and ESM. In Vite/Nitro's dev
path the helper loads as an ESM module, where `require` is undefined —
the call threw `ReferenceError: require is not defined`, the catch
block silenced it, `configured=true` still got flipped, and every
subsequent call short-circuited. Net effect: the proxy was never
installed in the very dev environment the fix was meant to repair, so
image-search kept ECONNREFUSED-ing on Openverse + Wikimedia and
landing zero filled placeholders.
Switch to a static `import { setGlobalDispatcher, EnvHttpProxyAgent }
from 'undici'`. undici is a transitive dep of h3 in this workspace
(verified resolved in node_modules), and it's also the package Node 18+
uses internally for fetch — pinning it as a direct dep on apps/web
makes the resolution intentional rather than reliant on the h3 chain.
Also swapped the hand-rolled ProxyAgent for undici's built-in
`EnvHttpProxyAgent`: it reads HTTPS_PROXY / HTTP_PROXY / NO_PROXY
itself (case-insensitive) and applies the no-proxy bypass list, which
saves us from re-implementing those rules.
Verified with both `bun -e` (workspace deps) AND a direct ESM Node
context: with HTTPS_PROXY set, `fetch(api.openverse.org/...)` now
returns 240 results for "salmon sushi" instead of the earlier
ECONNREFUSED. The "configured" guard still makes calls idempotent so
multiple endpoints can opt in without coordinating.
Yesterday's GPT-5.5 food-app run shipped with all 5 placeholder frames
carrying `image_search_query: "salmon sushi"`, even though only one of
the dishes was actually salmon sushi (the others were burger combo,
sushi restaurant card, chicken bowl, etc). Once the proxy fix lets
the search reach Openverse, the screen would render five identical
salmon-sushi photos instead of five different food shots.
Root cause is teaching: the previous skill text gave a single example
("burger fries") which the model copy-pasted to every placeholder on
the screen instead of mining each card's own title.
Updates:
- `elements.md` row 44: explicit "MUST receive its own query" + four
worked examples mapping card titles to per-card queries (Burger
House → "burger restaurant", Sakura Sushi → "sushi japanese",
etc.).
- `elements-cookbook.md`: replaces the single example with three
context-distinct calls + an inline comment warning against reuse.
- `jsonl-format.md` TYPES line: bolded "imageSearchQuery MUST be
UNIQUE per image — derive it from the surrounding card/dish/section
text" so the JSONL fallback path gets the same signal.
- `schema.md` image bullet: same uniqueness clause inline.
No code change in this commit — purely prompt-side teaching for the
JSONL + element-tool generation paths.
Root cause for all-blank-placeholders on the food-app brief: Node's
native fetch (used by the Nitro dev server's image-search endpoint)
ignores the system proxy by default. On machines that route outbound
HTTPS through a local proxy (clash / mihomo / corporate gateway —
mine sits at 127.0.0.1:7897), every Openverse + Wikimedia call from
the server silently ECONNREFUSEDs. The endpoint's catch block returns
`null` for Openverse → falls back to Wikimedia → that ECONNREFUSEDs
too → returns `[]`. Browser shows zero filled images.
Direct curl from the same machine uses HTTPS_PROXY automatically, which
is why a manual API check (e.g. `curl https://api.openverse.org/...`)
returned 240 results for "salmon sushi" while
`/api/ai/image-search?query=salmon%20sushi` returned `{results:[]}`.
`apps/web/server/utils/proxy-dispatcher.ts::configureProxyDispatcher`:
- Reads HTTPS_PROXY / https_proxy / HTTP_PROXY / http_proxy.
- If set, installs `undici.ProxyAgent` as the global fetch dispatcher
via `setGlobalDispatcher`. From that point on every server-side
`fetch()` routes through the proxy.
- Idempotent — multiple endpoints can call it without re-installing.
- No-op when no proxy env var is present (production / CI).
- Dynamic `require('undici')` so a build target that strips undici
doesn't crash at import time.
Wired into `image-search.ts` at module top so the dispatcher is
configured before the first request lands. Other endpoints making
external fetches can opt in with the same single-line call.
Verified standalone via Bun: with the helper in place,
`fetch('https://api.openverse.org/v1/images/?q=salmon+sushi')` returns
240 results. The dev server itself needs a restart to pick up the
server-side change (Vite server-code HMR doesn't re-evaluate Nitro
modules).
Previous fix left `role: 'image-placeholder'` on the frame even after
its fill was swapped to `[{type:'image', url, mode:'crop'}]` — the role
is what makes "this slot is meant to hold a photo" semantics survive
into history / codegen / downstream tooling, so stripping it would
trade one regression for another.
But that meant any follow-up generation (which calls
`resetImageSearchQueue` to clear `queuedNodeIds`) would re-walk the
tree, re-enqueue the same placeholder via role match, and overwrite
the already-good photo with whatever the next search returned.
`isUnfilledImagePlaceholderFrame` now gates every read: role match AND
fill is not already `type: 'image'`. Used in three places:
- `collectImageSearchTargets` only collects unfilled placeholders.
- `enqueueImageForSearch` early-returns if the caller passes an
already-filled placeholder (defense-in-depth for direct callers).
- `processQueue`'s re-check uses it instead of a plain
`isImagePlaceholderFrame`, so even a stale queue entry from before
someone else filled the frame gets dropped.
4 new tests in image-search-pipeline.test.ts cover the predicate
(default solid fill = unfilled; missing/empty fill = unfilled; image
fill = filled; non-placeholder role = always false) and a regression
test in `collectImageSearchTargets` that keeps the already-filled
placeholder out of the result while still picking up its sibling.
elements.md row 44 now spells out that passing 2-3 English keywords
(e.g. "burger fries", "modern office") via image_search_query is what
lets the auto-search pass swap the gray box for a relevant photo —
otherwise it searches the label or falls back to a generic placeholder.
elements-cookbook adds two example calls so the model has copy-paste
templates for the common case.
Without an explicit query, the auto-search pipeline can only fall back
to the placeholder's `label` (often unset for context-rich cards) or
finally a generic "placeholder" string — both produce off-topic stock
photos instead of, e.g., burger / sushi shots for a food-app brief.
Builders (`buildImagePlaceholder`, `buildImagePlaceholderV1`) now accept
an optional `image_search_query` param (snake_case to match the rest of
the params interface). When set, it gets stamped onto the resulting
frame as `imageSearchQuery` — the same camelCase field
`image-search-pipeline.ts::extractQueryForNode` already prefers over
`name` and the label child.
Tool definitions in `element-tool-defs-ext-2.ts` (v0) and
`element-tool-defs-ext-6.ts` (v1) expose the new property with a
description that nudges callers to pass 2-3 keywords ("burger fries",
"modern office workspace") for product / restaurant / hero contexts.
3 new tests in `add-image-placeholder-v0.test.ts`: query stamps onto
frame, omitted query leaves field undefined, empty-string query is
treated as missing.
The `add_image_placeholder_v0` / `_v1` element tools and JSONL payloads
that mimic them emit a `frame` carrying `role: 'image-placeholder'` (a
gray slate-100 box + centered icon_font child + optional label) — NOT
an `image` node. The auto-search pipeline only filtered on
`type === 'image'`, so every placeholder produced via element tools
silently bypassed the search hook. Latest GPT-5.5 food-app run shipped
8 placeholder frames; zero got auto-filled and the design landed with
all dashed-border icons instead of real photos.
Pipeline now:
- `isImagePlaceholderFrame` predicate identifies placeholder frames.
- `collectImageSearchTargets` returns mixed `{node, kind}` pairs
('image' for `type==='image'` with placeholder src, 'placeholder-frame'
for the role-keyed frames). Skips descending into placeholder
children (icon_font + label get wiped on fill anyway).
- `enqueueImageForSearch` accepts both shapes; queue items track `kind`.
- `processQueue` re-checks the right invariant per kind, and on success
uses `updateNode(id, { fill: [{type:'image',url,mode:'crop'}], children: [] })`
for placeholder frames (vs `updateNode(id, { src })` for image nodes).
Clearing children prevents the icon/label from rendering on top of
the searched photo.
- Streaming path (insertStreamingNode line 382) intentionally still
gates on `type === 'image'` — placeholder frames stream their
children separately, so enqueueing mid-stream would race with the
late-arriving icon. Placeholder frames are only enqueued via the
post-tree `scanAndFillImages` scan (orchestrator-tail + dispatcher
per-subtask), where the full tree is already in the doc.
`extractQueryForNode` looks for `imageSearchQuery` first, falls back
to a non-default `name`, then mines the optional
`role: 'image-placeholder-label'` text child for a hint. Generic
default still works ("placeholder") if nothing useful is on the frame.
7 new tests cover `isImagePlaceholderFrame` and
`collectImageSearchTargets` (placeholder + image mix, no descent into
placeholder children, missing root id).
`hasAnyFill` only checked that the first entry's `type` was a string,
which let several malformed shapes bypass injection: `[{type:'solid'}]`
(missing color), `[{type:'solid',color:''}]` (empty color), and
`[{type:'invalid'}]` (unknown variant). All three render as
transparent — effectively unfilled — so the inject pass should patch
them, but the truthy `type` made the function short-circuit and the
nav stayed bare.
Per-type validation:
- solid: color must be a non-empty string
- linear_gradient / radial_gradient: stops must be non-empty array
- image: src must be a non-empty string
- any other type: treated as unfilled (renderer can't paint it)
Two new tests: malformed solids (missing/empty color, unknown type) and
empty gradient + image-with-empty-src — all properly patched. Existing
preservation tests (real solid, linear_gradient with stops, radial
with stops, image with src) still pass.
Previous `hasSolidFill` only matched `type === 'solid'`. Sub-agents
legitimately put `linear_gradient` (sunrise hero, accent ribbon),
`radial_gradient` (splash entries), or `image` (branded photo banners)
on top app bars and other nav surfaces, and `hasSolidFill` would
return false for those — making the inject pass overwrite the
gradient/image with a flat `$color-surface` solid.
Renamed to `hasAnyFill`; matches any first-entry shape with a
recognized `type` field. Sub-agent intent (any non-empty fill) now
short-circuits the inject. Three new tests cover linear gradient,
radial gradient, and image fills explicitly — all preserved.
The previous "navbar in PROTECTED_ROLES" change was Codex-flagged as a
no-op: PROTECTED_ROLES only PREVENTS strip-pass deletion of an existing
fill, it doesn't ADD one. The actual food-app brief failure was that the
sub-agent emitted a bottom navigation row WITHOUT any fill at all,
relying on the parent surface for visual contrast — but the parent (the
cream root frame) doesn't supply that contrast, so the nav blends
straight into the cream background and visually disappears.
New deterministic pass: `injectMissingNavSurfaceFill`. For each direct
child of the page root whose role is one of {navbar, nav, tab-bar,
bottom-tab-bar, top-nav-bar, top-app-bar, tab-row} AND whose fill is
empty/missing, set `fill = [{type: solid, color: '$color-surface'}]`
so the renderer resolves it through the seeded palette and the nav
gets a visible white surface separation from the cream root.
Scope contract:
- Only direct children of the passed root frame (page root). Nav frames
nested inside cards / sections / banners are left alone.
- Never overrides an existing fill — sub-agent intent (e.g. an
intentionally dark `top-app-bar`) is preserved.
- Pure mutation; returns `true` when any nav was patched.
Wired into the same hook point as `stripRedundantSectionFills` (via
`design-canvas-ops.ts::generationCleanup`), so every generation cycle
sees both a strip pass (remove hedge fills) and an inject pass (add
the missing nav surface). Five new tests cover all nav role variants,
preservation of existing fills, scope (no recurse into cards), and
no-op on unrelated roles.
Two related issues from the GPT-5.5 food-app run:
1. Bottom navigation rendered without its surface fill, blending into
the cream root background. The strip-redundant-section-fills pass
didn't have any of the navigation roles (`navbar`, `nav`, `tab-bar`,
`bottom-tab-bar`, `top-nav-bar`) in PROTECTED_ROLES, so a navbar
carrying `fill: #FFFFFF` (or any SAFE_LIGHT tint) hit the
"safe-light hedge" branch and got stripped. Real-world navs
intentionally use a white surface to separate from a tinted root —
that fill is intended, not a hedge.
Fix: add the five navigation role names to PROTECTED_ROLES. New
test asserts a `role: navbar` frame with `fill: #FFFFFF` on a
`#FFF8F0` cream root keeps its fill.
2. Empty-src image placeholders inserted by the dispatcher's JSONL
fallback only got auto-filled at the orchestrator's tail (line
~1219, after every subtask completes). On a long brief that's a
visible lag; on an aborted/throwing brief the tail never runs and
images stay placeholder forever.
Fire-and-forget `scanAndFillImages(parentId)` from the dispatcher's
applied path so each subtask's image set starts searching as soon
as it lands. The orchestrator-tail scan still runs and dedups
through `queuedNodeIds`, so this is purely a latency / robustness
improvement (no double fetch).
`store.addNode(null, …)` routes the insert through `_children()` →
`getActivePageChildren(doc, activePageId)` — meaning the parent list
is the ACTIVE PAGE's children, not `doc.children`. The previous
append-index calc read `doc.children?.length` directly, which only
holds the legacy single-page fallback array. On a multi-page doc the
two diverge: `doc.children` may be empty or stale while the active
page already has N siblings, so the computed append index doesn't
correspond to the actual insertion target — landing either before
existing siblings (off-by-N) or out of bounds.
Use `getActivePageChildren(document, activePageId)` to read the same
list `addNode` writes into. Sub-agent generation runs on whichever
page the user has active, so this matches dispatch behavior exactly.
The non-null parent path (`getNodeById(parentId)` then read its
children length) was already correct — only the null-parent branch
needed fixing.
