Skip to content

fix: make NO_REPLY suppression and sanitization explicit - #785

Open
lilyshen0722 wants to merge 4 commits into
mainfrom
fix/783-no-reply-total-match
Open

fix: make NO_REPLY suppression and sanitization explicit#785
lilyshen0722 wants to merge 4 commits into
mainfrom
fix/783-no-reply-total-match

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Outcome

Agent messages now use two explicit sentinel contracts:

  1. suppression is total-match — only sentinel-only replies stay silent;
  2. sanitization removes bare mid-content producer leakage while preserving deliberate code-formatted mentions.

Closes #783.

Changes

  • keep whole-reply suppression for bare, duplicated, whitespace-separated, and fenced sentinel-only gateway output
  • strip bare mid-content tokens without suppressing substantive replies
  • preserve sentinel mentions inside variable-length backtick spans and fenced code
  • preserve the historical outer-fence unwrap for substantive fenced replies
  • align the system-exchange fallback suppression rule
  • update the operator doc
  • add unit and runtime-route coverage for bare leakage, code-formatted mentions, and fenced-silence precedence

Proof

Node 20:

  • focused unit + runtime-route suites: 34/34 green
  • tsc --noEmit -p tsconfig.typescheck.json: clean
  • mutation removing bare-token stripping: exactly the paired bare-leakage unit test turns red
  • mutation stripping inside code spans: exactly the paired code-formatting unit test turns red
  • mutation allowing fenced sentinel-only output through: exactly the paired precedence test turns red

The first code-span implementation triggered a high-severity CodeQL polynomial-regex alert on attacker-controlled message text. The shipped implementation uses delimiter pairing plus a linear scan and cleared CodeQL at the prior head; fresh checks are running for this comment/test-only follow-up.

Focused ESLint is not a useful signal for these legacy test files: it reports their pre-existing TS-module resolution and iteration-rule violations. No new lint finding was identified in the changed lines.

Scope boundary

  • Apply total-match NO_REPLY semantics in legacy agent bridges #784 tracks two legacy bridge .includes("NO_REPLY") matchers outside this kernel storage path.
  • _external/clawdbot is not initialized in this workspace, so its separate sentinel semantics remain explicitly unaudited.
  • The CLAUDE.md/skill wording correction and complete cross-layer checklist remain in the gated sprint-close docs batch.

@lilyshen0722 lilyshen0722 changed the title fix: preserve NO_REPLY inside substantive agent messages fix: make NO_REPLY suppression and sanitization explicit Jul 29, 2026
Comment thread backend/services/agentMessageService.ts Fixed
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Review — @sprint-review · 47c94325

Verdict: approve with one edge case to either fix or document. The fix is better than the line-anchoring I proposed, and the proof holds up.

Fails-without-the-fix: confirmed, precisely

Ran the PR's suite against main's agentMessageService.ts with the new tests in place:

✕ preserves code-formatted sentinel mentions
Tests: 1 failed, 4 passed

Exactly one test, no collateral. The new contract is genuinely tested.

No assertion inversions — worth noting explicitly

Checklist rule 3 would have required a cited ruling for any flipped assertion on main. There are none: 'Reply with NO_REPLY when done.''Reply with when done.' is preserved unchanged. The contract change is strictly additive — bare-token leakage behaves as before, backtick-quoted mentions are newly preserved. That's the cleanest possible shape for this change and it means the rule-3 question never arises.

The design is better than what I suggested

I proposed line-anchoring (strip only when the sentinel is alone on its line). This distinguishes bare token = producer leakage from backtick-quoted = deliberate mention, which is a sharper cut: it preserves the case that actually matters (evidence quotes, code samples, docs) without preserving genuine model leakage in prose. The variable-length backtick matcher handles inline spans and fenced blocks without touching whitespace-sensitive content around them, which keeps the 2026-07-16 indentation fix intact.

The systemExchangeTriggers fallback being aligned in the same PR is the right call — otherwise it becomes the fourth semantics.


The edge: a fenced sentinel-only message is suppressed, contradicting the code's own comment

The comment above the fence branch says:

A fully fenced reply is explicitly code-formatted … Preserve sentinel mentions inside it exactly.

That isn't true when the fence contains only the sentinel, because the total-match check runs before the fence check, against post-fence-strip content:

const stripped = outerFence ? outerFence[1] : raw;
const trimmed  = stripped.trim();
if (/^(?:NO_REPLY\s*)+$/.test(trimmed)) return '';   // ← fires first
if (outerFence) return trimmed;                       // ← never reached

Verified:

input result
```text\nNO_REPLY\n``` '' — message vanishes
`NO_REPLY` `NO_REPLY` — preserved ✅

So a doc or review comment that illustrates the sentinel as a standalone fenced block posts as nothing. That is precisely the documentation use case this PR exists to enable — the KB entry describing this rule, if it shows the token as a fenced example, disappears.

This is a real tradeoff, not an obvious bug, which is why I'm not blocking on it. Reversing the order means a model that wraps its silent reply in a code fence would post a visible NO_REPLY into a pod — the current order is defensively correct for that case. Two acceptable resolutions:

  1. Keep the order, fix the comment, and add a test pinning fenced-sentinel-only → '' as intended behavior rather than an accident. (My preference — it's the safe direction, and an untested edge is how this whole class of bug starts.)
  2. Exempt the outer-fence case from total-match, accepting the leakage risk.

Either way the current state has a comment asserting something the code doesn't do — which is the phantom-contract shape the sprint's own checklist rule 10 names.

Two consistency items outside this PR's diff

What I did NOT verify

  • I ran the unit suite and reproduced the main-comparison; I did not run the clawdbot-e2e service test in this diff, nor the full backend suite.
  • CI status on 47c94325 not checked at review time.
  • The edge-case results above are from direct calls to sanitizeAgentContent, not from a live pod post.
  • #784's two legacy .includes(...) bridges are correctly out of scope here; I have not looked at them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Agent message sanitizer strips NO_REPLY from substantive content

2 participants