fix(telegram): suppress JSON-wrapped NO_REPLY leaking to end users#37891
fix(telegram): suppress JSON-wrapped NO_REPLY leaking to end users#37891Clawborn wants to merge 2 commits intoopenclaw:mainfrom
Conversation
decodeURIComponent throws on invalid sequences like %ZZ. Without a try-catch the canvas file-resolver would propagate an unhandled URIError, crashing the request. Fall back to the raw path on failure.
…penclaw#37727) When a model emits `{"action":"NO_REPLY"}` instead of the bare `NO_REPLY` sentinel, the outbound pipeline previously failed to recognise it as a silent reply, causing the raw JSON to be delivered as a visible Telegram message. Extend `isSilentReplyText` to detect small JSON objects whose sole meaningful value equals the silent token. Add regression tests. Closes openclaw#37727
Greptile SummaryThis PR fixes two independent issues: (1) JSON-wrapped The The
Confidence Score: 3/5
Last reviewed commit: 84121ea |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Accept objects with 1–2 keys where at least one value is the silent token. | ||
| if (values.length === 0 || values.length > 2) { | ||
| return false; | ||
| } | ||
| return values.some( | ||
| (v) => typeof v === "string" && v.trim().toUpperCase() === token.toUpperCase(), | ||
| ); |
There was a problem hiding this comment.
2-key object allows false-positive suppression
The docstring says "the sole meaningful value equals the silent token", but the implementation accepts objects with up to 2 keys where any value matches — values.some(...). This means a model response like {"action":"NO_REPLY","content":"Goodbye, have a great day!"} would be silently swallowed even though it carries real user-visible content in "content".
The intent (and the docstring) appears to be that the only non-null value in the object should be the sentinel. Either the key-count guard should be reduced to values.length > 1 (i.e., allow only single-key objects), or some should be replaced with every so that all values must equal the token. As written, the 2-key path contradicts the stated contract.
// Option A – restrict to single-key objects only (matches docstring intent)
if (values.length === 0 || values.length > 1) {
return false;
}
return (
typeof values[0] === "string" &&
values[0].trim().toUpperCase() === token.toUpperCase()
);Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/tokens.ts
Line: 114-120
Comment:
**2-key object allows false-positive suppression**
The docstring says "the sole meaningful value equals the silent token", but the implementation accepts objects with up to 2 keys where **any** value matches — `values.some(...)`. This means a model response like `{"action":"NO_REPLY","content":"Goodbye, have a great day!"}` would be silently swallowed even though it carries real user-visible content in `"content"`.
The intent (and the docstring) appears to be that the **only** non-null value in the object should be the sentinel. Either the key-count guard should be reduced to `values.length > 1` (i.e., allow only single-key objects), or `some` should be replaced with `every` so that all values must equal the token. As written, the 2-key path contradicts the stated contract.
```typescript
// Option A – restrict to single-key objects only (matches docstring intent)
if (values.length === 0 || values.length > 1) {
return false;
}
return (
typeof values[0] === "string" &&
values[0].trim().toUpperCase() === token.toUpperCase()
);
```
How can I resolve this? If you propose a fix, please make it concise.| it("does not suppress objects with unrelated values", () => { | ||
| expect(isSilentReplyText('{"action":"hello"}')).toBe(false); | ||
| }); | ||
|
|
||
| it("does not suppress larger objects", () => { | ||
| expect(isSilentReplyText('{"action":"NO_REPLY","text":"hi","extra":"data"}')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Missing test for 2-key mixed-value object
The test suite covers a 1-key non-matching object ({"action":"hello"}) and a 3-key object, but there is no test for the critical 2-key case where one value is the sentinel and the other is unrelated content. Due to the values.some(...) logic, {"action":"NO_REPLY","content":"Hello!"} currently returns true (suppressed). A test documenting the intended behaviour here would catch the discrepancy:
it("does not suppress 2-key objects where only one value is the token", () => {
expect(isSilentReplyText('{"action":"NO_REPLY","content":"Hello!"}')).toBe(false);
});Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/tokens.test.ts
Line: 119-125
Comment:
**Missing test for 2-key mixed-value object**
The test suite covers a 1-key non-matching object (`{"action":"hello"}`) and a 3-key object, but there is no test for the critical 2-key case where one value is the sentinel and the other is unrelated content. Due to the `values.some(...)` logic, `{"action":"NO_REPLY","content":"Hello!"}` currently returns `true` (suppressed). A test documenting the intended behaviour here would catch the discrepancy:
```typescript
it("does not suppress 2-key objects where only one value is the token", () => {
expect(isSilentReplyText('{"action":"NO_REPLY","content":"Hello!"}')).toBe(false);
});
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84121ea650
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (values.length === 0 || values.length > 2) { | ||
| return false; | ||
| } | ||
| return values.some( | ||
| (v) => typeof v === "string" && v.trim().toUpperCase() === token.toUpperCase(), |
There was a problem hiding this comment.
This matcher suppresses any 1–2 key JSON object when any value equals NO_REPLY, which means legitimate structured replies can now be dropped if they include that string in one field (for example {"status":"NO_REPLY","message":"done"}); because isSilentReplyText is used as a suppression gate in outbound reply paths, this introduces silent message loss for valid JSON outputs rather than just handling the token-only sentinel payload.
Useful? React with 👍 / 👎.
2 participants
Problem
When a model emits
{"action":"NO_REPLY"}instead of the bareNO_REPLYsentinel, the outbound pipeline fails to recognise it as a silent reply, causing raw JSON to be delivered as a visible Telegram message.Fix
Extend
isSilentReplyTextwith aisJsonWrappedSilentTokenhelper that detects small JSON objects (≤2 keys, ≤200 chars) whose value matches the silent token. Safe against false positives on larger payloads.Tests
6 new test cases covering exact match, whitespace variants, case insensitivity, and negative cases.
Closes #37727