fix(nostr): strip internal tool-trace banners from outbound text#101760
fix(nostr): strip internal tool-trace banners from outbound text#101760Pick-cat wants to merge 10 commits into
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 20, 2026, 7:37 AM ET / 11:37 UTC. Summary PR surface: Source +7, Tests +124. Total +131 across 3 files. Reproducibility: yes. at source level: the adapter and direct inbound reply callback are distinct text-delivery paths, and the PR supplies a relay-loopback proof for both. I could not independently execute or inspect the exact checkout in this read-only environment. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Compare this branch and #103361 against current Do we have a high-confidence way to reproduce the issue? Yes, at source level: the adapter and direct inbound reply callback are distinct text-delivery paths, and the PR supplies a relay-loopback proof for both. I could not independently execute or inspect the exact checkout in this read-only environment. Is this the best way to solve the issue? Unclear: adding the shared sanitizer at both Nostr delivery boundaries matches the established sibling-channel pattern, but maintainers must first choose between this overlapping implementation and #103361. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0ac7b24d56ca. Label changesLabel justifications:
Evidence reviewedPR surface: Source +7, Tests +124. Total +131 across 3 files. View PR surface stats
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 (10 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review — Added inbound DM deliver sanitization before table conversion plus channel.inbound.test.ts regression for the gateway reply path. Evidence updated with inbound harness proof at b2a1bce. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review — addressed the [P2] "Drop fully stripped inbound replies" finding. Changes in commit
Local verification:
Live Nostr relay round-trip still depends on available credentials; the regression is at the sanitization layer before network delivery. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review — Evidence section updated with negative control + direct |
ccbb011 to
f9984c9
Compare
|
@steipete This Nostr tool-trace banner fix is green on CI and follows the sanitizer pattern already shipped for sibling channels, but ClawSweeper blocked it on |
1b3e3fe to
949ac6a
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
cdfc8e9 to
555b997
Compare
|
@clawsweeper re-review |
0193ff4 to
7f62413
Compare
Align with the pattern steipete landed in openclaw#102925: type sanitizeText as NonNullable<ChannelOutboundAdapter["sanitizeText"]> instead of the inherited optional Pick shape, and drop the now-redundant optional chaining on sanitizeText in the tests.
sanitizeText inherited the optional modifier from Pick<ChannelOutboundAdapter, "sanitizeText">, so even with the NonNullable intersection TS still inferred it as possibly undefined (TS2722 in the tests). Drop it from the Pick union so it is declared only via NonNullable<...> in the intersection, matching the steipete openclaw#102925 pattern.
nostrPlugin.outbound is typed as the SDK ChannelPlugin["outbound"] (sanitizeText optional), so the test cannot drop ?. the way steipete's synology pattern does (synology has a custom SynologyChatPlugin with a hand-written non-optional outbound). Call sanitizeText directly on nostrOutboundAdapter, whose NostrOutboundAdapter type declares sanitizeText as NonNullable<...>, so the call site is ?.-free and matches the canonical non-optional shape.
Co-authored-by: Cursor <[email protected]>
7f62413 to
b0e53d5
Compare
… import Co-Authored-By: nebulacoder-v8.0 <[email protected]>
Co-Authored-By: nebulacoder-v8.0 <[email protected]>
Part of #90684
What Problem This Solves
Fixes an issue where Nostr users receiving bot replies would see internal assistant tool-trace scaffolding lines (e.g.
⚠️ 🛠️search repos (agent)failed) leaked into outbound messages. These banners are internal runtime diagnostics that should not reach end users.Nostr has two user-visible reply paths: the shared outbound adapter and the gateway inbound DM
delivercallback. Both could forward raw assistant text before this change.Why This Change Was Made
Add
sanitizeAssistantVisibleTexton:nostrOutboundAdapter.sanitizeText(shared outbound delivery path)delivercallback before markdown table conversion (the main bot-reply path for incoming Nostr DMs)Also drop inbound replies that become empty after sanitization so blank encrypted DMs are not published.
Matches the accepted sibling pattern already shipped for Slack, Signal, Matrix, Feishu, Mattermost, Telegram, Google Chat, QQBot, IRC, SMS, Discord, WhatsApp, LINE, and Nextcloud Talk.
Non-goals: no relay protocol redesign; no committed proof harness in this PR.
Collision check: searched open/closed PRs for nostr tool-trace /
sanitizeAssistantVisibleText/ #90684 — this is the Nostr channel candidate under the cross-channel sanitizer issue. Siblings: merged #101708 (LINE), #101712 (Nextcloud Talk), #98705 (Feishu).User Impact
Evidence
Exact head:
0193ff493de8d5f99452aa5eb95ab2d5e74e0960current-
mainnegative (core strip keeps banner)Core
stripInternalRuntimeScaffoldingdoes not remove tool-trace failure banners (same pre-fix leak class as sibling channel PRs).Non-mocked loopback relay delivery (local harness — not shipped)
Local WebSocket Nostr relay + real
startNostrBus.sendDmencrypt/publish, applying productionnostrOutboundAdapter.sanitizeText/ inboundsanitizeAssistantVisibleText, then decrypting captured kind-4 EVENTs. No third-party relay and no personal nsec. Harness kept local (not committed).Focused tests
Verification
0193ff493de8d5f99452aa5eb95ab2d5e74e0960origin/mainReal behavior proof
ws://127.0.0.1:<ephemeral>relay; realnostr-toolsencrypt/publish/decrypt.Risk / Compatibility
Low/Med (message-delivery). Only changes assistant-visible text shaping on Nostr outbound/inbound deliver. Mitigation: sibling-channel pattern + loopback decrypt proof + focused gateway/outbound tests.
Review