Skip to content

fix: sanitize block reply previews#86634

Closed
anyech wants to merge 1 commit into
openclaw:mainfrom
anyech:fix/status-block-reply-sanitize
Closed

fix: sanitize block reply previews#86634
anyech wants to merge 1 commit into
openclaw:mainfrom
anyech:fix/status-block-reply-sanitize

Conversation

@anyech

@anyech anyech commented May 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Fixes a visible-output leak where streamed/block reply previews could receive raw internal runtime context before the normal user-facing sanitizer ran.

The final reply path already normalizes text via sanitizeUserFacingText, but the generic block reply callback path handed raw block payloads to queued/progress callbacks first. Channels that render live previews from those callbacks could expose internal runtime context.

Changes

  • Sanitize block reply text in dispatch-from-config before it reaches:
    • queued/progress callbacks,
    • TTS block accumulation,
    • routed block delivery,
    • dispatcher block delivery.
  • Drop block payloads that sanitize down to empty text and have no media.
  • Add a regression test proving queued and dispatched block replies strip internal runtime context sentinel text.

Test plan

git diff --check
timeout 600s node scripts/run-vitest.mjs run src/auto-reply/reply/dispatch-from-config.test.ts -t "sanitizes internal runtime context before queued and dispatched block replies"

Targeted regression passed in a dependency-populated stage checkout (1 passed, 106 skipped, rc=0). A fresh disposable clone with symlinked dependencies could not complete the same test because a local package dependency was unavailable in that checkout; CI should exercise the normal workspace dependency graph.

Real behavior proof

  • Behavior or issue addressed: Queued block previews and dispatched block replies should strip the internal runtime-context sentinel before visible delivery, while preserving the user-visible intro/outro text.

  • Real environment tested: Disposable OpenClaw source checkout of this PR branch on Linux with workspace dependencies installed from the locked package graph. The proof exercised the real block-reply dispatch/sanitization seam with the targeted source-level regression test. No production Gateway, live runtime state, live config, or real provider/channel orchestration was used.

  • Exact steps or command run after this patch:

    git diff --check
node <corepack-pnpm.mjs> install --frozen-lockfile
node scripts/run-vitest.mjs run src/auto-reply/reply/dispatch-from-config.test.ts -t "sanitizes internal runtime context before queued and dispatched block replies"

  • Evidence after fix: Copied terminal output from the disposable checkout showed that git diff --check produced no whitespace errors, and the targeted regression test passed:

    Test Files  1 passed (1)
Tests       1 passed | 106 skipped

  • Observed result after fix: The regression test confirms both queued block previews and dispatched block replies remove the internal-context sentinel and keep the surrounding visible text intact.

  • What was not tested: No live provider/channel orchestration, production Gateway, production runtime state, or production config was exercised. This proof is intentionally limited to the sanitizer/dispatch seam changed by this PR.

@openclaw-barnacle openclaw-barnacle Bot added size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 25, 2026
@clawsweeper

clawsweeper Bot commented May 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed May 29, 2026, 1:19 AM ET / 05:19 UTC.

Summary
The PR sanitizes generic block reply payloads before queued preview callbacks, TTS accumulation, routed delivery, and dispatcher block delivery, with one regression test for internal runtime-context stripping.

PR surface: Source +14, Tests +62. Total +76 across 2 files.

Reproducibility: yes. source inspection gives a high-confidence path: emit an onBlockReply payload containing internal runtime-context sentinels and observe current main forwarding it to queued/block delivery before the final sanitizer path runs. I did not run a live channel reproduction in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Change the import to ../../agents/embedded-agent-helpers/sanitize-user-facing-text.js.
  • [P1] Add redacted after-fix real behavior proof for the changed visible block-reply path; terminal/live output is fine if it shows the dispatch result.
  • Rerun the focused node scripts/run-vitest.mjs regression after fixing the import.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body supplies copied terminal output from a targeted source-level regression test with a fake dispatcher, but it explicitly says no live provider/channel, Gateway, runtime state, or config path was exercised. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The new import path is stale on current main, so merging this PR as written can break loading/building the core auto-reply dispatch module.
  • [P1] The intended sanitizer change is in a generic block-delivery path, so the final merge should prove queued previews and dispatched blocks still deliver valid visible content while stripping internal context.
  • [P1] The supplied proof is a targeted fake-dispatcher regression test, not real behavior proof from a live or packaged OpenClaw channel path.

Maintainer options:

  1. Repair import and prove delivery (recommended)
    Fix the sanitizer import to the current helper path, rerun the focused regression, and add redacted after-fix real behavior proof for the visible block-reply path before merge.
  2. Pause for nearby dispatch PRs
    If maintainers prefer to sequence the overlapping dispatch changes, hold this PR until the conflicting block-delivery PRs are merged or rebased cleanly.

Next step before merge

  • [P1] The PR needs a contributor-visible code correction plus real behavior proof; ClawSweeper should not queue an automated repair while proof remains mock-only.

Security
Cleared: No new supply-chain, secret-handling, dependency, workflow, or permission concern was found in the two-file diff; the blocker is functional merge safety.

Review findings

  • [P1] Import the sanitizer from the current helper path — src/auto-reply/reply/dispatch-from-config.ts:20
Review details

Best possible solution:

Use the existing embedded-agent-helpers sanitizer at the block-reply chokepoint, keep the focused regression, and add redacted real behavior proof showing block preview/delivery no longer exposes internal context.

Do we have a high-confidence way to reproduce the issue?

Yes, source inspection gives a high-confidence path: emit an onBlockReply payload containing internal runtime-context sentinels and observe current main forwarding it to queued/block delivery before the final sanitizer path runs. I did not run a live channel reproduction in this read-only review.

Is this the best way to solve the issue?

No, not as submitted: sanitizing at this chokepoint is the right narrow direction, but the import must use the current helper path and the PR needs real behavior proof before merge.

Full review comments:

  • [P1] Import the sanitizer from the current helper path — src/auto-reply/reply/dispatch-from-config.ts:20
    This new import points at ../../agents/pi-embedded-helpers/sanitize-user-facing-text.js, but current main only has src/agents/embedded-agent-helpers/sanitize-user-facing-text.ts. Merging this leaves dispatch-from-config unable to resolve the sanitizer module, so the auto-reply dispatch path will fail before the regression can run.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 8eb5ff08c86b.

Label changes

Label changes:

  • add merge-risk: 🚨 availability: The new import targets a helper directory absent from current main, which can prevent the auto-reply dispatch module from loading.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body supplies copied terminal output from a targeted source-level regression test with a fake dispatcher, but it explicitly says no live provider/channel, Gateway, runtime state, or config path was exercised. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove rating: 🌊 off-meta tidepool: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.

Label justifications:

  • P1: The item concerns a visible internal-context leak in a core reply path, and the current patch also has a merge-blocking module-resolution defect.
  • merge-risk: 🚨 message-delivery: The diff changes generic queued and dispatched block reply payloads, including dropping sanitized-empty text-only blocks, across channel delivery surfaces.
  • merge-risk: 🚨 availability: The new import targets a helper directory absent from current main, which can prevent the auto-reply dispatch module from loading.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body supplies copied terminal output from a targeted source-level regression test with a fake dispatcher, but it explicitly says no live provider/channel, Gateway, runtime state, or config path was exercised. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +14, Tests +62. Total +76 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 22 8 +14
Tests 1 62 0 +62
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 84 8 +76

What I checked:

Likely related people:

  • Peter Steinberger: Current-main blame and log output attribute the dispatch path and sanitizer-helper rename context in this shallow checkout to recent commits by Peter Steinberger. (role: recent area contributor; confidence: medium; commits: 8eb5ff08c86b, cb085ec5f1f5, 27ae826f6525; files: src/auto-reply/reply/dispatch-from-config.ts, src/agents/embedded-agent-helpers/sanitize-user-facing-text.ts, src/auto-reply/reply/normalize-reply.ts)
  • Gustavo Madeira Santana: History for onBlockReplyQueued points to the Matrix block-streaming change as adjacent feature history for queued block previews. (role: block streaming feature contributor; confidence: medium; commits: 8748b7c54cc2; files: src/auto-reply/reply/dispatch-from-config.ts, src/auto-reply/reply/dispatch-from-config.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. labels May 25, 2026
@clawsweeper

clawsweeper Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. label May 25, 2026
@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 26, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels May 26, 2026
@BingqingLyu

This comment was marked as spam.

Sign in to view
@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 29, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels May 29, 2026
@anyech

anyech commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

I consolidated the overlapping dispatcher work into #86672 so reviewers have one branch for this shared dispatch-from-config seam. This PR is now superseded by #86672.

Rationale:

Test Files 1 passed (1)
Tests 7 passed | 154 skipped (161)
git diff --check rc=0

I am closing this PR to reduce queue pressure and remove the cross-PR conflict. Please review/merge #86672 for the combined sanitizer + Discord marked room_event fallback dispatcher fix.

@anyech anyech closed this May 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anyech @BingqingLyu @RomneyDa