feat(ios): og:image thumbnails on link preview cards#101387
Merged
Merged
Conversation
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 3:11 AM ET / 07:11 UTC. Summary PR surface: Other +487. Total +487 across 2 files. Reproducibility: not applicable. as a bug reproduction; this is a feature PR. Source review gives a high-confidence merge-blocker path for the new image fetch/decode behavior, and the PR still lacks real UI proof. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Keep the tap-to-expand thumbnail direction, but land it only after the remote-image fetch/decode boundary is tightened or explicitly approved, MIME policy matches the implementation, ImageIO downsampling is hardened, and redacted device/simulator proof is added. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug reproduction; this is a feature PR. Source review gives a high-confidence merge-blocker path for the new image fetch/decode behavior, and the PR still lacks real UI proof. Is this the best way to solve the issue? No as submitted. The owner boundary in shared Apple chat UI is plausible, but the best fix must settle the image security policy, align decoder behavior, and provide real device or simulator proof before merge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 521bf99f179d. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Other +487. Total +487 across 2 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e49dd0c866
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+451
to
+455
| func fetchImage(_ originalURL: URL) async -> ChatLinkPreviewImageResult { | ||
| guard let response = await self.fetchResponse(originalURL, mode: .image), | ||
| let thumbnail = chatDecodeLinkPreviewThumbnail( | ||
| response.data, | ||
| mimeType: response.mimeType) |
Contributor
There was a problem hiding this comment.
When an expanded public preview page supplies an og:image URL whose hostname resolves to a private address or DNS-rebinds, this new image path calls fetchResponse and starts URLSession after only the literal host check. The remote-address connectionPolicy is evaluated only in didCompleteWithError after the GET/data transfer, so expanding a public link preview can still send a request into internal services. Please reject or resolve non-public image targets before creating the data task, or avoid fetching attacker-selected cross-host image URLs.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
steipete
force-pushed
the
feat/ios-link-preview-image-nash
branch
from
July 7, 2026 06:58
e49dd0c to
cf57bd7
Compare
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf57bd7251
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mimeType: String, | ||
| maxSourcePixels: Int = chatLinkPreviewImageMaxSourcePixels) -> ChatLinkPreviewThumbnail? | ||
| { | ||
| guard let source = CGImageSourceCreateWithData(data as CFData, nil), |
Contributor
There was a problem hiding this comment.
For compressed images that pass the 1 MB download cap but have large dimensions up to the 64 MP limit, this creates the CGImageSource with default options before reading properties and thumbnailing. Apple's ImageIO downsampling example sets kCGImageSourceShouldCache: false on the source before CGImageSourceCreateThumbnailAtIndex (https://devstreaming-cdn.apple.com/videos/wwdc/2018/219mybpx95zm9x/219/219_image_and_graphics_best_practices.pdf); leaving the source options as nil can let ImageIO cache a full-resolution decoded image before the 600 px thumbnail is produced, defeating the memory guard for large og:image files.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
steipete
added a commit
that referenced
this pull request
Jul 7, 2026
…plicit intents (#101799) * fix(android): keep cold-start gateway auto-connect from overriding explicit intents NodeRuntime's init-time auto-connect now atomically claims the first gateway lifecycle intent (CAS 0->1) and stands down permanently when any explicit connect/disconnect/switch intent already exists, so a late discovery emission can no longer override a user decision. This was also the root cause of the CI-only flakes in GatewayBootstrapAuthTest (refreshGatewayConnection_* at :616/:627 and switchToUndiscoveredGateway* on PRs #101002/#101387/#101396). GatewaySession.Connection.sendRequestFrame now starts its response watcher with CoroutineStart.ATOMIC: disconnect teardown cancels connectionJob right after failPending(), and a DEFAULT-start watcher still queued for dispatch was cancelled without running, silently dropping the terminal UNAVAILABLE onError owed to fire-and-forget callers (root cause of the flaky GatewaySessionInvokeTest.disconnectReportsUnknownOutcomeForFireAndForgetRpc). Tests neutralize runtime background work (cancel+join the runtime scope) before arming the registry and scripting lifecycle steps, and add regression coverage for the auto-connect intent claim. Proof: refresh flake reproduced at iteration 1 under taskset -c 0,1 on a Blacksmith Linux box; post-fix 3 pinned stress rounds (120/150 looped iterations) plus full :app:testPlayDebugUnitTest (109 classes, 0 failures) and :app:ktlintCheck green on the same box. * chore(i18n): resync native inventory line positions after NodeRuntime edits
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Jul 8, 2026
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Jul 8, 2026
…plicit intents (openclaw#101799) * fix(android): keep cold-start gateway auto-connect from overriding explicit intents NodeRuntime's init-time auto-connect now atomically claims the first gateway lifecycle intent (CAS 0->1) and stands down permanently when any explicit connect/disconnect/switch intent already exists, so a late discovery emission can no longer override a user decision. This was also the root cause of the CI-only flakes in GatewayBootstrapAuthTest (refreshGatewayConnection_* at :616/:627 and switchToUndiscoveredGateway* on PRs openclaw#101002/openclaw#101387/openclaw#101396). GatewaySession.Connection.sendRequestFrame now starts its response watcher with CoroutineStart.ATOMIC: disconnect teardown cancels connectionJob right after failPending(), and a DEFAULT-start watcher still queued for dispatch was cancelled without running, silently dropping the terminal UNAVAILABLE onError owed to fire-and-forget callers (root cause of the flaky GatewaySessionInvokeTest.disconnectReportsUnknownOutcomeForFireAndForgetRpc). Tests neutralize runtime background work (cancel+join the runtime scope) before arming the registry and scripting lifecycle steps, and add regression coverage for the auto-connect intent claim. Proof: refresh flake reproduced at iteration 1 under taskset -c 0,1 on a Blacksmith Linux box; post-fix 3 pinned stress rounds (120/150 looped iterations) plus full :app:testPlayDebugUnitTest (109 classes, 0 failures) and :app:ktlintCheck green on the same box. * chore(i18n): resync native inventory line positions after NodeRuntime edits
giodl73-repo
pushed a commit
to giodl73-repo/openclaw
that referenced
this pull request
Jul 8, 2026
giodl73-repo
pushed a commit
to giodl73-repo/openclaw
that referenced
this pull request
Jul 8, 2026
…plicit intents (openclaw#101799) * fix(android): keep cold-start gateway auto-connect from overriding explicit intents NodeRuntime's init-time auto-connect now atomically claims the first gateway lifecycle intent (CAS 0->1) and stands down permanently when any explicit connect/disconnect/switch intent already exists, so a late discovery emission can no longer override a user decision. This was also the root cause of the CI-only flakes in GatewayBootstrapAuthTest (refreshGatewayConnection_* at :616/:627 and switchToUndiscoveredGateway* on PRs openclaw#101002/openclaw#101387/openclaw#101396). GatewaySession.Connection.sendRequestFrame now starts its response watcher with CoroutineStart.ATOMIC: disconnect teardown cancels connectionJob right after failPending(), and a DEFAULT-start watcher still queued for dispatch was cancelled without running, silently dropping the terminal UNAVAILABLE onError owed to fire-and-forget callers (root cause of the flaky GatewaySessionInvokeTest.disconnectReportsUnknownOutcomeForFireAndForgetRpc). Tests neutralize runtime background work (cancel+join the runtime scope) before arming the registry and scripting lifecycle steps, and add regression coverage for the auto-connect intent claim. Proof: refresh flake reproduced at iteration 1 under taskset -c 0,1 on a Blacksmith Linux box; post-fix 3 pinned stress rounds (120/150 looped iterations) plus full :app:testPlayDebugUnitTest (109 classes, 0 failures) and :app:ktlintCheck green on the same box. * chore(i18n): resync native inventory line positions after NodeRuntime edits
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related: #100699
What Problem This Solves
The iOS/macOS tap-to-expand link-preview cards landed text-only in #101198 because the transcript had no policy-controlled way to load a remote thumbnail. This delivers the declared og:image follow-up.
Why This Change Was Made
The expanded card now shows the OpenGraph image through the same hardened fetcher the metadata uses — no new dependency, no ambient traffic. The image request starts only after the user's expand tap with Loaded metadata and a non-nil image URL; the URL must independently pass the non-public-host policy before any request. Image mode shares the host/redirect/connection/deadline rules and no-cookie configuration, with an image/jpeg|png|webp allowlist and a 1 MB cap. Decoding never materializes full-size bitmaps: a source-pixel bomb guard rejects oversized dimensions, then
CGImageSourceCreateThumbnailAtIndexproduces a bounded (≤600px) thumbnail. Decoded thumbnails and negative results sit in a bounded in-memory cache (32); decode failures degrade silently to the text card. The thumbnail slot appears only after a successful decode — text renders immediately, no skeleton, no layout jump. Thumbnails are decorative (accessibilityHidden); title/description carry the content.User Impact
Link-preview cards on iOS/macOS now show the page's image when one exists, with the same zero-ambient-fetch privacy contract as before.
Evidence
swift test --package-path apps/shared/OpenClawKit --filter ChatLinkPreviewTests— 24 tests / 9 suites green at the rebased head: consent timing (request only post-expansion), host policy on the image URL (private literal → zero requests), content-type allowlist, 1 MB cap, redirect rules in image mode, decode bounds, corrupt-data negative caching, cancellation, cache hits.swiftformat --lintclean on touched files; i18n inventory unchanged at head.