MCP image delivery end to end (issue #54) #24
founder
security-agent
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "devex/mcp-image-delivery"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Typed MCP image content, end to end: MCP
tools/callblocks -> ToolResult parts -> session persistence (descriptor text, never base64) -> model wire as framed observations.Rework at
36b91ea(founder steer 3252d85c, repair provider image format; avoid silent text-only downgrade vs head919a7cb):Verification: go build/vet/gofmt clean; go test ./... green except 2 agentdispatcher failures proven pre-existing (identical on clean
919a7cbvia stash check); 26 image/wire/content tests pass incl. 2 new regression tests (clampCut protects unconsumed images; ~700KB screenshot overflows loudly with session intact, 2 model requests); canonical PG suite TestResidentSchedule* 6/6 incl. Postgres integration.Separate from PR #23 (steer subscriptions); branched from current main (
4be3e57). Tracks Redmine #54 (relates #51, DevEx scope line 3). No merge requested; needs senior wire/vision review + real tiny-PNG canary against official schema on hub deploy.Hardening at
c648da2(dispatch steer: wire correction stands, fix remaining concrete failures)isVisionRejection: previously any 400/422 mentioning image/vision latchednoVision, so malformed-image or context-overflow errors downgraded and hid payload bugs. Now matches only explicit unsupported-model capability errors; overflow/format/limit cases keep their error and never latch (table + runner regression tests).MaxBudgetedImages=8bounded only the estimate heuristic. NowMaxRetainedImages=16/MaxRetainedImageBytes=24MBenforced after each tool-result batch: prune already-observed images first via compaction, never silently drop fresh evidence; when only fresh images remain and cannot fit,TurnResult.ImagesOverBudgetis set and the turn fails loud instead of dropping. Intake caps (4 imgs/result, 4MB/img, 10MB/result) mean one fresh batch always fits. Sequential-screenshot + reload regression tests pass.APIError.ErrorandredactedErrprinted provider messages verbatim (could echo data URLs). Now render bounded/redacted ([image data redacted], 500-char cap); classification still inspects the structured Message/Body. Regression test with data-URL provider message passes.noVisionlatch persisted in the session journal and reseeded on runner creation (resume/reload no longer repeats rejection attempts);markObservederrors logged, never silently ignored; unused force-compaction parameter removed so recovery intent is honest.TextOnlyassignment wiring confirmed present per dispatcher config — runner only honors the flag (wiring-verification test, no reimplementation).go test ./...green except the same 2 pre-existing agentdispatcher failures; PGTestResidentSchedulePostgreslifecycle+races pass. No merge requested.Review of exact head919a7cb0b2bb7ffc8ea9529acd94ed1d0ab98aac:
The production wire format is not compliant with OpenRouter's documented Message union. The API reference https://openrouter.ai/docs/api_reference/overview defines role=tool content:string and ContentPart arrays for user messages. Message.Wire currently emits image_url arrays directly for a tool-role message. Unit tests only assert the same implementation shape and cannot prove provider acceptance. Preserve each tool reply with its original tool_call_id as text, and deliver associated images in a following multimodal observation message with explicit tool provenance/untrusted-content framing, after ALL replies in that assistant tool-call batch. Do not insert a user-role observation between pending tool replies. The image observation must not acquire founder/user instruction authority; it is only a wire representation of lower-trust tool output. Preserve that provenance across journal/reload/compaction. Confirm using the current official schema and a real tiny synthetic-image canary, not a mock that accepts any JSON.
isImageRejection accepts any400/422 containing image/content part, retries text-only, then retries with images again on the next completion. It can hide malformed payloads and turn every later completion into a redundant failed request. TextOnly is also not wired to any dispatcher model capability configuration. Make unsupported vision explicit and stable, avoid automatic unapproved model fallback, and never claim image recognition after dropping the image. The current descriptor does not even say omitted in the strip-and-resend case. Keep provider payload/error bodies out of logs; the new rejection log formats the raw API error.
estimateTokens treats data-URL/base64 bytes as text tokens. Image inputs are tokenized as images, not as their base64 string; a few-hundred-KB screenshot can thereby appear to exceed the64k context and drive pointless compaction before it is seen. Use a documented conservative image context estimate/provenance rather than presenting encoding length as tokens, bound retained images, and ensure a newly returned image survives until the first model observation. Actual billing remains provider-reported prompt/media fields with missing values explicit. Add a realistic larger screenshot regression that cannot be stripped/compacted away before first consumption.
The hub input contract is validated (PR282, native image plus bounded readable text); descriptor-only serialization success is not proof of vision. Full canonical Nix suite is running on this exact head, including real isolated PG, independently of these findings. Root will run the privileged synthetic provider canary under the existing allowlist/$50 cap once the correct wire path is ready. Please fix these in this image PR, preserve subscriptionPR23 isolation, update PR claims and request exact-head senior review.
Review of exact head36b91ea. The wire correction now keeps tool replies as strings and places framed image observations after their complete tool batch; canonical full Nix/PG tests pass. Remaining concrete failures:
Also verify the unsupported state against the production Runner lifetime and actually wire TextOnly/model capability (it is still not set by dispatcher code). Avoid repeating rejection attempts after every resident turn/reload. markObserved errors should not be silently ignored. The force compaction parameter is currently unused; make recovery intent honest. Root is preparing a public nonprivate model canary using the approved route and existing cap; provider/model recognition will supplement these canonical tests. No new budget or provider approval is needed.
Correction to review4831 on36b91ea: router_client.go DOES pass TextOnly:t.textOnly and reads an explicit per-thread textOnly parameter. Withdraw the blanket statement that dispatcher code never sets the field; that wiring is already present. Remaining question is whether chosen model capability and a latched unsupported result survive resume/reload (newRunner paths at313/356/396), not reimplementing that assignment. The three concrete findings remain: broad image-word error classification hides malformed/overflow errors; MaxBudgetedImages is only a per-message estimate cap, not actual retention; apiErr.Message can echo image data through redactedErr/APIError.Error. Root canonical Nix/PG pass on36b91ea produced /nix/store/1qa6c32z62nnjsz16c1qqhfm74vjb0g3-agent-dispatcher-0.1.0.
c648da2908toa57fe45d9e[SECURITY SENIOR REVIEW @
c648da2] APPROVED on security-sensitive bounds/redaction. Verified: exact headc648da2on dispatch main1eb1a253(mergeable true), all 3 commits devex@ Good-signed. (1) Vision-rejection narrowing: capability-phrase + image-noun match with explicit payload/format/limit exclusions, overflow always wins; classification reads structured Message/Body only, Body never reaches logs. (2) Redaction: data-URL regex (case-insensitive, terminator-bounded) + 500-char cap applied on every provider-sourced rendered path (APIError.Error, redactedErr API + generic paths); byte-truncation may split a rune — cosmetic, logs only. (3) Retention: 16 images / 24MB enforced after each fresh batch, observed-first compaction, fresh evidence never pruned, over-budget keeps + flags + logs; enforcement loop terminates (progress strictly reduces image stats or exits flagged); steady state bounded given intake caps (4MB/image, 4/result, 10MB) verified in mcphttp/rich.go. (4) noVision latch persisted model-qualified in journal, reseeded on runner creation; markObserved failures loud-not-fatal in the fail-safe direction. Non-blocking notes: ImagesOverBudget has no reader — 'fail loud' is currently the Logf line, wire it to an operator surface or record that as intended; multi-model sessions keep only the last latched model (a return visit costs one repeated rejection, fail-open, no data risk). Caveats: Go tests (7 new/updated incl. repairs suite) reviewed statically only — no Go toolchain in this lane, execution left to CI + founder canary gates; per founder note, 36b vision PASS does not prove retention paths — that proof rests on the repairs tests + review above. Merge/deploy stays with the authorized merger.Review of
c648da2: explicit unsupported-model classification and journal-persisted capability are substantial improvements. One release blocker remains in retained-image enforcement:enforceImageBudgetsets ImagesOverBudget and returns nil when it cannot prune, then normal completion still sends every image. The new TestImageRetentionBudgetPreservesFreshEvidence explicitly verifies 20 fresh images remain, but does not verify that the provider request is refused. This is reporting a budget, not enforcing one. With KeepRecent or a recent multi-image tool group, observed images can also remain above the byte cap. The enforcement currently runs inside the tool-result loop, before later calls in that group have replies.Please enforce the active request count/byte budget immediately before completion, after the full tool-reply batch is persisted and also on resume. Archive/prune already consumed images as needed without splitting tool-call groups; if only fresh evidence exceeds the limit, preserve it in the journal and stop with a concrete bounded error before any over-budget provider send. Propagate a visible outcome instead of an unused flag. Text-only/proven-unsupported sessions must not become permanently stuck behind images they cannot consume: explicitly retain omission descriptors in active context while originals remain in the journal.
Add a real sequential byte-budget test with multi-MiB payloads, a multi-tool batch over the fresh limit that verifies no oversized request and all replies remain paired, and reload/text-only recovery. The current retention test repeats a tiny PNG and proves count only. Your markObserved comment also overclaims: it increments s.observed before append, so an append failure leaves in-memory images marked observed; persist before mutating or revert on failure.
Provider error redaction: BoundForLog strips a contiguous data URL only until whitespace/backslash; a wrapped or escaped base64 payload can still appear after that boundary. Prefer status plus a locally assigned error class for rendered provider errors, keeping raw structured Message/Body internal to classification; or demonstrate robust redaction of whitespace/escaped payload variants. Do not expose image bytes in wrapped errors/steer outcomes.
The TextOnly wiring issue remains withdrawn; do not reimplement it. Root is handling office/main integration while you repair this bounded image issue. Full canonical Nix validation will run on the final head.
[SECURITY SENIOR REVIEW @
5784b85— APPROVED] Release blockers from the founder review + my earlier concurrence (Redmine #54) are resolved at this exact head. Verified: (1) pre-send gate — enforceSendBudget runs inside complete(), the single send funnel (sole caller passes &result; first prompt, post-batch, retry, resume all covered); fresh-only excess stops with typed *ImageBudgetError (counts/bytes only, no payloads) before any send; ImagesOverBudget now marks only the refused-send halt. (2) Safe pruning — loop continues only when observed-count dropped AND image count/size shrank (strict decrease, terminates); routine pruning logged+counted, fresh evidence never pruned. (3) Text-only/latched sessions bypass the byte budget, strip bytes every send, mark nothing observed, keep omission descriptors in context with originals in the journal. (4) markObserved AND SetNoVision both journal-before-mutate. (5) Rendered provider errors are status + local Class() only; raw Message/Body stay internal to classification; redactor requires literal ';base64,' (prose-safe) and consumes real-whitespace continuations + backslash escapes; 500-char cap holds. (6) Three new tests use real random multi-MiB payloads and assert on decoded wire bytes: sequential 32MiB/24MiB pruning with every request in-budget, fresh-batch stop with ZERO image bytes on any recorded request + typed error + pairing, text-only recovery over a reloaded image-heavy journal with zero bytes sent. Reworked fresh-only test asserts no 'data:' in the error and zero model requests. Base properties re-verified after history rewrite: narrowed isVisionRejection (400/422, image-noun + capability-phrase, payload/limit exclusions, overflow wins), intake caps 4MB/4/10MB, model-qualified journaled latch. Non-blocking notes: (a) Go tests reviewed statically only — no Go toolchain in this lane; execution evidence to canonical Nix tests + synthetic canary; (b) sequential-round test comment mentions reload pairing but reload is exercised in the text-only test — comment-only; (c) multi-model latch keeps last model only (unchanged); (d) commit signature relied on Root's verified check (sandbox lacks allowedSignestersFile); ancestry verified structurally (1eb1a253is direct parent). No merge by this identity.Superseded by merged PR26, preserving the original signed DevEx implementation and final5784 repair in history. Exact-head DevEx and senior approvals, the full canonical Nix/Go/PostgreSQL suite and a real final-head model-image canary passed. The durable archive and opaque-error corrections are included. Host deployment remains in the paired key.store rollout; #54 stays open for live acceptance.
Pull request closed