Skip to content

fix: validate opening input before allocating a sandbox - #1840

Merged
jhgaylor merged 4 commits into
mainfrom
fix/validate-opening-prompt
Sep 11, 2026
Merged

fix: validate opening input before allocating a sandbox#1840
jhgaylor merged 4 commits into
mainfrom
fix/validate-opening-prompt

Conversation

@jhgaylor

@jhgaylor jhgaylor commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Opening images without text were silently dropped while the request still created a conversation and reserved a sandbox. Validate fresh and attached opening input before allocation: reject whitespace/non-text prompts, images without text, and malformed, empty, unsupported or oversized image bytes. Launches without an opening prompt remain valid; valid text/image bytes reach delivery.

Five files, +170/-6, extracted from #1754 onto #1818. The two HTTP fixtures now include real prompts and assert delivered bytes; an added HTTP regression requires image-only input to return 422 without creating rows. OpenAI-compatible image-only messages already supply a generated text prompt.

Validation: 72 focused start/attach tests and 102 domain/HTTP tests pass. Six regression cases fail on the unchanged callers. The first full gate exposed the two success-only HTTP fixtures described above; the corrected full mix precommit --seed 828329 passes: 4,896 tests +6 doctests, zero failures. Staged secret scan passes. CI passes (run).

Part of #1864

@BinaryBourbon BinaryBourbon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Verified the regression coverage the same way as #1826 — by removing the fix.

Deleted both PromptInput.validate_initial(attrs) clauses from the two with chains and re-ran:

10 tests, 6 failures

The six failures are exactly the refusal cases (invalid text, images-without-text, malformed/empty/unsupported/oversized bytes, across both create and attach); the four that still pass are the "valid input is accepted" cases, which should pass either way. That's the right ratio and it matches the PR body.

The bug is a good one to have caught: images without text were silently dropped after the request had already created a conversation and reserved a sandbox, so the user paid a slot for input that never arrived. Validating before allocation is the fix, and putting it first in the with — ahead of get_agent — means it costs nothing on the refusal path.

One thing I specifically checked, because it's where this kind of validator usually breaks: valid_image?/1 matches atom keys (%{media_type: _, data: _}). Every path that reaches attrs["images"] goes through FountainWeb.PromptImages.decode/1, which returns {:ok, [%{media_type: mt, data: binary}]} — the conversation controller, the team controller, and the OpenAI-compatible controller all do. So the shapes line up and no caller silently starts getting :invalid_images.

Also good that a launch with no opening prompt at all stays valid — {prompt, []} when prompt in [nil, ""] -> :ok is the first clause, so this only constrains launches that actually carry input. That's the case most likely to be broken by a validator like this and it's pinned by a test.

@BinaryBourbon BinaryBourbon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes — for the contract, not the code. The validation itself is right and belongs where it has been put.

Validating before Agents.get_agent/2 means a malformed opening prompt never reserves a sandbox, never creates a conversation and never starts a worker, and the tests assert that with Repo.aggregate counts on both Conversation and Sandbox across the create and attach paths rather than by inspecting the return value alone. Rejecting a whitespace-only prompt, a non-binary prompt and images-without-text all in one gate is the right grouping — each of those previously reached provisioning and cost a machine.

The break

POST /api/conversations with images and no prompt returned 201 before this PR and returns 422 invalid_prompt after it. This PR is where that behaviour was asserted, and the PR rewrites the assertion:

  • conversation_controller_test.exs — the case named "returns 201 ... when images array is provided" posted agent_id + images with no prompt. It now posts "prompt" => "Review".
  • turn_image_ingest_test.exspost_create/4 gained "prompt" => "Describe these images" so the whole media-type matrix keeps passing.

Two tests changed to accommodate a change in the public contract is the signal. The behaviour change may well be correct — an image with no instruction is not a prompt — but the contract has to move with it:

  1. schemas.ex still advertises the old shape. Line 522 prompt: "Optional first turn prompt.", line 536 images: "Optional images to attach to the initial prompt." Both are described as independently optional, prompt is not in required, and the four SDKs are generated from this spec. As it stands the OpenAPI document says a request is valid that the server now rejects. Please make the images description state that an opening prompt is required alongside them, and have prompt say blank and whitespace-only are refused.
  2. No docs or changelog entry. A client that attaches a screenshot with no caption gets a new 422 with no note anywhere.

Worth confirming the intended contract explicitly while you are in there: the OpenAI-compatible path already decided the other way — openai_controller.ex:604 synthesises "(see the attached image)" for an image-only message rather than refusing it, with the comment "An image with no words is still a prompt." That is a reasonable divergence between a compatibility shim and the native API, but right now the two disagree by accident rather than by decision. One sentence in the schema description settles it.

One hardening note, not blocking

valid_image?/1 matches %{media_type: media_type, data: data} — atom keys only — and anything else falls to the false clause, so a string-keyed image map is reported as :invalid_images rather than as a shape error. I traced every current producer and they are all safe: FountainWeb.PromptImages.decode/1 returns %{media_type: mt, data: data}, and the API controller, the LiveView, team_controller.ex:388 and openai_controller.ex (whose string-keyed %{"media_type" => ..., "data" => ...} at line 642 is intermediate and passes through decode/1) all go through it. So this is correct today.

It is fragile though: validate_initial/1 reads attrs["images"] with a string key and then requires atom keys one level down. A future caller that builds attrs by hand gets a confusing :invalid_images for a valid image. Either accept both key shapes as decode_image/1 does, or say in the @moduledoc that images must already have been through PromptImages.decode/1.

Related: PromptInput now re-checks media type and size, which FountainWeb.PromptImages already checked with a friendlier message. #1818 shared the constant; the rule is still in two places. Fine as defence in depth for non-web callers — worth a comment saying that is what it is.

Marginal

Team.Schedule validates prompt with min: 1, so a single-space prompt saves fine and now fails at launch with :invalid_prompt through schedules.ex:217. Extremely unlikely to matter; mentioning it only because the schedule error surfaces asynchronously.

jhgaylor added a commit that referenced this pull request Sep 11, 2026
Review of #1840 landed one blocking point: the launch contract changed and
nothing that describes it changed with it. A request carrying images and no
prompt returned 201 before this branch and 422 after it, and the branch
rewrote the two tests that asserted the old shape without touching the
OpenAPI document the four SDKs generate from.

- `schemas.ex` says what is actually enforced. `prompt` names the
  whitespace-only refusal; `images` says they require that prompt and names
  both 422s. They were each described as independently optional.
- `docs/api.md` gains the same two paragraphs for a human reader.
- CHANGELOG records the break under Changed, including the deliberate
  divergence from the OpenAI-compatible endpoint, which synthesizes a caption
  for an image-only message rather than refusing it.

Two smaller findings from the same review:

- `PromptInput` accepted atom-keyed images only. Every current caller routes
  through `PromptImages.decode/1`, which returns atom keys, so nothing was
  broken — but a context caller building the map by hand got
  `:invalid_images` for a valid image. It now reads either shape with
  `Map.get/2`, which also keeps a struct out of Access. The moduledoc says
  why the media-type and size checks repeat the web layer's, and why the
  OpenAI path decides the other way.
- `:invalid_prompt` and `:invalid_images` had no `FallbackController` clause,
  so every rejected launch fell to the terminal safety net: a warning in the
  log and a body with no `message`, unlike every other refusal the controller
  names. Both now answer 422 with a message that says what to send instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0181T6GGULcZfUa6dQtgNwtY
@jhgaylor

Copy link
Copy Markdown
Collaborator Author

Pushed 74c863a8.

The contract. schemas.ex now says what is enforced. prompt names the whitespace-only refusal; images says they require that prompt and names both 422s. They were each described as independently optional, which the four SDKs generate from. Same two paragraphs added to docs/api.md for a human reader, and a CHANGELOG entry under Changed that calls the break out.

The OpenAI divergence is now deliberate rather than accidental: the native API refuses an image with no words, the compatibility endpoint keeps synthesizing "(see the attached image)", and both the PromptInput moduledoc and the changelog say why.

Image key shapes. Accepts either now, via Map.get/2 rather than image[...] so a struct does not raise out of Access. Nothing was broken — every current producer routes through PromptImages.decode/1, including openai_controller.ex, whose string-keyed map at line 642 is intermediate — but validate_initial/1 reading attrs["images"] with a string key and then demanding atom keys one level down was a trap for the next context caller. Tests cover both shapes and the struct case.

The duplicated checks are called out in the moduledoc as deliberate: decode/1 belongs to the two HTTP transports, and Fountain.Team, a schedule or a future worker reaches start_conversation/2 without it. The web layer's friendlier message still wins for HTTP callers because it runs first.

One thing the review did not catch, which the test output did:

[warning] fallback: unmapped error atom :invalid_prompt on /api/conversations

Neither atom had a FallbackController clause, so every rejected launch fell to the terminal safety net — a warning in the log and a body with error but no message, unlike every other refusal the controller names. Both are named now and answer 422 with a message saying what to send instead.

114 tests green across the opening-input, conversation-controller, turn-image-ingest and fallback-controller suites, plus the docs and schema-guard tests. credo --strict and format clean, the three prose gates clean on docs/api.md, and scripts/sdk-contract/build.sh --check reports ok (163 operations, 189 schemas).

@BinaryBourbon BinaryBourbon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for one mechanical thing: the contract moved, but only half of it. The rest of this commit is right and I verified it.

The blocker: sdk/typescript/src/generated/openapi.ts is stale

schemas.ex is the source, and the generated TypeScript types are built from the spec it produces — CI regenerates them and fails on a diff (.github/workflows/ci.yml, "SDK types match the spec"). I ran exactly what that job runs:

$ scripts/sdk-contract/build.sh
wrote dist/openapi.json and sdk/contract/contract.json (163 operations, 189 schemas)
$ cd sdk/typescript && npm ci && npm run generate
$ git diff --stat -- src/generated/openapi.ts
 sdk/typescript/src/generated/openapi.ts | 4 ++--
-/** @description Optional images to attach to the initial prompt. */
+/** @description Optional images to attach to the initial prompt. They require that prompt: … */
 images?: components["schemas"]["ImageInput"][] | null;
-/** @description Optional first turn prompt. */
+/** @description Optional first turn prompt. A launch may open with no prompt at all, but … */
 prompt?: string;

scripts/sdk-contract/build.sh --check is fine — contract.json drops descriptions by design, so it moves only when the wire shape moves. It is the whole-document arm that catches this. Run scripts/sdk-contract/build.sh, then npm run generate in sdk/typescript, and commit src/generated/openapi.ts.

Worth noticing why this is the one thing that slipped: the point of the commit is that a contract change has to reach everything that describes the contract, and the file that a TypeScript caller actually reads is the one that did not get updated. A client on the SDK still sees prompt?: string documented as freely optional next to images.

Everything else checks out

The contract now says what is enforced. schemas.ex names both 422s and says they happen before a sandbox is reserved; docs/api.md says the same for a human; the CHANGELOG files it under Changed and records the deliberate divergence — the OpenAI-compatible endpoint still synthesizes a caption, and the moduledoc points at OpenAIController.non_empty/2 so the next reader finds the decision instead of rediscovering the disagreement. That was the whole of my last review's blocking point and it is properly done.

The FallbackController clauses are worth more than they look. I deleted both and re-ran:

1) unusable opening input names itself rather than falling to the safety net
   code: assert is_binary(body["message"]) and body["message"] != ""
[warning] fallback: unmapped error atom :invalid_prompt on /

So before this commit a rejected launch got a 422 with the right error key, no message, and a warning in the log on every ordinary client mistake — which would have read as a server-side defect in the logs rather than a user typo. Good catch on your own PR.

String-keyed images. Reverted valid_image?/1 to the atom-only head and re-ran opening_input_test.exs:

1) a context caller may supply string-keyed images
   left:  {:ok, _}   right: {:error, :invalid_images}

That one test carries the change. Map.get/2 over image[...] is the right call for the struct case, and %URI{} confirms it does not raise.

One note on the other two new tests: "a string-keyed image is held to the same rules" and "a struct is not an image" both pass on the pre-fix code as well — atom-only matching rejected every string-keyed map with :invalid_images too, so the assertion cannot tell "rejected for the right reason" from "not read at all." They are not wrong, they just are not the guard; the accept case is. If you want the rejection cases to bite, assert the positive alongside them in the same test, so the pair only passes when the map is actually being read.

No other gate moves. docs/api.md is clean on all six gating vale rules (only STE.Vocabulary suggestions, which advise), destink reports 112 pages clean, docs-style.py's two findings are pre-existing on codex.md and environment.md and neither is touched here. credo --strict and mix format --check-formatted clean. 159 tests green across conversations_start_test.exs, conversation_controller_test.exs, turn_image_ingest_test.exs and docs_test.exs; 22 green across the two files this commit touches.

Two smaller things, not blocking

  • The 10MB ceiling is a literal again. Fountain.Images.max_prompt_image_bytes/0 exists and #1818 in this same stack was the PR that put the limit in one place. The FallbackController message is built at runtime and could read it; it hardcodes "and between 1 byte and 10MB of data" instead. The schema description and docs/api.md are prose and fine as they are, but the message is code. One div(Fountain.Images.max_prompt_image_bytes(), 1024 * 1024) keeps it from drifting.
  • Team.Schedule is still the one asynchronous surface. min: 1 saves a single-space prompt that now fails at launch through schedules.ex:217. Marginal, as I said last time, and I am not asking for it here — just noting it did not get picked up so it does not look decided.

### Changed collides with #1768, which inserts its own section at the same position in the CHANGELOG. Whichever rebases second will conflict there.

@BinaryBourbon

Copy link
Copy Markdown
Contributor

CI landed on the same thing while I was writing: Release boot and the API contract is red, failing step SDK types match the spec (job). Same 4-line diff as above — scripts/sdk-contract/build.sh, then npm run generate in sdk/typescript, and commit src/generated/openapi.ts.

@jhgaylor

Copy link
Copy Markdown
Collaborator Author

Pushed dfeddc8c. You were right, and so was CI — the previous run failed on exactly the step you named:

success  SDK wire contract is current
failure  SDK types match the spec

Which is the distinction you drew: contract.json drops descriptions, so --check was never going to see this. Regenerated with the two commands from your review; the diff is the two lines you quoted and nothing else, and a second build.sh + npm run generate produces the same diff, so it is idempotent rather than accidentally stable. The SDK typechecks, builds, passes verify-contract (71 operations, 47 schemas, 5 enums) and its 103 tests against the new types.

No excuse for it — the whole point of the commit was that a contract change has to reach everything describing the contract, and the file a TypeScript caller actually reads was the one that missed out.

The 10MB literal. Fixed. The message is code, so it reads Fountain.Images.max_prompt_image_bytes/0 now. Schema description and docs/api.md stay prose, as you said.

The tests that did not bite. This was the more useful note. You were right that a rejection alone could not distinguish "refused for the right reason" from "never read" — atom-only matching refused every string-keyed map with :invalid_images too. Each case now asserts the accept alongside the refusal in the same test. Confirmed by reverting valid_image?/1 to the atom-only head:

1) a string-keyed image is read, not merely refused   left: :ok     right: {:error, :invalid_images}
2) string-keyed and atom-keyed images reach delivery  left: {:ok,_} right: {:error, :invalid_images}
3) a struct is refused rather than raised out of Access  left: :ok  right: {:error, :invalid_images}
13 tests, 3 failures

All three now, where before only the accept case did. I also checked Map.get/2 is load-bearing rather than defensive taste — swapping it back to image[...] fails the struct test on URI.fetch/2 is undefined (URI does not implement the Access behaviour).

Two things I had to change from your suggested shape, both mechanical: the paired assertions run against validate_initial/1 rather than through the context, because the accept half provisions a sandbox and the second case hit {:sandbox_quota_exceeded, %{count: 2, limit: 2}}; and Mimic will not take reject then stub on the same function inside one test. The delivery assertion uses the attach path instead, which proves the bytes survive unchanged without a second machine.

Team.Schedule left alone and undecided, as you framed it.

The CHANGELOG collision with #1768 is real — both insert a ### Changed at the same offset. #1768 is the one further down the stack, so I will take the conflict there when this rebases.

@jhgaylor jhgaylor added the release:skip-sdk Explicitly allows an SDK surface change without releasing a new package version. label Sep 11, 2026
@jhgaylor

Copy link
Copy Markdown
Collaborator Author

The SDK types step passes now. A different gate fired on the same push:

success  Release boot and the API contract      (SDK types match the spec ✓)
failure  A change to the SDK says whether it releases

Fair — touching sdk/typescript/** has to declare intent. Taking the sdk-no-release escape hatch rather than bumping, and the sentence the gate asks for:

Why no release. The change is JSDoc only. contract.json did not move, which is the same fact that made --check pass while the whole-document arm failed — nothing about the wire shape, the runtime or the type signatures changed, only the descriptions attached to two optional fields.

Why a bump here would be wrong rather than merely unnecessary. This branch's base carries 1.23.0. main is at 1.27.0 and npm latest is 1.24.0:

main:   1.27.0
branch: 1.23.0
npm:    1.24.0

npm version patch from here computes 1.23.1, which is below both — it either fails to publish or drags latest backwards, and it conflicts on rebase regardless. There are also five other open PRs bumping this package right now (#1893, #1885, #1876, #1860, #1759), and bumps have to merge in ascending order.

The regenerated descriptions ride along with whatever version main is at when this stack rebases, and the next genuine bump publishes them. Nothing is lost except the delay, which is the right trade against picking a version number off a base that is four minors stale.

@BinaryBourbon BinaryBourbon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes — and the thing I am blocking on is a suggestion I made in the last review. I gave you two options for valid_image?/1 and recommended the wrong one first. Taking the string-key option turns a clean refusal into a crashed turn, which is the exact property this PR exists to establish.

The blocker from last time is genuinely fixed

I ran the CI step rather than trusting the diff:

$ scripts/sdk-contract/build.sh --check
SDK wire contract: ok (163 operations, 189 schemas)
$ cd sdk/typescript && npm ci && npm run generate
$ git diff --exit-code -- src/generated/openapi.ts
GENERATED TYPES: in sync (no diff)

Regeneration is idempotent and the committed file is byte-identical to what the spec produces. Release boot and the API contract will go green.

The FallbackController message derives the ceiling now — rendered it to be sure it is not off by a factor of 1024:

{"error":"invalid_images","message":"each image needs a supported media_type (image/png, image/jpeg, image/gif, image/webp) and between 1 byte and 10MB of data"}

And the test rework does what you say. Reverting valid_image?/1 to the atom-only head now fails all three, where before only the accept case moved:

1) a string-keyed image is read, not merely refused
2) a struct is refused rather than raised out of Access
3) string-keyed and atom-keyed images reach delivery unchanged
13 tests, 3 failures

Swapping Map.get/2 for image[...] fails the struct case on UndefinedFunctionError: function URI.fetch/2 is undefined, so that choice is load-bearing and now pinned. Pairing each refusal with the accept was the right shape. 958 tests green across test/fountain/conversations, the conversation controller, the image-ingest matrix and the fallback controller; credo --strict and mix format --check-formatted clean.

The blocker: validate_initial/1 now accepts a shape nothing downstream can read

validate_initial/1 says :ok for a string-keyed image. Every consumer of that list still matches atom keys only:

# conversations.ex:1077 — _unsafe_insert_turn_images/2
|> Enum.reduce_while({:ok, 0}, fn {%{media_type: mt, data: data}, idx}, {:ok, count} ->

# output.ex:234 — write_image_temp_files/3
|> Enum.map(fn {%{media_type: mt, data: data}, idx} ->

Drove all three against the same map:

validate_initial(%{"prompt" => "Review", "images" => [%{"media_type" => "image/png", "data" => <<0,1,2>>}]})
  → :ok

TurnMachine.store_images(turn, [string_keyed])          → {:RAISED, FunctionClauseError}
Conversations._unsafe_insert_turn_images(id, [same])    → {:RAISED, FunctionClauseError}
Output.write_image_temp_files(handle, "t1", [same])     → {:RAISED, FunctionClauseError}

store_images/2 is on every path. conversation_server.ex:2316 calls it in run_turn/6 before the ACP branch, so this is not the legacy-spawn corner — claude, codex, gemini and opencode all reach it. Its {:error, changeset} arm logs and continues, but a FunctionClauseError is a raise, not an error tuple, so nothing catches it. It propagates out of kick_turn/4 into handle_call (:1518) or handle_cast (:1679), with no rescue on either. There is one at :789, and it covers provisioning, not this.

So the end state for a context caller who hands start_conversation/2 a hand-built string-keyed image:

before this branch on this branch
validation {:error, :invalid_images} :ok
conversation row not created created
sandbox not reserved reserved
first turn FunctionClauseError, server down

That is the inversion. The PR's own CHANGELOG entry is "refuses an opening prompt it cannot use, before it reserves a sandbox or creates the conversation" — and this one class of input it cannot use now goes all the way to a crashed turn on a machine the tenant paid for.

Reachability today is unchanged: every current producer routes through PromptImages.decode/1, which returns atom keys, so nothing in production hits either the old refusal or the new crash. It is latent both ways. What moved is which way it fails when someone does reach it, and the new "reach delivery unchanged" test writes the pass-through down as intended behaviour, which is what makes it worth fixing now rather than discovering later.

That test stops exactly one call short of catching this, because it mocks ConversationServer.send_prompt/4. The crash is the next thing that would have happened.

What I would do

My previous review offered two options. The second one is the right one:

Either accept both key shapes as decode_image/1 does, or say in the @moduledoc that images must already have been through PromptImages.decode/1.

Take the second. Put the atom-only head back on valid_image?/1, and say in the moduledoc that attrs["images"] is the decoded shape — which is already true of every caller, and which the rest of the pipeline has been assuming all along. A context caller then gets {:error, :invalid_images} before anything is spent, which is what this PR is for. The struct case still needs its clause, and %URI{} still pins it.

If you would rather keep both shapes accepted, then _unsafe_insert_turn_images/2 and write_image_temp_files/3 have to read both as well — and the delivery test should drive a real turn instead of mocking send_prompt, or it will keep passing over the crash.

Either way the "a string-keyed image is read, not merely refused" case has to move with the decision; it currently pins the behaviour I am asking you to reconsider.

Not blocking

Team.Schedule's min: 1 staying as-is is fine by me — you flagged it as noted rather than decided, which is the honest state of it.

### Changed still collides with #1768's section at the same position in the CHANGELOG; that is a rebase note, not a PR defect.

@jhgaylor

Copy link
Copy Markdown
Collaborator Author

Pushed 3f29d198. Taking option two, and you were right to block on it.

I verified the crash rather than accepting the reasoning, against one map:

validate_initial(%{"prompt" => "Review", "images" => [%{"media_type" => "image/png", "data" => <<0,1,2>>}]})
  → :ok

TurnMachine.store_images(turn, [same])               → {:RAISED, FunctionClauseError}
Conversations._unsafe_insert_turn_images(id, [same]) → {:RAISED, FunctionClauseError}
Output.write_image_temp_files(handle, "t1", [same])  → {:RAISED, FunctionClauseError}

And the rescue analysis holds — the three in conversation_server.ex are at :789 (provisioning), :1046 (a checkpoint Task.start) and :2193 (terminate/2). None is on the run_turn/6 path.

valid_image?/1 pattern-matches the decoded shape again. That also retires the Map.get/2 I was defending two reviews ago: with a pattern match a struct simply falls to the _ clause, so there is no Access call to raise out of and nothing to work around. The moduledoc states the contract and names the three consumers that enforce it, so the next person to loosen this reads why it is tight first.

The two tests moved with the decision, because the old ones pinned exactly the behaviour you asked me to reconsider:

  • "only the decoded shape is accepted" — bad media type, empty bytes, each key missing, oversized, a string-keyed map and %URI{}, each paired with the accept. Re-applying the lenient version fails it, so the decision is pinned rather than merely implemented.
  • "the accepted shape survives every consumer that runs after provisioning" — the gap you named. The delivery test mocks send_prompt/4 and stops one call short, so this drives _unsafe_insert_turn_images/2, store_images/2 and write_image_temp_files/3 for real. Building it turned up a wrinkle worth recording: store_images/2 swallows a duplicate-position changeset error by design, so sharing one turn between the first two assertions made it pass without reading the map. It takes a turn each now.

997 tests green across test/fountain/conversations, the conversation controller, the image-ingest matrix, the fallback controller and docs. credo --strict and format --check-formatted clean. Generated SDK types untouched this round and still in sync, so sdk-no-release still describes the change.

For the record on how this went: the string-key handling was a suggestion in your first review, I took it, and it took a third review to catch that it inverted the PR's own guarantee. The useful lesson is that "reach delivery unchanged" mocked the boundary one call before the failure — a pass-through test that stops at a mock will happily certify a shape nothing downstream can read.

@BinaryBourbon BinaryBourbon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. This is the right call, and the test that came with it is better than the one I asked for.

valid_image?/1 is the decoded shape or nothing again, the moduledoc says why with the three consumers named, and a caller holding bytes of its own is pointed at PromptImages.decode/1. That is the option that keeps a free refusal free.

Verified

The decision is pinned. Re-applied the lenient Map.get/2 version on top of this commit:

1) test only the decoded shape is accepted
   left:  {:error, :invalid_images}   right: :ok

So the string-keyed and %URI{} entries in that list are load-bearing now, and each is paired with the accept, which means the test can tell "refused for the right reason" from "refused wholesale". Anyone loosening this again has to delete an assertion to do it.

The consumer test is not decorative. This was the part I was unsure about — a test that drives three functions with a shape they already accept can easily assert nothing. So I broke one of them: changed Output.write_image_temp_files/3 to match %{"media_type" => mt, "data" => data}.

1) test the accepted shape survives every consumer that runs after provisioning
   ** (FunctionClauseError) no function clause matching in anonymous fn/1
      in Fountain.Conversations.Output.write_image_temp_files/3

It fails, with the exact error class it exists to prevent. That makes it a two-way guard: the first test stops the validator drifting looser than the pipeline, this one stops the pipeline drifting away from the validator. Splitting the turns so store_images/2's duplicate-position swallow could not hide a read is the detail that makes it work — sharing one turn would have passed either way.

Nothing downstream of the tightening lost a caller. I re-walked the one place a string-keyed image is constructed: openai_controller.ex:642 builds %{"media_type" => …, "data" => …} from a data URL, and content_images/1 hands it straight to decode_images/1PromptImages.decode/1{:ok, %{media_type: mt, data: data}}. Every other producer already went through decode/1. No caller sees a new :invalid_images.

The earlier round holds. scripts/sdk-contract/build.sh --check ok, npm run generate produces no diff — the generated types are still in sync and this commit did not disturb them. 987 tests green across test/fountain/conversations, the conversation controller, the image-ingest matrix, the fallback controller and docs; mix compile --warnings-as-errors, credo --strict and mix format --check-formatted clean.

Leaving the CHANGELOG alone is the right call: the entry describes POST /api/conversations, where images arrive as JSON and reach the context already decoded, and nothing about that changed this round.

Where the PR ends up

Worth saying plainly, because this one took three rounds to settle:

  • A launch that carries input Fountain cannot use is refused before Agents.get_agent/2, before the reservation and before the conversation row, and the tests assert that with row counts rather than return values.
  • :invalid_prompt and :invalid_images name themselves in the FallbackController with a message that says what to send instead, instead of falling to the terminal net and logging a warning on an ordinary client mistake.
  • schemas.ex, docs/api.md, the CHANGELOG and sdk/typescript/src/generated/openapi.ts all describe the behaviour that is actually enforced, including the deliberate divergence from the OpenAI-compatible endpoint.
  • The image shape has a written contract with a test on each side of it.

The two things I raised that are not addressed are both fine unaddressed: Team.Schedule's min: 1 still saves a single-space prompt that fails at launch, left noted rather than decided; and ### Changed collides with #1768's section at the same spot in the CHANGELOG, which is a rebase note for whoever merges second, not a defect here.

@jhgaylor
jhgaylor force-pushed the refactor/shared-prompt-image-limit branch from ebddf8a to d5810d4 Compare September 11, 2026 07:30
jhgaylor and others added 4 commits September 11, 2026 05:23
Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>
Review of #1840 landed one blocking point: the launch contract changed and
nothing that describes it changed with it. A request carrying images and no
prompt returned 201 before this branch and 422 after it, and the branch
rewrote the two tests that asserted the old shape without touching the
OpenAPI document the four SDKs generate from.

- `schemas.ex` says what is actually enforced. `prompt` names the
  whitespace-only refusal; `images` says they require that prompt and names
  both 422s. They were each described as independently optional.
- `docs/api.md` gains the same two paragraphs for a human reader.
- CHANGELOG records the break under Changed, including the deliberate
  divergence from the OpenAI-compatible endpoint, which synthesizes a caption
  for an image-only message rather than refusing it.

Two smaller findings from the same review:

- `PromptInput` accepted atom-keyed images only. Every current caller routes
  through `PromptImages.decode/1`, which returns atom keys, so nothing was
  broken — but a context caller building the map by hand got
  `:invalid_images` for a valid image. It now reads either shape with
  `Map.get/2`, which also keeps a struct out of Access. The moduledoc says
  why the media-type and size checks repeat the web layer's, and why the
  OpenAI path decides the other way.
- `:invalid_prompt` and `:invalid_images` had no `FallbackController` clause,
  so every rejected launch fell to the terminal safety net: a warning in the
  log and a body with no `message`, unlike every other refusal the controller
  names. Both now answer 422 with a message that says what to send instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0181T6GGULcZfUa6dQtgNwtY
The blocker was exact and CI agreed: `SDK types match the spec` failed on the
last push while `SDK wire contract is current` passed, because `contract.json`
drops descriptions by design and only moves when the wire shape does. The
whole-document arm is what catches a description change, and the file a
TypeScript caller actually reads still documented `prompt?: string` as freely
optional next to `images`.

    scripts/sdk-contract/build.sh
    cd sdk/typescript && npm ci && npm run generate

Two lines, the two the review named. Regeneration is idempotent (a second
run produces the same diff and no more), and the SDK typechecks, builds,
passes verify-contract and its 103 tests against the new types.

The irony was the point of the commit it fixes: a contract change has to
reach everything that describes the contract, and this was the one file that
did not get it.

Also from the review:

- The 10MB ceiling had become a literal again in the `FallbackController`
  message, one PR after #1818 put it in one place. That message is code, not
  prose, so it reads `Fountain.Images.max_prompt_image_bytes/0` now. The
  schema description and docs/api.md stay prose.
- The string-keyed image tests did not bite. `"held to the same rules"` and
  `"a struct is not an image"` passed on the pre-fix code too, because
  atom-only matching refused every string-keyed map with `:invalid_images`
  as well, so neither could tell "refused for the right reason" from "never
  read". Each case now asserts the accept alongside the refusal, which only
  holds when string keys are read at all. Verified by reverting
  `valid_image?/1` to the atom-only head: all three new tests fail, where
  before only the accept case did. Swapping `Map.get/2` back to `image[...]`
  fails the struct test on `URI.fetch/2 is undefined`, so that choice is
  load-bearing too.

  They moved to `validate_initial/1` rather than through the context, because
  the accept half provisions a sandbox and the second case hit the tenant's
  quota. The delivery assertion uses the attach path for the same reason: it
  is about the bytes surviving unchanged and needs no second machine to say
  so.

Not done, deliberately: `Team.Schedule`'s `min: 1` still saves a single-space
prompt that fails at launch. Left as noted rather than decided.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0181T6GGULcZfUa6dQtgNwtY
Taking the second of the two options the review offered, because the first
one — which I took last round — turns a free refusal into a crashed turn.

`validate_initial/1` accepted string-keyed images. Nothing downstream can
read them. Verified rather than reasoned about, against one map:

    validate_initial(%{"prompt" => "Review",
                       "images" => [%{"media_type" => "image/png",
                                      "data" => <<0,1,2>>}]})
      -> :ok

    TurnMachine.store_images(turn, [same])              -> {:RAISED, FunctionClauseError}
    Conversations._unsafe_insert_turn_images(id, [same]) -> {:RAISED, FunctionClauseError}
    Output.write_image_temp_files(handle, "t1", [same])  -> {:RAISED, FunctionClauseError}

`store_images/2` is on every runtime's path (`run_turn/6`, before the ACP
branch). It turns an `{:error, changeset}` into a log line and carries on, but
a `FunctionClauseError` is a raise, and the three `rescue`s in
conversation_server.ex cover provisioning, a checkpoint Task and `terminate/2`
— none of them this. So the shape went in at validation and came out as a dead
server on a sandbox the tenant had already paid to provision, which inverts
the entire point of validating before the reservation.

`valid_image?/1` pattern-matches the decoded shape again. A struct or a
string-keyed map falls to the `_` clause and is refused, with no Access call
to raise out of, so the `Map.get/2` gymnastics go too. The moduledoc now
states the contract and names the three consumers that enforce it, so the next
person to loosen this reads why it is tight before they do.

Tests moved with the decision, since the old ones pinned the behaviour being
reverted:

- `"only the decoded shape is accepted"` covers the bad media type, empty
  bytes, both missing keys, the oversized payload, a string-keyed map and a
  struct, each paired with the accept so the pair only passes when the map is
  read rather than rejected wholesale. Re-applying the lenient version fails
  it.
- `"the accepted shape survives every consumer that runs after provisioning"`
  closes the gap the review identified: the delivery test mocks
  `send_prompt/4` and stops one call short of the crash. This one drives
  `_unsafe_insert_turn_images/2`, `store_images/2` and
  `write_image_temp_files/3` for real. A turn each for the first two, because
  `store_images/2` swallows a duplicate-position changeset error by design and
  sharing one would have hidden whether it read the map at all.

No CHANGELOG change: the entry is about the HTTP API, where images arrive as
JSON and reach the context already decoded. The generated SDK types are
untouched this round and still in sync.

997 tests green across test/fountain/conversations, the conversation
controller, the image-ingest matrix, the fallback controller and docs.
credo --strict and format clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0181T6GGULcZfUa6dQtgNwtY
@jhgaylor
jhgaylor changed the base branch from refactor/shared-prompt-image-limit to main September 11, 2026 09:24
@jhgaylor
jhgaylor force-pushed the fix/validate-opening-prompt branch from 3f29d19 to 6c19d11 Compare September 11, 2026 09:24
@jhgaylor
jhgaylor added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit 90eb43e Sep 11, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:skip-sdk Explicitly allows an SDK surface change without releasing a new package version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants