reply: suppress uppercase NO_REPLY lead fragments#32172
reply: suppress uppercase NO_REPLY lead fragments#32172bmendonca3 wants to merge 2 commits intoopenclaw:mainfrom
Conversation
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟡 Case-sensitive validation bypasses silent-token prefix detection (lowercase control tokens leak)
Description
Vulnerable code: const trimmed = text.trimStart();
if (/[^A-Z_]/.test(trimmed)) {
return false;
}
const normalized = trimmed.toUpperCase();This is a behavior regression from the prior RecommendationTreat case normalization as part of canonicalization before validation, or make the validation regex case-insensitive. Option A (validate after canonicalization, preserves old robustness): const trimmed = text.trimStart();
const normalized = trimmed.toUpperCase();
if (/[^A-Z_]/.test(normalized)) return false;Option B (keep current structure but accept lowercase safely): const trimmed = text.trimStart();
if (/[^A-Z_]/i.test(trimmed)) return false;
const normalized = trimmed.toUpperCase();Either approach continues to reject digits/whitespace/punctuation while ensuring Analyzed PR: #32172 at commit Last updated on: 2026-03-02T22:48:21Z |
Greptile SummaryThis PR attempts to fix streaming partial suppression so that uppercase lead fragments of However, there is a critical inconsistency between the PR description, the unit tests, and the new e2e test case:
The actual behavioral change is narrower than stated: the fix prevents lowercase token fragments (e.g. Confidence Score: 1/5
Last reviewed commit: a734c58 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e448d0fa8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/auto-reply/tokens.ts
Outdated
| if (/[^A-Z_]/.test(trimmed)) { | ||
| return false; | ||
| } | ||
| const normalized = trimmed.toUpperCase(); | ||
| return token.toUpperCase().startsWith(normalized); |
There was a problem hiding this comment.
Keep bare "NO" from matching silent-token prefixes
This predicate now treats any uppercase token prefix (including "N"/"NO") as silent, which broadens suppression beyond token-like fragments and can drop legitimate leading content in streamed delivery paths. createStreamingDirectiveAccumulator applies this check to chunked streaming and final flushes (src/auto-reply/reply/streaming-directives.ts:51), so if a block chunk is exactly "NO" before later chunks arrive, that chunk is discarded as silent; once later chunks are streamed, final payloads may be omitted as already-streamed (src/auto-reply/reply/agent-runner-payloads.ts:86), leaving users with truncated output. Consider keeping prefix suppression scoped to unmistakable token fragments (for example requiring _) or only using this relaxed match for non-final partials.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Bare uppercase fragments like N/NO were being treated as silent-token prefixes, which can suppress legitimate streamed content.
Applied on head a734c58a0b:
isSilentReplyPrefixTextnow accepts only exact token matches or underscore-delimited token fragments (for exampleNO_,NO_RE).- Added regression expectations that
NandNOare not treated as silent prefixes.
Verification:
pnpm -s vitest run src/auto-reply/tokens.test.ts src/auto-reply/reply/reply-utils.test.ts
|
@greptile-apps please re-review latest head for updated confidence score. |
|
@chatgpt-codex-connector please re-review latest head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a734c58a0b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| partials: ["NO", "NO_", "NO_RE", "NO_REPLY"], | ||
| finalText: "NO_REPLY", | ||
| expectedForwarded: [] as string[], | ||
| shouldType: false, |
There was a problem hiding this comment.
Drop unreachable NO-suppression expectation
This new test case expects the "NO" partial to be suppressed, but isSilentReplyPrefixText still returns false for bare "NO" because it requires an underscore-delimited prefix unless the full token is present (NO_REPLY). In the runReplyAgent partial path, that means the first "NO" chunk is still forwarded, so this assertion is unreachable and will fail once this e2e case is executed.
Useful? React with 👍 / 👎.
| { | ||
| partials: ["NO", "NO_", "NO_RE", "NO_REPLY"], | ||
| finalText: "NO_REPLY", | ||
| expectedForwarded: [] as string[], | ||
| shouldType: false, | ||
| }, |
There was a problem hiding this comment.
E2E test case will fail — "NO" is not suppressed by the implementation
This test case expects expectedForwarded: [] for the partial sequence ["NO", "NO_", "NO_RE", "NO_REPLY"], but the implementation does not suppress the "NO" partial.
Tracing the call path for payload = { text: "NO" }:
handlePartialForTypingcallsisSilentReplyPrefixText("NO", SILENT_REPLY_TOKEN)→false(no underscore, not equal to"NO_REPLY")normalizeStreamingTextalso callsisSilentReplyPrefixText("NO")→falseagainsanitizeUserFacingText("NO")returns"NO"(a valid non-empty string)params.opts.onPartialReply({ text: "NO", mediaUrls: undefined })is called
The unit test in tokens.test.ts (line 88) confirms this:
expect(isSilentReplyPrefixText("NO")).toBe(false);This contradicts the PR description which states "now recognizes uppercase alpha-only token prefixes (N, NO)" — neither "N" nor "NO" is recognized as a silent prefix by the new code.
The e2e test assertion will fail:
expect(onPartialReply).not.toHaveBeenCalled(); // FAIL: called with { text: "NO", ... }You need to either:
- Update the e2e test to reflect that
"NO"is forwarded (expectedForwarded: ["NO"]), or - Change
isSilentReplyPrefixTextto returntruefor all-uppercase-alpha prefixes of the token (e.g. treat"NO"as suppressible), but then update the unit test accordingly and resolve the ambiguity with natural-language"No".
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/agent-runner.runreplyagent.e2e.test.ts
Line: 412-417
Comment:
**E2E test case will fail — "NO" is not suppressed by the implementation**
This test case expects `expectedForwarded: []` for the partial sequence `["NO", "NO_", "NO_RE", "NO_REPLY"]`, but the implementation does **not** suppress the `"NO"` partial.
Tracing the call path for `payload = { text: "NO" }`:
1. `handlePartialForTyping` calls `isSilentReplyPrefixText("NO", SILENT_REPLY_TOKEN)` → `false` (no underscore, not equal to `"NO_REPLY"`)
2. `normalizeStreamingText` also calls `isSilentReplyPrefixText("NO")` → `false` again
3. `sanitizeUserFacingText("NO")` returns `"NO"` (a valid non-empty string)
4. `params.opts.onPartialReply({ text: "NO", mediaUrls: undefined })` **is called**
The unit test in `tokens.test.ts` (line 88) confirms this:
```ts
expect(isSilentReplyPrefixText("NO")).toBe(false);
```
This contradicts the PR description which states "now recognizes uppercase alpha-only token prefixes (`N`, `NO`)" — neither `"N"` nor `"NO"` is recognized as a silent prefix by the new code.
The e2e test assertion will fail:
```ts
expect(onPartialReply).not.toHaveBeenCalled(); // FAIL: called with { text: "NO", ... }
```
You need to either:
- Update the e2e test to reflect that `"NO"` **is** forwarded (`expectedForwarded: ["NO"]`), or
- Change `isSilentReplyPrefixText` to return `true` for all-uppercase-alpha prefixes of the token (e.g. treat `"NO"` as suppressible), but then update the unit test accordingly and resolve the ambiguity with natural-language `"No"`.
How can I resolve this? If you propose a fix, please make it concise.
Summary
Describe the problem and fix in 2–5 bullets:
NOleaked beforeNO_REPLYwas complete.NOpartial even when the final reply was correctly suppressed asNO_REPLY.isSilentReplyPrefixTextnow recognizes uppercase alpha-only token prefixes (N,NO) while still rejecting mixed/lowercase natural-language starts (No).Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
NOlead fragments fromNO_REPLYturns.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
node --import tsx -e 'import { isSilentReplyPrefixText } from "./src/auto-reply/tokens.ts"; console.log(isSilentReplyPrefixText("NO"))'false(leak path).pnpm -s vitest run src/auto-reply/tokens.test.tspnpm -s vitest run --config vitest.e2e.config.ts src/auto-reply/reply/agent-runner.runreplyagent.e2e.test.ts -t "suppresses NO_REPLY partials but allows normal No-prefix partials"pnpm checkExpected
NO_REPLYare treated as suppressible prefixes.Actual
NOwas not classified as a silent prefix and could stream as a visible partial.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
isSilentReplyPrefixText("NO") === false.tokens.test.ts.agent-runner.runreplyagent.e2e.test.tswith partialsNO -> NO_ -> NO_RE -> NO_REPLYexpecting no forwarded partials.No(mixed-case natural language prefix) remains unsuppressed.Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
src/auto-reply/tokens.tssrc/auto-reply/tokens.test.tssrc/auto-reply/reply/agent-runner.runreplyagent.e2e.test.tsRisks and Mitigations
No) are explicitly covered by regression tests.