Repository navigation
fix(slack): preserve long legacy interactive text - #127994
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f797c56f2
ℹ️ 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".
|
Codex review: needs real behavior proof before merge. Reviewed August 24, 2026, 7:44 PM ET / 23:44 UTC. ClawSweeper reviewWhat this changesThis PR changes legacy Slack interactive text from one truncated section to Slack-aware bounded sections, with regression tests for long and formatted text. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 6 items remain Keep open: current main still truncates this legacy Slack path, but the branch conflicts with current main and replaces its shared renderer with a duplicate legacy implementation. It needs a rebase, a narrower owner-boundary fix, and real delivery proof. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherLegacy Slack interactive replies become Block Kit blocks, then the reply resolver groups them into messages of at most 50 blocks. This change affects the text-rendering step before the existing outbound Slack delivery path. flowchart LR
A[Legacy interactive text] --> B[Legacy-to-presentation bridge]
B --> C[Slack block renderer]
C --> D[Mrkdwn section chunks]
D --> E[Reply block resolver]
E --> F[Slack outbound messages]
Before merge
Findings
Agent review detailsSecurityNone. PR surfaceSource +5, Tests +215. Total +220 across 6 files. View PR surface stats
Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Rebase onto current main, retain the shared legacy-to-presentation path, add lossless legacy-text chunking at its owner boundary, and attach redacted mock-gateway or live Slack delivery evidence. Do we have a high-confidence way to reproduce the issue? Yes, by source: current main routes legacy interactive text to Is this the best way to solve the issue? No: the branch fixes the symptom but duplicates the renderer that current main centralized behind the legacy-to-presentation bridge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b71a09fd8a3e. LabelsLabel justifications:
EvidenceAcceptance criteria:
What 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
HistoryReview history (4 earlier review cycles)
|
|
Fixed in Deterministic RED covered both forms: inline code produced 4 chunks instead of 3, and fenced code produced 6 instead of 5. The owner-level fix now clears continuation state after a flush, reopens code lazily only for later content, and consumes a closer when there is no unflushed content. Both regressions reconstruct the protected token exactly from balanced, non-marker-only chunks. I also added a local mocked outbound boundary proof for both inline and fenced code: each input produces exactly 50 real Slack sections, calls the injected Validation: focused Slack 5 files / 195 tests; full Slack 137 files / 2,446 tests; |
|
Hi maintainers — could you please rerun the failed CI jobs for exact head The only primary failure is Thank you. |
|
Addressed the current ClawSweeper P1 in The finding was valid. A focused RED on the prior exact head showed that wrapper-ineligible positive limits emitted orphan code markers: inline limits 1/2 and fenced limits 1/5/6 now have regression coverage. The owner-level chunker fix consumes only the formatting delimiters when a balanced wrapper cannot fit and preserves the payload as bounded literal text. Validation on the new exact head:
The exact-head |
Co-authored-by: Peter Steinberger <steipete@gmail.com>
Co-authored-by: Peter Steinberger <steipete@gmail.com>
deb6247 to
bffcfcc
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Co-authored-by: Peter Steinberger <steipete@gmail.com>
* fix(slack): preserve long interactive and presentation text * fix(slack): retain every outbound delivery receipt Co-authored-by: Peter Steinberger <steipete@gmail.com> * test(slack): narrow presentation regression fixtures Co-authored-by: Peter Steinberger <steipete@gmail.com> * fix(slack): preserve single-message presentation edits Co-authored-by: Peter Steinberger <steipete@gmail.com> --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>

Closes #126018
What Problem This Solves
Slack silently discarded everything after 3,000 characters in legacy interactive replies. The current shared presentation renderer also truncated modern text and context blocks, and its capability check incorrectly rejected content that could have been rendered losslessly. The existing Slack mrkdwn chunker additionally removed whitespace at chunk boundaries and exploded short inline-code spans into separate chunks.
Why This Change Was Made
The fix keeps the current canonical
legacyInteractiveReplyToPresentation→buildSlackPresentationBlocksflow. That shared owner now splits both section and context text with the existing Slack-aware mrkdwn chunker, while the capability check recognizes both as natively renderable. Slack's existing reply resolver still owns its 50-block message boundary, and its existing send/accessibility planner still owns complete, bounded screen-reader fallbacks and actual provider posts.The existing mrkdwn chunker now preserves ordinary boundary whitespace and keeps inline/fenced code together until an actual size boundary; protected Slack entities, links, Unicode surrogate pairs, wrapper-ineligible configured limits, and controls retain their existing contracts. Actual provider-boundary testing also exposed a second owner bug: Slack's outbound payload adapter returned only the final segment's receipt, silently forgetting earlier attachments and provider posts. Its existing media callback and the existing Plugin SDK receipt aggregator now retain every nested physical delivery in order, globally reindex receipt parts, and preserve an earlier question card's exact finalization identity.
Sending a new message can split oversized presentation blocks; editing an existing message cannot. The edit action owner therefore preserves its existing single-message contract: oversized presentation text/context uses the existing complete fallback, and the existing Slack-rendered UTF-8 byte guard rejects updates over 4,000 bytes without silently truncating them. Safe complete edits below that limit still succeed, while send-side native block segmentation remains lossless. No duplicate renderer, new configuration, public API, dependency, or fallback path is introduced.
Production LOC: +68/-40 (net +28). Tests: +323/-30. The positive production delta is necessary to retain every delivered attachment/platform message and its question-card lifecycle metadata at the payload owner, while preserving the distinct unsplittable edit action contract; no new helper, compatibility layer, or alternate transport path is added.
User Impact
Legacy Slack interactive replies and modern Slack presentation text/context no longer lose authored content after 3,000 characters. Long replies keep their order, styling, complete screen-reader text, and exactly one copy of every trailing control. Existing Slack API section, Block Kit message, and accessible fallback limits remain enforced.
Evidence
node scripts/run-vitest.mjs extensions/slack/src/shared-interactive.test.ts extensions/slack/src/reply-blocks.test.ts extensions/slack/src/format.test.ts extensions/slack/src/channel.test.ts --run— 4 files, 150 tests passed.OPENCLAW_VITEST_MAX_WORKERS=1 node scripts/run-vitest.mjs extensions/slack/src/channel.test.ts extensions/slack/src/question-finalization.test.ts extensions/slack/src/send.blocks.test.ts --run— 3 files, 126 tests passed, including a new failing-before-fix regression covering media receipt + earlier question receipt + final text receipt, globally indexed parts, preserved final message identity, and original question-card finalization.OPENCLAW_VITEST_MAX_WORKERS=1 node scripts/run-vitest.mjs extensions/slack/src/channel.test.ts extensions/slack/src/format.test.ts extensions/slack/src/shared-interactive.test.ts extensions/slack/src/reply-blocks.test.ts extensions/slack/src/message-action-dispatch.test.ts extensions/slack/src/outbound-adapter.test.ts extensions/slack/src/outbound-payload.test.ts extensions/slack/src/question-finalization.test.ts extensions/slack/src/send.blocks.test.ts extensions/slack/src/actions.blocks.test.ts --run— 10 files, 325 tests passed, including 3,001-character text/context edits that remain complete, markup-expanded and multibyte updates that correctly reject above the 4,000-byte edit boundary, all attachment receipt IDs, and bounded native text/context sends.node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.extensions.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/extensions-test.tsbuildinfo. Slack SDK's genericBlock.type: stringis narrowed structurally before reading section/context properties; no casts oranyare introduced.node scripts/run-vitest.mjs extensions/slack/src/channel.test.ts -t 'delivers oversized .* in order across the real Slack outbound adapter' --run— 3 cases passed, covering legacy interactive text, modern presentation text, and modern presentation context.{"surface":"interactive","type":"text","inputCharacters":150001,"httpPostCount":51,"receiptPlatformMessageIds":51,"receiptOrderPreserved":true,"receiptIndicesSequential":true,"exactContentPreserved":true,"maximumAccessibilityFallbackCharacters":3000,"privateCallbackExposed":false,"actionCount":1} {"surface":"presentation","type":"text","inputCharacters":150001,"httpPostCount":51,"receiptPlatformMessageIds":51,"receiptOrderPreserved":true,"receiptIndicesSequential":true,"exactContentPreserved":true,"maximumAccessibilityFallbackCharacters":3000,"privateCallbackExposed":false,"actionCount":1} {"surface":"presentation","type":"context","inputCharacters":150001,"httpPostCount":51,"receiptPlatformMessageIds":51,"receiptOrderPreserved":true,"receiptIndicesSequential":true,"exactContentPreserved":true,"maximumAccessibilityFallbackCharacters":3000,"privateCallbackExposed":false,"actionCount":1} {"surface":"actual-provider-50-block-boundary","inputCharacters":3001,"httpPostCount":2,"receiptPlatformMessageIds":2,"blocksPerMessage":[50,2],"exactContentPreserved":true,"maximumAccessibilityFallbackCharacters":3000,"privateCallbackExposed":false,"actionCount":1}The 51 HTTP posts for 150,001 characters are intentional: Slack screen readers consume complete top-level text, Slack recommends 4,000 characters, and its documented truncation boundary is 40,000. Existing native-data/accessibility planning therefore subdivides logical 50-block batches when the 40,000-character hard boundary is crossed, without losing accessible content or private callback isolation. Existing covered behavior below the hard boundary, including complete fallbacks above 8,000 characters, remains unchanged. The independent 49-divider + 3,001-character case confirms the actual Slack HTTP provider still receives exactly two messages with 50 and 2 blocks at the Block Kit boundary. Official Slack limits: 3,000 characters per section and 50 blocks per message.
Original contribution by @original4422 (Daniel Peng), preserved as the primary Git author.