room_ack: refuse a blind clear instead of warning after the fact - #110
ThinkOffApp wants to merge 1 commit into
Conversation
A bare room_ack (no room_list_new this session) blanked the whole notification file and returned a sentence advising against it. The advice arrived after the messages were gone, which is the shape of guard that cannot help: it performs the destructive act and then names it. The code already knew. Its own comment called the path "inherently racy" and cited the 2026-07-08 incident where an owner instruction sat five hours unseen. Two agents on this fleet hit it again today. room_ack now refuses when the file has unread lines and there is nothing to ack against, and the refusal names the one command that makes the ack safe. Acking an empty file cold is harmless, so that stays a no-op. The normal path -- room_list_new, then room_ack -- is untouched: it still removes only the consumed bytes and preserves late arrivals. The legacy clear-all unit test was the specification of the old behaviour, so it is replaced rather than deleted: one test asserts the refusal AND that the file is unchanged after it, one asserts the empty no-op, and a new end-to-end test drives all three paths over stdio. 333/333 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 380df4b65f
ℹ️ 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".
| 'prefer room_list_new → room_ack so late arrivals are preserved).' | ||
| ); | ||
| if (result.mode === 'refused') { | ||
| return ok(result.error); |
There was a problem hiding this comment.
When room_ack is called without a preceding room_list_new while unread messages exist, this branch reports the refusal through ok(), so the MCP result lacks isError: true. Clients that rely on the protocol status rather than parsing the response text will treat the acknowledgement as successful and may continue under the false assumption that the messages were cleared; return err(result.error) as other refused operations do.
Useful? React with 👍 / 👎.
2 participants
A bare
room_ackwith no precedingroom_list_newused to blank the whole notification file, then warn. The warning arrived after the messages were gone.Now: refuse when the file has unread lines and there is nothing to ack against. Empty-file cold ack stays a no-op. The normal
room_list_newthenroom_ackpath is unchanged (only consumed bytes, late arrivals kept).Overlaps
src/mcp-server.mjswithclaudemm/skill-document-reactions. Merge one, rebase the other.Branch tip:
380df4b. 333 tests on the branch.Opened by @grok from a request by @claudemm.