test(cli): cover bounded capture stages - #2929
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Regression matrix for #2928, cleanly separated. APPROVE.
Notable coverage:
- Phase-record PII/credential leak guard (
capture.test.tsline 350-379) — useshttps://user:secret@example.com/privateand asserts the emittedHYPERFRAMES_CAPTURE_PHASEline does not contain"secret"or"/private". That's the right invariant to lock down — the phase schema legitimately carries no URL fields, but this test would catch a well-meaning-but-wrong "let's include the URL for debuggability" future edit. - Timeout-error path race — the "keeps a successful sibling caption when another request times out" test (line 116-146) uses call-order-based hang: first fetch returns a never-resolving Promise, second returns success. Since both fixtures share the same PNG magic bytes (
iVBORw==in base64), theimage_url.url.includes("iVBORw==")check identifies both, and the mock'smock.calls.length === 1distinguishes them by ordering. VerifiesPromise.allSettledcorrectly isolates timeouts to their own request. Nice. - SDK-ignores-abort scenario — the Gemini path test (line 194-208) mocks
generateContenttonew Promise(() => {})(never settles, ignores abort signal) and races against a 250ms wrapper timeout. Asserts capture terminates. This is the actual failure moderunBoundedVisionRequestwas built for — good behavioral test. - Contact-sheet budget test's
checks++ === 0pattern — first call returns 1000, subsequent return 0. Trace:createScrollContactSheetbuilds paths →createContactSheetPagesiterates pages → page 0 processes (first check: 1000, ok) → page 1 gate check (0, break). Result = one page. Correct — validates per-page budget threading viacreateContactSheetPageseven thoughsheetOptsstripsremainingMs.
Nits (none blocking):
checks++ === 0pattern is order-sensitive — ifcreateContactSheetPagesgets refactored to hoist a singleremainingMs()call out of the page loop, the "expects 1 page then breaks" test will misfire (checks would only be called once). Fragile, but the alternative (fake-time-based cutoff) is more complex. Acceptable trade-off for this invariant.- The
line.slice("HYPERFRAMES_CAPTURE_PHASE ".length)slice-magic — clarity would improve with.replace(/^HYPERFRAMES_CAPTURE_PHASE /, "")since the presence of a trailing space in the prefix constant is easy to miss. Not a defect.
CI clean at 647a5ec7.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 647a5ec7.
Focused test PR — exercises the runtime contract from #2928 at real budget boundaries. Strongest additions: the vision-hang scenarios prove the timeout wraps even when the provider never resolves ("terminates captioning when the vision provider never responds"), the mixed hang+success case verifies Promise.allSettled degrades one image without dropping siblings ("keeps a successful sibling caption when another request times out"), and the Gemini mockImplementation(() => new Promise(() => {})) block validates the outer race even when the SDK ignores its own AbortSignal ("terminates captioning when Gemini never settles even if the SDK ignores abort") — that last one is the exact "SDKs that ignore abort" scenario #2930's body cites. Contact-sheet tests use the checks++ counter to force budget exhaustion between pages, which is a clean way to hit "next iteration would exceed budget" without racing the wall clock.
Concerns
None.
Nits
HYPERFRAMES_VISION_TIMEOUT_MS=20+ outersetTimeout(250)race in the vision-hang tests leaves ~230 ms of slack. On a loaded CI runner (bun install cold cache, shared fs) the internal timeout can drift, and the outer race is a coarse "did we escape?" probe. Currently green, but if this ever flakes, doubling the outer race to 500 ms or bumping the internal to 50 ms would keep the shape and buy headroom. Not a blocker.- PNG magic-only fixtures (
Buffer.from([0x89, 0x50, 0x4e, 0x47])) survive the extension filter but wouldn't survive a real decoder. Correct here because every path isfetch-/generateContent-stubbed, but a one-line comment ("byte contents are irrelevant to these tests; only the .png extension matters for the image filter") next tomakeProjectWithImageswould flag intent for future readers who see the fixture and try to parse it.
What I didn't verify
- I didn't repro the tests locally; CI is green at
647a5ec7.
— Review by Rames D Jusso
ca9bba1 to
765a5ae
Compare
647a5ec to
45aead8
Compare
What
Add the larger regression matrix for bounded contact sheets, vision enrichment, phase telemetry, and capture-command JSON output.
Stack 2/4 for STUDIO-5419. Base:
fix/studio-5419-capture-runtime. Next:fix/studio-5419-capture-budget-propagation.Why
Keeping the regression matrix separate from the runtime implementation makes both reviews focused and keeps every PR below 700 changed lines.
How
Exercise the public command and capture helpers at their budget boundaries without changing production behavior in this layer. Changed LOC: 339.
Test plan