Skip to content

fix(nostr): strip internal tool-trace banners from outbound text#101760

Open
Pick-cat wants to merge 10 commits into
openclaw:mainfrom
Pick-cat:fix/nostr-sanitize-outbound-90684
Open

fix(nostr): strip internal tool-trace banners from outbound text#101760
Pick-cat wants to merge 10 commits into
openclaw:mainfrom
Pick-cat:fix/nostr-sanitize-outbound-90684

Conversation

@Pick-cat

@Pick-cat Pick-cat commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

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 deliver callback. Both could forward raw assistant text before this change.

Why This Change Was Made

Add sanitizeAssistantVisibleText on:

  1. nostrOutboundAdapter.sanitizeText (shared outbound delivery path)
  2. the inbound DM deliver callback 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

  • Before: Tool-trace failure banners could appear in Nostr bot replies on outbound delivery and inbound DM replies; trace-only replies could publish empty encrypted DMs.
  • After: Banners are stripped on both paths; fully stripped inbound replies are dropped. Normal assistant prose and markdown table conversion are preserved.

Evidence

Exact head: 0193ff493de8d5f99452aa5eb95ab2d5e74e0960

current-main negative (core strip keeps banner)

Core stripInternalRuntimeScaffolding does 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.sendDm encrypt/publish, applying production nostrOutboundAdapter.sanitizeText / inbound sanitizeAssistantVisibleText, then decrypting captured kind-4 EVENTs. No third-party relay and no personal nsec. Harness kept local (not committed).

node=v22.22.0
head=555b997a4793c43a71c527b2bb27e5f9c4ba5596 (proof captured pre-drop of local harness; behavior unchanged)
PASS raw_wire_has_banner plain="Done.\n⚠️ 🛠️ `search repos (agent)` failed"
PASS outbound_adapter_wire_clean plain="Done."
PASS outbound_trace_only_no_event
PASS inbound_deliver_wire_clean plain="Done."
PASS inbound_trace_only_dropped
{
  "scenario": "nostr loopback relay tool-trace sanitize",
  "relay": "ws://127.0.0.1:34059",
  "assertions": 5,
  "capturedEvents": 3,
  "paths": [
    "outbound.sanitizeText+sendDm",
    "inbound.sanitizeAssistantVisibleText+reply"
  ]
}
ALL PROOF ASSERTIONS: 5 passed, 0 failed

Focused tests

node scripts/run-vitest.mjs extensions/nostr/src/channel.inbound.test.ts extensions/nostr/src/outbound-tool-trace-sanitize.test.ts
 Test Files  2 passed (2)
      Tests  6 passed (6)
[test] passed 1 Vitest shard

Verification

  • Exact head: 0193ff493de8d5f99452aa5eb95ab2d5e74e0960
  • Rebased onto recent origin/main
  • Focused tests: 6 passed
  • Loopback relay proof: 5/5 assertions (local harness, not in diff)
  • No proof script in PR diff (avoids extension boundary / lint gates)

Real behavior proof

  • Behavior addressed: tool-trace banners stripped before Nostr DM encrypt/publish on outbound + inbound deliver; trace-only dropped.
  • Environment: local Linux WSL2; loopback ws://127.0.0.1:<ephemeral> relay; real nostr-tools encrypt/publish/decrypt.
  • Observed output after fix: (proof paste above)
  • What was not tested: credentialed round-trip against a public third-party relay / personal nsec.

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

  • Manually reviewed and verified
  • AI-assisted: Yes

@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 20, 2026, 7:37 AM ET / 11:37 UTC.

Summary
The branch sanitizes internal assistant tool-trace text in Nostr’s shared outbound adapter and direct inbound-DM reply callback, suppressing replies that become empty and adding focused regressions.

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: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

Risk before merge

  • [P1] There are two open PRs for the same Nostr behavior; landing both risks duplicate tests and conflicting ownership of the same delivery invariant.
  • [P1] The PR is behind main; before landing, compare the actual three-way merge result and exact-head checks rather than assuming the older branch still matches current Nostr ingress behavior.

Maintainer options:

  1. Choose one canonical Nostr implementation (recommended)
    Compare both open branches against current main and land only the candidate that preserves the current ingress lifecycle while sanitizing both delivery paths.
  2. Pause both candidates
    Pause the duplicate pair if neither exact-head merge result clearly preserves the current Nostr ingress contract.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Rebase the selected canonical Nostr sanitizer branch onto current main, verify both outbound and inbound direct-DM delivery paths, and close the duplicate candidate with the canonical PR link.

Next step before merge

  • [P2] A maintainer must select one of two overlapping proof-positive Nostr branches before any repair or landing action; there is no discrete mechanical defect to automate on this PR.

Maintainer decision needed

  • Question: Which open Nostr sanitizer PR should be the single canonical landing candidate: this branch or fix(nostr): strip internal tool-trace banners from outbound text #103361?
  • Rationale: Both branches claim the same user-visible fix and have positive proof, but choosing one requires current-main merge and ownership judgment that cannot be inferred safely from the supplied snapshots.
  • Likely owner: steipete — The discussion explicitly requested guidance from this person, and the branch history identifies their sibling-channel adapter pattern as the intended precedent.
  • Options:
    • Adopt this PR: Rebase and land this branch if its loopback relay proof and current inbound callback remain the best match for current main.
    • Adopt the competing PR (recommended): Use fix(nostr): strip internal tool-trace banners from outbound text #103361 if its current-ingress preservation and exact-head CI make it the cleaner canonical implementation.
    • Request a consolidated replacement: Ask for one current-main branch only if neither open PR applies cleanly after the merge-result comparison.

Security
Cleared: The patch only imports an existing plugin-SDK sanitizer and changes Nostr text shaping; the supplied diff adds no dependency, workflow, credential, permission, or artifact-execution surface.

Review details

Best possible solution:

Compare this branch and #103361 against current main, select the narrower current-ingress-compatible candidate, and land only that one with its exact-head checks green.

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 changes

Label justifications:

  • P1: Internal runtime diagnostics can currently reach Nostr recipients through two bot-reply delivery paths.
  • merge-risk: 🚨 message-delivery: The patch changes outbound text shaping and suppresses trace-only replies before encrypted Nostr DM delivery.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body provides after-fix loopback relay evidence using production Nostr encrypt/publish/decrypt behavior for both changed paths, with visible sanitized results and trace-only suppression.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix loopback relay evidence using production Nostr encrypt/publish/decrypt behavior for both changed paths, with visible sanitized results and trace-only suppression.
Evidence reviewed

PR surface:

Source +7, Tests +124. Total +131 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 8 1 +7
Tests 2 124 0 +124
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 132 1 +131

What I checked:

  • Two Nostr delivery paths: The PR adds the shared sanitizer to the outbound adapter and to the direct inbound-DM reply callback before Markdown-table conversion, then drops fully stripped replies; this covers the stated adapter bypass rather than only the generic outbound path. (extensions/nostr/src/gateway.ts:198, f0eb0052b96a)
  • Focused regressions: The submitted diff adds inbound mixed-text and trace-only suppression coverage plus outbound adapter sanitization coverage. (extensions/nostr/src/channel.inbound.test.ts:231, f0eb0052b96a)
  • Real delivery evidence: The PR body reports a non-mocked loopback Nostr relay proof using real encrypt/publish/decrypt behavior, with five assertions covering sanitized outbound and inbound paths and trace-only suppression. (555b997a4793)
  • Competing viable candidate: fix(nostr): strip internal tool-trace banners from outbound text #103361 addresses the same Nostr outbound and inbound-DM sanitizer boundary, is open, carries proof-sufficient and P1 labels, and reports exact-head upstream CI success. Its current-main mergeability and exact diff still need a maintainer comparison before either PR is closed. (4947413d6f0e)
  • Prior review continuity: The prior completed ClawSweeper cycle found no remaining patch defect and requested a maintainer choice of the canonical Nostr branch; the provided new head does not establish that choice. (7f6241356912)

Likely related people:

  • Pick-cat: Authored this Nostr sanitizer branch and the already merged related LINE sanitizer work, indicating direct experience with the shared channel-delivery pattern. (role: recent area contributor; confidence: medium; commits: ffd224cb161f, f0eb0052b96a; files: extensions/nostr/src/gateway.ts, extensions/nostr/src/channel.inbound.test.ts)
  • miorbnli: Authored the open competing Nostr sanitizer candidate that targets the same adapter and ingress boundary. (role: adjacent Nostr fix contributor; confidence: medium; commits: 4947413d6f0e; files: extensions/nostr/src/gateway.ts)
  • steipete: The branch history cites the required outbound-adapter typing pattern as matching a prior merged sibling-channel change associated with this area. (role: adjacent pattern contributor; confidence: low; commits: b333585661c7; files: extensions/nostr/src/gateway.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.
Review history (10 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-08T00:30:14.938Z sha 3e97101 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-08T00:38:58.033Z sha 3e97101 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-08T00:46:15.976Z sha 3e97101 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-09T07:20:54.810Z sha f9984c9 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-10T11:21:05.365Z sha 30e3bbd :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-10T13:06:48.573Z sha 1b3e3fe :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-16T03:30:09.788Z sha 0193ff4 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-17T06:55:07.151Z sha 7f62413 :: needs maintainer review before merge. :: none

@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. P1 High-priority user-facing bug, regression, or broken workflow. labels Jul 7, 2026
@Pick-cat

Pick-cat commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@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.

@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 7, 2026
@Pick-cat

Pick-cat commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — addressed the [P2] "Drop fully stripped inbound replies" finding.

Changes in commit 3e97101506:

  • extensions/nostr/src/gateway.ts: after sanitizeAssistantVisibleText(outboundText), return early if the sanitized text is blank, so a trace-only assistant reply does not result in an empty Nostr DM.
  • extensions/nostr/src/channel.inbound.test.ts: added regression test drops inbound DM replies that contain only internal tool-trace banners verifying sendReply is not called.

Local verification:

  • node scripts/run-vitest.mjs extensions/nostr/src/channel.inbound.test.ts --run → 4 passed
  • node scripts/run-vitest.mjs extensions/nostr/src/outbound-tool-trace-sanitize.test.ts --run → 2 passed
  • node scripts/run-oxlint.mjs extensions/nostr/src/gateway.ts extensions/nostr/src/channel.inbound.test.ts extensions/nostr/src/outbound-tool-trace-sanitize.test.ts → no findings

Live Nostr relay round-trip still depends on available credentials; the regression is at the sanitization layer before network delivery.

@clawsweeper

clawsweeper Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. label Jul 8, 2026
@Pick-cat

Pick-cat commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — Evidence section updated with negative control + direct node --import tsx plugin-hook proof for the outbound sanitizer, including the trace-only empty-output case. The inbound trace-only regression now asserts that fully-stripped replies are dropped before reply is called. All verification commands and outputs are in the updated PR body.

@Pick-cat
Pick-cat force-pushed the fix/nostr-sanitize-outbound-90684 branch 2 times, most recently from ccbb011 to f9984c9 Compare July 9, 2026 07:10
@Pick-cat

Copy link
Copy Markdown
Contributor Author

@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 needs proof — it wants real relay or non-mocked delivery logs, and my PR-body evidence is only function-level. I don't have a relay environment handy. Would you mind clearing the proof gate if the evidence looks sufficient, or letting me know your preferred proof path? Happy to add a non-mocked delivery test if that's a better fit.

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. label Jul 10, 2026
@Pick-cat
Pick-cat force-pushed the fix/nostr-sanitize-outbound-90684 branch from 1b3e3fe to 949ac6a Compare July 15, 2026 13:58
@Pick-cat

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@openclaw-barnacle openclaw-barnacle Bot added the scripts Repository scripts label Jul 15, 2026
@Pick-cat

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@Pick-cat
Pick-cat force-pushed the fix/nostr-sanitize-outbound-90684 branch from cdfc8e9 to 555b997 Compare July 15, 2026 14:11
@openclaw-barnacle openclaw-barnacle Bot added size: S and removed scripts Repository scripts size: M labels Jul 15, 2026
@Pick-cat

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed 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. labels Jul 16, 2026
@Pick-cat
Pick-cat force-pushed the fix/nostr-sanitize-outbound-90684 branch from 0193ff4 to 7f62413 Compare July 17, 2026 06:48
Pick-cat and others added 8 commits July 20, 2026 13:50
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.
@Pick-cat
Pick-cat force-pushed the fix/nostr-sanitize-outbound-90684 branch from 7f62413 to b0e53d5 Compare July 20, 2026 05:51
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: nostr Channel integration: nostr merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants