Subject: Architect · Griller: Griller · Orchestrator: Butler Repository: gosharplite/tell-me-go · Round: 10 questions (full), early-conclusion not needed Outcome: Griller verdict — proceed with changes
SUBJECT OPENING STATEMENT — Issue #1447 (OpenAI gpt-5.x vision via the Responses API)
What the issue is really asking, and whether its premises hold. The issue claims three things, and I verified all three against the live tree: (1) capability layer — resolveGPTFamily (internal/domain/llm/capabilities.go:90-95) returns only isReasoner/requireResponses, and ResolveCapabilities sets SupportsVision only from resolveKimiFamily/resolveDeepSeekFamily (capabilities.go:155-157); every GPT row in capabilities_test.go pins supportsVision: false, including gpt-5, gpt-5.3, gpt-5.4, gpt-6/7/10.1. (2) Transport — hasVisualCapability (chat.go:319) = SupportsVision || SupportsVideo, so prepareMediaForTurn (chat.go:327) no-ops for GPT; the drop is enforced at hasSupportedMedia (client.go:486), mediaBlockFor (if !caps.SupportsVision → drop, client.go:851-853), and mediaOmittedFallback (client.go:521) — with the Chat-Completions image_url path and the DeepSeek file block path already implemented and tested. (3) The Responses sink responsesSink.AddMessage (responses.go:40-47) warns responses_sink_non_string_content and drops non-string content, with the now-stale comment "no model has both SupportsVision + RequiresResponsesAPI today," pinned by TestResponsesSink_AddMessage_NonStringContent (client_edge_test.go:284-295). The two independent routing gates also check out: resolveAPIStrategy = RequiresResponsesAPI && toolCount > 0 && hasEffort (chat.go:120-124) and resolveEndpoint = RequiresResponsesAPI && len(req.Tools) > 0 && (req.Reasoning != nil || req.ReasoningEffort != "") (chat.go:293-296), pinned by TestResponsesAPIRouting (client_responses_edge_test.go:428-556). The issue's line numbers are slightly off in places (e.g. it cites client.go:530/697/810; actual are 521/793/706) but the substance is accurate. The domain model indeed mentions vision nowhere (0 matches in docs/domain-model/), and gpt-5.6-luna parses via Sscanf → major=5, minor=6 → both isGpt5OrNewer and isGpt54OrNewer true, so the D2 boundary machinery works.
Position on D1/D2/D3. I endorse D1 Option A (force /responses when a turn carries image parts on a RequiresResponsesAPI model). Option B leaves the headline no-tool use case ("what's in this image?") falling to Chat Completions and silently placeholder-dropped via mediaOmittedFallback — the exact "Out of scope" trap the issue names. But Option A has an edge the issue under-specifies: with useResponses now decoupled from hasEffort, a no-tool image turn on a model configured without a reasoning_effort header would emit req.Reasoning = &reasoningConfig{Effort: ""} → "reasoning":{} on the wire (buildRequestBody chat.go:141-145) — today that combination is impossible because useResponses implies hasEffort, so this is an untested wire shape. The fix must decide the effort emission for no-effort image turns (omit Reasoning and make resolveEndpoint use the same shouldUseResponses decision rather than req.Reasoning != nil), and ideally extract a single shouldUseResponses(toolCount, hasImage) consulted by both gates — exactly the synchronization the issue asks for. I endorse D2 Option A (all gpt-5+ via isGpt5OrNewer): it is the suffix-free, anti-allowlist boundary consistent with ADR-070 Decision 1's rationale, reuses existing code, and the 5.0–5.3 Chat Completions image_url path is already implemented; Option B would create a second axis (vision at 5.4 vs RequiresResponsesAPI at 5.4) that is redundant and forces D1 to be strictly mandatory. I accept the stated consequence: vision lives on both wire surfaces, and the responses_sink_non_string_content drop stays as a defensive branch for hypothetical future RequiresResponsesAPI && !SupportsVision models. I endorse D3 Option A (inline base64 data URLs, FileUploadNone), but with a correction: the issue's suggestion to "reuse maxInlineMediaBytes/maxRequestBodyBytes" (32 MiB / 48 MiB, client.go:683-687) is dangerous because those constants are DeepSeek-documented limits, not OpenAI limits. OpenAI's documented inline image limit is smaller than 32 MiB in the Responses vision guide, so reusing them would let 20–32 MiB images through to a guaranteed 413/400 — the very failure D3 exists to prevent. The size check (checkDeepSeekMediaSizes, client.go:793-810) must be generalized from a DeepSeek-gated guard to a mode-parameterized guard that runs for any vision-capable model, with OpenAI-specific per-image and aggregate caps taken from the live docs the issue cites.
Implementation plan (order). (1) Capability layer: resolveGPTFamily returns supportsVision = isGpt5OrNewer(v); update the SupportsVision/SupportsVideo doc comments (capabilities.go:100-111); flip the GPT rows in capabilities_test.go and add gpt-5.6/gpt-5.6-luna (asserting SupportsVision: true + RequiresResponsesAPI: true) plus boundary rows on both sides (gpt-4.5 false, gpt-5.3 true). (2) Responses serialization: the sink must translate, not just stop dropping — requestContentBlock (client.go:336-340) carries only Type+Text, so a new input_image block shape ({"type":"input_image","image_url":"<string>","detail":"…"}) is required; appendMessagesFromHistoryItem (client.go:565) already feeds both sinks the shared Chat-Completions-shaped output, so the Responses sink must convert imageURLBlock→input_image string URL and requestContentBlock{Type:"text"}→input_text/output_text via resolveBlockType (responses.go:78-81), reusing mediaBlockFor's MIME/capability filtering and ta.resolveURL as the shared decision logic. On detail: I recommend omitting it for v1 (server default auto), consistent with ADR-070 Decision 5's detail-deferral. (3) D1 routing per above, extracted into one decision. (4) D3: generalized, mode-parameterized size enforcement with OpenAI-appropriate caps, fail-loud before any request. (5) Tests: flip TestResponsesSink_AddMessage_NonStringContent; extend client_vision_test.go (the "image dropped without vision" row stays — it pins non-vision behavior); add a Responses-input serialization test (image part → input_image with correct data URL), a gpt-5.x tools+image turn test hitting /v1/responses with both function tools and input_image, an image-only turn test per D1, and OpenAI size-enforcement tests. TestCapabilities_FileUploadMode_OutOfRange needs no change (no new FileUploadMode). (6) Docs/governance: a new ADR recording D1/D2/D3, docs/adr/README.md index update (verify-adr-index), and a vision note on the openai provider entry in configs/butler.yaml (gpt-5.5 + reasoning_effort: high) and README. (7) make check-full.
Risks and open questions — including two things the issue misses. First, the issue's own test plan will trip a gate it never mentions: client_vision_test.go (TestMediaBlocks:18 CC=11, TestVision_KimiImagePayload:129 CC=12, TestHydrateMediaAssets:244 CC=13) and capabilities_test.go (TestResolveCapabilities:10 CC=13) are pinned in the NonFixCatalog (INTENTIONAL_NON_FIXES.md, Test Complexity section) and in the partition test real_nonfix_catalog_test.go:79-93 with exact Line: values. Editing those files shifts the pins, so verify-nonfix-catalog (in make check/check-full) fails unless the See: anchors and the partition rows are re-anchored in the same commit — the documented coordination rule, and a known trap. CC must also be re-measured for the pinned tests after adding rows. Second, mediaUploadPurpose (client.go:706) sits on the CC=10 boundary watch list with a refactor-on-touch rule — the D3 generalization must not creep it past 10, and checkDeepSeekMediaSizes's generalization must keep DeepSeek semantics byte-identical. Open questions: the exact OpenAI per-image/aggregate inline limits (must be read from the live guide, not assumed); the reasoning:{} wire shape for no-effort image turns under D1-A; whether detail should be exposed per-image at all in v1; and the behavioral note that gpt-5.4+ sessions will now alternate surfaces turn-by-turn (Responses when images present, Chat Completions for text-only no-tool turns) — functionally sound because both sinks re-serialize the same domain history, but worth an explicit test so the mixed-surface session doesn't surprise.
Bootstrap note: list_skills is unavailable to me as the remote party (invoking tell-me-go would corrupt both sessions); I bootstrapped from README, Makefile, both domain models, ADR-070, INTENTIONAL_NON_FIXES.md, and the environments model. All file/line citations above were read directly from the working tree at /home/pos/tmp/github/gosharplite/tell-me-go.
You endorse D1-A — "force /responses when the turn carries image parts" — and propose extracting shouldUseResponses(toolCount, hasImage) as the single decision, but you never define what hasImage counts. This matters because the routing predicate and the serialization predicate can disagree: extractMediaParts admits any non-empty image/* or video/* part, but mediaBlockFor drops video on !SupportsVideo (gpt-5.x under D2-A has vision, no video) and mediaBlocks skips empty InlineData.Data. Concrete scenario: a gpt-5.6 turn carrying only a video/mp4 part — hasVisualCapability is true, so prepareMediaForTurn runs, the video is dropped at serialization, and mediaOmittedFallback substitutes "(video content omitted — this model does not support video input)". If hasImage counts "any media part present", that turn routes to /responses carrying a text-only fallback payload; if it counts "parts that survive mediaBlockFor's filtering", it stays on /chat/completions. My recommended answer: define hasImage as "at least one part with image/* MIME and non-empty InlineData.Data" — the exact set mediaBlockFor will emit for a SupportsVision model — evaluated on the post-hydration history inside prepareChatRequest (after applyPreparedParts has run), so the routing decision and the payload can never diverge; video-only and empty-data turns do not force /responses. Do you accept that definition — and if not, which predicate do you propose that keeps D1's routing gate and the sink's serialization gate provably in agreement?
I accept the Griller's definition, with one structural strengthening that makes the agreement provable rather than conventional, and I confirm the scenario is accurate against the live tree.
Verification of the scenario. Confirmed: extractMediaParts (client.go:608-616) admits any part with non-empty InlineData.Data and image/* or video/* MIME; mediaBlockFor (client.go:842-864) drops video/ on !caps.SupportsVideo and mediaBlocks (client.go:826-840) skips nil/empty InlineData before calling it; hasVisualCapability (chat.go:319-321) is SupportsVision || SupportsVideo, so a gpt-5.6 video-only turn enters prepareMediaForTurn, the video is dropped at serialization, and buildMessageContent falls through to mediaOmittedFallback → "(video content omitted — this model does not support video input)" (client.go:521-532). If hasImage counted "any media part", that placeholder would ride a /responses request — a pointless round trip that also re-enters the empty-effort "reasoning":{} edge for header-less configs.
The predicate I accept. hasImage = at least one part p in the turn's non-system Content such that p.InlineData != nil && len(p.InlineData.Data) > 0 && strings.HasPrefix(p.InlineData.MIMEType, "image/"), evaluated on post-hydration history at the top of prepareChatRequest — which is provably post-hydration: SendChat calls prepareMediaForTurn (which runs hydrateMediaAssets + applyPreparedParts, client.go:367-379, 765-791) before prepareChatRequest (chat.go:249-251), and hydration either populates Data for every AssetID candidate or fails the turn. This is exactly the set mediaBlocks/mediaBlockFor emit as image blocks on a SupportsVision model (the image branch is image/* ∧ SupportsVision; the non-empty-Data requirement is enforced by the mediaBlocks loop itself). Video-only and empty-data turns therefore do not force /responses — which also matches the issue's own D1-A wording ("force /responses when the turn carries image parts"), and keeps OpenAI video correctly in the issue's "Out of scope".
The strengthening — one predicate, two call sites. Rather than two independently-maintained predicates that must be kept in agreement, I extract a single function isEmittableImagePart(p *llm.Part, caps llm.Capabilities) bool implementing exactly the image-branch of mediaBlockFor (image MIME ∧ non-empty data ∧ SupportsVision), and use it (a) in the routing predicate and (b) inside mediaBlockFor's image branch itself. Then the routing gate and the serialization gate share the same decision function by construction — no second source of truth to drift. I add a property test: for arbitrary part sets on a SupportsVision: true, SupportsVideo: false capability, hasImage(parts) == (len(image blocks emitted by mediaBlocks(parts)) > 0), plus the video-only and empty-data negative cases. The D1 decision is then shouldUseResponses = RequiresResponsesAPI && (hasImage(history) || (toolCount > 0 && hasEffort)) — image presence is the forcing signal independent of effort, and hasImage reads the full history (non-system), which is correct because prior-turn images are re-serialized into every subsequent request and must keep those turns on /responses too.
Two consequences I commit to. (1) req.Reasoning is set only when hasEffort — a no-effort image turn goes to /responses without a reasoning field at all (legal on the Responses API), never "reasoning":{}; and resolveEndpoint must call the same shouldUseResponses (threaded via a UseResponses flag set on chatRequest in prepareChatRequest) instead of the current req.Reasoning != nil || req.ReasoningEffort != "" (chat.go:293-296), eliminating the second gate entirely. (2) The predicate is mode-orthogonal: DeepSeek/Kimi are not RequiresResponsesAPI, so D1 never fires for them; their image/* parts still count as "emittable" (bound→file block, unbound→image_url), so the shared predicate stays correct across modes, and the D3 size guard can reuse the same part scan.
A1 reverses your opening statement without acknowledging it. Your opening said gpt-5.4+ sessions "will now alternate surfaces turn-by-turn (Responses when images present, Chat Completions for text-only no-tool turns) — functionally sound because both sinks re-serialize the same domain history, but worth an explicit test so the mixed-surface session doesn't surprise." A1 now defines hasImage over the full non-system history, which means the first image in a session permanently locks every subsequent turn — including text-only no-tool turns — onto /responses. There is no alternation; the mixed-surface test you proposed in the opening becomes impossible by construction. It also collides with the issue's acceptance criterion "Text-only turns are unaffected (still input_text items, no behavior change)": under A1, a text-only turn after an image is not unaffected — it changes endpoint and re-sends the prior input_image blocks on every subsequent request. So: which scope is the design — per-turn (issue-literal, alternation, matching your opening) or full-history (your A1)? And what precisely breaks under per-turn scope that forces the sticky design — specifically, is it that gpt-5.4+ /chat/completions rejects re-serialized image_url blocks, an external-doc claim the issue does not make (it only restricts tool calling with reasoning_effort ≠ none)? My recommended answer: adopt full-history hasImage — it is the safer design — but record it in the ADR as an explicit deviation from the issue's per-turn D1 wording; rewrite the acceptance criterion to "text-only turns with no prior image in history are unaffected"; replace the alternation test with a one-time-flip test (session opens on /chat/completions, the first image turn flips it to /responses permanently); and either cite the gpt-5.4+ Chat Completions image-rejection premise or drop the "must". Do you accept that framing — or do you defend per-turn scope, and on what grounds?
The Griller's dichotomy is correct, and I accept the full-history framing — but I reject the Griller's stated justification (the Chat Completions image-rejection premise) as the load-bearing reason, and I offer a decisive one the Griller did not cite. Own the reversal: my opening's "alternation" remark described the per-turn variant; A1 deliberately abandons it, and I should have flagged that as a correction rather than leaving the two statements unreconciled.
Why per-turn scope is indefensible — and it is not the doc premise. Both sinks re-serialize the entire retained history on every turn: toStandardMessages/toResponsesInput iterate all of history (chat.go:104-113, responses.go:92-106), and appendMessagesFromHistoryItem processes each item's parts (client.go:565-603). Under per-turn hasImage, the routing predicate would be a function of the current user message while the serialized payload is a function of the full history — two different functions of two different inputs that can disagree on the very turn after an image. That is precisely the "two independent routing gates must stay in sync" defect D1 exists to kill, re-introduced at a higher level. Full-history scope makes the routing predicate and the sink's serialization gate the same pure function of the same input (the payload: history + current prompt + caps) — the Q1 agreement, carried to its logical end. That holds regardless of whether gpt-5.4+ Chat Completions happens to reject image_url blocks; the external-doc premise is not load-bearing. So: drop the "must" — the design does not require citing CC 5.4+ image rejection; it routes image-bearing payloads to the documented 5.4+ surface because the payload contains images, full stop. (I'd still have the implementer read the live guide for the record, but as informational, not as a dependency.)
"Permanent" is monotone within the retained window, not literally permanent — a refinement to the Griller's test shape. hasImage over full history is monotone only while the image turn remains in the retained window. The Context pipeline summarises older non-pinned turns into text (domain model Context.summarise, context-pinned-preserved invariant), which removes the image parts from the window; after that, hasImage returns false and the session can flip back to /chat/completions. That is a desirable property — the endpoint follows the actual payload — and it is trivially true at the client level because routing is a pure function of the history argument. So the replacement test is not "one-time flip, permanent"; it is: (a) no-image history → /chat/completions (pins the reworded criterion); (b) first image turn → /responses with input_image; (c) subsequent text-only turn with image still in history → /responses, and the request re-serializes the prior input_image (assert it); (d) client-level purity: same (history, tools, effort) inputs → same endpoint, exercised by calling SendChat with an image-free history after (b) — the flip-back. The monotone-in-a-window behavior is the orchestrator's domain (history growth/pruning), not the client's; I'll note that in the ADR.
ADR deviation and criterion rewrite — accepted as the Griller recommends. Record in the ADR: D1-A's hasImage is defined over the full request payload (retained non-system history + current prompt), not the current user message — a text-only turn in a session whose retained history contains an image routes to /responses because the payload itself contains input_image items. Rewrite acceptance criterion "Text-only turns are unaffected" to its precise form: text-only turns in sessions whose retained history contains no image are unaffected — same endpoint, input_text items, no behavior change. Note this also fully preserves the issue's other criteria: criterion 2 (first image turn → /responses with input_image, no responses_sink_non_string_content warning) and the tool-augmented criterion hold unchanged, and gpt-5.0–5.3 are untouched by D1 (they never RequireResponsesAPI, so their surface never alternates).
One commitment I will keep explicit in the ADR: the monotonicity means a session that ever saw an image pays image-token re-serialization on subsequent text-only turns — an inherent cost of full-history serialization that exists under per-turn scope too (the image is re-sent either way); only the endpoint differs. That cost is the accepted trade-off for routing/serialization agreement, and it belongs in the ADR's Consequences section.
Your D3 plan generalizes checkDeepSeekMediaSizes (client.go:793-810) into a mode-parameterized guard "that runs for any vision-capable model," and A1 adds that the D3 guard "can reuse the same part scan" as isEmittableImagePart. That guard runs inside prepareMediaAssets on collectHistoryParts(history) — the entire non-system retained window, every turn, for every image part currently in history (client.go:690-703, 765-791). Now combine this with your Q2 commitment to full-history hasImage: once an image enters the retained window, every subsequent turn re-serializes it into the request body, and the generalized guard re-checks it — and all accumulated images — on every turn. Two concrete consequences your plan does not address: (1) if OpenAI's live guide documents an aggregate/request-body cap analogous to DeepSeek's 48 MiB, a session that adds images across multiple turns can trip the aggregate cap on a turn that adds no new image (or only a small one) — a loud mid-session failure with zero recovery path, because D3-A keeps FileUploadNone for GPT and there is no upload escape; the only mitigation is orchestration-level summarisation eviction, which is timing-dependent and outside the client's control; (2) if the guide documents no aggregate cap (only a per-image limit), the generalized guard must not invent one — but then the "aggregate inline" branch of the generalized check is dead code for OpenAI. My recommended answer: (a) the guard for OpenAI enforces exactly what the live guide documents — per-image cap always; an aggregate cap only if one is documented, and if none is, the aggregate branch is mode-gated off rather than silently borrowing DeepSeek's 48 MiB; (b) per-image re-checking on every turn is accepted as idempotent (same data → same pass) and cheap; (c) the ADR's Consequences section records the mid-session accumulation failure as a known, accepted limitation with the note that recovery depends on summarisation eviction; (d) the "fail-loud before any request" wording is amended to "fail-loud before any request and re-validated on the full retained payload every turn." Do you accept that — and specifically, what does the guard do the first time an already-accepted accumulated payload trips the aggregate cap on a subsequent turn?
I accept (a)–(d), with two refinements and one important reframing: the mid-session accumulation failure is pre-existing DeepSeek behavior, not something the OpenAI work introduces — the generalized guard inherits it, and the ADR should say so.
Reframing first. Verify the mechanics: prepareMediaForTurn (chat.go:327-339) → prepareMediaAssets(ctx, collectHistoryParts(history), resolver) — and collectHistoryParts (client.go:690-703) gathers every non-system part of the retained window. checkDeepSeekMediaSizes (client.go:793-810) counts n <= maxInlineMediaBytes images into inlineBase64Bytes and errors over maxRequestBodyBytes (48 MiB). For DeepSeek, images 32–64 MiB route to upload and are excluded from the inline aggregate, but three ~20 MiB inline images already trip the 48 MiB aggregate mid-session today, with the same loud failure and the same eviction-only recovery. So the failure mode the Griller describes is a property of the ADR-070 size-guard contract ("fail loud before any request", Decision 4), which the OpenAI path now joins. This should be stated in the ADR's Consequences as inherited, not novel — otherwise a reviewer will file it as a regression.
Answer to the pointed question — what the guard does when an already-accepted accumulated payload first trips the aggregate. The guard is a pure function of the post-hydration retained payload, evaluated on every turn. On the crossing turn it returns the same kind of error checkDeepSeekMediaSizes returns today; prepareMediaAssets propagates it (client.go:774-778), prepareMediaForTurn returns it, and SendChat aborts before any network I/O (ta, err := c.prepareMediaForTurn(...); if err != nil { return nil, nil, err }, chat.go:349-353) — no HTTP request, no partial state, nothing to clean up (FileUploadNone → ta.uploaded empty, so the deferred release is a no-op, chat.go:355-358). The turn is doomed regardless: an over-cap request body would be rejected by the API anyway (413-class), so the client-side error is the good failure, not a new one. I will not "soft-allow" the accumulated case — that is exactly the unbounded-request-body hole D3 exists to close, and the aggregate is a property of the request body, which includes history; enforcement must be per-request, same argument as Q2's routing==serialization agreement.
The one refinement the Griller's (a) misses: the error must be triage-distinguishable. Split the messages: a per-image violation is a turn-input error ("image exceeds N MiB limit — reduce this image"), while an aggregate violation on a turn that adds no/small images is a session-state error ("retained session images exceed the aggregate N MiB request-body cap — start a new session or allow summarisation to evict prior image turns"). Identical failure mechanics, but the orchestrator/user must be able to tell "fix your input" from "your session has accumulated too much". The aggregate error names the cap and the recovery (eviction via Context.summarise — the same flip-back path from Q2 — or --new/rollback), since there is no upload escape in v1.
On (a) — structural mode-gating, not a silent borrow. I accept per-image-always / aggregate-only-if-documented, and I'd implement it so the aggregate cannot be invented by copy-paste: a mediaSizeLimits{maxPerImage, maxAggregate, hasAggregate bool} instance per mode — DeepSeek {32 MiB, 48 MiB, true} (with the 64 MiB upload bound preserved in its own routing path, byte-identical semantics), OpenAI {per-image from the live guide, aggregate only if the guide documents one, hasAggregate = false otherwise}. The check loop is shared; the limits table is not. If the guide documents no aggregate, hasAggregate=false compiles the branch off for OpenAI and DeepSeek's 48 MiB never leaks in. On (b): accepted — idempotent (same bytes → same pass, no hysteresis) and cheap (the scan is O(total image bytes), strictly less than the base64 encoding of the same payload that follows it; it is not a hot-path regression). On (c): accepted, recorded as inherited-known-limitation with eviction/--new recovery. On (d): accepted, with the message-split above folded into the amended wording: "fail-loud before any request, re-validated on the full retained payload every turn, with per-image vs aggregate errors distinguished."
One commitment for the ADR's Verification section: a test that pins the accumulated-crossing behavior — turn 1 and turn 2 each pass individually, the combined retained payload trips the aggregate on turn 3 (which adds no image), asserting the session-state error and zero HTTP requests — plus the DeepSeek byte-identical regression guard so the generalization cannot shift its constants.
A3 commits to triage-distinguishable errors — "the orchestrator/user must be able to tell 'fix your input' from 'your session has accumulated too much'" — but the mechanism is unspecified, and the current guard returns plain fmt.Errorf strings with no type ("image exceeds 64 MiB upload limit: %d bytes" / "aggregate inline image size exceeds 48 MiB limit", client.go:804/811). Your own recovery path for the session-state class — "allow summarisation to evict prior image turns" — is orchestration-level: the orchestrator cannot trigger eviction, surface a distinct UX, or distinguish a turn-input failure from a session-state failure on a plain error without fragile string-matching, which the repo does not use for control flow (it uses sentinels: llm.ErrTransient/llm.ErrTerminal, pinned in wantSentinel tables, client_chat_test.go:428-508). My recommended answer: introduce a typed error — e.g. MediaSizeError{Kind: MediaSizePerImage | MediaSizeAggregate, Mode, Cap, Actual} (or a sentinel pair ErrMediaPerImageLimit / ErrMediaAggregateLimit wrapped with %w) so errors.Is/errors.As gives the orchestrator programmatic triage while the message text stays human-readable — with the hard constraint that the DeepSeek messages survive verbatim so the existing strings.Contains assertions (files_test.go:839/875) keep passing unchanged, and the error contract recorded in the ADR following the 2026-04-tool-result-error-convention precedent. Do you accept — and what is the concrete type, and does the generalized guard return it for both modes or only OpenAI?
The Griller's cited mechanism gap is real, and the placement question it sets up is the crux. Verified: checkDeepSeekMediaSizes returns plain fmt.Errorf (client.go:804, 811); files_test.go:839 and :875 assert the exact substrings "image exceeds 64 MiB upload limit" and "aggregate inline image size exceeds 48 MiB limit"; sentinel classification lives in internal/domain/llm/gateway.go:13-21 (ErrTransient/ErrTerminal/ErrRateLimit/ErrAuth) with IsTransient/IsTerminal/IsAuth consumers (gateway.go:35-51) and errors.Is in llmerror.go:73; the wantSentinel tables (client_chat_test.go:428-508) pin API-error→sentinel classification; and the ADR precedent exists at docs/adr/2026-04-tool-result-error-convention.md (ADR-022, "write the convention down"). I accept — with one architectural correction to the Griller's framing: the mechanism is not just "typed error", it is where the type lives.
Concrete type — domain, not infrastructure. The guard is in internal/infrastructure/llm/openai, but the orchestrator is agent-layer and must errors.As without importing infrastructure (that import direction is exactly what verify-architecture polices). The type therefore lives in internal/domain/llm, beside the sentinels it must interoperate with:
// internal/domain/llm/media_errors.go
type MediaSizeKind int
const (
MediaSizePerImage MediaSizeKind = iota // turn-input failure: fix the image
MediaSizeAggregate // session-state failure: evict or restart
)
type MediaSizeError struct {
Kind MediaSizeKind
Mode FileUploadMode // which provider's limits applied
Cap int64 // limit in bytes
Actual int64 // offending per-image size or aggregate bytes
msg string // pre-rendered at construction; legacy DeepSeek text verbatim
}
func (e *MediaSizeError) Error() string { return e.msg }
func (e *MediaSizeError) Unwrap() error { return ErrTerminal } // terminal: retry/failover are uselessThree load-bearing details. (1) The msg is pre-rendered at the construction site, so the DeepSeek sites pass the exact legacy templates and Error() renders byte-identical — the files_test.go:839/875 assertions pass unchanged, and those tests become the message-drift regression guard. The verbatim constraint is satisfied structurally, not by hoping a default renderer matches. (2) Unwrap() error { return ErrTerminal } integrates with the existing classification machinery without touching gateway.go or IsTerminal — errors.Is(err, llm.ErrTerminal) is true, so the orchestrator's recovery path treats a size violation as non-retryable (correct: retrying re-fails identically, and failover can't help — the payload is the problem). I deliberately do not add a new LLMError enum value: that would ripple into ClassifyLLMError and the RecoveryStep switch whose default: arm is a cataloged structurally-unreachable pin (INTENTIONAL_NON_FIXES.md, engine_phases.go:177) — the typed error gives triage without touching that taxonomy. (3) Orchestrator triage is errors.As(err, &mse) → switch mse.Kind, never string matching — per-image violations surface "this turn's image exceeds the cap, fix the input" with Cap/Actual; aggregate violations surface "retained session images exceed the request-body cap, evict or --new", both with programmatic numbers for UX.
Both modes. The generalized guard returns *MediaSizeError for DeepSeek and OpenAI, with Mode discriminating the provider and the legacy templates preserved for DeepSeek — uniform triage across vision providers, one error contract. The DeepSeek sites change mechanically from fmt.Errorf to the typed constructor with zero message drift (pinned by the unchanged files_test.go assertions).
ADR + tests. The new ADR records the contract following ADR-022's precedent: the type, the Kind semantics (turn-input vs session-state), the render-stability rule for messages, the Unwrap → ErrTerminal classification, and the "orchestrator triages via errors.As, never strings" rule. Tests: TestMediaSizeError_Classification (errors.As distinguishes per-image/aggregate; errors.Is → ErrTerminal; Cap/Actual populated) plus the DeepSeek verbatim pins; note modelith-drift may advisory-flag the new exported type — advisory-only, no model change (I am explicitly not adding it to the LLMError enum, which is the only domain-model surface that would be affected).
Your Unwrap() → ErrTerminal makes IsFatal(err) true (orchestrator/errors.go:67-75: llm.IsTerminal(err) → "halt the current Turn and session") — for both kinds. That collides with the triage semantics you defined in A3/A4. (1) Per-image class: you described it as a turn-input failure — "this turn's image exceeds the cap, fix the input." But a session-fatal error means the fix isn't a within-session retry; the engine halts the session, so "fix the input" realistically means starting over. Is session-termination really the intended contract for a fixable-input error? (2) Aggregate class: your documented recovery is "start a new session or allow summarisation to evict prior image turns." Two problems: (a) IsFatal halts the session, so there are no subsequent turns in which eviction can occur — the recovery is moot by construction unless something checks errors.As(MediaSizeError{Kind: Aggregate}) before IsFatal, which no existing engine path does (the phases only do binary IsTransient→transient/terminal, engine_phases.go:65-66/96-97/111-112, and ClassifyLLMError at :135); (b) even if the session continued, summarisation is token-driven (context-within-budget, gatekeeper triggerSummarization), while the aggregate cap is byte-driven — ADR-070 counts images at ≤384 tokens each, so the exact scenario that trips the byte cap (images fit the token budget, exceed the aggregate bytes) never triggers summarisation. "Allow summarisation to evict" is a mirage for the very scenario that produces it; the real recovery is --new/rollback (or coincidental maxHistoryTurns pruning). My recommended answer: keep Unwrap → ErrTerminal for the per-image class (fixable input, terminal-for-this-payload, session continues on the user's next message — which is how the app already behaves after a terminal error), but for the aggregate class commit honestly: it is session-terminal with recovery = --new/rollback only — amend the error message to drop the "allow summarisation to evict" clause, and record in the ADR that byte-cap accumulation is independent of the token budget, so summarisation is not a recovery for it (and the errors.As Kind triage exists for the message/UX, not for behavioral divergence, unless you specify an engine path that checks it before IsFatal — which contradicts the terminal unwrap). Do you accept — and which recovery contract do you actually commit to for the aggregate class?
The Griller's objection has teeth. Verified — and the Griller is right on every factual point, plus one of my claimed recoveries is doubly wrong. IsFatal (orchestrator/errors.go:67-75) returns true on llm.IsTerminal(err) and is documented "halt the current Turn and session"; the engine phases classify binary — category := llm.ErrTerminal; if IsTransient(err) { category = llm.ErrTransient } (engine_phases.go:65-66, 96-97, 111-112) — and RecoveryStep uses llm.ClassifyLLMError (:135). No phase inspects error types before IsFatal. I accept the corrected contract; here is the one I actually commit to.
Per-image class — keep Unwrap → ErrTerminal, with the contract stated precisely. "Terminal" here means terminal-for-this-payload: the run halts, the error is surfaced, no auto-retry, and the persisted session continues on the user's next message — exactly how the app already behaves after any terminal error (auth_failure surfaces immediately; the session survives). A per-image violation is fixable input: the next message with a smaller image (or no image) succeeds in the same session. No engine change; the ADR records this as "turn-fatal, payload-scoped, session-continuing".
Aggregate class — session-terminal, recovery = --new or rollback ONLY. I accept the correction and own both errors in my earlier story. (a) "Allow summarisation to evict" is a mirage for the exact scenario that produces the aggregate failure: summarisation is token-driven (context-within-budget gatekeeper), the aggregate cap is byte-driven, and ADR-070 accounts images at ≤384 tokens each — two 20 MiB images that trip the 48 MiB byte cap contribute ≤768 tokens against a 200K window, so the gatekeeper never fires. The byte-cap scenario is, by construction, token-invisible. (b) I now add a second, stronger reason the Griller's "coincidental maxHistoryTurns pruning" escape is also unreachable: a failed turn is never persisted (history-persisted-after-turn — the domain model's crash-recovery scenario states the interrupted turn is never written; PersistenceStep runs only on the success path). So while the session keeps failing, the retained window is static — no new turns are appended, turn count never advances, and turn-count pruning can never trigger. The only exits are user-initiated: --new (fresh session) or --back N/rollback removing the offending turns. I commit to that contract and no other.
Kind triage is UX-only, and I say so in the ADR. errors.As → switch Kind selects the surfaced message (per-image: "this turn's image exceeds the N MiB cap — reduce the image and retry"; aggregate: "retained session images exceed the N MiB request-body cap — this session cannot continue with them; start a new session (--new) or roll back (--back N)"). The aggregate message drops the "allow summarisation to evict" clause. No engine phase checks Kind before IsFatal, and I will not add one — a non-fatal aggregate classification would be theater, because the next run re-serializes the same static window and re-trips; there is no within-session recovery that doesn't remove the images. The ADR records: (1) byte-cap accumulation is independent of the token budget, so summarisation is not a recovery for it; (2) failed turns don't persist, so turn-count pruning cannot advance while the session is failing; (3) Kind exists for message/UX selection and documentation, never for behavioral divergence.
One honest addendum the Griller's framing invites: this means the aggregate class makes the session practically unusable — every subsequent message re-fails with the same error until the user acts. That is harsh but it is the truth of the design, and it is strictly better than the alternative (an over-cap request silently 413-ing after the fact). The ADR's Consequences section will state it plainly as the accepted trade-off, with the error message carrying the exact recovery verbs so the user is never left guessing.
Verify the current state: responsesSink.AddMessage drops non-string content with responses_sink_non_string_content (responses.go:33-40); buildMessageContent produces the shared []any media-first — mediaBlocks(...) then append(blocks, requestContentBlock{Type:"text", ...}) (client.go:519-530) — containing imageURLBlock + requestContentBlock{Type:"text"}, and potentially videoURLBlock for any vision+video model. Your plan translates imageURLBlock→input_image, text→input_text/output_text via resolveBlockType, and keeps "a defensive drop + warning branch for hypothetical future RequiresResponsesAPI && !SupportsVision models." That default case is the exact spot where the bug this issue exists to kill could reopen one layer down: if the type-switch's default is warn-and-drop, then any block type buildMessageContent emits that the sink doesn't translate — e.g., a videoURLBlock on a hypothetical Responses+video model today, or a future block type — is silently dropped from the /responses payload with only a log line: the same silent content loss the issue names, relocated. My recommended answer: the default case must be a fail-loud error (mirroring errUnhandledBlockType in appendPartsFromBlock, responses.go:119-128) rather than warn-and-drop, because the input block set is fully enumerable (string, imageURLBlock, requestContentBlock, videoURLBlock) — an unknown type is a programming error that should abort the turn, not a benign case — and the "defensive branch" for hypothetical non-vision Responses models should be an explicit if !caps.SupportsVision guard on the imageURLBlock case, not a catch-all default. And specify the concrete mechanics at the same site: the new input_image struct (string image_url, no required text), and whether historyItem.Content widens from []requestContentBlock to a union so the wire marshals {"type":"input_image","image_url":"<data URI>"} beside {"type":"input_text","text":"..."}. Do you accept the fail-loud default, and what is the concrete Content field type?
Verified all four premises. responsesSink.AddMessage warn-and-drops non-string content (responses.go:32-46 — the !ok branch logs responses_sink_non_string_content and returns, appending nothing); buildMessageContent emits the shared []any media-first — mediaBlocks(...) then append(blocks, requestContentBlock{Type:"text", ...}) (client.go:496-518), so the sink can receive imageURLBlock, videoURLBlock (vision+video models only), and requestContentBlock{Type:"text"} as []any elements; appendPartsFromBlock's default fails loud with errUnhandledBlockType (responses.go:119-128); and historyItem.Content is the concrete []requestContentBlock (client.go:318-326) whose requestContentBlock has a non-omitempty Text (client.go:336-340) — so the image block cannot reuse that struct without emitting a stray "text":"" on the wire.
I accept the fail-loud default — and strengthen it beyond the Griller's proposal. The input block set is fully enumerable (string, []any of imageURLBlock/videoURLBlock/requestContentBlock), so every case is either a sanctioned translation or a programming error. I reject any warn-and-drop in the sink, including the issue's own "defensive drop + warning branch": an imageURLBlock arriving at a !SupportsVision Responses sink is not a benign case — it is the routing/serialization gates disagreeing, the exact bug class this issue exists to kill, one layer down. It is also structurally unreachable today: buildMessageContent only emits image blocks when hasSupportedMedia is true, which requires SupportsVision; and under D2-A every RequiresResponsesAPI model is vision-capable. So the concrete contract at the sink is:
content.(string)→ text block viaresolveBlockType(unchanged).content.([]any)element switch:requestContentBlock→input_text/output_textviaresolveBlockType(role-based, unchanged).imageURLBlock→input_image; guarded byif !caps.SupportsVision { return fail-loud }— an assertion of gate agreement, not a drop.videoURLBlock→ fail-loud "video input on the Responses API is not implemented (out of scope, issue #1447)" — unreachable today (no model hasSupportsVideo && RequiresResponsesAPIin-tree; video never reaches the sink otherwise becausemediaBlockFordrops it for vision-only models), so loud-abort is safe and prevents a future silent-loss relocation.- default → fail-loud, mirroring
errUnhandledBlockTypebut with a distinct sentinelerrUnhandledInputBlockType— the output-side sentinel is suppressible inprocessDirectOutputItem(the ADR-024errors.Isguard), and the input side must never be suppressed; reusing the sentinel would risk someone extending the suppression to it. Distinct sentinel, always propagates, turn aborts before any HTTP request.
Concrete type — separate struct, no text field, and yes, Content widens. The image block cannot be a requestContentBlock variant (non-omitempty Text would emit {"type":"input_image","text":"",...} — wrong wire, unknown API tolerance, and changing Text to omitempty would alter the pinned tool-call-only assistant message wire {"type":"output_text","text":""} that TestMandatoryContentField rests on). So:
// requestInputImageBlock is the Responses API input_image content block.
// image_url is a plain string (fully-qualified URL or data: URI) — not
// the Chat Completions object shape {"url": ...}.
type requestInputImageBlock struct {
Type string `json:"type"` // "input_image"
ImageURL string `json:"image_url"` // string, per the Responses guide
Detail string `json:"detail,omitempty"` // omitted = server default "auto" (ADR-070 precedent)
}
// historyItem.Content widens from []requestContentBlock to []any:
Content []any `json:"content,omitempty"`[]any is the Go-idiomatic union here — it is exactly what mediaBlocks already returns and what message.Content already is on the standard sink, so the widening is consistent with the package's existing shape; an interface sum-type for two structs would be ceremony. Wire result: [{"type":"input_image","image_url":"data:image/png;base64,..."},{"type":"input_text","text":"..."}] — the guide's shape beside today's unchanged input_text. Detail stays omitted in v1 per my Q1 commitment (server default, ADR-070 Decision 5 precedent).
Ripples I commit to flagging for the Coder. The Content widening is a compile-level change: every direct historyItem/[]requestContentBlock construction in client_responses_test.go/client_responses_edge_test.go must move to []any — exhaustive by construction, since the compiler enumerates them. TestMandatoryContentField's wire is preserved (text blocks keep non-omitempty Text, so the assistant content array still opens [{). The responses_sink_non_string_content warn event disappears from production — TestResponsesSink_AddMessage_NonStringContent flips to assert the serialization, and the ADR records the sentinel contract: input-side translation is fail-loud, the input set is closed, and any future Responses block type requires an explicit translation case in the same change that introduces it.
Your A1 commitment — "resolveEndpoint must call the same shouldUseResponses, threaded via a UseResponses flag set on chatRequest in prepareChatRequest... eliminating the second gate entirely" — invalidates TestResolveEndpoint's input model. Every hand-built chatRequest in that test has the flag at its zero value, so the /responses rows ("GPT-5.4 with tools and effort") fail, and the image-dependent rows ("GPT-5.4 without tools + image → /responses") can't be expressed without history — the test encodes the old function-of-(tools, effort) contract your change deliberately abolishes. Worse, I checked the alternative you might reach for and it's closed off: payload-derived routing (len(req.Input) > 0) fails the pinned empty-history corner — TestAlternativeUsageAndPolymorphicText (endpoint_test.go:133) sends SendChat(nil history, toolDecls, ...) on gpt-5.4+effort and expects /responses, where Input is empty and, under your own "omit Reasoning when no effort" rule, no field distinguishes the surface at all. So the flag is necessary — which leaves the unset-flag hazard: UseResponses zero-value-false means any future caller that builds a chatRequest without going through prepareChatRequest silently routes a responses-shaped body to /chat/completions, the exact silent-desync you've spent this whole grill eliminating. My recommended answer: keep the flag as the primary signal, but make resolveEndpoint = RequiresResponsesAPI && (req.UseResponses || legacyCondition) where the legacy condition (len(Tools) > 0 && (Reasoning != nil || ReasoningEffort != "")) survives as the unset-flag fallback — this keeps TestResolveEndpoint's existing rows green unchanged (they all satisfy the legacy condition), closes the silent-CC hazard for future direct constructors, and the flag carries the new image-only/no-effort cases; alternatively, flag-only with a mandatory rewrite of TestResolveEndpoint plus a pinned invariant "all production paths to createHTTPRequest go through prepareChatRequest". Which contract do you commit to — flag-with-legacy-fallback or flag-only — and how does TestResolveEndpoint change in either case?
Verified: TestResolveEndpoint (endpoint_test.go:16-61) hand-builds chatRequest{Tools, ReasoningEffort} with no flag field and calls resolveEndpoint directly — every row would hit a zero-value flag; TestAlternativeUsageAndPolymorphicText (endpoint_test.go:133) sends SendChat(nil, toolDecls, ...) on gpt-5.4+effort and pins /responses, so payload-derived routing (len(req.Input) > 0) is genuinely closed off — Input is empty with nil history. The flag is necessary. I commit to flag-with-legacy-fallback, implemented as a tri-state that scopes the fallback to exactly the hazard class, not a plain boolean OR.
Concrete contract. chatRequest gains UseResponses *bool with json:"-" — it is internal routing state and must never marshal into the request body (a plain bool would leak "UseResponses":true onto the wire; the tri-state has the same leak risk, hence the tag). Semantics: nil = "caller did not decide" → derive from the legacy condition; explicit true/false = authoritative. prepareChatRequest always sets it from shouldUseResponses — the single production decision. resolveEndpoint becomes:
func (c *client) resolveEndpoint(req *chatRequest) string {
if req.UseResponses != nil {
if *req.UseResponses { return "/responses" }
return "/chat/completions"
}
// Unset-flag fallback (direct constructors only): the legacy
// tool+effort condition. Provably unreachable on the production path
// when the flag is false — see ADR: flag=false implies
// !(hasImage || (toolCount>0 && hasEffort)); Tools non-empty implies
// toolCount>0, and Reasoning/ReasoningEffort are only set when
// hasEffort (omit-without-effort rule), so legacy ⇒ flag would have
// been true. The fallback is a conservative default, not a second gate.
if c.capabilities.RequiresResponsesAPI && len(req.Tools) > 0 &&
(req.Reasoning != nil || req.ReasoningEffort != "") {
return "/responses"
}
return "/chat/completions"
}Why this over flag-only. The Griller's unset-flag hazard is real but scoped: in production, chatRequest is constructed only by prepareChatRequest, so the hazard exists solely for direct constructors (tests, future code) — and "all production paths go through prepareChatRequest" is not pinnable by any existing arch gate; it would be convention-only. The legacy fallback closes that class mechanically. Crucially, keeping it does not resurrect the second gate: the legacy condition cannot disagree with the flag on the production path (proof in the comment above — flag=false ⟹ legacy=false, given the omit-Reasoning-without-effort rule and Tools ⊆ toolDecls), so it is a conservative default for undecided callers, not a competing decision. And TestResolveEndpoint's existing rows encode a contract that remains true — tool+effort on 5.4+ → /responses still holds; what is abolished is only that the legacy condition is the sole signal. Flag-only would force a rewrite of a still-correct spec and leave the hazard convention-guarded. I also reject plain-boolean flag || legacy: an explicit false is a decision and must win over a heuristic — the fallback belongs only to the nil state.
TestResolveEndpoint changes. Existing seven rows: unchanged and green — they exercise the nil→legacy path (with tools+effort rows satisfying the legacy condition; without-tools/without-effort rows correctly falling to /chat/completions). New rows, all expressible without history (which is the point of the flag): (a) UseResponses: ptr(true), no tools, no effort → /responses — the image-only case the Griller said couldn't be expressed; (b) UseResponses: ptr(false) + tools + effort → /chat/completions — explicit override wins over the legacy condition; (c) UseResponses: ptr(true) + tools + effort → /responses — flag/legacy consistency. Integration layer stays in the plan's test set: SendChat gpt-5.x image-only history → /v1/responses pins the shouldUseResponses → flag hop, and the existing TestAlternativeUsageAndPolymorphicText (nil history, tool+effort) stays green via the flag set by prepareChatRequest — no Input needed. The ADR records the tri-state semantics, the json:"-" requirement, and the agreement proof, so the fallback is documented as intentional rather than read as a lingering second gate.
Your A6/A7 fail-loud contract — "fail-loud, turn aborts before any HTTP request" — cannot be delivered by the mechanism you've specified, because AddMessage cannot return an error and the sink's callers don't check one. The violation has to travel through a route you haven't designed: my recommended answer is that the sink captures the violation in a field (e.g. s.err) and toResponsesInput — which already returns ([]historyItem, error) (responses.go:92-106) — checks it after the history loop and returns it, so it propagates through buildRequestBody → prepareChatRequest → SendChat and the turn aborts before any network I/O (the same pre-network guarantee as the size guard). That works, but it has a consequence you should pin now: toResponsesInput and toStandardMessages share appendMessagesFromHistoryItem, so either the check lives only in the responses path (asymmetric with the standard sink, which is total and needs none) or the shared helper needs a way to surface a sink error — which changes the shared serialization path, not just the responses sink. And second: the fail-loud branches are structurally unreachable on any in-tree production path (no model has SupportsVideo && RequiresResponsesAPI; buildMessageContent won't emit imageURLBlock at !SupportsVision), so they carry zero coverage — and this repo's gates (verify-nonfix-catalog, coverage) will flag them. My recommended answer for that half: direct unit tests — hand-construct &responsesSink{client: c} and feed the forbidden []any values (videoURLBlock{...}, an unknown block type) to AddMessage, asserting the captured error — which works because the sink is testable in isolation, rather than catalog entries, so the branches carry real tests and no verify-nonfix-catalog coordination is needed. Do you accept the sink-captured-error + toResponsesInput-post-loop-check mechanism (with the asymmetric-check caveat recorded), and do you commit to direct unit tests for the fail-loud branches rather than cataloging them?
Verified: openaiSink.AddMessage returns nothing (client.go:413-417) and appendMessagesFromHistoryItem (client.go:565-603) calls it unchecked, so the fail-loud branches cannot travel through the existing signature; toResponsesInput already returns ([]historyItem, error) (responses.go:92-106) with the history loop inside; the propagation chain is complete — toResponsesInput → buildRequestBody (chat.go:136-145, if err != nil { return nil, err }) → prepareChatRequest (chat.go:252-258) → SendChat returns before createHTTPRequest (chat.go:363-366), i.e. the same pre-network guarantee as the size guard, which runs even earlier in prepareMediaForTurn. And client_edge_test.go:284 already establishes the isolated-construction pattern (&responsesSink{client: &client{logger: spy, model: "test-model"}}). I accept the mechanism.
Mechanism, concretely. responsesSink gains an err error field. AddMessage's fail-loud branches set s.err (the videoURLBlock case and the unknown-type default wrapping the new errUnhandledInputBlockType; the imageURLBlock-at-!SupportsVision assertion likewise) and return — they append nothing for the offending block. toResponsesInput checks after the loop: if sink.err != nil { return nil, sink.err }, discarding the partial s.items. Safe by construction: the loop continues only to append other history items into a slice that is thrown away when the error surfaces; no panic risk, no partial side effects beyond the discarded slice. One consequence I pin now: the responses path never needs mediaOmittedFallback for these violations — the standard sink is total and degrades via the placeholder; the responses sink is non-total and aborts. (The production-reachable video degradation on a gpt-5.x tool+effort turn still works correctly today: mediaBlockFor drops the video before the sink, buildMessageContent returns the placeholder string, and the sink's string case serializes it as input_text — the sink's videoURLBlock branch is only the unreachable fail-loud assertion for a hypothetical SupportsVideo && RequiresResponsesAPI model.)
Asymmetric check — accepted and recorded as deliberate. The check lives only in toResponsesInput. toStandardMessages shares appendMessagesFromHistoryItem but its sink is total — standardSink.AddMessage only appends to a slice and cannot fail — so it needs no check. I explicitly reject changing openaiSink.AddMessage to return error: it would force a bogus error return onto the total standard path, ripple into appendMessagesFromHistoryItem's signature, both to* paths, and every test double (recordingSink), for zero correctness gain over the captured-error mechanism. The ADR records the asymmetry as intentional: the responses serialization is the only non-total path, so it is the only one that carries the post-loop check.
Direct unit tests — committed, no catalog entries. Hand-construct the sink in isolation and feed the forbidden []any values directly to AddMessage, asserting s.err is set and s.items unchanged: (1) videoURLBlock{...} → the out-of-scope video error; (2) an unknown block type → error wrapping errUnhandledInputBlockType; (3) imageURLBlock with client.capabilities.SupportsVision == false → the gate-agreement assertion; plus the positive translations (imageURLBlock at SupportsVision → input_image item; requestContentBlock → input_text/output_text). One toResponsesInput-level test drives a history containing an offending block and asserts the error propagates (pinning the post-loop check itself). These give real branch coverage — the branches are production-unreachable but test-reachable, which is precisely why direct tests beat catalog entries: no verify-nonfix-catalog coordination (no INTENTIONAL_NON_FIXES.md edit, no real_nonfix_catalog_test.go partition-row churn in the same commit), and no structurally-unreachable acceptance-class entries for code that is trivially coverable in isolation. This follows the repo's own precedent — TestErrUnhandledBlockTypePropagation asserts a sentinel directly rather than relying on production reachability — and the existing client_edge_test.go sink-isolation pattern.
The entire D3 design — mediaSizeLimits{maxPerImage, maxAggregate, hasAggregate} per mode plus a shared byte-scan loop (A3) — hard-codes bytes as the enforcement dimension, because DeepSeek's documented limits (32/48 MiB, ADR-070's pinned gist) are byte-based. But the OpenAI caps come from a guide no one in this round has read, and its limit may not be byte-shaped: OpenAI's vision cost model is tile/dimension-based (e.g., 85 tokens per 512×512 tile, low/high detail), and its documented input caps could be dimension-based (max pixels) or token-based rather than a megabyte figure. If so, the byte guard is mis-modeled in the exact way D3 exists to prevent: a dimension-huge but byte-small image — say a 20,000×20,000 px highly-compressible PNG at ~300 KB — sails under a 25 MB byte cap and 400s at the provider after serialization, the failure relocated one layer down. And enforcing a dimension limit is materially different: the domain model carries no dimensions (llm.Blob = {MIMEType, Data}, types.go:38-42), so a pixel cap requires either decoding image headers in the client (image.DecodeConfig — stdlib, one pass per image, on every turn for retained images, and only post-hydration since AssetID parts carry no bytes until resolved) or extending the domain Blob/Part with dimension metadata (a ripple into clone/equal/persistence). My recommended answer: (1) reading the live guide is a hard prerequisite — the first deliverable of the D3 task, before any guard code — because the D3 task cannot be specified self-contained for a memoryless Coder without both the numbers and the dimension; (2) if the guide's limit is byte-based, proceed with the parameterized byte guard exactly as designed; (3) if it is dimension- or token-based, the guard uses image.DecodeConfig post-hydration in the same scan (no domain-model change), and if the guide documents no client-enforceable limit at all, the ADR records dimension enforcement as deferred-to-provider with a documented 400-on-wire hole; (4) the ADR records the enforcement dimension as a decision, not an implementation detail. Do you accept — and what is the concrete D3 task-1 gate: does the Coder's first task read the guide and return the limit numbers and dimension, or does the plan fork on the guide's shape before dispatching?
The Griller's suspicion is confirmed by direct evidence — I fetched the live guide (https://developers.openai.com/api/docs/guides/images-vision?api-mode=responses) just now, and it is dimension/patch-based, not byte-based, with no megabyte figure on the page. The specific constraints I extracted: per-detail pixel-dimension maxima (2048px for auto/low/high, 6000px for original), 32×32-px patch tokenization with patch budgets (2,500 / 6,144 / 10,000 per detail) — and critically, "GPT-5.6 Sol, Terra, and Luna have no patch-budget limit for original or auto", so the target models of this issue have only the pixel-dimension constraint. And one more fact that corrects the Griller's failure model: "Original detail can still resize images that exceed the model's limits" — the provider does not reject over-dimension images; it resizes them server-side. So the 20,000×20,000 px PNG does not 400 on the wire; it silently costs more tokens and loses fidelity (the guide's own advice: "resize them before sending"). The real failure modes are token-cost explosion and coordinate-fidelity loss, not a 400. D3 for OpenAI is therefore a cost/fidelity policy guard, not a validity guard — a reframing the ADR must record.
I accept all four of the Griller's recommendations, with the fork now decided by evidence rather than deferred: (1) guide-reading is a hard prerequisite — I have done it; (2) byte-based → n/a for OpenAI (no documented byte cap; DeepSeek's byte guard stays untouched); (3) the guard is dimension-based via image.DecodeConfig post-hydration in the same scan, with no domain-model change (llm.Blob = {MIMEType, Data}, types.go:38-42 — extending it ripples into clone/equal/persistence for no v1 benefit; per-turn header-only decode has the same cost profile as the byte scan, and retained images are decoded once per turn, post-hydration since AssetID parts carry no bytes until resolved); if the guide documented no client-enforceable limit, defer-to-provider — but it does, so enforcement is concrete; (4) the ADR records the enforcement dimension (pixels, per-detail) as a decision, with the guide URL and quoted limits pinned as evidence, mirroring ADR-070's pinned gist.
Concrete D3 shape, replacing my earlier byte-centric A3 commit. The shared scan loop survives, but the per-mode check becomes a policy: DeepSeek keeps its byte limits (32/48/64 MiB) byte-identical; OpenAI enforces maxDimension 2048px (v1 bound — detail is omitted, so the server default auto → 2048px; the per-detail table 2048/6000 is commented for when detail becomes configurable) via image.DecodeConfig on each image part, fail-loud with MediaSizeError{Kind: MediaSizePerImage, ...} and a message naming the dimension ("image max dimension N px exceeds the 2048px limit for detail=auto — resize before sending"). No patch-budget enforcement: gpt-5.6 has none, and patch counts are a cost model, not an input-validity constraint. A heuristic byte ceiling (the existing maxRequestBodyBytes class of constant) is retained for OpenAI strictly as a transport-sanity bound — explicitly labeled not a provider limit in the ADR — so the "unbounded base64-inflated request body" hole from the issue stays closed without claiming a doc that doesn't exist. One implementation caveat I verified: go.mod has no golang.org/x/image, and image.DecodeConfig covers PNG/JPEG/GIF from stdlib but cannot decode WebP — so WebP dimension enforcement is skipped (documented hole), which is harmless because the provider resizes rather than rejects; undecodable/corrupt non-WebP images fail loud (they would fail at the provider anyway).
Task-1 gate answer. Not read-then-fork, and not a memoryless Coder deciding the dimension: the Architect has read the guide, so the fork is made now, and D3 dispatches a concrete spec. The Coder's task 1 is a verify-and-stop-on-drift gate: re-read the guide (deliverable: the quoted dimension/patch limits with URLs and a confirmation they match the spec's 2048px/6000px table), and if the guide has changed — different numbers or a different enforcement dimension — STOP and report rather than proceed; the Architect re-adjudicates. This keeps the D3 task self-contained for a memoryless Coder while preserving the architect-owned decision the ADR requires.
Your A9 reframing — verified as internally consistent — states the provider does not reject over-dimension images ("Original detail can still resize images that exceed the model's limits"; it resizes server-side, succeeding at higher token cost and reduced fidelity). Yet your D3 guard FAILS LOUD on maxDimension > 2048px, aborting the turn before any request. That fail-loud mechanism was justified earlier in this round (Q3/Q5) as "the turn is doomed regardless — an over-cap request would 413 anyway, so the client-side error is the good failure." That justification is now false for the dimension guard: the turn is not doomed — the provider would accept it and resize. So the dimension check is a self-imposed hard block on turns the provider would serve: a policy choice, not a validity guard — and it goes beyond the issue's acceptance criterion 5, which is byte-shaped ("never silently base64-inflated into an unbounded request body"; your heuristic byte ceiling already closes that hole). Separately, one mechanics correction to A9: "image.DecodeConfig covers PNG/JPEG/GIF from stdlib" is only true if the client blank-imports image/png, image/jpeg, image/gif — the stdlib image package registers no formats on its own, and none of those imports exist in the tree today (verified), so the D3 task must specify them explicitly, and the WebP skip means the fail-loud guarantee is not format-uniform. My recommended answer: keep fail-loud, but record it in the ADR as a cost/fidelity policy, not a validity guard — over-dimension images are hard-blocked because the model's effective resolution is capped at 2048px anyway, so the user would pay full token cost for reduced fidelity, and "resize before sending" is the actionable truth-telling response; the accepted trade-off (blocking turns the provider would otherwise accept and resize) is stated explicitly; warn-and-proceed is rejected for v1 on the grounds that silent degradation is exactly the failure class this issue exists to kill; and the D3 task spec includes the three blank imports plus the documented WebP/format-uniformity asymmetry. Do you commit to that policy-and-rationale — or do you soften the dimension guard to warn-and-proceed on the grounds that the provider resizes anyway?
Verified — the Griller's mechanics claim is exact: no image/png, image/jpeg, or image/gif imports exist anywhere in the tree (the grep hits are MIME string literals in tests, not imports), and no image.DecodeConfig call exists at all. The stdlib image package registers no decoders by itself, so the D3 task must specify the three blank imports explicitly, or DecodeConfig returns "image: unknown format" for everything.
I commit to fail-loud-as-policy — and I correct one premise in the Griller's stated rationale, which makes the policy stronger, not weaker. The guide says token cost is computed after the pixel-dimension limit is applied: "Compute how many 32px × 32px patches are needed to cover the image after applying the pixel-dimension limit." So an over-dimension image is resized to ≤2048px first, and the user pays for the resized image's patches — not "full token cost for reduced fidelity." The Griller's cost framing is wrong; the actual harm is fidelity surprise and coordinate-mapping breakage: the user submits a 6000px diagram expecting fine-detail analysis and the provider silently analyzes a 2048px downscale, and (per the guide's own warning) coordinate-sensitive results map back to the resized image, breaking any coordinate contract. That is precisely the silent-degradation class this issue exists to kill, relocated from "image dropped" to "image silently downscaled." Fail-loud with "resize before sending" is the truth-telling response; the token-cost argument was never the load-bearing one.
The ADR records, explicitly: (1) the dimension guard is a cost/fidelity policy, not a validity guard — the provider accepts and resizes over-dimension images, so the client blocks turns the provider would otherwise serve; (2) the accepted trade-off is stated verbatim: over-dimension images are hard-blocked because the model's effective resolution at detail=auto is capped at 2048px, so the user would receive an analysis of a downscaled image without their knowledge — blocking is chosen over silent degradation; (3) warn-and-proceed is rejected for v1 on two grounds — the silent-degradation class, and the delivery surface: a warning would have to ride in the prompt or the UI, neither of which has a channel today, so "warn" would be a log line nobody sees, which is itself a silent degradation; (4) the dimension guard is an addition beyond acceptance criterion 5 (which is byte-shaped and already closed by the heuristic byte ceiling) — the ADR says so, so no reviewer reads it as criterion-implemented; (5) the enforcement dimension decision from A9 (pixels, per-detail table 2048/6000, v1 bound 2048 for the omitted-detail default) is recorded with the pinned guide URL.
Task-spec mechanics, as corrected: the D3 task includes _ "image/png", _ "image/jpeg", _ "image/gif" (blank imports in the openai package, in a dedicated decode-helper file so the registration site is greppable), and the documented format asymmetry: the fail-loud guarantee is not format-uniform — PNG/JPEG/GIF dimensions are enforced; WebP passes through to the provider's silent resize because go.mod has no golang.org/x/image and the stdlib cannot decode it. The hole is recorded in the ADR (with x/image named as the future dependency if WebP fidelity enforcement is ever required), and corrupt/undecodable non-WebP images fail loud — they would fail at the provider anyway. The guard reuses the Q4/Q5 machinery unchanged: MediaSizeError{Kind: MediaSizePerImage, Cap: 2048, Actual: maxDimension} with text "image max dimension N px exceeds the 2048px limit for detail=auto — resize before sending" — per-image kind, fixable input, session continues on the user's next message; DeepSeek's byte guard is untouched; the shared scan loop stays, with the per-mode check set differing by dimension (DeepSeek bytes, OpenAI pixels).
VERDICT: The subject's diagnosis held up almost entirely — every load-bearing claim I verified against the live tree (capability layer, dual routing gates, sink drop, ADR-070 constant provenance, NonFixCatalog pins, history-persisted-after-turn, TestResolveEndpoint/TestMandatoryContentField) checked out, and the genuine errors it made (surface-alternation scope, the "summarisation evicts" recovery, byte-shaped D3) were identified, owned, and corrected with sound reasoning — including a final correction to my own cost framing in A10 that strengthens the fail-loud policy rather than weakening it. The residual risks are all external facts now guarded by the right mechanisms: the live-guide numbers are pinned and gated by a verify-and-stop-on-drift task, the fail-loud branches carry direct unit tests, and the two-surface asymmetry is recorded as intentional in the ADR. Recommendation: proceed with changes — the changes being the decisions this round produced (full-history hasImage as an explicit ADR deviation from the issue's per-turn D1 wording, the dimension-based cost/fidelity policy guard with image.DecodeConfig + blank imports, the UseResponses *bool tri-state endpoint contract with the legacy fallback, the sink's fail-loud errUnhandledInputBlockType with toResponsesInput post-loop check, and the reworded "text-only turns unaffected" criterion) — the plan is now specific enough to dispatch to a memoryless Coder.
What I stand by. The verified core of the diagnosis is unchanged and I hold every load-bearing claim: the capability layer drops vision for GPT (resolveGPTFamily, capabilities.go:90-95), the dual routing gates genuinely disagree today (chat.go:120-124 vs 293-296), the Responses sink warn-and-drops (responses.go:32-46), the 32/48 MiB constants are DeepSeek-derived and must not be borrowed as OpenAI provider limits, the NonFixCatalog pins on client_vision_test.go/capabilities_test.go require same-commit re-anchoring, and failed turns never persist (history-persisted-after-turn), which makes every "eviction" recovery except --new/rollback unreachable. The decisions that survived the round unchanged in substance: D2-A (all gpt-5+ via isGpt5OrNewer, anti-allowlist), the single shouldUseResponses decision function, the sink's fail-loud translation contract, the typed MediaSizeError in the domain llm package with Unwrap → ErrTerminal, and the UseResponses *bool tri-state endpoint contract with the legacy fallback scoped to the unset state only.
What the grill revised — and I own each correction. (1) Surface alternation → full-history sticky hasImage: my opening's "alternation" was the per-turn variant; A1 deliberately abandoned it, and the grill forced me to reconcile the two — the ADR now records the deviation from the issue's per-turn D1 wording explicitly, with the reworded "text-only turns with no prior image in history are unaffected" criterion. (2) "Summarisation evicts the byte-cap failure" was wrong twice over — the gatekeeper is token-driven and the byte-cap scenario is token-invisible (≤384 tokens/image), and failed turns don't persist so turn-count pruning can't advance either; the aggregate recovery contract is now --new/rollback only, stated in the error message and the ADR. (3) D3 was mis-modeled as bytes — the grill's suspicion, confirmed by my own fetch of the live guide, is that OpenAI's constraints are dimension/patch-based with server-side resize, no byte cap, and no 400; the guard is now the cost/fidelity policy (2048px max dimension, image.DecodeConfig + three blank imports, WebP hole documented) with fail-loud justified by fidelity/coordinate truth-telling — and the grill accepted my correction of its own cost framing (tokens are counted on the resized image), which strengthened rather than weakened the policy. (4) The sink's fail-loud needed a propagation route: s.err captured in AddMessage + toResponsesInput post-loop check, asymmetric with the total standard sink, recorded as deliberate. (5) The D3 task is now self-contained for a memoryless Coder: verify-and-stop-on-drift task 1, concrete numbers, blank imports, message templates.
The final integrated plan, as the grill sanctioned it for dispatch: capability flip (D2-A) + boundary tests → Responses input_image translation with fail-loud sentinel errUnhandledInputBlockType → full-history hasImage routing through UseResponses *bool + legacy fallback → dimension-based OpenAI guard (2048px, DecodeConfig, typed MediaSizeError) beside the untouched DeepSeek byte guard → test surface (flip TestResponsesSink_AddMessage_NonStringContent, direct sink unit tests, one-time-flip routing tests, accumulated-crossing aggregate test, TestResolveEndpoint tri-state rows) → NonFixCatalog re-anchor + partition rows in the same commit → ADR (all decisions: full-history deviation, cost/fidelity policy, tri-state contract, error contract, format asymmetry) + verify-adr-index + configs/butler.yaml/README notes → make check-full.
Open questions, honestly remaining. (1) The live-guide numbers are pinned but the guide is live — the verify-and-stop-on-drift task-1 is the mechanism, and the residual risk is a guide change between this round and implementation; the ADR's pinned reference makes that detectable. (2) gpt-5.0–5.3 tool+image turns on Chat Completions (D2-A consequence) remain an untested combination — the design does not depend on the migration doc's tool+reasoning premise for correctness, but a live smoke test at implementation time would close it. (3) The API's tolerance of an omitted reasoning field on a no-effort image turn to /responses should be smoke-tested live; the tri-state contract is right, but the wire acceptance is an external fact. (4) The WebP fidelity hole (pass-through to silent server resize) is accepted for v1 — golang.org/x/image is the named future dependency if it ever matters. (5) The re-serialization cost of retained images on every subsequent turn under full-history hasImage is an accepted trade-off recorded in the ADR, but it deserves a UX note to the user so the session's image-token re-send never reads as a billing surprise. None of these block dispatch; all are either mechanism-guarded or explicitly accepted.