feat(image): inline preview + spend confirmation#21
Conversation
VickyXAI
left a comment
There was a problem hiding this comment.
Pre-landing review
Structurally clean. The image.ts budget/charge refactor that collapses the duplicated edit/generate branches is behavior-preserving: edit validations (!image, EDIT_MODELS) preserved, checkBudget before charge, recordSpending only after a successful call. sharp is already a direct dep (used in qr.ts), so no new dependency risk.
One thing to resolve before merge, plus a few nits.
1. [P1] Spend confirmation is ON by default — contradicts "no behavior change unless opted in"
With no env set:
CONFIRM_OFF=/^(0|false|off|no)$/i.test("")→ falseTHRESHOLD=Number("" || 0)→ 0, sousd <= THRESHOLDonly catches the free case already handled byusd <= 0
So a $0.06 gpt-image-2 call on any elicitation-capable client (Claude Code advertises elicitation) reaches elicitInput and prompts on every generation. The PR says "Defaults unchanged… no confirm prompt… no behavior change unless opted in" — that's false for those clients.
Pick one and make code + description agree:
- opt-out (confirm-by-default, defensible for pay-per-call) → fix the description, and consider a non-zero default
THRESHOLDso sub-cent drafts (cogview-4@ $0.015) don't each pop a dialog; or - opt-in → add a master enable gate (e.g. require
BLOCKRUN_CONFIRM_SPEND=on).
2. [P2] cancel/ESC is treated as approval → charge proceeds
Only action === "decline" aborts. MCP elicitation actions are accept | decline | cancel; a user who hits ESC/Cancel intending to abort still gets charged. The comment documents the desktop-app-returns-cancel quirk, so it's a deliberate fail-open — but "Cancel doesn't cancel" is surprising. Confirm it's intended, and consider saying so in the prompt text ("use Decline to cancel").
3. [P3] inline-image.ts — no size cap before buffering + MAX_BYTES comment is off
Buffer.from(await resp.arrayBuffer())buffers the whole response with no byte cap andsharp()decodes with nolimitInputPixels; theMAX_BYTESguard runs only after decode+encode. Trusted upstream URL so low risk, but a cheap download cap hardens it.MAX_BYTESchecksthumb.byteLength(raw JPEG) while the comment says "encoded thumbnail" — base64 inflates ~33%, so a 900 KB limit allows ~1.2 MB of actual context. Fix the check or the comment.
4. [P3] No tests for the new functions
confirmSpend (decline / session-approve / off / threshold branches), shouldInline (param-beats-env precedence), buildInlineImageBlock (null fallback) all have real branching logic and no coverage. A test pinning the decline-vs-cancel semantics from #2 would be the most valuable.
Net: resolve #1 before merge (default behavior + wrong description). #2 needs a one-line intent confirmation. #3–#4 are non-blocking.
blockrun_image gains two opt-in UX layers, both off / no-op by default: - Inline preview: an `inline` param (or BLOCKRUN_INLINE_IMAGES env) returns a downscaled JPEG thumbnail as a type:"image" block alongside the full-res URL, so rich clients render the result in-conversation. Auto-skips above a size cap and on any fetch/encode error (URL-only fallback). Tunable via BLOCKRUN_INLINE_MAX_DIM / _QUALITY / _MAX_BYTES. - Spend confirmation: before charging, confirmSpend() asks via MCP elicitation with an 'approve all this session' checkbox (session-scoped auto-approve). No-ops on clients without elicitation and is fail-open; only an explicit decline aborts. Controls: BLOCKRUN_CONFIRM_SPEND=off, BLOCKRUN_CONFIRM_THRESHOLD. New utils: src/utils/inline-image.ts (sharp thumbnail), src/utils/confirm-spend.ts.
Avoids double-prompting when a plugin already gates spend via a PreToolUse hook, and prevents an unexpected second dialog in the bare MCP. Spend confirmation now only runs when explicitly enabled.
42b9587 to
0894c73
Compare
- confirm-spend: clarify the elicitation prompt that Decline (not Cancel/ ESC) is what aborts the charge — the cancel=fail-open behavior is deliberate (desktop clients return 'cancel' on confirm) but was surprising. - inline-image: measure the MAX_BYTES guard against the base64 string that actually enters context (not the raw JPEG, which is ~33% smaller), and add defensive source caps (Content-Length + buffer ceiling, sharp limitInputPixels) before decode. - tests: cover shouldInline precedence, buildInlineImageBlock (null fallbacks + real encode), and confirmSpend (free/threshold/no-elicitation/decline/ cancel/approve-all/throw) — pinning the decline-vs-cancel semantics.
Two opt-in blockrun_image UX layers, both defaulting to prior behavior: - inline: a downscaled JPEG thumbnail returned as a type:"image" block alongside the full-resolution URL, so rich clients (e.g. the VS Code extension) render the result in-conversation. Best-effort (auto-skips above a size cap / on fetch/encode error); tunable via BLOCKRUN_INLINE_* env. New utils/inline-image.ts. - confirmSpend: MCP elicitation before charging, with an "approve all this session" checkbox. Off by default (opt in with BLOCKRUN_CONFIRM_SPEND=on). Fail-open — only an explicit decline aborts; session auto-approve latches only on an explicit accept. New utils/confirm-spend.ts. Re-integrated on top of the 0.24.x image.ts charge path (the PR was written against pre-0.24 code): confirmSpend runs once inside the reserveBudget try/finally — a decline releases the reservation and charges nothing — and the SSRF guard + Content-Length cap on edit source images stay intact. Co-authored-by: KillerQueen-Z <1211904451@qq.com>
…tion Lands PR #21 (re-integrated on the 0.24.x charge path). Both features default off; SSRF/Content-Length/reserveBudget guards intact. 84 tests.
|
Thank you @KillerQueen-Z! 🙏 Landed in v0.25.0 (commit Since What changed in the re-integration:
Closing as landed. Thanks again for the contribution + the thorough review follow-ups! |
Follow-up to #20 (profiles). Adds two opt-in UX layers to
blockrun_image— both default to prior behavior, safe to publish.1. Inline image preview
inlineparam (orBLOCKRUN_INLINE_IMAGES=1) returns a downscaled JPEG thumbnail as atype:"image"block alongside the full-res URL, so rich clients (VS Code extension, desktop) render the result in-conversation. Best-effort: auto-skips above a size cap and on fetch/encode errors (URL-only fallback). Tunable:BLOCKRUN_INLINE_MAX_DIM/_QUALITY/_MAX_BYTES. New filesrc/utils/inline-image.ts(uses existingsharpdep).2. Spend confirmation
confirmSpend()asks via MCP elicitation before charging, with an "approve all this session" checkbox (session-scoped auto-approve). Off by default — opt in withBLOCKRUN_CONFIRM_SPEND=on(avoids double-prompting when a plugin already gates spend via a PreToolUse hook). Threshold viaBLOCKRUN_CONFIRM_THRESHOLD. No-ops on clients without elicitation; fail-open — only an explicitdeclineaborts (so a confirmed call is never wrongly cancelled; some desktop clients returncanceleven on confirm). New filesrc/utils/confirm-spend.ts.Scope / testing
src/tools/image.ts(+ 2 new utils + 2 new test files). Rebased on currentmain(carries the multi-image/mask edit work; charge path unified so confirmation runs once before the edit/generate call).inline, no confirm prompt, no behavior change unless opted in.tsc --noEmitclean;npm run buildsucceeds;npm testgreen (25 tests).Review follow-ups (addressed)
BLOCKRUN_CONFIRM_SPEND=on); description updated to match.cancelon confirm) but the prompt now says "choose Decline — Cancel/ESC lets it proceed"; semantics pinned by tests.MAX_BYTESnow measures the base64 string (what actually enters context), plus Content-Length + buffer +limitInputPixelssource caps before decode.shouldInline,buildInlineImageBlock, andconfirmSpend(incl. the decline-vs-cancel branches).