fix(capture): let vision captioning authenticate the way a server can - #3561
Conversation
Three defects in one phase, all of which end with a capture that reports "Captioned N/N images" and then "0 images captioned with Gemini" — a successful-looking run that hands the agent nothing to see by. 1. Credential. The captioner only accepted an API key. A server deployment holds a service account, not a key, and a rejected key is indistinguishable from an unset one here: every request returns empty text and no error. Vertex is now a first-class provider, ranked above the bare key and below an explicit OPENROUTER_API_KEY opt-in, configured by HYPERFRAMES_VERTEX_SERVICE_ACCOUNT + HYPERFRAMES_VERTEX_PROJECT_ID (region via HYPERFRAMES_VERTEX_LOCATION). It carries its own model default because the Gemini API's flash-lite preview id is not resolvable on Vertex. 2. Empty captions. Thinking tokens are drawn from maxOutputTokens, so a model left free to think can spend the whole budget and return no text — a successful request with no caption. thinkingBudget is pinned to 0; a one-line factual caption needs no reasoning. 3. Native abort. Rasterizing a batch of SVGs concurrently drove up to SVG_BATCH simultaneous librsvg renders through libvips and corrupted the heap: `free(): unaligned chunk detected in tcache 2` (SIGABRT) during this phase, twice in fourteen days, losing the whole capture each time. A native abort cannot be caught, so the concurrency is removed rather than handled — rasterization is serialized and libvips' worker pool is bounded, while the vision requests, which are the slow leg, stay parallel. Throughput barely moves: 225 captions across three real captures, 0 failures, 13-25s each. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The serialization fix shipped without a regression test on the grounds that native heap corruption is not unit-testable. The corruption is not, but the property that prevents it is: `sharp` is mocked to record how many renders are in flight, and a six-SVG batch must never reach two. A deliberately slow caption stub makes overlapping renders the faster path, so a future refactor that "optimises" the loop back to `Promise.all` fails here instead of aborting in production. Also covered: `sharp.concurrency(1)` is applied — serializing the loop while leaving libvips' pool at the host core count still fans one render across every core — and an unrasterizable SVG is skipped without breaking serialization for its siblings. Verified as a real guard: reverting only contentExtractor.ts to origin/main fails 7 of the 22 cases in this file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7b09a69 to
436c1b6
Compare
…rtex captioned The provider gate in `contentExtractor` accepts Vertex when a project and a service account are both set -- which is the configuration a server deployment actually has. The header written next to the captions still tested only for an API key, so a capture whose captions Vertex had just generated was labelled "GEMINI_API_KEY not set -- descriptions below are catalog-derived". That header is not cosmetic: it travels into the context the template editor reads, telling it to distrust captions that are real. Mirror the same two variables here, and name every provider in the fallback text instead of only the API key.
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at head 4cdc0d962e0fcf8f188b864afd3f7c7bbf140693. All 8 required contexts on main ran and passed (Build, Test, Typecheck, Test: runtime contract, regression, Render/Tests on windows-latest, Semantic PR title) — derived from the ruleset endpoint and matched by exact name.
Strengths
contentExtractor.ts:266-303— "a rejected key here is indistinguishable from an unset one: every request returns empty text, no error is raised, and the phase reports success" is the actual root cause, and it explains the reported signature (Captioned 12/12followed by0 images captioned) rather than just asserting it. Naming two independent causes of one symptom — auth and the thinking budget — is what makes this a fix instead of a patch.thinkingConfig: { thinkingBudget: 0 }— thinking tokens drawing frommaxOutputTokensis a genuinely non-obvious failure mode, and pinning it to 0 for one-line factual captions is right.- Serializing rasterization rather than wrapping it: a native
SIGABRTfrom libvips cannot be caught by the surroundingtry/catch, so removing the concurrency is the only fix that works. Correct call, and the comment says why. - The unparseable-service-account path warns without echoing the credential, and there is a test asserting that. Easy thing to get wrong.
Findings
important — sharp.concurrency(1) is a process-global set inside a captioning function and never restored (contentExtractor.ts, the SVG branch). libvips' pool is process-wide, so every later sharp operation in the same process stays single-threaded after any capture that reaches the SVG phase — including callers that never asked for captioning. The bound is correct for the rasterize loop; the scope is wider than the reason for it. Reading the prior value and restoring it after the loop, or setting it once at CLI entry with this comment attached, keeps the fix without the side effect.
note — thinkingBudget: 0 also changes the existing API-key path. The thinkingConfig lands in the shared @google/genai branch, which serves both vertex and gemini. The Gemini-API default model is still gemini-3.1-flash-lite-preview, and every measurement in the description is a Vertex capture. Some Gemini models reject a zero thinking budget outright, and if that preview id is one of them the API-key path moves from "0 captions" to "request error". One capture on the GEMINI_API_KEY path would settle it.
note — sequencing, and why closing #3362 is right. This PR reads HYPERFRAMES_VERTEX_SERVICE_ACCOUNT / HYPERFRAMES_VERTEX_PROJECT_ID; #3362 read GOOGLE_SERVICE_ACCOUNT_INFO with an ADC fallback and the SDK's own GOOGLE_GENAI_USE_VERTEXAI convention. Nothing on experiment-framework master sets the new names today (0 hits), so on its own this PR changes nothing in production — experiment-framework #48887's _vision_credentials_env is what bridges from the GOOGLE_SERVICE_ACCOUNT_INFO that EF already carries in 25 places. That makes the namespaced names the better contract and #3362 genuinely superseded, but it also makes the stated order load-bearing: this merges and publishes first, EF second.
Verdict: APPROVE
Reasoning: Three real causes of one production failure, each fixed at the right layer with measured evidence (225 captions across three sites, zero failures). The sharp.concurrency scope is worth tightening but does not block.
— Rames Jusso
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head 4cdc0d962e0fcf8f188b864afd3f7c7bbf140693.
I traced the provider decision and its downstream truthfulness as one contract. OPENROUTER_API_KEY remains the explicit override; Vertex requires both project and service-account JSON; Gemini remains the final key-backed path. The asset-descriptions.md header now mirrors that same predicate, so a Vertex-captioned capture no longer tells the editor that its descriptions are catalog-only. Partial or malformed Vertex credentials fail closed, the warning does not echo credential material, and the model/location defaults are provider-specific.
The native-crash fix is also at the right boundary: SVG rasterization is serialized before the still-parallel vision leg, and the regression measures actual concurrent Sharp renders rather than mocking the production predicate back at itself.
I concur with Rames on the non-blocking process-global scope of sharp.concurrency(1): it persists after this function and can reduce later Sharp throughput in the same process. That should be restored after this loop or moved to a deliberate process entrypoint, but it does not invalidate the capture result.
All exact-head checks are green, including Windows, runtime contract, preview parity, and all regression shards.
Verdict: APPROVE
Reasoning: The service-auth, empty-caption, and native-abort failure modes are fixed with discriminating tests and a consistent downstream credential contract. No blocking correctness issue remains in this PR.
— Magi
`sharp.concurrency(1)` is process-global and was set once, for the whole life of the process. The bound is right for the rasterize loop -- a native abort in libvips cannot be caught, so the renders must not overlap -- but its scope was every later sharp caller in the process, none of which asked for captioning, all of them pinned to one thread from then on. Now the host's value is read first and restored in a `finally` around the rasterize loop, so a skipped SVG cannot cost the process its threads either. The vision requests below are network work and gain nothing from a pinned pool. The mock had to grow the getter half of sharp's API -- `concurrency()` with no argument reports the current value -- since save-and-restore is untestable without it. Verified as a real guard: dropping only the restore fails both new cases. Raised by Rames Jusso in review of #3561 and concurred by Magi. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed at
The mock had to grow the getter half of sharp's API ( |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 1a519a21c12e5bb94a7c02b2263cbc227d53624c.
The only code delta since my prior approval closes the process-global Sharp scope note correctly. It snapshots sharp.concurrency() before the SVG work, applies 1 only around each serialized raster batch, and restores the host value in finally before the parallel network caption calls. The restore therefore runs for both ordinary success and a per-file raster failure, and the two new tests discriminate both cases with a non-1 host value.
All eight required exact-head checks pass. The optional player-perf suite is being rerun after an unrelated scrub failure; this capture-only delta does not touch the player path and it is not a required merge context.
Verdict: APPROVE
Reasoning: The original functional review stands, and the follow-up now bounds the native pool without leaking a process-wide throughput change.
— Magi
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 1a519a21. My earlier approval was pinned to 4cdc0d96, so it no longer covered this head.
This commit implements a note I raised, which is exactly the case where a reviewer confirms instead of checking, so I re-derived it from the file rather than reading the test names.
Verified:
contentExtractor.ts:518capturessharp.concurrency()before anything sets it, and after the sharp-load guard at:502-510, so the saved value is the real host value rather than a1this function just installed.:540setssharp.concurrency(1)inside the per-batch loop, withtryat:541andfinallyat:575restoring at:578. The bound is scoped to the renders that need it and given back on the throw path, which is the part that matters — a skipped SVG previously cost the rest of the process its threads for the life of the worker.- The mock now models the real dual API (getter with no argument, process-global setter otherwise). That was a prerequisite, not incidental: with a setter-only mock the restore is untestable.
HOST_CONCURRENCY = 8seeded insharpStateis the right call — asserting against8means a missed restore surfaces as a wrong value instead of coincidentally matching1.- The added throw-path test rasterizes into a directory where an SVG should be, which is a fair stand-in for how an exotic SVG fails through sharp.
CI at this head: all 8 required contexts green, matched by exact name (Build, Render on windows-latest, Semantic PR title, Test, Test: runtime contract, Tests on windows-latest, Typecheck, regression). Five Perf: runs are still in progress; none of them is a required context, so they are not gating — worth a glance when they land, since this touches the rasterization path.
Verdict: APPROVE
Reasoning: The process-global leak I flagged is closed at the right scope with the restore on the error path, and the test changes make the restore observable rather than assumed.
— Rames Jusso
|
Thanks — and your correction on Your happy-dom measurement saved a fake test on #3562. I reproduced it ( So I went at the root cause instead: a lint rule for treating a timeline's return value as a tween. Details on #3562. |
Why
Four mobile-ad template remixes were reported by a customer-facing team; all four shipped an identity that was not the advertiser's. Tracing them, the agent's problem was never the prompt — it was that nothing it could read ever reached it. This PR fixes the capture-side half of that. (The consuming half is in
experiment-framework; see Sequencing.)The failure signature is a capture that looks fine:
Six of six stdout-logged captures that reached the vision phase showed exactly that. Three separate causes:
1. The captioner could only authenticate with an API key. A server deployment holds a service account, not a key — and a rejected key here is indistinguishable from an unset one: every request returns empty text, no error is raised, and the phase reports success. Vertex is now a first-class provider, ranked above the bare key and below an explicit
OPENROUTER_API_KEYopt-in:HYPERFRAMES_VERTEX_SERVICE_ACCOUNTHYPERFRAMES_VERTEX_PROJECT_IDHYPERFRAMES_VERTEX_LOCATIONus-central1HYPERFRAMES_VERTEX_MODELgemini-2.5-flashVertex carries its own model default deliberately: the Gemini API's flash-lite preview id is not resolvable there.
2. Thinking consumed the whole output budget. Thinking tokens are drawn from
maxOutputTokens, so a model left free to reason can spend the entire budget and return no text — a successful request with no caption, which is the second half of the log above.thinkingBudgetis pinned to0; a one-line factual caption needs no reasoning.3. A native abort was losing whole captures. Rasterizing an SVG batch concurrently drove up to
SVG_BATCHsimultaneous librsvg renders through libvips and corrupted the heap:Twice in fourteen days, each time losing the entire capture. A native abort cannot be caught by the surrounding
try/catch, so the concurrency is removed rather than handled: rasterization is serialized and libvips' worker pool is bounded, while the vision requests — the slow leg — stay parallel.Result
Measured on three real captures with a real service account:
225 captions, zero failures, zero crashes. Serializing rasterization cost effectively nothing.
Tests
packages/cli/src/capture/contentExtractor.test.ts— 6 new cases for the Vertex path (13 → 19 in the file):vertexai/project/region/credentials rather thanapiKeythinkingConfig: { thinkingBudget: 0 }and the Vertex-specific model defaultinternal-errorwithout echoing the credential into warningsOPENROUTER_API_KEYstill winsFull
packages/clisuite: 19 failed / 1587 passed before this change, 19 failed / 1593 passed after — the same pre-existing failures (unbuilt workspacegenerated/artifacts), plus the 6 new passes.The serialization fix now has a guard too
The first draft of this PR flagged the rasterization fix as untested on the grounds that native heap corruption is not unit-testable. The corruption is not — but the property that prevents it is. Second commit mocks
sharpto record how many renders are in flight and asserts a six-SVG batch never reaches two, with a deliberately slow caption stub so that overlapping renders would be the faster path. A refactor that "optimises" the loop back toPromise.alltherefore fails here instead of aborting in production. Also covered:sharp.concurrency(1)is applied, and an unrasterizable SVG is skipped without breaking serialization for its siblings.Verified as a real guard, not a tautology: reverting only
contentExtractor.tstoorigin/mainfails 7 of the 22 cases in this file.End-to-end
This build has since been run through
experiment-framework's real capture activity (heygen-com/experiment-framework#48887) against all three reported sites, via a PATH build labelled0.8.21-vertex.1so the pinned-version check accepts it. All three returnedbrowser_capturewith real Vertex captions and zero failures.Sequencing
experiment-frameworkpasses these env vars through, but it pinsHYPERFRAMES_VERSION = "0.8.15", and 0.8.15 contains noHYPERFRAMES_VERTEX_*handling (verified against the published tarball). So:chore: releaseships it to npmUntil step 3, the EF side is inert. Worth merging this one first for that reason.
🤖 Generated with Claude Code