fix(agents): suppress replies led by NO_REPLY - #1257
Conversation
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Gated at head 0baf32e4 (base fix/log-the-sentinel-strip @ ccfef7f8). All three stated claims reproduce; one blocking gap in the new guard.
Claims verified
-
npx jest --runInBand agentMessageService→ 96 passed / 96 total, 9 suites. ✅ -
npm run tsc:check(tsconfig.typescheck.json) → clean. ✅ (Note: a baretsc -p tsconfig.jsonis red, but only in pre-existingtest-discord-*.tsscripts the typecheck project excludes — unrelated to this PR.) -
Mutation → exactly 3 assertions fail, all
Expected: "" / Received: "Here is the real answer."✅One caveat on how I ran it: my first mutation attempt (
if (false && startsWithLeadingBareSentinel)) brokeobservenarrowing and the suite failed to compile —Tests: 0 total, which reads exactly like a clean run if you only grep for✕. The applied mutation istrimmed.startsWith(sentinel)→trimmed.startsWith("ZZZ_NEVER_MATCHES")at line 1842, anchor-count asserted at 1 before running. -
The OpenClaw limitation in the CLAUDE.md rewrite is substantiated at this PR's own submodule pin (
_external/clawdbot@5d88a3f1), not just asserted: CHANGELOG carries "Auto-reply/NO_REPLY: stripNO_REPLYtoken from mixed-content messages instead of leaking raw control text" and "suppress only exactNO_REPLYfinal replies while still filtering streaming partial sentinel fragments." Documenting the limit rather than claiming runtime-uniformity is the right call. -
Empty
sanitizedContentfalls through to the existingreason: 'silent_or_empty'skip and firesmaybeRecordAgentDmConclusion— identical to total-match suppression. Consistent, no new path. -
MERGEABLE;merge-treeagainst the other two open PRs touchingagentMessageService.ts(#1233, #1218) is conflict-free. CI still pending onTest & CoverageandE2E Tests.
Blocking: concatenated leading sentinels bypass the new suppression
The new guard tests one sentinel length —
trimmed.startsWith(sentinel) && !isWordCharacter(trimmed[sentinel.length])— while the strip loop 20 lines below correctly consumes a run (while (trimmed.startsWith(sentinel, sentinelEnd)) sentinelEnd += sentinel.length;). For "NO_REPLYNO_REPLY...", trimmed[8] is 'N', a word character, so the guard declines and the reply posts.
Measured at 0baf32e4:
| input | output | silent |
|---|---|---|
NO_REPLY\nHere is the real answer. |
"" |
✅ |
NO_REPLY NO_REPLY\nHere is the real answer. |
"" |
✅ |
NO_REPLYNO_REPLY\nHere is the real answer. |
"Here is the real answer." |
❌ |
NO_REPLYNO_REPLYNO_REPLY\nHere is the real answer. |
"Here is the real answer." |
❌ |
Behaviour differs by a single space, which is producer-arbitrary.
This matters more than a generic edge case, because the concatenated form is the documented shape here. The test file you edited opens with it as a live-pod leak found 2026-07-03 — "concatenated NO_REPLY sentinels (NO_REPLYNO_REPLY) sailing through the word-boundary strip and posting verbatim" — and the retained comment three lines above your new code says gateways "have historically joined silent blocks into NO_REPLYNO_REPLY". The total-match branch already carries compat for it (/^(?:NO_REPLY\s*)+$/) and the strip loop already carries it. The new suppression is the only one of the three that doesn't.
To be precise about severity: this is not a regression — I confirmed with the feature disabled that base behaviour for the doubled input is the same "Here is the real answer.". It is an incompleteness in the new feature, and the one producer shape it most needed to cover.
Fix is small — advance over the run before the word-character check:
let sentinelEnd = 0;
while (trimmed.startsWith(sentinel, sentinelEnd)) sentinelEnd += sentinel.length;
const startsWithLeadingBareSentinel = (
sentinelEnd > 0 && !isWordCharacter(trimmed[sentinelEnd])
);Please add the concatenated case to suppresses substantive replies that begin with a bare sentinel — without it the suite can't tell the two apart, and the mutation evidence looks equally strong either way.
Non-blocking notes
-
A fully fenced reply still bypasses leading suppression.
```\nNO_REPLY\nHere is the real answer.\n```returns the body with the sentinel intact. That's the pre-existingif (outerFence) return trimmed;early return, unchanged by this PR and deliberate (a transport-fenced reply is treated as code-formatted). But the CLAUDE.md rewrite now enumerates three contracts and doesn't mention this carve-out, and a gateway that wraps output in a transport fence would emit the sentinel verbatim. Worth one clause in the rule. -
CLAUDE.md wording. "Current runtime limit: the backend enforces … for direct API/MCP posts" is accurate and I'd keep it. Consider naming the submodule pin the claim was checked against — per the repo's own rule, a claim about the gateway decays on the next bump, and
5d88a3f1is what makes it re-checkable.
Separating the leading-suppression warning from #1252's strip-edit counter is the right call and the does not count a leading-bare suppression as a strip edit test pins it well — that's the part of this PR I'd most want kept as-is.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Re-gated at head a64e9437 (base fix/log-the-sentinel-strip @ ccfef7f8, unmoved). Both stated claims reproduce. The blocker from my previous review is still open — this commit documents the DM-memory consequence but does not touch the guard.
Claims verified
npx jest --runInBand agentMessageService→ 97 passed / 97 total, 9 suites. ✅npm run tsc:check→ clean. ✅- Mutation (
trimmed.startsWith(sentinel)→startsWith("ZZZ_NEVER_MATCHES")at line 1843, anchor-count asserted at 1) → 4 assertions fail, up from 3. The new test does discriminate.
Still blocking: concatenated leading sentinels
Re-measured at a64e9437, not carried over from the last review:
| input | output | silent |
|---|---|---|
NO_REPLY\nHere is the real answer. |
"" |
✅ |
NO_REPLY NO_REPLY\nHere is the real answer. |
"" |
✅ |
NO_REPLYNO_REPLY\nHere is the real answer. |
"Here is the real answer." |
❌ |
NO_REPLYNO_REPLYNO_REPLY\nHere is the real answer. |
"Here is the real answer." |
❌ |
Unchanged. The guard still tests one sentinel length while the strip loop below consumes a run, and NO_REPLYNO_REPLY is the exact shape this test file's header names as the 2026-07-03 live-pod leak. Fix and suggested test are in my previous review.
The new test discriminates by DB timeout, not by its assertion
starts the conclusion-memory flow for a leading bare sentinel fails under mutation like this:
✕ starts the conclusion-memory flow for a leading bare sentinel (10013 ms)
● MongooseError: Operation `users.findOne()` buffering timed out after 10000ms
Not expect(result).toEqual(...), and not expect(conclusion).toHaveBeenCalledWith(...). With suppression disabled, postMessage walks into the real posting path, hits an unmocked Mongo, and hangs for the full default timeout. So what the test actually pins is "leading suppression short-circuits before we reach Mongo" — related to, but not the same as, the assertion a reader sees.
Two consequences worth fixing: it costs 10s on every red run in an otherwise sub-second file, and its discriminating power is incidental — if anyone later adds a Mongo mock or a global connection to this file, the negative case changes path and the test can stop discriminating without anyone noticing. Mocking the store (or asserting on a rejection you control) would make the failure land on the expect.
Documentation outruns the tests here
The prose added in this commit says, in three places, that the takeaway uses "the sender's preceding substantive message rather than the suppressed body" (CLAUDE.md), that leading-bare replies "are skipped" by the scan (systemExchangeTriggers.ts), and that derivation is "the immediately-preceding message from the same sender that sanitizeAgentContent considers substantive" (ADR-012 table).
None of that is tested. The new test mocks maybeRecordAgentDmConclusion outright, so neither the agent-dm gating nor the takeaway derivation is exercised — podId: '507f1f77bcf86cd799439011' is never established as a DM. And grepping the four files that touch this area (agentMemoryService.systemExchanges.test.ts, agent-memory-envelope.test.js, agent-memory-identity-casing.test.js, chatNoise.test.js) for NO_REPLY returns nothing — findPreviousNonSilentMessage's skipping behaviour has no coverage at all, before or after this change.
The commit is titled "pin leading silence DM memory flow"; what it pins is the postMessage → helper call. That's a fine thing to pin, but the docs now assert the half that isn't pinned. Either narrow the prose or add a findPreviousNonSilentMessage test where a stored leading-bare row is skipped in favour of the substantive turn behind it.
One note on that read path, reasoned rather than measured (I have no DB access here, and I'm flagging it as such): because this scan re-sanitizes stored history, leading-bare rows already in messages are now reclassified as silent. I expect that population to be near-empty — backend-path rows had the leading token stripped before storage, and the gateway strips mixed-content sentinels before the backend ever sees them — but it is a retroactive reclassification of stored data, and it is not a claim I verified.
The Node 22 attribution is inverted
"The unchanged memory-writer unit suite has a reproduced pre-test Node 22 dependency failure."
The failure is real and is correctly identified as unrelated to this PR, but it is a Node 26 failure, not a Node 22 one. At this head:
- Node 22 (
/opt/homebrew/opt/node@22/bin):jest memory→ 12 suites, 214 tests, all pass. - Node 26 (machine default): 8 of 12 suites fail with
Test suite failed to run — TypeError: Cannot read properties of undefined (reading 'prototype'), thrown fromnode_modules/buffer-equal-constant-time/index.js:37underrequire('jsonwebtoken').
That is the known jsonwebtoken → jws → jwa → buffer-equal-constant-time ceiling on Node 26. It takes out __tests__/unit/models/AgentMemory.test.ts too — a model test that imports nothing from this diff — which is what establishes it as engine-level rather than PR-level. Node 22 is the workaround, so please re-run under Node 22 and drop the caveat; as written it would send the next reader looking for a dependency problem that doesn't exist on the version they should be using.
Merge state
MERGEABLE; merge-tree clean against #1233 and #1218. CI pending on Test & Coverage and E2E Tests. Base #1252 unmoved at ccfef7f8.
The comment rewrites in systemExchangeTriggers.ts are a genuine improvement — routing "is this silent?" through sanitizeAgentContent as the named predicate instead of restating NO_REPLY semantics in each comment is exactly right, and it's what will keep those comments true the next time the sentinel rules move.
|
Two things on the 1. "The suppressed body exists nowhere else" is true for a narrower population than it reads. Measured at
So for the native tier the log line is genuinely cosmetic. The population where those 120 characters are the whole artifact is MCP and CLI seats: the ruling applies to them, and they have no 2. The ruling does not reach openclaw moltbots at all, and this is worth stating somewhere before it ships. The gateway strips the sentinel before the backend ever sees the message. Re-measured against the current pin (gitlink The regex is line-anchored, so a leading bare sentinel is deleted as a line and the prose after it is posted. By the time The logging consequence is benign — nothing is suppressed there, so nothing is lost to a truncated log. The ruling consequence is not: AX entry 43's leak shape survives unchanged on every openclaw moltbot after this merges, and it survives silently, because the one component that could observe it has already had the evidence removed upstream. That may be entirely acceptable — the ruling was written for the seats it does reach — but it should be a stated scope rather than an assumption that TASK-067 closed the class. Not verified: the ADR-012 and |
|
Gate at Ran rather than read. Both suites on Node 22 from a clean worktree at this head: 33/33 passed ( The pin is behavioural, not textual — three compiling mutations, each with a non-zero
So the suite discriminates the ratified behaviour at both the sanitizer and the ensemble consumer, and the warn line is asserted on content ( The runtime-uniformity scope is documented and accurate. The CLAUDE.md rule now states the limit explicitly: the backend enforces leading-sentinel suppression for direct API/MCP posts, and the OpenClaw gateway strips the sentinel before the backend receives it, so that tier still strip-and-posts. That is exactly what I measured against the current pin (gitlink Unapplied: the One small thing, non-blocking. Not verified: the ADR-012 wording change, |
|
Re-gate at
And the constant is behaviourally pinned, not just named. Suites at this head: 34/34 (up one from One status correction: Nothing else outstanding from me on this PR. |
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Your structural argument is correct — I checked both legs and I withdraw that part of my finding. Note the head moved to 7bfccd66 (four commits past the a64e9437 we were discussing), so this re-gates there.
Your clarification: verified, conceded
Both legs hold at 7bfccd66:
postMessagepasses the hook{podId, senderAgentName, senderInstanceId}and nothing else (line 1140) — the content is not in the argument.- The suppressed body is never persisted;
postMessagereturns{skipped: true, reason: 'silent_or_empty'}from inside the same block, before any write.
So "rather than the suppressed body" is structurally guaranteed and does not need a test. My previous review was wrong to group it with the untested claims — I read the clause as an assertion about takeaway selection when it is an assertion about what is reachable, and those need different evidence. Dropped.
The blocker is resolved
hasLeadingBareNoReply advances over the run before the word-character check. Measured at 7bfccd66:
| input | sanitizeAgentContent |
|---|---|
NO_REPLYNO_REPLY\nHere is the real answer. |
"" ✅ |
NO_REPLYNO_REPLYNO_REPLY\nHere is the real answer. |
"" ✅ |
Suites green under Node 22: agentMessageService + agentEnsembleService + systemExchanges → 11 suites, 130 tests. npm run tsc:check clean. The concatenated regression test landed, and isSilentNoReply / sanitizeAgentContent agree on every NO_REPLY shape I threw at them — they diverge only on empty/whitespace input, where the sanitizer returns '' and isSilentNoReply returns false. That divergence is correct (empty is not a sentinel silence) and preserves the old === 'NO_REPLY' behaviour at the ensemble call site.
This commit did more than share semantics — it repaired a dead require chain
Worth stating plainly because the commit message doesn't, and someone will otherwise find it later as a mystery.
agentMessageService.ts ends in module.exports = AgentMessageService — the class itself. Measured against the real module:
AMS.sanitizeAgentContent: function
AMS.AgentMessageService: undefined
AMS.default: undefined
The pre-7bfccd66 lookup was AMS.AgentMessageService?.sanitizeAgentContent ?? AMS.default?.sanitizeAgentContent. Both operands are undefined, so sanitize was undefined and findPreviousNonSilentMessage fell through to the /^(?:NO_REPLY\s*)+$/ backstop on every call — despite the comment above it calling the sanitizer "single source of truth … keeps NO_REPLY semantics in sync with the swallow logic in postMessage." It wasn't in sync; it was never invoked. Adding AMS.sanitizeAgentContent as the first operand makes that comment true for the first time.
Two consequences:
- This is a real fix, and a good one — but the read path's behaviour change is much wider than "share leading silence semantics." It went from one total-match regex to the full sanitizer (fence handling, strip-and-post, leading-bare). That is the commit's largest behavioural delta and it is currently undescribed.
- It retroactively corrects my last review. I flagged "stored leading-bare rows are now reclassified as silent" as reasoned-not-measured. At
a64e9437that was simply false — the sanitizer wasn't running. It becomes true at7bfccd66. So the reclassification is real, is introduced by this commit, andfindPreviousNonSilentMessagestill has no test. That gap matters more now than when I raised it, because the path is live for the first time.
Minor: the backstop's comment is measurably false
// Keep that backstop aligned with the live total-match + leading-bare rule.
: (/^(?:NO_REPLY\s*)+$/.test(trimmed)
|| /^NO_REPLY(?:$|[^A-Za-z0-9_])/.test(trimmed) ? '' : trimmed)It disagrees with the live rule on three shapes I measured — NO_REPLYNO_REPLY\nprose and its tripled form (the second regex sees N after the first sentinel and declines, the exact defect just fixed in hasLeadingBareNoReply), and a fenced sentinel-only reply.
It is also unreachable: AMS.sanitizeAgentContent now always resolves, so the sanitize branch always wins and neither the isSilentNoReply branch nor the regex branch can execute. So this is dead code — but dead code that re-encodes, incorrectly, the one rule this PR spent three commits getting right, under a comment asserting it is aligned. The third copy of a rule is where the next regression lives. I'd collapse the ternary to isSilentNoReply alone, or drop the backstop and let an absent import fail loudly.
Same shape, lower stakes: hasLeadingBareNoReply re-runs the outer-fence match on input the caller has already fence-stripped (sanitizeAgentContent passes trimmed after its own if (outerFence) return). Harmless today, but it means the fence rule now exists in three places.
Merge state
MERGEABLE, base fix/log-the-sentinel-strip. CI re-queued on the new head — all four checks pending as of this comment. The SKILL.md and agentProvisionerServiceK8s HEARTBEAT-text updates are a good catch; those are the surfaces that actually teach agents the rule, and syncing them in the same PR as the behaviour change is the right instinct.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Re-gated at 966192b0. All four claims reproduce, and the history-scan test is the real thing — it mocks the pool and models but leaves agentMessageService unmocked, so the live sanitizer actually runs. That closes the coverage gap I raised. One finding on the new test itself.
Claims verified
- 35 focused tests =
chatNoise(20) +systemExchangeTriggers.leadingNoReply(1) +agentEnsembleService(14). ✅ npm run tsc:check→ clean. ✅- Predicate-off mutation (
return sentinelEnd > 0 && !nextIsWordCharacter→return false, anchor asserted at 1) → the new history assertion fails on a realexpect, not a timeout:7 tests fail in total across the two suites. ✅✕ skips a stored leading-bare reply before recording the sender's prior substantive takeaway (132 ms) Expected: ObjectContaining {"takeaway": "The auth patch is ready for the final gate."} - Concatenated leading run suppressed. ✅
The new test cannot detect the dead-require-chain regression
This is the one thing I'd change, and it's a one-string fix.
I forced sanitize and isSilentNoReply to undefined — exactly the pre-7bfccd66 state, where AMS.AgentMessageService and AMS.default were both undefined and the lookup silently fell through. The new test still passes:
✓ skips a stored leading-bare reply before recording the sender's prior substantive takeaway (1023 ms)
Tests: 1 passed, 1 total
Because the fixture row is NO_REPLY\n…, and the regex backstop's /^NO_REPLY(?:$|[^A-Za-z0-9_])/ matches it. So the test passes whether the shared predicate did the work or the backstop did — which means it does not actually pin the wiring it was written to pin.
Changing one string in the fixture fixes it. With the row as NO_REPLYNO_REPLY\n… and the chain still dead:
✕ skips a stored leading-bare reply …
Expected: ObjectContaining {"takeaway": "The auth patch is ready for the final gate."}
1: {"instanceId": "nova", "takeaway": "NO_REPLYNO_REPLY\nThis suppressed body must not become the takeaway."}
2: {"instanceId": "pixel", "takeaway": "@nova: NO_REPLYNO_REPLY\nThis suppressed body must not become the takeaway."}
The backstop declines on the run, the suppressed body becomes the takeaway, and it gets written into both peers' memory envelopes with the sentinel intact. That's the failure the test exists to prevent, and the concatenated fixture is the input that can see it. Same test, same cost, strictly more coverage — and it keeps working after the backstop is removed.
The backstop finding is now worse than "dead and wrong"
I called it dead code last round. The measurement above upgrades it: the backstop is actively masking the wiring in the only test that covers this path. It is the reason a dead require chain reads as green. It is still unchanged at this head.
Two options, either fine: delete the ternary and call isSilentNoReply alone, or drop the backstop entirely and let an absent import throw. What I'd avoid is keeping a third hand-rolled copy of a rule that has now been wrong twice in this PR's history.
Minor
systemExchangeTriggers.leadingNoReply.test.ts opens with // @ts-nocheck, so tsc:check passing says nothing about this file. Given it is the only executable statement of the cross-module contract, I'd rather it were type-checked — the mocks look like the reason, and typing queryResult would likely be enough to drop the pragma.
Merge state
MERGEABLE, base fix/log-the-sentinel-strip, CI re-queued on the new head.
Worth saying: across seven commits this went from a guard that missed its own headline case to a shared predicate with the concatenated regression pinned, the ensemble consumer converted, the agent-facing SKILL/HEARTBEAT text synced, and a real history-scan test. The remaining item is a fixture string and a dead ternary — both smaller than anything already fixed here.
|
Re-gate at It is more than a regression test.
Verified rather than accepted:
CI is not complete at this head. Right now: Unchanged from the previous round: this is the stacked 5-check subset, so |
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Both points are addressed. Re-gated at 9bb6d8b8 — note the head is two commits past the d64a219e you named (1c0a63e5 and 9bb6d8b8, both comment-only on the 4096 excerpt rationale). Everything below is measured at 9bb6d8b8.
The backstop removal fixed the fixture problem without touching the fixture
This is the better of the two options I offered, and I want to be explicit that it fully resolves the finding — the fixture row is still NO_REPLY\n… and no longer needs to change.
Forcing sanitize to undefined (the pre-7bfccd66 dead-chain state) now fails the history test, where at 966192b0 it passed:
✕ skips a stored leading-bare reply before recording the sender's prior substantive takeaway
Expected: ObjectContaining {"takeaway": "The auth patch is ready for the final gate."}
Received: {"takeaway": "agent-dm concluded (no prior content captured)"}
With no third rule to silently absorb the failure, the existing fixture discriminates on its own. That's the structurally better fix — I was proposing to make the test stronger; removing the duplicate rule made the code honest, which is what made the test strong for free.
The degraded record is also the right shape: "agent-dm concluded (no prior content captured)" says it doesn't know, rather than inventing a takeaway from the suppressed body. Declining beats guessing here, and the early return null means an absent predicate can never write a false memory entry into both peers' envelopes.
Claims verified
- 35 focused tests — chatNoise (20) + leadingNoReply (1) + agentEnsembleService (14), 3 suites, all pass. ✅
npm run tsc:check→ clean. ✅- Predicate-off mutation (
return sentinelEnd > 0 && !nextIsWordCharacter→return false, anchor asserted at 1) → 8 failures across all three suites, including the ensemble turn test and the history-scan test. The regression is pinned at every consumer, not just at the sanitizer. ✅ - Sentinel behaviour re-measured at this head rather than carried forward:
| input | sanitizeAgentContent |
isSilentNoReply |
|---|---|---|
NO_REPLY\n… |
"" |
true |
NO_REPLYNO_REPLY\n… |
"" |
true |
NO_REPLYNO_REPLYNO_REPLY\n… |
"" |
true |
`NO_REPLY`\n… |
preserved | false |
```text\nNO_REPLY\n``` \n… |
preserved | false |
Reply with NO_REPLY when done. |
"Reply with when done." |
false |
Shipped the fix.\nNO_REPLY |
"Shipped the fix." |
false |
NO_REPLYING is a word. |
preserved | false |
isSilentNoReply retains a live consumer (agentEnsembleService.ts:340), so removing its branch from the scan didn't strand it.
No blocking findings
The only item left is the one I already logged as minor: systemExchangeTriggers.leadingNoReply.test.ts still opens with // @ts-nocheck, so tsc:check green says nothing about the file that now carries the whole cross-module contract. Not worth another round on its own — worth doing whenever that file is next touched.
Merge state
MERGEABLE, base fix/log-the-sentinel-strip (unmoved at ccfef7f8), CI re-queued on the new head. This still merges into #1252 before main, so #1252 is the next gate.
Nine commits from a guard that missed the concatenated shape its own test-file header documented, to: the shape suppressed, the predicate shared across three consumers, the duplicate rule deleted rather than patched, the agent-facing SKILL and HEARTBEAT text synced, and a history-scan test that now fails if the wiring breaks. Deleting the fallback instead of hardening it is the change I'd point at — a third copy of a rule that had already been wrong twice was the actual defect, and the mutation evidence only got trustworthy once it was gone.
1 participant
Summary
NO_REPLYpostMessagepasses only pod and sender identity to that hook, so the suppressed body structurally cannot become a takeawayVerification
npm test -- --runInBand agentMessageService(97 tests)npm run tsc:checkagentMemoryService.systemExchangesunit suite fails before test execution under Node 22 inbuffer-equal-constant-time; reproduced on the untouched workspaceDepends on #1252: this PR targets its branch so the strip-edit observability change lands first.