Reasoning regex makes colon optional and case-insensitive

The regex /^Reasoning:?$/i uses :? (optional colon) and the i case-insensitive flag. The documented leak pattern is specifically the streaming prefix "Reasoning:" — always capitalized, always with a colon. Making the colon optional means the bare word "Reasoning" (no colon) sent by a user as their entire reply would also be silently suppressed. Similarly, the i flag suppresses "reasoning" or "REASONING" as standalone replies.

Consider requiring the colon and keeping the match case-sensitive to scope the suppression precisely to the known leak pattern:

This is a comment left during a code review.
Path: src/auto-reply/reply/reply-utils.ts
Line: 39-41

Comment:
**Reasoning regex makes colon optional and case-insensitive**

The regex `/^Reasoning:?$/i` uses `:?` (optional colon) and the `i` case-insensitive flag. The documented leak pattern is specifically the streaming prefix `"Reasoning:"` — always capitalized, always with a colon. Making the colon optional means the bare word `"Reasoning"` (no colon) sent by a user as their entire reply would also be silently suppressed. Similarly, the `i` flag suppresses `"reasoning"` or `"REASONING"` as standalone replies.

Consider requiring the colon and keeping the match case-sensitive to scope the suppression precisely to the known leak pattern:

```suggestion
  if (/^Reasoning:$/.test(text.trim())) {
    return null;
  }
```

How can I resolve this? If you propose a fix, please make it concise.

Already exact at the current head, and this was locked in b3ad9ed68f58e39c2264693702205acb93b91bdc. The regression coverage now proves Reasoning and REASONING: still pass through while only exact Reasoning: is suppressed.

Magic number for JSON payload length threshold

The value 200 is used as a hard cutoff for what counts as a "lightweight" JSON NO_REPLY payload, but it is not documented or extracted into a named constant. A reader has no way to know where 200 came from or whether it was intentional. There is also an off-by-one: a 200-character string passes the trimmed.length > 200 check (it equals 200, not greater), while a 201-character {"action":"NO_REPLY",...} slips through unsuppressed.

Consider extracting the constant with a comment explaining the rationale:

and at module level:

/** Maximum byte length of a JSON payload that can be treated as a bare NO_REPLY sentinel. */
const MAX_NO_REPLY_JSON_BYTES = 200;

This is a comment left during a code review.
Path: src/auto-reply/reply/reply-utils.ts
Line: 5

Comment:
**Magic number for JSON payload length threshold**

The value `200` is used as a hard cutoff for what counts as a "lightweight" JSON NO_REPLY payload, but it is not documented or extracted into a named constant. A reader has no way to know where 200 came from or whether it was intentional. There is also an off-by-one: a 200-character string passes the `trimmed.length > 200` check (it equals 200, not greater), while a 201-character `{"action":"NO_REPLY",...}` slips through unsuppressed.

Consider extracting the constant with a comment explaining the rationale:

```suggestion
  if (trimmed.length === 0 || trimmed.length > MAX_NO_REPLY_JSON_BYTES) {
    return false;
  }
```

and at module level:
```ts
/** Maximum byte length of a JSON payload that can be treated as a bare NO_REPLY sentinel. */
const MAX_NO_REPLY_JSON_BYTES = 200;
```

How can I resolve this? If you propose a fix, please make it concise.

Addressed in 5b1bc7ed48850f54d4ebecba1c8b323fdbbfc7f8. That commit extracts NO_REPLY_JSON_SENTINEL_MAX_LENGTH and documents why only tiny control-like JSON blobs are treated as sentinels.

Persist explicit reasoning off overrides across turns

This line now backfills resolvedReasoningLevel from the model default, which can be "on", but /reasoning off still clears sessionEntry.reasoningLevel instead of storing an explicit off override (src/auto-reply/reply/directive-handling.impl.ts, lines 321-323). In that setup, disabling reasoning only affects the current turn; the next message has no stored override, so this fallback re-enables reasoning, making user intent non-persistent on reasoning-capable models.

Useful? React with 👍 / 👎.

This is scoped to #37940, not this sanitizer PR. #37940 fixes the catalog-aware resolvedReasoning path for session_status and the synchronous status surfaces.

Catalog-less fallback misses catalog-only reasoning models

The new resolveReasoningDefault fallback here checks user config but receives no catalog, so any model whose reasoning: true flag comes purely from Pi SDK catalog discovery (not from an explicit entry in the user's models.providers config) will return "off".

The most important production callsite that hits this path is session-status-tool.ts (line 433), which calls buildStatusMessage without supplying resolvedReasoning. The updated system prompt (also in this PR) now tells agents to "ALWAYS call session_status first" for reasoning/model questions — yet the session_status tool will report Reasoning: off for any user running gpt-5.4 via catalog-based discovery rather than an explicit config entry.

The /status command path already does the right thing (resolvedReasoning: resolvedReasoningLevel ?? (await resolveDefaultReasoningLevel())), so the inconsistency is specifically in the session_status tool path. To close the gap, session-status-tool.ts should resolve and pass resolvedReasoning the same way commands-status.ts does.

This is a comment left during a code review.
Path: src/auto-reply/status.ts
Line: 383-385

Comment:
**Catalog-less fallback misses catalog-only reasoning models**

The new `resolveReasoningDefault` fallback here checks user config but receives no catalog, so any model whose `reasoning: true` flag comes purely from Pi SDK catalog discovery (not from an explicit entry in the user's `models.providers` config) will return `"off"`.

The most important production callsite that hits this path is `session-status-tool.ts` (line 433), which calls `buildStatusMessage` without supplying `resolvedReasoning`. The updated system prompt (also in this PR) now tells agents to "ALWAYS call session_status first" for reasoning/model questions — yet the `session_status` tool will report `Reasoning: off` for any user running gpt-5.4 via catalog-based discovery rather than an explicit config entry.

The `/status` command path already does the right thing (`resolvedReasoning: resolvedReasoningLevel ?? (await resolveDefaultReasoningLevel())`), so the inconsistency is specifically in the `session_status` tool path. To close the gap, `session-status-tool.ts` should resolve and pass `resolvedReasoning` the same way `commands-status.ts` does.

How can I resolve this? If you propose a fix, please make it concise.

This is also scoped to #37940, not this sanitizer PR. #37940 threads catalog-aware reasoning resolution through session_status and the other synchronous status surfaces.

Restrict bare Reasoning: suppression to leak contexts

This check suppresses any outbound message whose trimmed text is exactly Reasoning:, regardless of whether it came from a leaked reasoning stream. In normal chat flows, a legitimate user-requested reply like “send exactly Reasoning:” will now be dropped by buildReplyPayloads (text becomes undefined and the payload is filtered out), which is a behavior regression from prior commits; consider gating this rule on an explicit reasoning/leak signal instead of raw text alone.

Useful? React with 👍 / 👎.

This PR intentionally matches the documented leak token at the shared outbound boundary. Adding explicit leak-context metadata would require a broader reply-pipeline change and is outside the scope of this sanitizer PR.

Falsy check discards empty-string text that sanitizer explicitly preserves

sanitizeOutboundText has a dedicated early-return for empty strings — if (text === "") return text — specifically to distinguish "preserve as-is" ("") from "actively suppress" (null). The three-valued contract is:

• null → suppress (caller should drop the field)
• undefined → input was undefined (pass-through)
• string (including "") → keep unchanged

Using text ? text : undefined collapses the null and "" cases into the same outcome (undefined). A payload arriving with text: "" plus a mediaUrl or channelData will silently lose its (admittedly empty) text field, even though the sanitizer said to preserve it.

The straightforward fix is a null-specific check:

Or equivalently, text ?? undefined works too, since ?? only short-circuits on null/undefined, not on "".

This is a comment left during a code review.
Path: src/auto-reply/reply/agent-runner-payloads.ts
Line: 86

Comment:
**Falsy check discards empty-string text that sanitizer explicitly preserves**

`sanitizeOutboundText` has a dedicated early-return for empty strings — `if (text === "") return text` — specifically to distinguish "preserve as-is" (`""`) from "actively suppress" (`null`). The three-valued contract is:
- `null` → suppress (caller should drop the field)
- `undefined` → input was undefined (pass-through)
- `string` (including `""`) → keep unchanged

Using `text ? text : undefined` collapses the `null` and `""` cases into the same outcome (`undefined`). A payload arriving with `text: ""` plus a `mediaUrl` or `channelData` will silently lose its (admittedly empty) text field, even though the sanitizer said to preserve it.

The straightforward fix is a null-specific check:

```suggestion
      text: text !== null ? text : undefined,
```

Or equivalently, `text ?? undefined` works too, since `??` only short-circuits on `null`/`undefined`, not on `""`.

How can I resolve this? If you propose a fix, please make it concise.

I left that collapse intentional in this PR. The sanitizer only needs null to suppress leaked text at the shared outbound boundary, and preserving empty-string payload shape exactly would be a separate payload-normalization follow-up.

Await buildReplyPayloads before reading replyPayloads

buildReplyPayloads returns a Promise, but this helper accesses .replyPayloads synchronously, so build(...) becomes undefined instead of an array. That means the new outbound-sanitizer tests in this block are not exercising the intended code path (and will fail/assert on wrong values), which undermines coverage for the regression this commit is trying to prevent; the helper and related assertions should await the async call.

Useful? React with 👍 / 👎.

Preserve streamed payload keys when sanitizing final text

When block streaming aborts (didStream() === true and isAborted() === true), buildReplyPayloads falls back to final payloads and de-duplicates against blockReplyPipeline.hasSentPayload(...), which uses a content key that includes text (createBlockReplyContentKey in block-reply-pipeline.ts). Sanitizing normalized.payload.text here can change a streamed payload like {"action":"NO_REPLY"} (or bare Reasoning:) into undefined, so the fallback key no longer matches what was already streamed and the same media/channelData payload can be sent again.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector review

Codex Review: Didn't find any major issues. 🎉

Index misalignment when media duplicates are removed mid-array

mediaFilteredPayloadEntries iterates dedupedPayloadEntries by position and indexes into mediaFilteredPayloads using the same index. But mediaFilteredPayloads is a filtered subset of dedupedPayloads — if any payload is removed from the middle (e.g. payload at index 1 of 3 is a media duplicate), every subsequent entry picks up the wrong payload and the last entry silently disappears.

Concrete example:

• dedupedPayloads = [payA, payB, payC] — payB has a media URL already sent
• mediaFilteredPayloads = [payA, payC] — payB removed
• Iteration: dedupedPayloadEntries[1] (entryB) gets mediaFilteredPayloads[1] = payC (wrong), while entryC's mediaFilteredPayloads[2] is undefined → dropped entirely

The effect is that entryC is silently lost and payC is emitted with entryB's rawText, which can cause the content-key dedup in the block-streaming path to compare the wrong raw text against the pipeline's sentContentKeys.

The dedupedPayloadSet approach used a few lines above is the correct pattern — match by object identity rather than position:

const mediaFilteredPayloadSet = new Set(mediaFilteredPayloads);
const mediaFilteredPayloadEntries = dedupedPayloadEntries.filter(
  (entry) => mediaFilteredPayloadSet.has(entry.payload),
);

This is a comment left during a code review.
Path: src/auto-reply/reply/agent-runner-payloads.ts
Line: 223-229

Comment:
**Index misalignment when media duplicates are removed mid-array**

`mediaFilteredPayloadEntries` iterates `dedupedPayloadEntries` by position and indexes into `mediaFilteredPayloads` using the same index. But `mediaFilteredPayloads` is a filtered subset of `dedupedPayloads` — if any payload is removed from the middle (e.g. payload at index 1 of 3 is a media duplicate), every subsequent entry picks up the wrong payload and the last entry silently disappears.

Concrete example:
- `dedupedPayloads = [payA, payB, payC]` — payB has a media URL already sent
- `mediaFilteredPayloads = [payA, payC]` — payB removed
- Iteration: `dedupedPayloadEntries[1]` (entryB) gets `mediaFilteredPayloads[1]` = **payC** (wrong), while entryC's `mediaFilteredPayloads[2]` is `undefined` → dropped entirely

The effect is that entryC is silently lost and payC is emitted with entryB's `rawText`, which can cause the content-key dedup in the block-streaming path to compare the wrong raw text against the pipeline's `sentContentKeys`.

The `dedupedPayloadSet` approach used a few lines above is the correct pattern — match by object identity rather than position:

```ts
const mediaFilteredPayloadSet = new Set(mediaFilteredPayloads);
const mediaFilteredPayloadEntries = dedupedPayloadEntries.filter(
  (entry) => mediaFilteredPayloadSet.has(entry.payload),
);
```

How can I resolve this? If you propose a fix, please make it concise.

Unrelated model change included in sanitizer PR

The addition of transport: String to PushTestResult (and the identical change in apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift) appears unrelated to the outbound text sanitizer described in this PR. The PR description, linked issues, and background context all focus on the auto-reply NO_REPLY/Reasoning leak fix; there is no mention of push test transport metadata.

Because transport is a non-optional String it is a source-breaking change for any Swift call-sites that construct PushTestResult directly. If this change belongs to a different fix or feature, landing it here makes it harder to revert either change independently and adds noise to the sanitizer's review surface.

This is a comment left during a code review.
Path: apps/macos/Sources/OpenClawProtocol/GatewayModels.swift
Line: 1109-1141

Comment:
**Unrelated model change included in sanitizer PR**

The addition of `transport: String` to `PushTestResult` (and the identical change in `apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift`) appears unrelated to the outbound text sanitizer described in this PR. The PR description, linked issues, and background context all focus on the auto-reply NO_REPLY/Reasoning leak fix; there is no mention of push test transport metadata.

Because `transport` is a non-optional `String` it is a source-breaking change for any Swift call-sites that construct `PushTestResult` directly. If this change belongs to a different fix or feature, landing it here makes it harder to revert either change independently and adds noise to the sanitizer's review surface.

How can I resolve this? If you propose a fix, please make it concise.

Preserve entries when media dedupe rewrites payload objects

buildReplyPayloads rebuilds mediaFilteredPayloadEntries by checking object identity (new Set(mediaFilteredPayloads) + has(entry.payload)), but filterMessagingToolMediaDuplicates creates new payload objects when it strips duplicate media. That means rewritten payloads never match their original entry.payload references and get dropped entirely, so a reply like {text:"hello", mediaUrl:"..."} with a deduped media URL can become no final payload instead of preserving the text-only reply.

Useful? React with 👍 / 👎.

Set membership check breaks for media-stripped payloads

filterMessagingToolMediaDuplicates is implemented with payloads.map(...). When a payload has a duplicate media URL to strip it returns a new object ({...payload, mediaUrl: undefined, ...}); only unchanged payloads return the original reference (return payload; // No change at reply-payloads.ts:152).

Because mediaFilteredPayloadSet is built from those potentially-new references, and dedupedPayloadEntries still holds the original entry.payload references, the has() check silently fails for every payload that had its media URL stripped:

dedupedPayloadEntries[i].payload  →  originalPayloadRef
mediaFilteredPayloads[i]          →  newPayloadRef  (different object after .map spread)
mediaFilteredPayloadSet.has(originalPayloadRef)  →  false  ← entry dropped!

The consequence is that any payload whose media URL was deduplicated — but still has non-empty text or channelData that should be delivered — is completely silently dropped from replyPayloads. Before this refactor, mediaFilteredPayloads flowed directly to filteredPayloads, so the modified payload (with stripped media URL) was still sent. After this refactor it is lost.

The filterMessagingToolDuplicates path does not have this problem because it uses .filter() and always preserves object references.

One fix: track entries by content key rather than object reference for the media-filter step, or perform the media-dedup directly on the entries array so the same object always flows through:

// Instead of a separate media-filtered payloads array, apply the media filter
// directly on entries and let each entry carry a potentially-updated payload.
const mediaFilteredPayloadEntries = dedupeMessagingToolPayloads
  ? applyMediaFilterToEntries(dedupedPayloadEntries, messagingToolSentMediaUrls)
  : dedupedPayloadEntries;

This is a comment left during a code review.
Path: src/auto-reply/reply/agent-runner-payloads.ts
Line: 223-226

Comment:
**Set membership check breaks for media-stripped payloads**

`filterMessagingToolMediaDuplicates` is implemented with `payloads.map(...)`. When a payload has a duplicate media URL to strip it returns a **new object** (`{...payload, mediaUrl: undefined, ...}`); only unchanged payloads return the original reference (`return payload; // No change` at `reply-payloads.ts:152`).

Because `mediaFilteredPayloadSet` is built from those potentially-new references, and `dedupedPayloadEntries` still holds the original `entry.payload` references, the `has()` check silently fails for every payload that had its media URL stripped:

```
dedupedPayloadEntries[i].payload  →  originalPayloadRef
mediaFilteredPayloads[i]          →  newPayloadRef  (different object after .map spread)
mediaFilteredPayloadSet.has(originalPayloadRef)  →  false  ← entry dropped!
```

The consequence is that any payload whose media URL was deduplicated — but still has non-empty text or `channelData` that should be delivered — is completely silently dropped from `replyPayloads`. Before this refactor, `mediaFilteredPayloads` flowed directly to `filteredPayloads`, so the modified payload (with stripped media URL) was still sent. After this refactor it is lost.

The `filterMessagingToolDuplicates` path does not have this problem because it uses `.filter()` and always preserves object references.

One fix: track entries by content key rather than object reference for the media-filter step, or perform the media-dedup directly on the entries array so the same object always flows through:

```ts
// Instead of a separate media-filtered payloads array, apply the media filter
// directly on entries and let each entry carry a potentially-updated payload.
const mediaFilteredPayloadEntries = dedupeMessagingToolPayloads
  ? applyMediaFilterToEntries(dedupedPayloadEntries, messagingToolSentMediaUrls)
  : dedupedPayloadEntries;
```

How can I resolve this? If you propose a fix, please make it concise.

Sanitize block-streamed text before emitting replies

This sanitizer is only executed in buildReplyPayloads, but when block streaming succeeds the same function drops all final payloads (shouldDropFinalPayloads), so this path is skipped and streamed chunks are emitted from createBlockReplyDeliveryHandler via normalizeStreamingText in src/auto-reply/reply/agent-runner-execution.ts without sanitizeOutboundText. In block-streaming environments, leaked control text like {"action":"NO_REPLY"} or bare Reasoning: will still be delivered to users, so the fix does not cover the primary streamed delivery path.

Useful? React with 👍 / 👎.

Good catch. The streaming path (normalizeStreamingText in agent-runner-execution.ts) does bypass this sanitizer. This PR only covers the final payload path in buildReplyPayloads. Fixing the block streaming path would also require changes in createBlockReplyDeliveryHandler, so I kept that out of scope here. I can open a follow-up for that.