Repository navigation
fix(slack): honor the configured image downscale limit on downloadFile - #134242
Merged
steipete merged 3 commits intoSep 1, 2026
Merged
Conversation
agents.defaults.imageMaxDimensionPx is documented as the max image side length "when sanitizing transcript/tool-result image payloads", but the Slack downloadFile action never forwarded it. imageResultFromFile has five call sites; the four in the browser plugin all pass imageSanitization and this one did not, so an operator lowering or raising the limit still got Slack file downloads pinned to the built-in 1200px default. Read the same setting the browser plugin reads and pass it. cfg is already a parameter of handleSlackAction. Precedent: openclaw#84595 fixed the identical omission on the browser screenshot path. Net production LOC: +7.
MarMarLabs
marked this pull request as ready for review
August 31, 2026 15:55
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
Contributor
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: edc4d9818d
ℹ️ 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".
Review is right that this one proved nothing. Absence and
{ maxDimensionPx: undefined } are equivalent downstream, because
sanitizeToolResultImages defaults its opts to {} and then resolves
maxDimensionPx against the same 1200px fallback either way.
So the case only failed against the parent implementation by dereferencing an
absent object, not by detecting a behavior change, and it would have rejected
a valid implementation that omits the key when nothing is configured.
The remaining case still fails when the forward is reverted, exactly one of
112.
Forward the configured image limit to the canonical sanitizer and verify actual downloaded-image output through the message action adapter. Co-authored-by: MarMar Labs <[email protected]>
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed August 31, 2026, 9:01 PM ET / September 1, 2026, 01:01 UTC. ClawSweeper reviewWhat this changesThe PR forwards the existing image-dimension setting from Slack file downloads to the shared image sanitizer and adds action-level output-dimension coverage. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 4 items remain Keep open: the one-line Slack owner-bound repair addresses a current-main omission and has no correctness finding, but the supplied proof is still mock-only for this external PR. Priority: P2 Review scores
Verification
How this fits togetherSlack’s download-file action turns a Slack attachment into an image tool result for the agent. The shared image-result pipeline sanitizes the image before it is replayed to a model. flowchart LR
A[Agent download-file request] --> B[Slack action adapter]
B --> C[Slack file runtime]
C --> D[Shared image-result sanitizer]
D --> E[Agent-visible image result]
Before merge
Agent review detailsSecurityNone. PR surfaceSource +1, Tests +72. Total +73 across 2 files. View PR surface stats
Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the direct per-result forwarding repair and obtain redacted evidence from a configured real Slack download before merge. Do we have a high-confidence way to reproduce the issue? Yes, from current source: Slack omits the documented limit at its image-result call, while the shared sanitizer falls back to 1200 when that option is absent. The reviewed test provides a focused reproduction shape but was not executed in this read-only review. Is this the best way to solve the issue? Yes. Passing the existing setting at the Slack result owner matches every browser sibling and preserves the shared sanitizer as the sole owner of defaulting, normalization, and byte limits. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d26d4a59ee4c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Contributor
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 1, 2026
openclaw#134242) * fix(slack): honor the configured image downscale limit on downloadFile agents.defaults.imageMaxDimensionPx is documented as the max image side length "when sanitizing transcript/tool-result image payloads", but the Slack downloadFile action never forwarded it. imageResultFromFile has five call sites; the four in the browser plugin all pass imageSanitization and this one did not, so an operator lowering or raising the limit still got Slack file downloads pinned to the built-in 1200px default. Read the same setting the browser plugin reads and pass it. cfg is already a parameter of handleSlackAction. Precedent: openclaw#84595 fixed the identical omission on the browser screenshot path. Net production LOC: +7. * test(slack): drop the no-config image sanitization case Review is right that this one proved nothing. Absence and { maxDimensionPx: undefined } are equivalent downstream, because sanitizeToolResultImages defaults its opts to {} and then resolves maxDimensionPx against the same 1200px fallback either way. So the case only failed against the parent implementation by dereferencing an absent object, not by detecting a behavior change, and it would have rejected a valid implementation that omits the key when nothing is configured. The remaining case still fails when the forward is reverted, exactly one of 112. * fix(slack): honor image limits at the result owner Forward the configured image limit to the canonical sanitizer and verify actual downloaded-image output through the message action adapter. Co-authored-by: MarMar Labs <[email protected]> --------- Co-authored-by: Peter Steinberger <[email protected]> Co-authored-by: MarMar Labs <[email protected]>
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
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.
What Problem This Solves
Slack image downloads ignore
agents.defaults.imageMaxDimensionPx, even though the existing setting governs tool-result images. A configured limit of 600 or 2000 still produces a 1200-pixel maximum side because the Slack result owner omits the option when callingimageResultFromFile.What Changed
Forward the existing configured value in
imageSanitization.maxDimensionPx. The canonical image sanitizer continues to own the default, integer normalization, minimum, and byte limit. The repair needs one production line; it adds no configuration or SDK surface.Replace the submitted argument-shape test and broad SDK mock with focused coverage through the real Slack channel action adapter and image processing pipeline. Final branch delta: production +1/-0, tests +72/-0. Existing Slack action tests remain unchanged.
Evidence
Maintainer proof used independently authored equivalent changes on trusted main
c0f1cbfddb909541b7ad88920d8ae0e59fff89b7. The final PR owner file, applied to that main revision, matches the tested source byte-for-byte. The original contributor ancestry remains intact; final candidate commit:8e2fb3f812208e876013357c6e1303c2e2d2ffaf.The regression calls
createSlackActions("slack").handleActionwithdownload-file. A synthetic 2600×1300 PNG passes through the real lazy action dispatcher, file-result owner, sanitizer, and Rastermill processing. Only the downloaded-file location and conversation metadata are fixture seams. Assertions decode the returned image bytes and inspect actual dimensions, the 5 MiB bound, andmedia.outbound: false.imageMaxDimensionPx: 600imageMaxDimensionPx: 2000Both configured cases fail before the repair; the default control passes. All three pass afterward. This proves the real channel action and actual processed-image output. It does not claim an authenticated Slack download or Gateway roundtrip; the changed boundary starts after file download, and the Slack API contract is unchanged.
The relevant shared directory, CLI, conversation discovery, LINE account/group/allowlist, Slack action/download, and image sanitizer suites passed: 231 tests across nine files. The two channel repairs' regression files add eight passing tests in total. Complete changed-scope type/lint/format/boundary checks and full
pnpm buildpassed. Independent Codex review of the complete candidate found no actionable findings.{ "passed": true, "flow": "Slack channel action to real image-result sanitizer", "casesPassed": 3, "actualOutputDimensionsChecked": true, "byteLimitChecked": true, "authenticatedSlackCalls": false }Review and Release Context
The prior inline review's concern about an implementation-coupled no-config test is addressed by checking unchanged default image output directly. The reviewed candidate is pushed; exact-head hosted CI is pending; the original PR body's earlier test totals do not describe this final test suite.
User-visible release context: Slack downloaded images now honor the configured tool-result image dimension limit. Thanks @marmar9615-cloud for identifying and fixing the missing option; contributor credit is preserved in the commit.