Skip to content

fix(acp): accept https:// URIs in image content blocks#24816

Open
truenorth-lj wants to merge 1 commit into
anomalyco:devfrom
truenorth-lj:fix-acp-image-https-uri
Open

fix(acp): accept https:// URIs in image content blocks#24816
truenorth-lj wants to merge 1 commit into
anomalyco:devfrom
truenorth-lj:fix-acp-image-https-uri

Conversation

@truenorth-lj
Copy link
Copy Markdown

Issue for this PR

Closes #24815

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

packages/opencode/src/acp/agent.ts:1394 previously read:

} else if (part.uri && part.uri.startsWith("http:")) {

That prefix only matches http://https:// fails the check (the 5th char is s, not :), so the entire case "image": branch falls through and parts.push never runs. The image is silently dropped from the prompt with no error.

This breaks ACP clients doing two-stage uploads (sign a GCS / S3 URL, pass the https:// URL via the ACP image block instead of inlining base64).

This PR aligns the check with the rest of the codebase, where every other URL test uses both prefixes:

File Pattern
packages/opencode/src/session/instruction.ts:146,168 startsWith("https://") || startsWith("http://")
packages/opencode/src/cli/cmd/import.ts:98 startsWith("http://") || startsWith("https://")
packages/opencode/src/tool/webfetch.ts:23 !startsWith("http://") && !startsWith("https://")
packages/opencode/src/config/config.ts:1270 startsWith("http://") || startsWith("https://")

The fix is one line:

-          } else if (part.uri && part.uri.startsWith("http:")) {
+          } else if (part.uri && (part.uri.startsWith("http://") || part.uri.startsWith("https://"))) {

This matches what the surrounding code clearly intends — parseUri(part.uri) two lines above already accepts both schemes — and brings this branch in line with the conventions used everywhere else in the repo.

How did you verify your code works?

Reproduced the bug locally before the fix:

  1. Run a local ACP client that sends session/prompt with { "type": "image", "uri": "https://example.com/x.png", "mimeType": "image/png" }.
  2. Inspect the parts array passed downstream — image part is missing.
  3. Apply the patch.
  4. Repeat — image part is now present with url set to the https:// URL.

The change is the most narrowly scoped possible (one boolean expression), and there are no tests for the surrounding case "image": switch in this file, so I haven't added one in this PR — happy to add one if you point me at the test file pattern you'd want.

Screenshots / recordings

N/A (no UI change).

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

The previous prefix check `startsWith("http:")` only matches `http://...`
URLs and silently drops `https://...` URLs. Every other URL check in this
codebase (instruction.ts, import.ts, webfetch.ts, config.ts) checks both
prefixes — this appears to be a typo.

The bug surfaces when an ACP client sends an image content block with a
two-stage upload URL (e.g. a GCS signed URL), which is always https. The
case falls through and the image is silently dropped from the prompt.

Repro:
  Send ACP session/prompt with content:
    { type: "image", uri: "https://example.com/x.png", mimeType: "image/png" }
  Expected: image part forwarded to LLM provider
  Actual: image silently dropped (no error, no parts.push)

Fix: align with the other 5 URL checks in this codebase by accepting both
http:// and https:// prefixes explicitly.
@truenorth-lj
Copy link
Copy Markdown
Author

Polite nudge — this has been open for 4 days with CI green and no conflicts on dev.

The fix is a one-line typo (startsWith("http:")startsWith("http://") || startsWith("https://")). Every other URL check in the codebase pairs both prefixes; only this one is missing https://, silently dropping images for any ACP client doing two-stage signed-URL uploads (GCS / S3 / Cloudfront).

@kitlangton — would you have a moment to look? Happy to add a unit test or split if anything's off.

I also just opened a related ACP fix (#25422 / issue #25421) — different bug, same area. Same offer there: regression test on request.

@truenorth-lj
Copy link
Copy Markdown
Author

@rekram1-node — would you have a moment to look at this one? It's been open 10 days with CI green and no conflicts.

The fix is one line (startsWith("http:")startsWith("http://") || startsWith("https://")) — every other URL check in the codebase already pairs both prefixes, only this one was missing https://, which silently drops images for any ACP client doing two-stage signed-URL uploads (GCS / S3 / Cloudfront).

Saw you've been merging ACP-area PRs from external contributors recently (#25698, #25591) — hoping to reach the right reviewer. Happy to add a regression test against createFakeAgent if useful, or feel free to push back if anything's off.

Also — if there's a path to getting onto the contributor / vouch list so future ACP fixes don't sit in the unfiltered queue, I'd appreciate the pointer.

@truenorth-lj
Copy link
Copy Markdown
Author

Hey — just doing housekeeping on stale PRs. Happy to close this one if the approach isn't right, or if it's blocked on anything I haven't addressed.

The fix is a one-line startsWith("http:")startsWith("http://") || startsWith("https://") to match every other URL check in the codebase, but no worries if it doesn't fit upstream priorities right now — just let me know either way and I'll act on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ACP image content blocks with https:// URI silently dropped at agent.ts:1394

1 participant