fix(webchat): hide heartbeat chat output using showAlerts#69084
fix(webchat): hide heartbeat chat output using showAlerts#69084Jmarzab wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e822255c51
ℹ️ 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".
| const cfg = loadConfig(); | ||
| const visibility = resolveHeartbeatVisibility({ cfg, channel: "webchat" }); | ||
| return !visibility.showOk; | ||
| return !visibility.showAlerts; |
There was a problem hiding this comment.
Keep heartbeat ack suppression keyed to showOk
shouldHideHeartbeatChatOutput() now checks showAlerts, but the ack/noise filter in normalizeHeartbeatChatFinalText() is what removes HEARTBEAT_OK-style output when acknowledgements are disabled. With common visibility settings (showOk=false, showAlerts=true), this change makes shouldHideHeartbeatChatOutput() return false, so heartbeat acknowledgements are no longer stripped/suppressed in webchat and can leak into user-visible chat transcripts. This regresses the showOk contract for heartbeat ack visibility.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR changes
Confidence Score: 3/5Not safe to merge: the fix reverses default suppression behavior and breaks at least two existing tests in server-chat.agent-events.test.ts. The one-line change is semantically motivated but produces a default-behavior regression and invalidates existing test coverage that specifically asserts suppression. Both are present defects on the changed path. src/gateway/server-chat.ts line 61 and the corresponding test file src/gateway/server-chat.agent-events.test.ts (not in the diff but directly broken by this change). Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/server-chat.ts
Line: 61
Comment:
**Default behavior reversal breaks existing users and tests**
Swapping `showOk` → `showAlerts` flips the out-of-the-box suppression behavior. `showOk` defaults to `false`, so `!showOk` was `true` (suppress). `showAlerts` defaults to `true`, so `!showAlerts` is now `false` (show). Any user with no explicit heartbeat visibility config will suddenly see heartbeat text in webchat after this change.
This also breaks the existing test `"suppresses heartbeat ack-like chat output when showOk is false"` in `server-chat.agent-events.test.ts`: the global mock already sets `showAlerts: true`, so after this change `shouldHideHeartbeatChatOutput` returns `false` and both the delta and the final broadcasts happen — the test expects 0 delta calls and `message: undefined` on the final, so it will fail. The test at line 1435 is similarly affected (expects stripped text but receives the full `HEARTBEAT_OK` string because the stripping path is never reached when `shouldHideHeartbeatChatOutput` returns `false`).
To preserve backward compatibility and fix the tests, either default `showAlerts` to `false` in `heartbeat-visibility.ts`, or update the test mocks to `showAlerts: false` for the suppression scenarios.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(webchat): hide heartbeat chat output..." | Re-trigger Greptile |
| const cfg = loadConfig(); | ||
| const visibility = resolveHeartbeatVisibility({ cfg, channel: "webchat" }); | ||
| return !visibility.showOk; | ||
| return !visibility.showAlerts; |
There was a problem hiding this comment.
Swapping showOk → showAlerts flips the out-of-the-box suppression behavior. showOk defaults to false, so !showOk was true (suppress). showAlerts defaults to true, so !showAlerts is now false (show). Any user with no explicit heartbeat visibility config will suddenly see heartbeat text in webchat after this change.
This also breaks the existing test "suppresses heartbeat ack-like chat output when showOk is false" in server-chat.agent-events.test.ts: the global mock already sets showAlerts: true, so after this change shouldHideHeartbeatChatOutput returns false and both the delta and the final broadcasts happen — the test expects 0 delta calls and message: undefined on the final, so it will fail. The test at line 1435 is similarly affected (expects stripped text but receives the full HEARTBEAT_OK string because the stripping path is never reached when shouldHideHeartbeatChatOutput returns false).
To preserve backward compatibility and fix the tests, either default showAlerts to false in heartbeat-visibility.ts, or update the test mocks to showAlerts: false for the suppression scenarios.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-chat.ts
Line: 61
Comment:
**Default behavior reversal breaks existing users and tests**
Swapping `showOk` → `showAlerts` flips the out-of-the-box suppression behavior. `showOk` defaults to `false`, so `!showOk` was `true` (suppress). `showAlerts` defaults to `true`, so `!showAlerts` is now `false` (show). Any user with no explicit heartbeat visibility config will suddenly see heartbeat text in webchat after this change.
This also breaks the existing test `"suppresses heartbeat ack-like chat output when showOk is false"` in `server-chat.agent-events.test.ts`: the global mock already sets `showAlerts: true`, so after this change `shouldHideHeartbeatChatOutput` returns `false` and both the delta and the final broadcasts happen — the test expects 0 delta calls and `message: undefined` on the final, so it will fail. The test at line 1435 is similarly affected (expects stripped text but receives the full `HEARTBEAT_OK` string because the stripping path is never reached when `shouldHideHeartbeatChatOutput` returns `false`).
To preserve backward compatibility and fix the tests, either default `showAlerts` to `false` in `heartbeat-visibility.ts`, or update the test mocks to `showAlerts: false` for the suppression scenarios.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a828b468ee
ℹ️ 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".
| const cfg = loadConfig(); | ||
| const visibility = resolveHeartbeatVisibility({ cfg, channel: "webchat" }); | ||
| return !visibility.showOk; | ||
| return !visibility.showAlerts; |
There was a problem hiding this comment.
Preserve showOk-only heartbeat acknowledgments in webchat
Switching shouldHideHeartbeatChatOutput() to !visibility.showAlerts causes webchat to suppress heartbeat output whenever alerts are disabled, including HEARTBEAT_OK acknowledgments that should still be visible when showOk=true. With showOk=true and showAlerts=false, emitChatDelta() now bails out for heartbeat runs and final normalization suppresses the ack token, so users who intentionally configured “OK-only” heartbeat visibility no longer receive any chat acknowledgment.
Useful? React with 👍 / 👎.
|
Validated and completed this fix end-to-end. What changed:
Why:
Validation:
|
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. This PR should stay open for maintainer/contributor follow-up, but it should not merge as-is: the patch conflates heartbeat OK acknowledgments with alert visibility, regressing both default webchat suppression and OK-only heartbeat configurations; the contributor’s later discussion also agrees the current branch should not merge unchanged. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes, for the PR defect: source inspection shows the proposed gate makes the default Is this the best way to solve the issue? No, this is not the best way to solve the issue because it replaces one visibility contract with another and bypasses current heartbeat token stripping for visible alerts. The better fix is a small Gateway decision that preserves both flags and tests the default and mixed configurations. Security review: Security review cleared: The diff only changes Gateway heartbeat visibility filtering and adjacent tests; no new permissions, dependencies, token handling, install hooks, network calls, or code-execution paths were introduced. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 9313471fa579. |
|
I think clawsweeper is basically right here.
Swapping the webchat suppression gate from So I don’t think this patch should merge as-is. The narrow fix should preserve both flags separately and add explicit tests for:
|
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
|
ClawSweeper applied the proposed close for this PR.
|
Summary
Problem
Heartbeat-generated agent output could still appear in webchat even when
channels.defaults.heartbeat.showAlerts = false.Why it matters
It adds noisy system text to the visible chat transcript and makes it harder to distinguish real user/assistant messages from heartbeat output.
What changed
shouldHideHeartbeatChatOutput()now usesvisibility.showAlertsinstead ofvisibility.showOkwhen deciding whether to suppress heartbeat chat output in webchat.What did NOT change (scope boundary)
No config schema, heartbeat execution, delivery logic, or non-webchat behavior was changed.
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Closes #
Related #
✔ This PR fixes a bug or regression
Root Cause (if applicable)
showOkinstead ofshowAlerts, creating a semantic mismatch between backend heartbeat visibility and visible chat suppression.showAlerts=falseshould suppress heartbeat text in webchat.chatevent path, so the wrong visibility flag caused user-visible transcript noise.Regression Test Plan (if applicable)
Coverage level that should have caught this
Target test or file
Gateway/server-chat heartbeat visibility coverage around
shouldHideHeartbeatChatOutput().Scenario the test should lock in
When heartbeat output is produced and webchat visibility resolves with
showAlerts=false, heartbeat text should be suppressed from webchat chat output.Why this is the smallest reliable guardrail
The bug is caused by the interaction between heartbeat visibility resolution and gateway chat filtering, so a seam/integration test is the smallest level that exercises the real decision point.
Existing test that already covers this
None known.
If no new test is added, why not
This PR is intentionally minimal and focused on the source fix only; manual runtime verification was performed.
User-visible / Behavior Changes
Heartbeat text is no longer shown in webchat when
channels.defaults.heartbeat.showAlerts = false.Normal chat behavior is unchanged.
Diagram (if applicable)
Security Impact (required)
Repro + Verification
Environment
channels.defaults.heartbeat.showAlerts = falseSteps
showAlerts = falseExpected
Actual
showOkbeing usedEvidence
Human Verification (required)
Verified scenarios
Edge cases checked
Not verified
Review Conversations
Compatibility / Migration
Risks and Mitigations
Risk
Users relying on previous
showOkbehavior may see different visibility.Mitigation
showAlertssemantics